Skip to content

mcpdo: keep the sign-in helper's failure classification without a TTY (#2962) - #2971

Merged
cliffhall merged 2 commits into
v2/mainfrom
v2/fix/2962-auth-helper-exit-classification
Nov 12, 2026
Merged

cliffhall merged 2 commits into
v2/mainfrom
v2/fix/2962-auth-helper-exit-classification

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2962

Problem

Without a TTY, mcpdo connect runs OAuth in the detached sign-in helper. Any failure the helper reported before producing the authorization URL was turned into auth_required / exit 3 by the parent, because the helper relayed only the message text. A fetch-guard refusal therefore exited 3 instead of fetch_refused / 12 (and lost its url), unlike the one-shot CLI and the TTY path.

Fix

  • The helper now classifies its failure with classifyError — in the process where the original error object still exists — and relays exitCode plus the envelope fields on the error event (helperErrorEvent). The catch-all error classification is not relayed.
  • The parent rebuilds the classified CliExitCodeError from the event (relayedHelperError): exit code must be a known non-zero EXIT_CODES value, every field is type-checked, every string sanitized. auth_required stays the fallback for an unclassified (or malformed) relay.
  • The EMA login helper emits through the same event and shares the same fix.

Verification

Reproduced per the issue against a composable server with "mode":"protected-resource","authorizationServers":["http://169.254.169.254"], </dev/null:

{"error":{"code":"fetch_refused","message":"Sign-in helper failed: Refusing to fetch from a link-local or metadata address (169.254.169.254)","url":"http://169.254.169.254/.well-known/oauth-authorization-server"}}
exit=12

(Before: auth_required / 3.) Unit tests cover both sides of the relay and the malformed-input fallbacks; npm run local:gate is green.

🤖 Generated with Claude Code

…#2962)

The detached sign-in helper now classifies a flow failure with
classifyError, where the original error object still exists, and relays
the exit code and envelope fields on its error event. The parent
rebuilds that CliExitCodeError (type-checked, sanitized) so a non-TTY
connect reports e.g. fetch_refused / 12 with its url, as the TTY path
does. auth_required stays the fallback for an unclassified failure.
The EMA login helper shares the same event and gets the same fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reconstructing wrapped fetch errors drops the relayed cause, leaving non-TTY output without the refusal reason; preserve the sanitized cause and add a final-envelope round-trip assertion.

1 open issue
What changed in this PR

This PR addresses #2962 by preserving classified OAuth failures across mcpdo’s detached sign-in helper, bringing non-TTY error reporting closer to the TTY and one-shot CLI paths.

Changes:

  • Relay classified exit codes and error fields from both sign-in helpers.
  • Validate and sanitize relayed fields, retaining auth_required as the fallback.
  • Add tests for classification, sanitization, and malformed relay data.
File Description
clients/​mcpdo/​src/​connection/​ema-login-helper.ts Uses the shared classified error-event builder.
clients/​mcpdo/​src/​connection/​auth-helper.ts Builds and reconstructs classified helper errors.
clients/​mcpdo/​__tests__/​ema-login-helper.test.ts Checks that EMA configuration failures retain their classification.
clients/​mcpdo/​__tests__/​auth-helper.test.ts Covers error relaying, validation, sanitization, and fallbacks.

Review effort: Balanced

…2962)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1: 1 finding (relayed cause dropped on reconstruction) — fixed in fb5e98d with a round-trip test; replied in thread. Requesting round 2.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block the changes preserving classified OAuth failures in non-TTY connections.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review closed: round 2 was clean (no inline comments, no open issues, no suppressed comments) — the loop stopped on the first clean round.

@cliffhall
cliffhall merged commit a53ce55 into v2/main Nov 12, 2026
12 checks passed
@cliffhall
cliffhall deleted the v2/fix/2962-auth-helper-exit-classification branch November 12, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mcpdo: a sign-in helper failure without a TTY loses its exit-code classification (fetch_refused reported as auth_required)

2 participants