feat(agents): add Kimi Code CLI support - #248
Conversation
johannesjo
left a comment
There was a problem hiding this comment.
Requesting changes for one merge-blocking coordinator-path issue.
electron/mcp/agent-args.ts:62 — Coordinator-created Kimi children never load their per-task MCP config.
Coordinator.createTask writes each child config to /tmp/parallel-code-subtask-.json natively or the coordinator .parallel-code/subtask-.json in Docker, then relies on buildMcpLaunchArgs to pass that path. This new Kimi branch returns an empty argument list. Kimi Code 0.31.1 discovers only its user mcp.json, project-root .mcp.json, and cwd/.kimi-code/mcp.json (published loader source), so neither generated child path is read. The child therefore starts without the parallel-code MCP server and cannot call land_self or signal_done, breaking the default Kimi coordinator-child flow in both native and Docker modes.
Please write or merge the unique child config into an auto-discovered child-worktree location while preserving token isolation and cleanup/restoration, and add coordinator child-creation coverage for Kimi. The new store test exercises top-level coordinator restoration only; it does not cover Coordinator.createTask.
|
Addressed the coordinator-child blocker in
Validation:
|
johannesjo
left a comment
There was a problem hiding this comment.
Re-reviewed current head 4d3d594. The original coordinator-child blocker is addressed, the Kimi flags and discovery paths match the published 0.32.0 implementation, and CI passes. Two merge-blocking lifecycle issues remain:
-
electron/mcp/coordinator.ts:1554— A tracked.mcp.jsonbreaks or contaminates self-landing. The child config is merged directly into the task worktree's project-root.mcp.json. Adding that path to.git/info/excludedoes not hide modifications to an already tracked file. Such a task therefore either failsprepareCleanSelfLandingWorktree()because.mcp.jsonis dirty, or can commit and merge the generatedparallel-codeentry containing the ephemeral token. Cleanup/restoration currently happens only after the merge/delete path. Please ensure the generated entry is restored or removed before landing validation and merge (or use an auto-discovered path guaranteed not to be tracked), and add a tracked-.mcp.jsonself-landing test. -
electron/mcp/coordinator.ts:1571— Restart hydration loses the original restoration snapshot.previousParallelCodelives only intask.autoDiscoveredMcpConfig; that state is neither persisted nor supplied tohydrateTask. After an application restart, hydration reads the previously generated entry as though it were the original user entry, writes fresh credentials, and teardown later restores the stale generated entry instead of the user's value or deleting the file. Persist/hydrate the original snapshot (or durably identify generated state), and add a create → process restart/hydrate → deregister restoration test.
Requesting changes until these two paths preserve Git state and restoration semantics.
Signed-off-by: Liang Hu <lh3057@columbia.edu>
Signed-off-by: Liang Hu <lh3057@columbia.edu>
4d3d594 to
d4ae1a6
Compare
|
Addressed both lifecycle blockers in
Validation: 412 focused tests; full suite 1,768 passed/23 skipped plus 9 client tests; type checks, static checks, and frontend build pass. |
johannesjo
left a comment
There was a problem hiding this comment.
Re-reviewed current head d4ae1a6, including two independent verification passes. The original coordinator-child MCP wiring and restart-persistence issues are addressed. CI is green, exact-head TypeScript checking passes, and all 262 coordinator tests pass. Three merge blockers remain:
-
Critical — Kimi 0.33 stalls new coordinator children at the workspace-trust gate.
docker/Dockerfile:53installs Kimi without a version pin, so new images now receive@moonshot-ai/kimi-code@0.33.0. That release enables agent-core-v2 by default and asks whether to trust each new folder before creating a session; project.mcp.json/.kimi-code/mcp.jsonservers stay disabled until accepted (0.33 release source, trust gate).--yolocontrols tool permissions and does not bypass this pre-session prompt. Every child starts in a fresh background worktree, so it waits at the dialog before loading the generated MCP server or receiving its assignment. Please either pin a known-compatible release or explicitly support the new trust flow without silently weakening its security boundary, and add a real-PTY fresh-worktree startup test. -
Critical — failed restoration still allows token-bearing config to enter Git history.
restoreTaskAutoDiscoveredMcpConfig()clears the recovery snapshot before success is known and returnsfalsefor fingerprint mismatches and read/write/unlink failures. BothlandSelfandmergeTaskcontinue anyway;mergeTaskthen runsgit add -Aand auto-commits. A mismatched generated entry can therefore be committed with both ephemeral MCP tokens. If that entry was committed earlier, even successful restoration cannot remove the earlier secret-bearing commit from a default non-squash merge. Managed restoration must fail closed, retain its snapshot for retry, and secret-bearing generated config must be kept out of tracked Git history. Add mismatch/write-failure and prior-generated-commit tests asserting no stage, commit, or merge occurs. -
Important — semantic restoration does not restore a tracked
.mcp.jsoncleanly. Both the initial write and restoration serialize the whole document with two-space indentation and no preserved trailing newline (restore write). A tracked file with compact JSON, different indentation, CRLF, or a final newline remains byte-dirty, soland_selfrejects it. The new test declares Git clean from parsed-object equality and cannot catch this. Preserve/restore exact original bytes or use an auto-discovered path verified to be untracked, then cover it with a real temporary Git repository.
Verdict: needs changes.
|
Re-reviewed current head CI is green. On the exact head I additionally ran
Verdict: needs changes because the current lifecycle can retain credentials in Git/app-state history and can discard unrelated user configuration edits. |
|
Addressed the remaining Kimi MCP lifecycle blockers in |
|
Follow-up hardening is in
Validation: full unit suite (1,775 passed, 23 skipped), client suite (9 passed), |
|
The latest |
|
Re-reviewed head 1. Should this reuse the coordinator's existing mechanism instead of building a parallel one?
I'd like to see one shared helper covering both the coordinator's 2. Is the child guard stricter than my own coordinator path?This is the one I got wrong across rounds 2–4, and I want to correct it before asking for anything further. The coordinator writes the same class of ephemeral token into So either the coordinator path carries the same exposure I called merge-blocking for children, or the bar I applied to children was higher than the project's own. I think it's the latter. Concretely: I'm withdrawing the escalations from rounds 2–4 as merge-blocking. If you'd rather simplify toward the existing convention — drop 3. Blocker: Kimi children never receive the sub-task preambleDifferent class of issue, and it's why I can't merge as-is: the MCP wiring all works, but the child is never told to use it.
The result is that Fix should be one line, adding 4. Two smaller robustness items
CIThe To summarize what I need: the preamble fix in §3. §1 and §2 are decisions for you — tell me which direction you want and I'll review against that rather than adding requirements. |
|
Addressed the Kimi child preamble blocker in
Validation passed locally: |
|
For sections 1 and 2, I am keeping the current hardening in this PR and not expanding it into a coordinator-path refactor. The Kimi child path now uses an untracked discovery path, restores only its managed entry, and keeps the fail-closed safeguards; generalizing the coordinator and child lifecycle can be handled as a separate focused change. The section 3 preamble blocker is addressed in |
johannesjo
left a comment
There was a problem hiding this comment.
Re-reviewed head 08efdb4. CI is green on that exact commit (both quality and GitGuardian).
§3 is fixed and verified. injectSubTaskPreamble routes kimi to AGENTS.md, and both PREAMBLE_MD_FILES and PREAMBLE_ARTIFACT_PATHS already carry AGENTS.md, so detection, land-time stripping, and the dirty-path allowance in prepareCleanSelfLandingWorktree all work with no further change. I also confirmed against Kimi's own loader that the discovery choice is right: resolveMcpJsonPaths merges {...user, ...projectRoot, ...project}, so <cwd>/.kimi-code/mcp.json wins over <projectRoot>/.mcp.json. findProjectRoot uses stat on .git, which succeeds on a worktree's .git file, so the .mcp.json fallback also resolves to the task worktree rather than the main checkout.
§1 and §2 are settled — you chose to keep the current hardening and defer the shared-helper refactor, which was one of the two options on the table. Not reopening either.
Three things below. None of them expand the scope of this PR; two of the three are deletions.
1. setMCPServerInfo still throws out of a bare loop (§4.1 from last round, not addressed)
coordinator.ts:690 still calls writeKimiAutoDiscoveredMcpConfig with no try/catch, and the method has five reachable throw sites: isTrackedGitPath spawn error, git status ∉ {0,1}, malformed JSON in either candidate, a pre-existing parallel-code entry, both paths tracked.
I verified the missing-worktree trigger empirically — spawnSync('git', …, { cwd: '/nonexistent' }) returns { error: ENOENT, status: null }, which isTrackedGitPath converts into a throw. And a task can sit in that state: cleanupTask returns early on deleteTask failure while leaving the task in this.tasks with mcpConfigPath still set, so the loop does not skip it.
Consequences are both of the ones you named. One bad task skips the config rewrite for every remaining sibling, and the throw escapes into register.ts:1760 — I checked the whole handler, its only try/catch is the .mcp.json read/parse near the top. So StartMCPServer rejects after the remote server was started, the Docker MCP server was copied and setDockerContainerName set, but before setCoordinatorSpawnDefaults, the coordinator .mcp.json write, and the returned mcpLaunchArgs: a half-started coordinator.
Log-and-continue per task.
2. History scan has no maxBuffer and still uses --all (§4.2 from last round, not addressed)
coordinator.ts:1768-1771 still runs git log --all -p --format= -- .mcp.json .kimi-code/mcp.json through promisify(execFile) with only { cwd }. I confirmed the failure mode on the Node version in use (v22.18.0): with exactly those options, 2 MiB of stdout rejects with ERR_CHILD_PROCESS_STDIO_MAXBUFFER. A repo that tracks .mcp.json with real history — precisely the case that forces the .kimi-code/mcp.json selection and reaches this code — can exceed 1 MiB, and landSelf turns that into an opaque landing_escalated.
Separately, --all walks every ref, and PARALLEL_CODE_MCP_TOKEN is the coordinator's subtaskToken, shared by all its children — so a token committed on one child's branch would block landing for unrelated siblings.
Worth restating the tradeoff now that the path selection has landed: the chosen file is guaranteed untracked and added to .git/info/exclude, and git add -A — the only automatic staging path, in both mergeTask and prepareCleanSelfLandingWorktree — skips excluded untracked files. So this scan only catches an agent that ran git add -f on its own MCP config, and it spends a full-history walk on every Kimi land and merge to do it.
Either fix it (${baseBranch}..HEAD plus an explicit maxBuffer) or take the offer from last round and drop assertManagedMcpTokensAbsentFromGitHistory along with the fail-closed land/merge coupling. Dropping it removes this risk entirely and is my preference, but either is fine — I'm not going to relitigate it a third time.
3. New: a problem in the unused candidate hard-blocks every Kimi task
coordinator.ts:1611-1629 reads both candidates eagerly in the candidates map, then the validation loop throws if either one defines mcpServers["parallel-code"] — all before a candidate is selected.
So a repo that tracks a .mcp.json containing a parallel-code server fails every Kimi child creation with .mcp.json already defines mcpServers["parallel-code"], even though .kimi-code/mcp.json is untracked, absent, and would have been the path actually used. Committing a .mcp.json for team-shared MCP servers is ordinary, and a project that uses this app's own server will name that key parallel-code. The malformed-JSON case is broader still: any tracked, malformed .mcp.json blocks Kimi through readMcpJsonContent in the same map, despite never being written to.
Fix is a simplification — select the candidate first, then read and validate only that one. That also shrinks the blast radius of item 1.
Smaller things
-
agent-args.ts:21vspreamble.ts:88— the two layers classify Kimi differently: the MCP layer matches basename=== 'kimi', the preamble matches substringincludes('kimi'). I checked the registry and@moonshot-ai/kimi-code@0.32.0ships only akimibin, so the default path is consistent and nothing is broken today. Butpreamble.test.tsdrives the new test withagentCommand: 'kimi-code --yolo'— a form the MCP layer classifies as non-Kimi, which falls throughbuildMcpLaunchArgsto['--mcp-config', configPath](the exact flag this PR exists to avoid) and skips the auto-discovered write entirely. Either align the two predicates or usekimiin the test, so it stops advertising a form that isn't wired. -
docker/Dockerfile:53—@moonshot-ai/kimi-code@0.32.0is the only pinned CLI on that line and the reason for the pin (0.33's workspace-trust gate) exists only indockerfile.test.tsand this thread. Please put a one-line comment at the pin itself; otherwise the next routine bump silently reintroduces the hang. The guardexpect(dockerfile).not.toContain('@moonshot-ai/kimi-code ')is also brittle — it keys on a trailing space and passes on a reformatted line. -
coordinator.ts:2044—refreshTaskMcpConfigAfterLandingFailurealso runs on the merge success path, to re-arm the child's config after a non-cleanup merge. The behavior is right, the name isn't. -
coordinator.ts:1762— thetokens.length === 0guard is unreachable:restoreTaskAutoDiscoveredMcpConfigonly returns a definedmanagedEntrywhen its fingerprint matches an entry we wrote, and every such entry carries both tokens. -
coordinator.ts:994—createTask's catch clears in-memory state and the MCP config but not the worktree/branch already created bycreateBackendTask. Pre-existing shape, but this PR adds four new throw sites between creation andspawnAgent, so orphaned worktrees get materially more reachable. Fixing item 3 removes most of that exposure.
Checked and not carrying forward
I looked at whether excluding only .kimi-code/mcp.json rather than the whole .kimi-code/ directory could leave other generated files dirty in the worktree — prepareCleanSelfLandingWorktree throws on any non-preamble dirty path, so it would have broken every land_self. It doesn't: per Kimi's data-locations doc, all runtime data (sessions, logs, credentials, plugins) lives under $KIMI_CODE_HOME, and project-local .kimi-code/ holds only user-authored config (mcp.json, skills/, agents/, AGENTS.md). The narrow single-file exclusion is the correct choice here — excluding the directory would have wrongly hidden a user's own .kimi-code/skills/ from git status. Good call.
Verdict: needs changes, though smaller ones than the header suggests. Item 3 is a new functional bug I'd want fixed. Items 1 and 2 were raised last round under "two smaller robustness items" and were neither addressed nor acknowledged — in fairness my summary that round said what I needed was §3, so I'm not treating them as a promise you broke, but they're still real and still open. All three fixes are small, and two of them remove code rather than add it.
The credential-lifecycle work from rounds 2–4 reads well, and the tests track it closely — restart round-trip, concurrent-edit preservation, refuse-overwrite, and both fail-closed land/merge paths. Thanks for the persistence through a long review.
|
The latest head |
Summary
kimiagent with current resume and permission-bypass flags.kimi-codeauth/config directory--mcp-configargument and cover native, Docker, and coordinator launch behaviorFixes #109.
Validation
npx vitest run electron/ipc/agents.test.ts electron/mcp/agent-args.test.ts electron/ipc/pty.test.ts src/lib/agent-args.test.ts src/store/tasks.test.tsnpm run checknpm run check:staticnpm testnpm run test:security-rulesnpm run build:frontendnpm run build:remotenpm run build:mcp