Skip to content

feat(agents): add Kimi Code CLI support - #248

Open
LarryHu0217 wants to merge 13 commits into
johannesjo:mainfrom
LarryHu0217:codex/kimi-cli-109
Open

feat(agents): add Kimi Code CLI support#248
LarryHu0217 wants to merge 13 commits into
johannesjo:mainfrom
LarryHu0217:codex/kimi-cli-109

Conversation

@LarryHu0217

Copy link
Copy Markdown
Contributor

Summary

  • register Kimi Code as a built-in kimi agent with current resume and permission-bypass flags
  • install Kimi Code in the bundled Docker image and share its .kimi-code auth/config directory
  • avoid the unsupported generic --mcp-config argument and cover native, Docker, and coordinator launch behavior

Fixes #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.ts
  • npm run check
  • npm run check:static
  • npm test
  • npm run test:security-rules
  • npm run build:frontend
  • npm run build:remote
  • npm run build:mcp

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the coordinator-child blocker in 69a9fad.

  • Kimi children now merge their per-task parallel-code MCP entry into the child worktree's auto-discovered .mcp.json before spawn, in both native and Docker flows.
  • Per-child task IDs and done tokens remain isolated.
  • Existing MCP servers and any previous parallel-code entry are preserved and restored during teardown; concurrent edits are not overwritten.
  • Hydration/server-info refreshes also update the auto-discovered config.
  • Added coordinator creation, isolation, restoration, and concurrent-edit coverage.

Validation:

  • npm test (1637 passed, 23 skipped)
  • npm run check
  • npm run check:static
  • npm run build:frontend && npm run build:mcp
  • targeted Semgrep scan of the changed production files (0 findings)

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. electron/mcp/coordinator.ts:1554 — A tracked .mcp.json breaks 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/exclude does not hide modifications to an already tracked file. Such a task therefore either fails prepareCleanSelfLandingWorktree() because .mcp.json is dirty, or can commit and merge the generated parallel-code entry 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.json self-landing test.

  2. electron/mcp/coordinator.ts:1571 — Restart hydration loses the original restoration snapshot. previousParallelCode lives only in task.autoDiscoveredMcpConfig; that state is neither persisted nor supplied to hydrateTask. 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.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed both lifecycle blockers in d4ae1a6.

  • The original .mcp.json snapshot and generated-entry fingerprint now persist through autosave, IPC synchronization, and restart hydration.
  • Self-landing and merge restore a managed generated entry before Git validation, so tracked files cannot carry the temporary token into a commit.
  • A failed landing re-injects the temporary entry only when restoration remains safe; concurrent user edits remain untouched.
  • Added restart round-trip, tracked-.mcp.json self-landing, persistence, hydration, and sync coverage.

Validation: 412 focused tests; full suite 1,768 passed/23 skipped plus 9 client tests; type checks, static checks, and frontend build pass.

@LarryHu0217
LarryHu0217 requested a review from johannesjo August 4, 2026 14:29

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Critical — Kimi 0.33 stalls new coordinator children at the workspace-trust gate. docker/Dockerfile:53 installs 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.json servers stay disabled until accepted (0.33 release source, trust gate). --yolo controls 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.

  2. Critical — failed restoration still allows token-bearing config to enter Git history. restoreTaskAutoDiscoveredMcpConfig() clears the recovery snapshot before success is known and returns false for fingerprint mismatches and read/write/unlink failures. Both landSelf and mergeTask continue anyway; mergeTask then runs git add -A and 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.

  3. Important — semantic restoration does not restore a tracked .mcp.json cleanly. 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, so land_self rejects 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.

@johannesjo

Copy link
Copy Markdown
Owner

Re-reviewed current head 0cb1285 from scratch. The earlier Docker-version, fail-closed restoration, and byte-for-byte restoration concerns are addressed. I also checked the native Kimi 0.33 trust flow against the existing question handoff/forced coordinator auto-trust behavior and am not carrying that concern forward.

