[2593 by GPT-5.6 Sol] MCP: introduce a structured error envelope (code, message, stage, retryable) with secret redaction for every tool call

[propr-dev]

AI Implementation Summary

Closes #2593

Branch: 2593/gpt-5.6-sol-mcp-introduce-a-structure-20260928-2357-n39
Commits: ✅ Changes committed (85850e1)

---

AI Processing Completed

Execution Details:

• Issue: MCP: introduce a structured error envelope (code, message, stage, retryable) with secret redaction for every tool call #2593
• Repository: integry/propr
• Status: Success
• Execution Time: 10m 39s
• Tokens used: 1.64M tokens [1.62M input + 17.91K output]
• API cost: $1.39
• Timestamp: Sep 29, 2026, 12:08 AM UTC
• LLM Model: GPT-5.6 Sol
• Subscription usage: Weekly +1%

Summary:
Implemented GitHub issue #2593.

Key changes:

• Added the structured MCP error envelope and centralized classification/redaction in errorEnvelope.ts.
• Extended McpError with optional stage, retryability, details, and toEnvelope().
• Persisted full error envelopes for failed operations, including underlying causes for uncertain mutations.
• Returned protocol errors through both JSON text content and structuredContent.
• Updated access-log classification to record envelope codes.
• Added comprehensive classification, persistence, transport, and secret-redaction tests.
• Added the new test file to test:mcp.

Validation completed:

• npm run test:mcp — 111 tests passed.
• npm run typecheck -w @propr/api — passed.
• npm run lint -w @propr/api — passed.

No visual preview was generated because the change is backend-only.

Detailed Logs:

• Conversation: 27 messages
• Session: null

Log files stored at:

• conversation: /tmp/claude-logs/issue-2593-2026-09-29T00-08-06-871Z-conversation.json
• output: /tmp/claude-logs/issue-2593-2026-09-29T00-08-06-871Z-output.txt

Latest Conversation Messages

---

This PR was created automatically by ProPR after processing issue #2593.

---

💡 Need changes?

Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!

[propr-dev]

/ultrafix
Triggered automatically by the requested execution settings.

[propr-dev]

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /review

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• codex:gpt-6-astra

View Task Details

[propr-dev]

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR centralizes MCP error classification and adds durable structured error details, but needs changes before merge. Mutation uncertainty handling regresses for workflow failures, and access logging can misclassify uncertain mutations.

✅ Shared error representation — Protocol errors expose matching JSON text and structuredContent through one helper.

✅ Durable diagnostic causes — Non-domain mutation failures retain their underlying classification while disabling automatic retry.

✅ Focused regression coverage — Added tests cover persistence, classification, and several credential formats. The supplied current-head checks report 18 passed and no failures.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F1: 🔴 Preserve uncertainty for server-side McpErrors

• Required behavior: A mutation failure that may follow external side effects must remain uncertain rather than being recorded as a definite failure.

• Evidence: packages/api/mcp/errorEnvelope.ts:classifyError and packages/api/mcp/operations.ts:McpOperations.run

  1. A mutation invokes callWorkflow, whose handler returns HTTP 500.
  2. The supplied adapter converts that response into McpError('WORKFLOW_REJECTED', ..., 500), explicitly advising inspection before retrying.
  3. McpOperations.run classifies it with sideEffectsPossible: true.
  4. The new unconditional isMcpError(error) exemption preserves WORKFLOW_REJECTED; persistence consequently records state: 'failed' instead of state: 'unknown'.

  The durable receipt now asserts failure where the previous implementation preserved uncertainty for every McpError with status ≥500. Idempotency prevents replay under the same key, but cannot correct the misleading receipt when a caller decides whether to issue another action. Existing tests cover a generic error and a 409 McpError, leaving this branch uncovered. static trace: verified against the supplied adapter, classifier, and operation persistence code.

• Minimum fix: Preserve the previous status boundary when exempting McpErrors from uncertainty wrapping. Add a regression asserting that a workflow 500 produces an unknown receipt with the underlying error retained as its cause.

F2: 🔴 Log uncertain mutations as errors

• Required behavior: Access-log outcomes must distinguish authorization or input denials from mutations whose outcome the instance cannot establish.

