Uh oh!
There was an error while loading. Please reload this page.
AIT-239: Address MCP review feedback - #30
Conversation
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughClaude MCP operations now use bounded execution and structured cleanup results. MCP installation supports JSON output, while logout reports cleanup success or warnings in both JSON and human-readable modes. ChangesMCP lifecycle and command output
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LogoutCommand
participant removeClaudeMcp
participant ClaudeCode
User->>LogoutCommand: run logout
LogoutCommand->>removeClaudeMcp: remove Claude MCP configuration
removeClaudeMcp->>ClaudeCode: execute cleanup with timeout
ClaudeCode-->>removeClaudeMcp: return cleanup result
removeClaudeMcp-->>LogoutCommand: return ok and detail
LogoutCommand-->>User: print logged-out status or warning
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ord669
commented
Jul 22, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:16f90c4e26
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/commands/__tests__/mcp.test.ts (1)
42-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the resolved absolute entrypoint.
mcp.tscallsresolve(process.argv[1]), but this test compares against rawprocess.argv[1]. It only matches when the runner already supplies an absolute path, so relative invocations can fail and the absolute-path contract is not actually asserted. Useresolve(process.argv[1])in the expectation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/__tests__/mcp.test.ts` at line 42, Update the headersHelper expectation in the MCP test to resolve process.argv[1] before constructing the expected command, matching the absolute-entrypoint behavior in mcp.ts while preserving the existing JSON stringification and argument order.src/commands/mcp.ts (1)
39-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve cleanup failures during duplicate recovery.
If
removeClaudeMcp(true)times out or fails, its{ ok: false, detail }result is ignored and another 10-secondclaude mcp add-jsonattempt runs. This can add another 10 seconds and mask the actual cleanup error; check the result before retrying.Suggested fix
- removeClaudeMcp(true);+ const cleanup = removeClaudeMcp(true);+ if (!cleanup.ok) {+ throw new ConfigurationError(+ cleanup.detail ?? 'Claude MCP cleanup failed',+ 'MCP_INSTALL_FAILED',+ );+ } result = spawnSync('claude', args, CLAUDE_OPTIONS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/mcp.ts` around lines 39 - 40, Update the duplicate-recovery flow around removeClaudeMcp(true) to inspect its result before invoking the retry spawnSync('claude', args, CLAUDE_OPTIONS). If cleanup returns ok: false, preserve and surface its detail instead of attempting another add-json call; only retry when cleanup succeeds.src/auth/__tests__/logout.test.ts (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the cleanup-warning branch.
Add a test where
removeClaudeMcp()returns{ ok: false, detail: '...' }, assertinglogged_out_with_warning, the propagated detail, and the human-readable warning output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/auth/__tests__/logout.test.ts` around lines 63 - 67, Add a logout test covering the cleanup-warning branch by mocking removeClaudeMcp() to return an unsuccessful result with a detail string, then assert the response status is logged_out_with_warning, the detail is propagated, and the output includes the human-readable warning message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/mcp.ts`:
- Around line 11-12: Update headersHelper so both executable and script paths
are safely escaped for shell execution rather than JSON-encoded, preventing
metacharacter expansion when Claude invokes the generated command. Use the
project’s existing shell-escaping utility or a fixed wrapper, and add a
regression test covering characters such as $() and backticks.
---
Nitpick comments:
In `@src/auth/__tests__/logout.test.ts`:
- Around line 63-67: Add a logout test covering the cleanup-warning branch by
mocking removeClaudeMcp() to return an unsuccessful result with a detail string,
then assert the response status is logged_out_with_warning, the detail is
propagated, and the output includes the human-readable warning message.
In `@src/commands/__tests__/mcp.test.ts`:
- Line 42: Update the headersHelper expectation in the MCP test to resolve
process.argv[1] before constructing the expected command, matching the
absolute-entrypoint behavior in mcp.ts while preserving the existing JSON
stringification and argument order.
In `@src/commands/mcp.ts`:
- Around line 39-40: Update the duplicate-recovery flow around
removeClaudeMcp(true) to inspect its result before invoking the retry
spawnSync('claude', args, CLAUDE_OPTIONS). If cleanup returns ok: false,
preserve and surface its detail instead of attempting another add-json call;
only retry when cleanup succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8b28b23-ab56-465f-9bf5-7150b4b528c8
📒 Files selected for processing (4)
src/auth/__tests__/logout.test.tssrc/auth/logout.tssrc/commands/__tests__/mcp.test.tssrc/commands/mcp.ts
Uh oh!
There was an error while loading. Please reload this page.
ord669
commented
Jul 22, 2026
@codex review |
ord669
commented
Jul 22, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/mcp.ts (1)
47-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve combined Claude CLI diagnostics in both failure paths.
Both paths can discard stdout: installation does not refresh output after retrying, while cleanup never combines stdout and stderr.
src/commands/mcp.ts#L47-L51: recompute combined output after the retry and use it for the final error fallback.src/commands/mcp.ts#L76-L80: use combined stdout/stderr when constructing cleanup failure details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/mcp.ts` around lines 47 - 51, The Claude CLI failure handling in install and cleanup must preserve diagnostics from both stdout and stderr. In src/commands/mcp.ts lines 47-51, recompute the combined output after any retry and use it as the final ConfigurationError fallback; in src/commands/mcp.ts lines 76-80, use the combined stdout/stderr output when constructing cleanup failure details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/mcp.ts`:
- Around line 47-51: The Claude CLI failure handling in install and cleanup must
preserve diagnostics from both stdout and stderr. In src/commands/mcp.ts lines
47-51, recompute the combined output after any retry and use it as the final
ConfigurationError fallback; in src/commands/mcp.ts lines 76-80, use the
combined stdout/stderr output when constructing cleanup failure details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c287244-2171-49ab-b226-37d309e599de
📒 Files selected for processing (3)
src/auth/__tests__/logout.test.tssrc/commands/__tests__/mcp.test.tssrc/commands/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/tests/mcp.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:beaa3e2426
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ord669
commented
Jul 22, 2026
@codex review |
ord669
commented
Jul 22, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ord669
commented
Jul 22, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/mcp.ts (1)
81-85: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve combined output in failure details.
The code combines stdout and stderr for classification but reports only stderr. If Claude writes diagnostics to stdout, logout loses the useful failure reason; use the trimmed combined output as the fallback detail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/mcp.ts` around lines 81 - 85, Update the failure detail construction in the MCP cleanup result to use the trimmed combined stdout-and-stderr output as its fallback, rather than only result.stderr. Preserve the timedOut(result.error) message and the existing result.error?.message precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/mcp.ts`:
- Around line 81-85: Update the failure detail construction in the MCP cleanup
result to use the trimmed combined stdout-and-stderr output as its fallback,
rather than only result.stderr. Preserve the timedOut(result.error) message and
the existing result.error?.message precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15c66ae9-e114-4c49-80bf-47406daf7f7c
📒 Files selected for processing (2)
src/commands/__tests__/mcp.test.tssrc/commands/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/tests/mcp.test.ts
Uh oh!
There was an error while loading. Please reload this page.
* AIT-239 address MCP review feedback * AIT-239 harden MCP helper execution * AIT-239 make MCP cleanup cross-platform
Summary\n- bound every Claude subprocess call with a shared timeout\n- use an absolute executable helper for MCP headers\n- surface MCP cleanup failures during logout\n- honor global JSON output for MCP installation\n\n## Validation\n- 116 focused tests pass\n- TypeScript check passes\n- production build passes\n\nFollow-up to #29. Linear: AIT-239
Summary by CodeRabbit
mcp install --agent claudecan output{ status: 'configured', agent: 'claude' }in JSON mode.