CI is green. On the exact head I additionally ran npm run typecheck, npm run compile, the full unit suite (1,771 passed, 25 skipped), and the client suite (9 passed); all passed. Three merge blockers remain:

  1. Security: a tracked .mcp.json can still put the ephemeral MCP tokens into merged Git history. writeKimiAutoDiscoveredMcpConfig writes the token-bearing config into the project root and only adds it to .git/info/exclude, which does not protect a file that was already tracked. If the child commits that modification, restoring the final worktree does not remove the secret-bearing commit. Both the default merge path (squash ?? false) and self-landing (explicitly non-squash) preserve it. I reproduced this with a tracked .mcp.json: after the child commit, restoration, and non-squash merge, git log -S ephemeral-token -- .mcp.json still found the token-bearing commit on the target branch. Please avoid writing managed credentials to a tracked path (or fail task creation clearly), and reject landing if an earlier task commit contains the managed token; restoring only the tip is insufficient.

  2. Data loss: restoration overwrites concurrent edits outside mcpServers["parallel-code"]. The guard fingerprints only the owned entry (lines 1645–1648), but when that entry matches, the code replaces the entire current file with the launch-time raw snapshot (lines 1650–1654). I reproduced a user changing another MCP server plus a top-level setting while the task ran: the fingerprint still matched and both edits were reverted. Either fingerprint the complete generated document and fail closed on any change before restoring the raw bytes, or restore only the owned entry while preserving all other current content.

  3. Security: the raw pre-existing MCP config is copied into renderer state and ordinary app-state backups. previousContent captures the whole file and is sent to the renderer, then toPersistedTask includes it in the saved payload. A normal MCP config may contain API keys in server env fields, so this duplicates those credentials into state.json; the persistence layer writes with default process permissions and copies the prior state to state.json.bak before replacement (lines 27–42), allowing the secret-bearing snapshot to linger after restoration. Keep the raw backup out of renderer/general app state—e.g. a main-process-only mode-0600 backup with only non-secret metadata persisted—and remove it after restoration.

Verdict: needs changes because the current lifecycle can retain credentials in Git/app-state history and can discard unrelated user configuration edits.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the remaining Kimi MCP lifecycle blockers in 3f91f45.\n\n- Writes managed Kimi child MCP credentials to Kimi's auto-discovered .kimi-code/mcp.json instead of project-root .mcp.json, and excludes that generated path.\n- Removes raw pre-existing MCP config content from renderer/persisted task state.\n- Restores only the owned parallel-code entry so concurrent edits to other MCP servers/settings are preserved.\n- Fails closed before self-landing/merge if the managed token is already present in task Git history.\n\nValidation: npx vitest run electron/mcp/coordinator.test.ts src/store/persistence.test.ts src/store/tasks.test.ts, npm run check, npm run check:static, npm run build:frontend && npm run build:mcp, git diff --check, and the push hook full unit/client tests passed. Hosted CI is running on the pushed head.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Follow-up hardening is in 1508018 after an additional audit of the credential lifecycle:

  • Selects only an untracked Kimi discovery path, falls back to the other supported path, and fails task creation if both are tracked.
  • Refuses a pre-existing parallel-code entry instead of copying it into persisted state; persisted restoration metadata is now path plus fingerprint only.
  • Preserves concurrent edits while removing only the managed entry.
  • Recovers the managed tokens from the mode-0600 per-task config when the discovery file was deleted, so the Git-history guard still runs.
  • Scans Git history without putting either token in process arguments.

Validation: full unit suite (1,775 passed, 23 skipped), client suite (9 passed), npm run check, npm run check:static, targeted MCP/store tests (418 passed), frontend and MCP production builds, and git diff --check.

@LarryHu0217

LarryHu0217 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

The latest quality job was cancelled after waiting 15 minutes in the runner queue and executed zero steps. GitHub Status now confirms an active major Actions outage causing hosted jobs to remain queued or time out, so this is not a code failure. Once Actions recovers, a maintainer rerun will be needed because GitHub returns 403: Must have admin rights to Repository when a PR author tries to rerun an upstream workflow. The local push gate completed successfully on head 1508018: 1,775 unit tests passed (23 skipped), 9 client tests passed, and compile, typecheck, lint, and format checks passed.

@johannesjo

Copy link
Copy Markdown
Owner

