Uh oh!
There was an error while loading. Please reload this page.
feat(persona-kit): translate codex mcpServers into launch args - #85
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extends the persona-kit to wire MCP servers for the Codex harness by translating persona ChangesCodex MCP Server Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/persona-kit/src/interactive-spec.ts`:
- Around line 119-120: The dotted-key prefix construction in the loop using
mcpServers and the local variable prefix (mcp_servers.${name}) can produce
invalid TOML keys if name contains dots or other special chars; update the code
in packages/persona-kit/src/interactive-spec.ts to either validate server names
against an allowed charset (e.g., /^[A-Za-z0-9_-]+$/) before using them or
escape/quote the segment when building the dotted key (e.g., produce
mcp_servers."escapedName" by adding a small helper escapeTomlKey(name) that
returns a properly backslash-escaped quoted key, and use that helper when
setting prefix inside the for (const [name, server] ...) loop).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 626e0e00-e681-4d6f-b39d-eb790b754e14
📒 Files selected for processing (4)
packages/cli/README.mdpackages/persona-kit/src/interactive-spec.test.tspackages/persona-kit/src/interactive-spec.tspackages/persona-kit/src/types.ts
Uh oh!
There was an error while loading. Please reload this page.
| warnings: string[] | ||
| ): void { | ||
| for (const [name, server] of Object.entries(mcpServers).sort(([a], [b]) => a.localeCompare(b))) { | ||
| const prefix = `mcp_servers.${name}`; |
There was a problem hiding this comment.
🟡 MCP server name not quoted in TOML dotted-key path, producing invalid or mis-parsed TOML
In appendCodexMcpServerArgs, the MCP server name is interpolated directly into the TOML key path as a bare key: const prefix = \mcp_servers.${name}`. TOML bare keys only support [A-Za-z0-9_-]. If the server name contains dots (e.g. "foo.bar"), TOML interprets mcp_servers.foo.bar.urlas a 4-level nested path instead of the intendedmcp_servers."foo.bar".url. If the name contains spaces or other special characters, the TOML is syntactically invalid. No upstream validation in parseMcpServers (packages/persona-kit/src/parse.ts:400`) constrains MCP server names to bare-key-safe characters, so any JSON object key flows through unchecked.
| constprefix=`mcp_servers.${name}`; | |
| constprefix=`mcp_servers.${toTomlBasicString(name)}`; |
Was this helpful? React with 👍 or 👎 to provide feedback.
khaliqgant
commented
May 11, 2026
Addressed CodeRabbit feedback in commit 9bac2c2.
Validation: corepack pnpm --filter @agentworkforce/persona-kit test (pass). |
Summary
Relay alignment
Validation