Repository navigation
fix(persona): restore full ai-hist MCP via npx -p ai-hist (revert bundled WHY-only server) - #209
Conversation
…led WHY-only server) The pr-reviewer rework (f6977ea, on #206) replaced the ai-hist injection with a bundled zero-dep server exposing only list_trajectories/read_trajectory — which drops the HOW (cross-tool history) and none of the locked contract tools (search_history/why_for_task/search_trajectories/get_session/get_context/stats). Restore the unified why+how MCP by launching codex-1's published ai-hist package directly: `npx -y -p ai-hist ai-hist-mcp` — the ai-hist-mcp bin ships inside ai-hist@0.3.0, so no separately-published wrapper package is required. - buildAiHistMcpServer: command 'npx', args ['-y','-p','ai-hist','ai-hist-mcp'] - remove the bundled ai-hist-mcp-server.ts + AI_HIST_SERVER_SCRIPT const - restore test assertions to the npx form; drop now-unused node:path imports buildAiHistMcpServer is the single chokepoint for both the CLI spawn and cloud-defaults (B), so this restores both injection paths at once. B's contract-emit side is unaffected. persona-kit: 267 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR removes the bundled Node.js MCP server implementation and migrates to launching the ai-hist server through the external ChangesMCP Server Launch Migration to NPX
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the launch mechanism of the ai-hist MCP server, replacing the execution of a local script via process.execPath with an npx invocation (npx -y -p ai-hist ai-hist-mcp). Consequently, the local script file is removed, and the relevant tests are updated. The reviewer recommends pinning the ai-hist package to a specific version (e.g., ai-hist@0.3.0) in both the implementation and the test assertions to mitigate supply chain security risks, prevent unexpected breaking changes, and avoid performance overhead from registry checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| command: process.execPath, | ||
| args: [AI_HIST_SERVER_SCRIPT], | ||
| command: 'npx', | ||
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], |
There was a problem hiding this comment.
Using npx -y -p ai-hist without a pinned version introduces supply chain security risks and reliability issues (e.g., unexpected breaking changes if a new version of ai-hist is published). Additionally, it can cause performance overhead due to registry update checks.\n\nConsider pinning the package to a specific version (like ai-hist@0.3.0) to ensure deterministic, secure, and reliable execution.
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], | |
| args: ['-y', '-p', 'ai-hist@0.3.0', 'ai-hist-mcp'], |
| command: process.execPath, | ||
| args: [(server as { args: string[] }).args[0]], | ||
| command: 'npx', | ||
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], |
There was a problem hiding this comment.
Update the test assertion to match the pinned version of the ai-hist package.
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], | |
| args: ['-y', '-p', 'ai-hist@0.3.0', 'ai-hist-mcp'], |
| command: process.execPath, | ||
| args: [(server as { args: string[] }).args[0]], | ||
| command: 'npx', | ||
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], |
There was a problem hiding this comment.
Update the test assertion to match the pinned version of the ai-hist package.
| args: ['-y', '-p', 'ai-hist', 'ai-hist-mcp'], | |
| args: ['-y', '-p', 'ai-hist@0.3.0', 'ai-hist-mcp'], |
|
Reviewed PR #209 against the current checkout. I did not find a demonstrated breakage requiring code changes. Validated:
I cleaned generated |
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #209 against the current checkout. I did not find a demonstrated breakage requiring code changes. Validated:
I cleaned generated |
Problem
An automated PR-reviewer commit (
f6977ea, applied to #206) replaced the ai-hist MCP injection with a bundled, zero-dependencyai-hist-mcp-server.tslaunched vianode. That server exposes onlylist_trajectories+read_trajectory— it drops the HOW (AI_HIST_DBis unwired) and exposes none of the locked contract tools (search_history,why_for_task,search_trajectories,get_session,get_context,stats). As merged, personas got a minimal why-only reader instead of the unified why+how MCP the team built (and bypassed codex-1's publishedai-hist@0.3.0entirely).Fix
Restore the unified MCP by launching codex-1's published package directly:
npx -y -p ai-hist ai-hist-mcp. Theai-hist-mcpbin ships insideai-hist@0.3.0, so this needs no separately-published wrapper package (sidesteps the npm first-publish/OIDC blocker).buildAiHistMcpServer:command: 'npx',args: ['-y','-p','ai-hist','ai-hist-mcp']ai-hist-mcp-server.ts+AI_HIST_SERVER_SCRIPTnode:pathimportsbuildAiHistMcpServeris the single chokepoint for both the CLI spawn (C) and cloud-defaults (B), so this one revert restores both injection paths. B's contract-emit side (#207) is unaffected — it still writes the full contract files; this only fixes which MCP reads them.Verification
why_for_task(why) +search_history(how) over stdio againstai-hist@0.3.0.Follow-up (optional, non-blocking): once the
ai-hist-mcpwrapper package is bootstrapped on npm, the args can simplify to['-y','ai-hist-mcp'].🤖 Generated with Claude Code