Re-reviewed head 1508018. The credential-lifecycle work from rounds 2–4 all landed, and the tests track it closely. Rather than open a fifth round on the same axis, I want to put two questions on the table that change the shape of this PR — plus one functional gap that is a different class of problem entirely.

1. Should this reuse the coordinator's existing mechanism instead of building a parallel one?

deregisterCoordinator already does this exact job for the coordinator's own auto-discovered config: merge only the parallel-code key, remember what we wrote, restore-or-delete on teardown, unlink the file if no servers and no other top-level keys remain. Compare restoreTaskAutoDiscoveredMcpConfig against the existing block — same safeToRestore comparison against what we wrote, same hasServers / hasOtherKeys / unlink tail. This is now a second implementation with its own semantics rather than a generalization of the first.

I'd like to see one shared helper covering both the coordinator's .mcp.json and the child's .kimi-code/mcp.json. That is likely a meaningful reduction of this diff and removes the risk of the two copies drifting.

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 .mcp.json in its own worktree (register.ts), protected by a .git/info/exclude entry and nothing else — no fingerprint persisted across restart, no fail-closed restoration, no git-history token scan. The child path now has all three, because I asked for them one round at a time.

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. .git/info/exclude plus restore-on-teardown is the standard this codebase already ships, and the child path clears it comfortably.

Concretely: I'm withdrawing the escalations from rounds 2–4 as merge-blocking. If you'd rather simplify toward the existing convention — drop assertManagedMcpTokensAbsentFromGitHistory and the fail-closed land/merge coupling, keep the untracked-path selection, the exclude entry, and the restore-only-our-own-entry logic — I'd take that PR. Keeping the current hardening is also fine. What I don't want is for this to keep growing to satisfy a standard I don't hold my own code to.

3. Blocker: Kimi children never receive the sub-task preamble

Different 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.

injectSubTaskPreamble dispatches on substrings of the lowercased command — codex/opencodeAGENTS.md, geminiGEMINI.md, copilot.agent.md. kimi matches none of them, so it falls through to the default Claude Code branch, which writes a systemPrompt key into <worktree>/.claude/settings.local.json. Kimi doesn't read that file — its own default system prompt names AGENTS.md as the project-guidance file it merges hierarchically (system.md), and the kimi-code repo carries nested AGENTS.md files throughout.

The result is that SUB_TASK_MODE_PREAMBLE — the text instructing the child to commit and call land_self, and to use signal_done only when explicitly asked — never reaches a Kimi child. The whole point of the config plumbing is those two tools.

Fix should be one line, adding kimi to the existing AGENTS.md branch. PREAMBLE_MD_FILES already contains AGENTS.md, so land-time stripping needs no change. Please add a test asserting a Kimi child gets an AGENTS.md preamble rather than .claude/settings.local.json — none of the current coverage would catch this, since it all targets config-file mechanics rather than whether the child is instructed at all.

4. Two smaller robustness items

  • coordinator.ts:689writeKimiAutoDiscoveredMcpConfig sits in a bare for loop in setMCPServerInfo with no try/catch, and it throws on a missing worktree (isTrackedGitPathspawnSync error ENOENT), a non-git cwd (status 128), malformed JSON in either candidate, a pre-existing parallel-code entry, or both paths tracked. One task in that state aborts the config rewrite for every remaining sibling of the coordinator, and the throw escapes into the StartMCPServer handler at register.ts:1755, which doesn't catch it. A task can reach that state — cleanupTask returns early and keeps the task in this.tasks when deleteTask fails. Please log-and-continue per task.

  • coordinator.ts:1757 — if the history scan stays, execAsync is promisify(execFile) with no maxBuffer, so Node's 1 MiB default applies to git log --all -p. A project that tracks .mcp.json with real history — precisely the case that pushes the write to .kimi-code/mcp.json and reaches this code — can exceed that and reject with ERR_CHILD_PROCESS_STDIO_MAXBUFFER, which landSelf turns into an opaque escalation. Scoping to ${baseBranch}..HEAD instead of --all is both cheaper and closer to the actual question ("did this task commit the token"); set an explicit maxBuffer either way.

CI

