fix(kimi): the launcher registers managed sessions; the hook adopts instead of minting (#40)#46
Merged
Merged
Conversation
Managed `c2c start kimi -n <name>` sessions were unreachable: `c2c send <name>` returned "alias '<name>' is not registered" (3/3 in a live e2e). Root cause (not what the issue guessed): the SessionStart hook DID fire and DID register — under a freshly-minted alias against Kimi's real session id. Kimi Code >= 0.27 runs sessions inside a shared, long-lived `kimi server` daemon and spawns hook commands from THAT daemon's environment, so `c2c hook kimi` cannot see the managed instance's C2C_MCP_SESSION_ID / C2C_MCP_AUTO_REGISTER_ALIAS. One daemon serves many sessions, so its env can never identify any of them: the hook is structurally incapable of being the identity authority for a managed session. Broker evidence for all three e2e starts, plus the bundled hook runner's `env: options.env` (undefined for our TOML block, so Node inherits the daemon's env), is in .collab/findings/2026-07-19T09-40-00Z-claude-kimi-managed-sessions-never-register.md (gitignored dir). - c2c_start.ml: new `register_managed_kimi_session` — registers session_id = instance name, alias = `-n` (or --alias/role override), recording the launch cwd and inner pid, then READS THE REGISTRY BACK. Called from the kimi branch of the launch path BEFORE the notifier is armed, so `resolve_kimi_notifier_session_id` binds the daemon to <name>.inbox.json — the inbox `c2c send <alias>` writes to. A failure prints an actionable message (alias, peer-visible error, broker root, recovery command) instead of leaving the session silently unreachable. A name/alias divergence is now stated explicitly rather than silently. - c2c_hook_cmd.ml: the hook adopts a live managed registration owning its cwd instead of minting a competing identity (which also re-keyed the notifier onto an inbox no mail lands in). Two live managed instances in one directory are indistinguishable from the hook payload, so it bails with an explanation rather than hijacking the wrong one. - c2c_hook_cmd.ml: the bare `exit 0` on an unresolvable session id now logs one stderr line (what was missing, the consequence, the recovery). Still exit 0 — the hook must never fail the host turn. That silence is why this took a live e2e to find. Tests (hermetic, no live kimi, no forked notifiers): managed-start registration + alias-override + loud-failure wording in test_c2c_kimi_notifier; hook adoption, ambiguity bail, dead-row rejection and the loud unresolvable-sid path in test_c2c_hook_kimi. The three behavioural hook tests were confirmed RED against the pre-fix binary.
…pre-fork Review came back PASS-WITH-NITS. F1 is a real regression the first commit introduced; the rest are hardening and documentation. F1 (MUST FIX). #9's `decide_notifier_rekey` treats `requested_sid == alias` as a placeholder and refuses to re-key. Post-#40 the DEFAULT managed binding is `alias == name == session_id`, so the authoritative binding became byte-identical to a placeholder: a leftover live notifier for the same alias bound to a different sid (SIGKILLed outer loop, failed `c2c restart` teardown) would never converge onto <name> — it falls through to the stale-binary branch and Skip_current's on an unchanged binary. Symptom is #40's own symptom, i.e. the fix re-introduced it on a narrower path. Fixed by NOT overloading "sid == alias" any further: an explicit `?authoritative` flag (c2c_kimi_notifier.ml:947, threaded through ensure_daemon at :981) skips the placeholder guard, and the launcher claims it only when its own registration landed AND the resolver picked that row (`kimi_notifier_arm_is_authoritative`, c2c_start.ml:2255; used at :5348). The stale placeholder comment at c2c_start.ml:5302-5341 is rewritten to say which half of it #40 supersedes and why. F3. The managed registration moved BEFORE the client fork (c2c_start.ml:5037) — kimi's SessionStart hook can fire the moment the child is up, and the old post-fork placement left a window where the hook saw no managed row, minted a competing alias and armed a second notifier. The pid recorded is now the OUTER wrapper's: deliberate, and the better liveness signal, since the outer loop survives `c2c restart` cycling the inner child. F4. Adoption corroborates `pid_start_time` against the live process (c2c_hook_cmd.ml:1333) instead of a bare /proc/<pid> existence check, closing PID reuse — the launcher records it, so the data was already there. Deliberately NO recency window: unlike its notifier-binding sibling, managed sessions legitimately run for days and a bound would stop the hook adopting a live instance. F5. A failed registration is now also appended to broker.log as `managed_registration_failed` (c2c_start.ml:5069). The client's full-screen TUI paints over the outer loop's stderr immediately, so a terminal-only message is not actually loud. F2 (documented, not fixed). Adoption identifies the managed instance owning the DIRECTORY, not the session that fired the hook, so a co-located vanilla TUI is adopted too. The comment claiming the pid "is what makes this safe" is corrected — the pid pair is an anti-PID-reuse guard, not proof of identity. The obvious fix (first-wins claim of the payload sid on the managed row) would silently strand a managed session that ever re-mints its session id, trading a cosmetic wrong-identity for real deafness; not worth it without knowing when kimi re-mints. F6 comment on the deliberately-skipped statefile write on adoption. F7 cwd normalization (realpath + trailing-slash strip) on both sides of the match. Tests: the F1 regression is pinned by a rekey-decision test proving the managed convergence case, plus a narrow-claim test for the authoritative condition; F4 pid-reuse rejection and F7 trailing-slash adoption added to the hook suite. F1 confirmed RED by neutering the flag (pre-F1 semantics): 1 failure, "authoritative arm re-keys a leftover daemon onto the managed inbox".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #40: managed
c2c start kimi -n <name>sessions never registered on the broker, soc2c send <name>failed outright withalias '<name>' is not registered.Root cause — both of the issue's hypotheses were wrong
The hook does fire and does register. It registers the wrong identity.
Kimi Code ≥0.27 is a client/server split: sessions live in a shared, long-lived
kimi serverdaemon, and that daemon spawns hook processes inheriting its environment. The managed identity varsc2c startsets on the TUI are therefore invisible toc2c hook kimi, which falls through to auto-minting a competing alias. This is structural, not a race — one daemon serves many sessions, so its env cannot identify any of them. The hook can never be the identity authority for a managed session; the launcher is the only component that knows-n.Evidence: three managed starts produced three hook registrations and zero managed aliases.
Changes
session_id = <name>,alias = -n(or--alias/role override), recording launch cwd + outer pid, then reading the registry back to confirm. Failure is loud and actionable (and also written to broker.log — a message the full-screen TUI paints over isn't loud).-nis the alias; divergence from--alias/role is stated explicitly.exit 0on unresolvable session id now logs one stderr line (still exit 0).Review round (PASS-WITH-NITS; all folded in)
The reviewer caught an interaction bug between this fix and #9: post-fix the managed case is
alias == name == session_id, butdecide_notifier_rekeytreatedrequested_sid == aliasas a placeholder and refused to re-key — silently disabling #9's convergence guard for exactly the configuration this issue targets (symptom: send succeeds, session deaf, after a SIGKILLed outer loop). Fixed with an explicit?authoritativeflag, claimed only when the launcher's own registration landed and the resolver picked that row — so a session_index sid or bare-alias fallback stays non-authoritative and #9's guarantee is untouched. Proven red by neutering the flag.Also: registration moved before the fork (F3);
pid_start_timecorroboration on adoption (F4); failures mirrored to broker.log (F5); cwd realpath-normalized (F7). The reviewer's suggested "first-wins sid claim" for the co-located-vanilla case was deliberately declined — it would fix a cosmetic wrong-identity but could strand a managed session that re-mints its session id, trading cosmetic for real deafness on an unknown.Verification — proven in the wild
Unit:
test_c2c_hook_kimi12,test_c2c_kimi_notifier52,test_c2c_setup_kimi22,test_c2c_doctor_hooks46,test_c2c_start207 — all green, built-first, fork-free. Red/green confirmed (3 of 4 new hook tests fail with the fix reverted).Live tmux e2e (managed launch,
kimi-code/k3):session_id=i40test,alias=i40test, launcher-registered — exactly one row.c2c send i40test→ok(pre-fix:not registered), delivery confirmed via REST state (last_promptcarries the envelope,busy: true) — 3.9s cold, 2.19s / 2.23s warm.adopting managed session 'i40test' … — not minting a new alias.i40test.inbox.json, drained to[].Known follow-ups (filed separately, not blockers)
Teardown strips
pid/pid_start_timefrom the managed row, making it unadoptable — so #40 can resurface after an unclean exit. The in-session MCP server registers a competing row because~/.kimi-code/mcp.jsonpins one global install-time alias.c2c restarton kimi fails on a 5s outer-exit timeout (pre-existing; the diff touches no pidfile/timeout code).Closes #40.