Repository navigation
fix: v2.10.0 merge review findings — TUI error redaction, mcpdo daemon flush, CodeQL #78-#81 - #2639
Merged
Merged
Conversation
…acting display boundary (#2638) #2490 made the TUI show caught error text only through errorText's errorMessage(), which redacts URL query secrets. The Tasks tab, Subscriptions tab and Roots editor added by #2432 still rendered err.message directly (TasksTab through its own unredacted copy), so a server error quoting an OAuth URL reached the screen verbatim. All three now use the shared errorMessage(); TasksTab's copy is gone. Each suite gains a test that throws an error quoting ?code=s3cret and asserts the secret never reaches the display; all three fail with the source change reverted. Found by Copilot on the v2.10.0 milestone merge (#2637). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
run.ts wrote its startup diagnostic and called process.exit(1) at once. On a pipe or file stderr is asynchronous, so exit could discard the message. Await the write via core/cli's awaitableError, the same flush-then-exit the shared CLI error handler uses. Found by Copilot on the v2.10.0 milestone merge (#2637). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
- #78: zshDescribeEntry escaped ':' but not '\' in a _describe name. Escape backslashes first, then colons; a test renders a name holding both. Flag names never contain either today, so this was unreachable. - #80, #81: the mcpdo stored-auth test matched its server with url.includes("example.com"); compare the full URL instead. - #79: the namespace-ledger test's sentinel edit is an anchored /^\{/ replace, with an assertion that the sentinel really differs, so the 'no rewrite' check can never pass vacuously. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused fixes address the linked issue using existing helpers and targeted regression checks, with no unresolved findings.
0 open findings
What changed in this PR
Addresses #2638’s release-review findings across the TUI, mcpdo daemon, and CLI completion code.
Changes:
- Routes three TUI error displays through shared redaction, with regression tests.
- Waits for daemon startup diagnostics to flush before exiting.
- Corrects zsh escaping and strengthens test assertions flagged by CodeQL.
| File | Description |
|---|---|
| clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts | Makes the no-rewrite sentinel check explicit. |
| clients/tui/src/components/TasksTab.tsx | Reuses shared error redaction. |
| clients/tui/src/components/SubscriptionsTab.tsx | Redacts displayed errors. |
| clients/tui/src/components/RootsModal.tsx | Redacts save errors. |
| clients/tui/__tests__/TasksTab.test.tsx | Tests displayed secret redaction. |
| clients/tui/__tests__/SubscriptionsTab.test.tsx | Tests displayed secret redaction. |
| clients/tui/__tests__/RootsModal.test.tsx | Tests the redacting error boundary. |
| clients/mcpdo/src/daemon/run.ts | Awaits stderr output before exiting. |
| clients/mcpdo/__tests__/connection-stored-auth.test.ts | Matches stored server URLs exactly. |
| clients/cli/src/completion.ts | Escapes backslashes before colons for zsh. |
| clients/cli/__tests__/completion.test.ts | Covers combined zsh escape characters. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Copilot review loop closed: round 1 was clean (0 findings — no inline comments, nothing in the headline or a suppressed block), so no further round was requested. |
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2638
Fixes everything Copilot and CodeQL raised on the v2.10.0 milestone merge, PR #2637 (#2623). Each item is in code this release ships.
What changed (one commit each)
c6c29b8a). The Tasks tab, Subscriptions tab and Roots editor added by TUI has no UI for Tasks, resource subscriptions, or Roots #2432 showed caught errors witherr.messagedirectly, bypassing the redacting display boundary that Redact URL query secrets in error text displayed by the web and TUI clients #2490 introduced (utils/errorText.ts).TasksTabhad its own unredacted copy oferrorMessage. All three now use the shared helper, and the copy is gone.Request failed: https://auth.example/cb?code=s3cret&state=okand asserts the frame showscode=%5BREDACTED%5D&state=okand nevers3cret..messagedisplays; the only one left,saveResult.ts, formats local file-write errors, not server text.65a1e2e4).daemon/run.tsnow awaitsawaitableErrorbeforeprocess.exit(1), which is the shared CLI error handler's flush-then-exit. Forcing a startup failure (MCP_STORAGE_DIRunder a file) with stderr on a pipe printsmcpdo daemon: ENOTDIR: …on every run. The race did not reproduce with a message this short before the fix either, so the evidence is the pattern plus this drive.run.tsis excluded from coverage as a bootstrap.b25bb58a).zshDescribeEntrynow escapes\before:, and a test renders a name holding both.includes("example.com")./^\{/, and asserts the sentinel really differs, so its "no rewrite" check cannot pass vacuously.Verification
npm run local:gate: every stage passes (CLI 584 + 2 skipped, TUI 630, mcpdo 490, web 8957, scripts 991, DCO 3/3,smoke:mcpdopassed) exceptsmoke:web:firefox, which cannot launch Playwright's Firefox on macOS 27 (#2625).local:storybook: 529 passed.🤖 Generated with Claude Code