The quality failure is a job cancelled after 15 minutes in the queue with zero steps executed — the Actions outage, not your code. I'll rerun it. I ran npm run typecheck on 1508018 locally and it passes; I did not run the suite this round.


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.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the Kimi child preamble blocker in 7fc70ae.

  • injectSubTaskPreamble now routes Kimi commands to AGENTS.md, matching the file Kimi Code reads for project guidance.
  • Added a regression test that kimi-code --yolo receives an AGENTS.md preamble and does not create .claude/settings.local.json.

Validation passed locally: npm run test:unit -- electron/mcp/preamble.test.ts, npm run check, npm run check:static, npm run build:frontend && npm run build:mcp, git diff --check, and the pre-push full gate (1,776 unit tests / 23 skipped, 9 client tests). Hosted checks are running on the pushed head.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

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 7fc70ae.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:21 vs preamble.ts:88 — the two layers classify Kimi differently: the MCP layer matches basename === 'kimi', the preamble matches substring includes('kimi'). I checked the registry and @moonshot-ai/kimi-code@0.32.0 ships only a kimi bin, so the default path is consistent and nothing is broken today. But preamble.test.ts drives the new test with agentCommand: 'kimi-code --yolo' — a form the MCP layer classifies as non-Kimi, which falls through buildMcpLaunchArgs to ['--mcp-config', configPath] (the exact flag this PR exists to avoid) and skips the auto-discovered write entirely. Either align the two predicates or use kimi in the test, so it stops advertising a form that isn't wired.

  • docker/Dockerfile:53@moonshot-ai/kimi-code@0.32.0 is the only pinned CLI on that line and the reason for the pin (0.33's workspace-trust gate) exists only in dockerfile.test.ts and this thread. Please put a one-line comment at the pin itself; otherwise the next routine bump silently reintroduces the hang. The guard expect(dockerfile).not.toContain('@moonshot-ai/kimi-code ') is also brittle — it keys on a trailing space and passes on a reformatted line.

  • coordinator.ts:2044refreshTaskMcpConfigAfterLandingFailure also 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 — the tokens.length === 0 guard is unreachable: restoreTaskAutoDiscoveredMcpConfig only returns a defined managedEntry when its fingerprint matches an entry we wrote, and every such entry carries both tokens.

  • coordinator.ts:994createTask's catch clears in-memory state and the MCP config but not the worktree/branch already created by createBackendTask. Pre-existing shape, but this PR adds four new throw sites between creation and spawnAgent, 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.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

The latest head 061ec67 includes the remaining hardening from your last review: per-task Kimi config refresh failures are isolated, the token-history scan is scoped to the task branch with an explicit buffer, and only the selected auto-discovery candidate is parsed and validated. The Kimi command/preamble tests and Docker pin guard are also aligned. The branch is clean and ready for a fresh re-review when convenient.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed head 061ec67. CI is green on that exact commit (quality + GitGuardian), and the PR is MERGEABLE/CLEAN.

All three round-4 blockers are genuinely fixed, plus both smaller items I called out:

Round-4 item Status
§1 setMCPServerInfo throws out of a bare loop Fixed — coordinator.ts:687-696 wraps the call in try/catch + logWarn. One bad task no longer skips its siblings, and the throw no longer escapes into register.ts to leave a half-started coordinator. Covered by keeps restarting sibling Kimi configs after one task refresh fails.
§2 history scan: no maxBuffer, --all Fixed — coordinator.ts:1771-1775 now uses ${baseBranch}..HEAD with an explicit 8 MiB maxBuffer. Dropping --all also removes the cross-sibling false positive from the shared subtaskToken.
§3 a problem in the unused candidate hard-blocks every Kimi task Fixed — coordinator.ts:1641-1649 selects the candidate first, then reads and validates only that one. only parses the selected Kimi discovery path asserts the unused path is never parsed.
Dockerfile pin rationale + brittle guard Fixed — comment sits at the pin itself, and the trailing-space not.toContain assertion is replaced with a positive check on the comment.
preamble.test.ts advertising kimi-code --yolo Fixed — the test now drives 'kimi', so it no longer advertises a form the MCP layer classifies as non-Kimi.

One functional issue remains.


An absent managed entry is treated as a fingerprint mismatch, and the two resulting failures deadlock each other

Both the write path and the restore path compare mcpEntryFingerprint(servers['parallel-code']) against the stored fingerprint. undefined hashes to the fingerprint of the literal string 'undefined', so "our entry isn't there" is indistinguishable from "someone replaced our entry". The two sides then fail in opposite directions:

Nothing clears task.autoDiscoveredMcpConfig on failure, and hydration re-supplies the same persisted state after a restart, so the task stays permanently unlandable and unmergeable by the coordinator. Recovery means deleting the task or hand-reconstructing a file whose expected sha256 the user cannot compute.

The likely trigger is one this PR creates: .kimi-code/mcp.json is added to .git/info/exclude, which makes it exactly the kind of file git clean -xfd removes — an ordinary agent build-cleanup step.

The asymmetry is already visible in the code. :1712-1718 handles file missing correctly by falling back to the per-task config; entry missing falls through to the mismatch branch instead.

The narrow fix is to treat managedEntry === undefined as benign in both places — nothing of ours is on disk, so there is nothing to restore and nothing to refuse to overwrite — while keeping fail-closed for an entry that is present but different, since that one may still carry our tokens and the current caution is right there. does not overwrite a Kimi child MCP entry changed after creation covers only the present-but-different case; the absent case needs its own test.

Still open from last round's smaller items

  • coordinator.ts:1766 — the tokens.length === 0 guard is still unreachable dead code.
  • coordinator.ts:1748refreshTaskMcpConfigAfterLandingFailure still runs on the merge success path. Behavior is right, name still isn't.

Minor, new

  • coordinator.ts:1617-1626isTrackedGitPath still spawns git ls-files for both candidates even when priorState already pins one and only that one is used. Also spawnSync here has no timeout, while the sibling helper resolveGitInfoExcludePath uses timeout: 3000.
  • coordinator.ts:1730-1731 — restore unlinks the file when nothing remains after removing our entry, so a user file that was already {} or {"mcpServers":{}} before we touched it gets deleted. The coordinator's own .mcp.json handling tracks mcpFileExistedBefore for exactly this; this path doesn't. Low impact.
  • coordinator.ts:1673-1678 — the appendGitInfoExcludeBlock return value is discarded. On 'failed'/'missing' the token file shows up as untracked and prepareCleanSelfLandingWorktree rejects land_self with a message that names the file without explaining where it came from. A logWarn on any non-appended/present result would make that diagnosable.

Verdict: one fix wanted — the absent-entry case above. Everything else here is cosmetic or low-impact.

The credential-lifecycle design reads well now, and the tests track it closely: restart round-trip, concurrent-edit preservation, refuse-overwrite, both fail-closed land/merge paths, and the new selected-candidate-only parse. Thanks for staying with this through five rounds.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the remaining absent-managed-entry blocker in c5cd4b24.

  • The write path now treats a missing parallel-code entry as recoverable while still refusing a present, changed entry.
  • Restore treats an already-removed managed entry as clean, clears the restoration state, and preserves the historical entry for the existing token-history guard.
  • Added regression coverage for landing after deletion and recreating the managed entry during refresh.

Validation on the exact head:

  • npx vitest run electron/mcp/coordinator.test.ts — 273 passed
  • npm run compile — passed
  • changed-file ESLint, Prettier, and git diff --check — passed
  • hosted quality and GitGuardian checks — passed

@johannesjo

Copy link
Copy Markdown
Owner

Review of 061ec67..c5cd4b2 ("fix(mcp): recover missing Kimi managed entries")

Two production lines, two new tests. The refresh-side change is right; the restore-side change opens a fail-open hole in the token guard.

1. Blocking — restore returns restored with no entry, silently skipping the git-history token check

electron/mcp/coordinator.ts:1724

if (managedEntry === undefined) {
  const historicalManagedEntry = this.readManagedMcpEntryFromTaskConfig(task, state);
  task.autoDiscoveredMcpConfig = undefined;
  this.syncAutoDiscoveredMcpConfig(task);
  return { status: 'restored', managedEntry: historicalManagedEntry };  // may be undefined
}

readManagedMcpEntryFromTaskConfig returns undefined whenever task.mcpConfigPath is gone or its entry no longer matches the recorded fingerprint. Both callers guard the history scan on the entry being present:

if (restoreMcpConfig.managedEntry !== undefined) { await this.assertManagedMcpTokensAbsentFromGitHistory(...) }

(coordinator.ts:1903 in landSelf, coordinator.ts:2015 in mergeTask)

So managedEntry: undefined does not fail closed — it skips assertManagedMcpTokensAbsentFromGitHistory entirely and landing/merge proceeds. The sibling branch 15 lines up (config file missing entirely, coordinator.ts:1713) handles the same situation correctly with if (managedEntry === undefined) return { status: 'failed' }; — the new branch drops that line.

Verified, not theoretical. Running a variant of the new land_self test with git log returning + PARALLEL_CODE_MCP_TOKEN=subtask-token:

  • on c5cd4b2: landSelf resolves and mergeTask is called — a token-bearing history gets merged to main
  • on the parent 061ec67: throws Unable to restore managed Kimi MCP config before self-landing…

This is a regression of the last line of defense — the .git/info/exclude write at coordinator.ts:1674 is best-effort, which is precisely why the history scan exists.

Suggested fix — one line, restoring symmetry with the branch above:

const historicalManagedEntry = this.readManagedMcpEntryFromTaskConfig(task, state);
if (historicalManagedEntry === undefined) return { status: 'failed' };

With that applied, 544/544 existing tests pass; the only casualty is this PR's own new land_self test (see below).

2. The new land_self test codifies the fail-open

electron/mcp/coordinator.test.ts:1591mockExistsSync.mockImplementation((path) => path === configPath) makes the per-task parallel-code-subtask-* config not exist, so the historical entry is unrecoverable and the test asserts landing succeeds anyway. Compare coordinator.test.ts:1638 ("fails closed on token-bearing history even when the discovery config was deleted"), which mocks both paths. Suggest mirroring that mock here so the test proves recovery (entry recovered → history checked → lands), plus a second case asserting failed when neither copy is recoverable.

3. Refresh-side change looks correct

coordinator.ts:1654 — adding existingParallelCode !== undefined to the "refusing overwrite" guard is the right call: a deleted managed key is not user content being clobbered, and the !isManagedCandidate throw at :1645 plus the candidate.tracked throw at :1636 still protect foreign entries and Git-tracked files. The new refresh test covers it well.

4. Nits

  • Once the fix above lands, the two branches have identical bodies — they can collapse into one (const content = existsSync(state.path) ? readMcpJsonContent(state.path) : {}, then a single managedEntry === undefined branch).
  • Recreating a token-bearing file happens silently; a logWarn('coordinator.kimi_mcp', 'managed entry missing; recreating', …) in both new paths would help post-hoc debugging.

Verification performed: read writeKimiAutoDiscoveredMcpConfig, restoreTaskAutoDiscoveredMcpConfig, readManagedMcpEntryFromTaskConfig and both call sites at head; ran coordinator.test.ts at c5cd4b2 (273 passing); added a token-bearing-history test that lands at head and fails closed at the parent; applied the one-line fix and re-ran both suites; tsc --noEmit clean.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the fail-open blocker in 8293bc4f. Restoration now fails closed when the managed entry cannot be recovered, the recovery test uses the historical task config, and a new regression test confirms unrecoverable state escalates without merging. Hosted quality and GitGuardian checks pass on this exact head. Ready for re-review.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — and for the long chain of hardening commits. I want to be clear up front that the containment design is the good part of this PR: fingerprinting what you wrote, refusing to touch a config you didn't write, moving the restore ahead of git add -A in mergeTask, and gating land/merge on a token scan of history is the right shape for the problem. The 17 coordinator tests cover the paths that matter. Most of what's below is about holes in that scheme rather than disagreement with it.

Reviewed at 8293bc4f.


1. Blocking: native mode has no answer to Kimi's workspace-trust gate

docker/Dockerfile pins @moonshot-ai/kimi-code@0.32.0 because "newer releases block fresh worktrees on workspace trust." Every task here gets a fresh worktree, so that's a real concern — but the pin only exists in Docker. Native mode spawns whatever kimi is on the user's PATH, and README.md now lists Kimi Code alongside the other natively-supported CLIs with no caveat.

From Moonshot's changelog:

  • 0.33.0 — "Ask whether to trust the current folder on startup."
  • 0.36.0 — "Show project MCP launch targets in the workspace trust prompt, default to declining trust."

The 0.36 line interacts badly with this PR specifically: writeKimiAutoDiscoveredMcpConfig puts a .kimi-code/mcp.json into every sub-task worktree, and that path isn't Docker-gated. On a current Kimi, a fresh worktree gets a trust prompt that enumerates the project MCP servers and defaults to declining — so native coordinator sub-tasks stall by default, and it's this PR's own MCP file that surfaces the prompt.

--yolo doesn't cover this: it isn't passed unless the user enables skipPermissions (defaultSkipPermissions is false), and Kimi's docs scope it to "trusted working directories" — trust is a precondition for --yolo, not something it grants.

Worth noting the repo already has a weaker mitigation for exactly this shape of problem: agents.ts gives Copilot prompt_ready_delay_ms: 1_000 with a comment naming its folder-trust dialog. The Kimi AgentDef sets no delay.

Either wire up trust pre-seeding for native mode, or scope the README claim to Docker mode. Shipping neither leaves the documented native path broken on any current Kimi install.

(The upstream facts are from Moonshot's published docs/changelog, not from running 0.33+ — happy to be corrected if you've tested it and it behaves differently.)

2. Token containment: three gaps

a. The atomic temp file isn't excluded. atomicWriteFileSync writes join(dirname(filePath), '.parallel-code-atomic-<uuid>.tmp') — so for these configs that's <worktree>/.parallel-code-atomic-*.tmp or <worktree>/.kimi-code/.parallel-code-atomic-*.tmp, containing the full token payload. The exclude block at coordinator.ts:1673 covers only .kimi-code/mcp.json / .mcp.json. A crash between write and rename leaves a git-visible token file that nothing cleans up, that restoreTaskAutoDiscoveredMcpConfig doesn't know about, and that mergeTask's git add -A will stage. Narrow window, one-line fix: add .parallel-code-atomic-*.tmp to the exclude block.

b. The history scan can be defeated by gitattributes. coordinator.ts:1782 runs git log <range> -p --format= -- .mcp.json .kimi-code/mcp.json. If the repo's .gitattributes marks those paths -diff (or binary), git log -p emits Binary files differ instead of content, history.includes(token) is false, and the check passes on a branch that does carry the token. Adding --text (and considering --no-textconv) closes it. Also note git log -p omits merge-commit diffs by default. I'd frame this as hardening rather than a live exploit, but the fix is cheap and the check is load-bearing.

c. One hydration path can drop state and skip the scan entirely. validateAutoDiscoveredMcpConfigState returns undefined on any rejection — including validate(undefined, …) — with no log on any of its four rejection paths, and neither call site logs. In the existing-task branch, coordinator.ts:2261 assigns that undefined over live in-memory state, and the rewriteHydratedSubtaskMcpConfig call at :2266 is not wrapped in try/catch. So a task can end up still in this.tasks, still mergeable, with a token file on disk and recorded state of "none" — at which point restoreTaskAutoDiscoveredMcpConfig returns {status:'none'} and mergeTask skips assertManagedMcpTokensAbsentFromGitHistory altogether.

The realistic trigger isn't tampering, it's crash consistency: the file write at :1666 happens before syncAutoDiscoveredMcpConfig at :1671 → IPC → store → debounced autosave. A hard kill anywhere in that window leaves the token file on disk with nothing persisted.

The new-task branch is fine — its catch deletes the task, so it's a wedge, not a leak. It's specifically the existing-task branch that composes into a missed scan.

3. Two states with no recovery path

a. Orphaned entry wedges hydration permanently. Following on from 2c: once state is lost but the file remains, the next writeKimiAutoDiscoveredMcpConfig hits existingParallelCode !== undefined && !isManagedCandidate and throws already defines mcpServers["parallel-code"]. register.ts doesn't catch, both callers just markTaskMcpError, and Retry re-invokes the identical call for the identical deterministic throw. Only hand-editing the file clears it. The fail-closed instinct is right — the problem is the code can't tell "user's entry" from "our own orphan", and logs nothing either way. A marker field in the written entry would let you distinguish them.

b. The refuse-overwrite branch is also a dead end. :1652-1662 logs and returns without updating task.autoDiscoveredMcpConfig, but the per-task config at task.mcpConfigPath was already rewritten with the new token at :690. State and disk now disagree permanently, so restore hits the fingerprint mismatch and returns 'failed' — and landSelf and mergeTask both refuse forever. Trigger is just an agent editing the parallel-code entry in its own worktree, which isn't exotic.

4. Smaller things

  • isTrackedGitPath spawns twice per call, synchronously. :1618-1625 computes tracked for both candidates via .map before selecting at :1627, so the second spawnSync is always dead work. On restart this runs once per hydrating Kimi task on the Electron main process (via :2383). Lazy evaluation would halve it. Credit where due: :1614 returns early for non-Kimi, so nothing is paid otherwise.
  • The exclude write has a silent failure mode. git-exclude.ts:84-92 returns 'missing' without calling onError when resolveGitInfoExcludePath fails, and coordinator.ts:1673 discards the return value. Writing the exclude before the config would also shrink the window in 2a.
  • Restore narrows the user's file permissions. :1746 rewrites a surviving user .mcp.json with { mode: 0o600 }, and resolveMode honours an explicit mode over the existing one — so a user's 0644 .mcp.json is silently and permanently chmodded to 0600 and reformatted to 2-space JSON.
  • refreshTaskMcpConfigAfterLandingFailure is called on success paths (:2058, after runGitMerge returns). The behaviour is right and the this.tasks.has(taskId) guard correctly skips a cleaned-up task; the name just isn't. Something like reinstateTaskMcpConfig reads better at all six call sites.
  • Four call sites, three error policies. :691 and :1793 catch-and-warn; :1001 fails task creation; :2383 propagates (caught at :2329, uncaught at :2266). The hard-fail choices are defensible — one comment stating the policy would make it look deliberate.
  • dockerfile.test.tsexpect(dockerfile).toContain('# Keep Kimi below 0.33') pins comment prose and breaks on any rewording; the @0.32.0 assertion below it is the one doing real work. Also an odd home for a Dockerfile test under electron/mcp/. And a pin justified by an upstream bug wants a tracking issue, or it quietly ages out.
  • coordinator.ts is now 2920 lines. The Kimi helpers (parseMcpJsonContent, readMcpJsonContent, mcpEntryFingerprint, isTrackedGitPath, validateAutoDiscoveredMcpConfigState) plus the six methods are self-contained and would lift cleanly into electron/mcp/kimi-mcp-config.ts.

5. PRIVACY.md needs updating

PRIVACY.md's "Where the token can land" list is unchanged by this PR, and it's now incomplete. It documents the coordinator's worktree .mcp.json, the OS-temp configs, and the atomic tmp files — but not this PR's new class of location: a token-bearing config inside every Kimi sub-task's worktree. That list is explicitly meant to be exhaustive, so it should gain the new path (and the .git/info/exclude behaviour that goes with it).


Happy to look again once 1 and 2 are addressed. The rest is mostly polish and can land alongside or after.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the requested native-support and token-containment blockers in 9645906.

  • Scoped Kimi support in the README to the Docker-pinned 0.32 path and documented why current native Kimi can pause on workspace trust.
  • Added both the auto-discovered config and adjacent atomic temp-file pattern to .git/info/exclude before credential writes, failing closed if exclusion cannot be established.
  • Hardened the history scan with merge diffs, --text, and --no-textconv.
  • Preserved valid live Kimi restoration state when persisted hydration input is rejected.
  • Updated PRIVACY.md and added regression coverage for each path.

Hosted quality and GitGuardian checks pass on 9645906. Ready for another review when convenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add native support for Kimi CLI / Kimi K2.6 agent

2 participants