• Evidence: packages/api/mcp/accessLog.ts:classifyMcpFailure and packages/api/mcp/toolExecution.ts:runTool

  1. A mutation throws the GitHub HTTP 422 error already represented in the added operation test.
  2. Classification produces OUTCOME_UNKNOWN with status 422, and operation persistence records state: 'unknown'.
  3. The changed invocation catch stores that classification in access.failure.
  4. classifyMcpFailure derives the outcome solely from status, recording this uncertain mutation as denied. noteToolOutcome preserves that classification because access.failure is already set.
  5. Replaying the same operation instead uses receiptFailure, which records error with status 500.

  The first attempt and its replay therefore report different outcome categories for the same unknown operation, obscuring uncertain mutations in the audit log. The current dispatch test only exercises a generic 500 failure, so it does not prevent this sequence. static trace: verified through the supplied classifier, invocation catch, and receipt fallback.

• Minimum fix: Classify OUTCOME_UNKNOWN as an error regardless of its underlying HTTP status, and test the first attempt and replay of a mutation that throws a GitHub 422.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Cover the deployed MCP token prefix

The new redactor recognizes pia_mcp_, while the supplied authentication and existing adapter code recognize propr_mcp_. Add that deployed prefix and test it across messages, details, and causes. The mismatch is demonstrable, but the supplied production paths do not establish a concrete source that places such a credential into these error messages, so this remains a defensive follow-up.

S2: 🟢 Exercise envelopes through protocol dispatch

The added transport assertion calls toToolErrorResult directly. A real client test would verify callback errors and document that SDK input validation occurs before the callback and therefore bypasses the envelope helper. That validation boundary predates this PR, so extending its behavior is optional rather than a regression fix.

Score

The implementation has useful centralization and passing current-head checks, but two verified classification regressions remain. This review used static tracing only; no commands were run.

Score: 6/10

---

🤖 Review Details

• Model: GPT-6 Astra
• Time: 53s
• Tokens: 53,677 (52,122 in / 1,555 out)
• Cost: $0.50

View Task

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}

[propr-dev]

✅ Applied the requested follow-up changes in commit a8e349a

Addressed 1 AI review comment (IDs: 5881219643)

Addressed findings F1, F2

Summary of Changes

Implemented F1 and F2.

• Server-side McpErrors with status ≥500 now preserve mutation uncertainty as OUTCOME_UNKNOWN, retaining the original error as the cause.
• Access logs now classify OUTCOME_UNKNOWN as error regardless of HTTP status.
• Added regressions for workflow HTTP 500 persistence and GitHub HTTP 422 first-attempt/replay logging consistency.

Validation passed:

• npm run test:mcp — 111 tests
• API typecheck
• API lint
• git diff --check

No independent findings or visual changes; no preview was needed.

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @propr-ultrafix
• Time: 4m 5s
• Tokens: 531,569 (527,183 in / 4,386 out)
• Cost: $0.48

View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• codex:gpt-6-astra

View Task Details

[propr-dev]

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR centralizes MCP error classification and adds useful durable diagnostics, but needs a localized redaction correction before merge.

✅ Mutation uncertainty preserved — Failed mutations retain OUTCOME_UNKNOWN, disable automatic retry, and persist the underlying classified cause.

✅ Consistent error representations — toToolErrorResult() produces matching JSON text and structured content, with explicit test coverage.

✅ Passing current checks — The supplied authoritative status reports 18 passed checks and no failures or pending checks.

This review is based on static inspection; no commands were run.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F3: 🔴 Redact the actual MCP token prefix

• Required behavior: Newly exposed upstream error messages must redact the application's MCP credentials before returning or persisting them.

