Skip to content

fix(result-utils): avoid collapsing error payloads to data#106

Open
AielloChan wants to merge 1 commit intosteipete:mainfrom
AielloChan:main
Open

fix(result-utils): avoid collapsing error payloads to data#106
AielloChan wants to merge 1 commit intosteipete:mainfrom
AielloChan:main

Conversation

@AielloChan
Copy link

Summary

This PR fixes result parsing in src/result-utils.ts so mcporter no longer collapses full error payloads to data when a parsed JSON object contains additional fields.

Before this change, any object with a data field could be reduced to data alone. That meant payloads shaped like:

{
  "status": "error",
  "summary": "Failed to create base: name is required",
  "error": {
    "type": "USER_ERROR",
    "message": "Base name is required and cannot be empty or only whitespace",
    "retryable": false,
    "code": "INVALID_NAME"
  },
  "data": {},
  "meta": {},
  "trace_id": "trace-123"
}

could end up rendered as:

{}

This PR preserves the full object unless data is the only field.

Root Cause

tryParseJson() treated any object containing data as an envelope wrapper and returned record.data directly. That assumption is valid for some transport envelopes but incorrect for general API payloads where data is just one field among many.

structuredContent handling had already been partially corrected to preserve full objects in some cases, but the shared parsing path still unwrapped too aggressively.

What Changed

  • Added a shared unwrapJsonEnvelope() helper in src/result-utils.ts.
  • Reused that helper from both parseStructuredContent() and tryParseJson().
  • Kept existing json envelope behavior unchanged.
  • Changed data unwrapping to happen only when data is the sole key in the object.
  • Preserved full objects when data appears alongside status, error, summary, meta, or similar fields.

Behavior After This PR

New parsing rules:

  • If an object contains json, return json.
  • If an object contains only data, return data.
  • If an object contains data and any other keys, return the full object.

Tests Added

Added regression coverage for:

  • structuredContent payloads that contain data plus top-level error fields
  • text content that parses into JSON containing data plus top-level error fields

Updated test files:

  • tests/result-utils.test.ts
  • tests/cli-output-utils.test.ts

Verification

Passed:

  • pnpm build
  • pnpm check
  • pnpm exec vitest run tests/result-utils.test.ts tests/cli-output-utils.test.ts

Full pnpm test is not green in this environment, but the failures appear unrelated to this change. They are dominated by existing sandbox and environment issues such as:

  • listen EPERM for localhost or unix socket listeners
  • EPERM writing ~/.mcporter/credentials.json
  • an existing tests/server-proxy.test.ts failure unrelated to result parsing

User Impact

This makes failed tool results much easier to debug because mcporter now preserves the full error payload instead of showing {} when data happens to be present and empty.

Linked Issue

Closes #105

@AielloChan
Copy link
Author

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Keep it up!

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mcporter collapses error payloads to {} when a JSON result includes data plus other fields

1 participant