• Evidence: packages/api/mcp/errorEnvelope.ts:20, MCP_TOKEN, and packages/api/mcp/errorEnvelope.ts:127, githubMessage.

  1. A tool propagates a GitHub-shaped error containing a bare propr_mcp_… credential in its diagnostic message. The existing github_rejected_fixture provides a concrete callable error path; substituting this string into its response message exercises the case.
  2. githubMessage() now exposes that diagnostic through redactSecrets(). Its MCP expression recognizes only pia_mcp_…, so the propr_mcp_… value survives. packages/api/mcp/server.ts explicitly recognizes propr_mcp_ as an application bearer-token prefix.
  3. For reads, the credential reaches both protocol representations. For mutations, it survives in cause.message and is written to mcp_operations.result.

  The other expressions do not match a bare propr_mcp_… value, and repeated sanitization therefore provides no protection. Existing tests exercise only pia_mcp_…. Verification provenance: static trace of the supplied classifier, server, persistence code, and callable fixture; proposed regression: use a synthetic propr_mcp_… token in that fixture and assert its absence from protocol output and the persisted result.

• Minimum fix: Extend MCP-token redaction to recognize propr_mcp_ credentials and add regression coverage for both direct errors and persisted uncertainty causes.

Suggestions

These are optional follow-ups and are not sent to /fix.

S3: 🟢 Exercise envelopes through the protocol

The new transport assertion calls toToolErrorResult() directly. Add an actual MCP client request that reaches the registered callback and throws, then assert text/structured equality. Also document or test the separate SDK validation path: the supplied server context confirms that schema rejection happens before this callback. This would strengthen integration coverage without expanding the current fix.

S4: 🟢 Preserve status on receipt replay

receiptFailure() still substitutes 400 or 500 on replay, even though persisted envelopes now contain the original status. The updated test explicitly shows 422 becoming 500. Reading the stored status, with a fallback for older receipts, would improve observability. This inconsistency predates the PR and is not a merge blocker.

Score

The implementation is focused and current checks pass, but the newly exposed diagnostic path misses the application's actual MCP credential prefix. A small correction and targeted regression coverage should resolve the blocker.

Score: 6/10

---

🤖 Review Details

• Model: GPT-6 Astra
• Time: 1m 6s
• Tokens: 51,744 (49,721 in / 2,023 out)
• Cost: $0.60

View Task

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}

[propr-dev]

✅ Applied the requested follow-up changes in commit b984c3e

Addressed 1 AI review comment (IDs: 5881386449)

Addressed finding F3

Summary of Changes

Implemented F3.

• Extended MCP credential redaction to cover propr_mcp_… tokens in errorEnvelope.ts.
• Added regressions for both protocol representations in mcpErrorEnvelope.test.ts.
• Verified mutation uncertainty causes are redacted before persistence and replay in mcpAccessLogDispatch.test.ts.

Validation:

• npm run test:mcp — 111 passed
• API typecheck — passed
• API lint — passed
• git diff --check — passed

No independent findings or visual changes.

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @propr-ultrafix
• Time: 4m 29s
• Tokens: 641,923 (635,662 in / 6,261 out)
• Cost: $0.62

View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• codex:gpt-6-astra

View Task Details

[propr-dev]

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR centralizes MCP error classification, adds structured error envelopes, and persists useful causes for uncertain mutations. It is ready to merge within the reviewed scope.

✅ Mutation uncertainty preserved — Failed mutations retain durable operation handles and expose OUTCOME_UNKNOWN with retryable: false, while preserving the classified underlying cause.

✅ Consistent error serialization — toToolErrorResult() mirrors the error envelope into JSON text and structuredContent, with credential redaction applied to both.

✅ Persistence coverage — Added tests check stored envelopes, replay behavior, and credential removal before database persistence.

This review used static analysis of the supplied diff and context; no commands were run. Authoritative current-head checks report 18 passed and no failures or pending checks.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S5: 🟢 Cover validation over the transport

The supplied dispatch test demonstrates that SDK input validation happens before the callback in packages/api/mcp/server.ts. Those rejections therefore bypass the new envelope formatter, while the new validation test exercises classifyError() directly. A transport-level assertion would document this boundary and help determine whether SDK-generated validation responses should eventually use the same envelope. This is an existing dispatch limitation, not a demonstrated regression introduced by this PR.

Score

The implementation provides coherent classification, conservative mutation handling, and focused persistence and redaction tests. Current checks pass; verification here was limited to static review.

Score: 8/10

---

🤖 Review Details

• Model: GPT-6 Astra
• Time: 29s
• Tokens: 53,228 (52,438 in / 790 out)
• Cost: $0.46

View Task

---

💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

_{🤖 Review by ProPR}