From c7d32b1eb7697d77d126249468fa212372de5a7f Mon Sep 17 00:00:00 2001 From: Max Kaye Date: Sun, 19 Jul 2026 19:27:53 +1000 Subject: [PATCH 1/2] fix(kimi): #40 register managed sessions from the launcher, not the hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Managed `c2c start kimi -n ` sessions were unreachable: `c2c send ` returned "alias '' 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 .inbox.json — the inbox `c2c send ` 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. --- CLAUDE.md | 10 +++ ocaml/c2c_start.ml | 110 +++++++++++++++++++++++++- ocaml/c2c_start.mli | 32 ++++++++ ocaml/cli/c2c_hook_cmd.ml | 90 +++++++++++++++++++++- ocaml/cli/test_c2c_hook_kimi.ml | 102 ++++++++++++++++++++++++ ocaml/test/test_c2c_kimi_notifier.ml | 111 +++++++++++++++++++++++++++ 6 files changed, 450 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7f0d9b2f..6373745a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -186,6 +186,16 @@ app-server, OpenCode the plugin API). That is an upstream ask, not our bug. pid-checked) → `C2C_KIMI_SERVER_PORT` → liveness-probed `server.log` record → default 58627. Never trust `server.log` unprobed — kimi writes the `"server listening"` record on cold start only, so it ages into a dead port. + **The SessionStart hook is NOT the identity authority for managed sessions + (#40).** 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 a managed instance's + `C2C_MCP_SESSION_ID` / `C2C_MCP_AUTO_REGISTER_ALIAS`. `c2c start kimi` + therefore registers the alias itself (`register_managed_kimi_session`, + session_id = instance name, cwd + live pid recorded) and the hook *adopts* + that row by cwd + live pid instead of minting a competing alias. Two managed + kimi instances in one directory are indistinguishable to the hook — it bails + loudly rather than guess. Legacy notification-store runbook: `.collab/runbooks/kimi-notification-store-delivery.md` (deprecated). - **OpenCode**: SIGUSR1 to the *inner* OpenCode pid (not the outer wrapper) diff --git a/ocaml/c2c_start.ml b/ocaml/c2c_start.ml index 7300b476..9ffc1e84 100644 --- a/ocaml/c2c_start.ml +++ b/ocaml/c2c_start.ml @@ -2125,7 +2125,8 @@ let read_pid_start_time (pid : int) : int option = with Sys_error _ | End_of_file -> None let eager_register_managed_alias ~(broker_root : string) ~(session_id : string) - ~(alias : string) ~(pid : int) ~(client_type : string) : unit = + ~(alias : string) ~(pid : int) ?(cwd : string option) + ~(client_type : string) () : unit = (* Managed codex-headless needs broker reachability before the bridge has produced a thread id. Ensure the broker dir exists before taking the registry lock so the eager registration path can bootstrap cleanly. *) @@ -2143,7 +2144,14 @@ let eager_register_managed_alias ~(broker_root : string) ~(session_id : string) ; ("client_type", `String client_type) ] @ (match pid_start_time with | Some n -> [ ("pid_start_time", `Int n) ] - | None -> [])) + | None -> []) + (* #40: record the launch cwd. The kimi SessionStart hook uses it to + recognise that a managed instance already owns this workspace (the + hook cannot see the managed identity env — see + [register_managed_kimi_session]). *) + @ (match cwd with + | Some c when String.trim c <> "" -> [ ("cwd", `String c) ] + | _ -> [])) in with_file_lock lock_path (fun () -> let regs = @@ -2171,6 +2179,63 @@ let eager_register_managed_alias ~(broker_root : string) ~(session_id : string) in write_json_file_atomic reg_path (`List (row :: kept))) +(* --------------------------------------------------------------------------- + * #40 — managed kimi broker registration + * + * WHY THE LAUNCHER MUST REGISTER: Kimi Code >= 0.27 runs sessions inside a + * SHARED, long-lived `kimi server` daemon (`~/.kimi-code/server/server.log` + * logs "server already running (pid=..., port=...)" when a second TUI attaches) + * and spawns SessionStart hook commands from THAT daemon's process + * environment. The managed identity vars `c2c start` puts in the TUI's env + * (C2C_MCP_SESSION_ID / C2C_MCP_AUTO_REGISTER_ALIAS, see [build_env]) are + * therefore invisible to `c2c hook kimi`: the hook mints a fresh alias against + * Kimi's real session id, and `c2c send ` reports + * "alias '' is not registered". The hook can never be the identity + * authority for a managed session — one daemon serves many sessions, so its + * env cannot describe any of them. The launcher knows the alias, so the + * launcher registers. + * --------------------------------------------------------------------------- *) + +(* Actionable operator message for a failed managed-kimi registration. Pure so + * the wording is unit-testable. *) +let managed_kimi_registration_failure_message ~(name : string) ~(alias : string) + ~(broker_root : string) ~(reason : string) : string = + Printf.sprintf + "error: c2c start kimi: broker registration for alias '%s' FAILED (%s).\n\ + \ This session is UNREACHABLE: `c2c send %s ...` will report \ + \"alias '%s' is not registered\".\n\ + \ Broker root: %s\n\ + \ Fix: check that the broker dir is writable (`c2c doctor`), then \ + `c2c stop %s && c2c start kimi -n %s`.\n" + alias reason alias alias broker_root name name + +(* Registration is written under the registry flock, then READ BACK. A silent + no-op write is exactly the failure mode #40 was; verifying turns it into a + loud, actionable error. Returns [Error reason] rather than raising so the + launch path decides how loud to be. *) +let register_managed_kimi_session ~(broker_root : string) ~(name : string) + ~(alias : string) ~(pid : int) ~(cwd : string) : (unit, string) result = + match + eager_register_managed_alias ~broker_root ~session_id:name ~alias ~pid + ~cwd ~client_type:"kimi" () + with + | exception e -> Error (Printexc.to_string e) + | () -> + (match + (try + let broker = C2c_mcp.Broker.create ~root:broker_root in + Ok + (List.exists + (fun (r : C2c_mcp.registration) -> + String.lowercase_ascii r.alias + = String.lowercase_ascii alias) + (C2c_mcp.Broker.list_registrations broker)) + with e -> Error (Printexc.to_string e)) + with + | Error e -> Error ("registry read-back failed: " ^ e) + | Ok true -> Ok () + | Ok false -> Error "registry write did not persist the alias") + let instance_dir name = instances_dir // name let per_agent_managed_heartbeats ~(name : string) : managed_heartbeat list = managed_heartbeats_from_toml_path (instance_dir name // "heartbeat.toml") @@ -4426,7 +4491,7 @@ let run_pty_loop ~(name : string) ~(extra_args : string list) (* Register the managed alias *) (try eager_register_managed_alias ~broker_root ~session_id ~alias:name - ~pid ~client_type:"pty" + ~pid ~client_type:"pty" () with e -> Printf.eprintf "warning: registration failed: %s\n%!" (Printexc.to_string e)); (* PTY deliver loop (runs in parent, polling broker and writing to master) *) @@ -5068,8 +5133,38 @@ let run_outer_loop ~(name : string) ~(client : string) ~session_id:name ~alias:(Option.value alias_override ~default:name) ~pid - ~client_type:"codex-headless" + ~client_type:"codex-headless" () with _ -> ())); + (* #40: register the managed kimi alias from the LAUNCHER. The + SessionStart hook cannot do it — Kimi Code >= 0.27 spawns hooks + from the shared `kimi server` daemon, whose env has no + C2C_MCP_AUTO_REGISTER_ALIAS for this instance. See + [register_managed_kimi_session]. Registering here also makes + `-n ` the broker alias (unless --alias/role overrides it), + and must happen BEFORE the notifier is armed below so + [resolve_kimi_notifier_session_id] binds the daemon to + .inbox.json — the inbox `c2c send ` writes to. *) + (if client = "kimi" then begin + let alias = Option.value alias_override ~default:name in + let cwd = try Sys.getcwd () with _ -> "" in + (match + register_managed_kimi_session ~broker_root ~name ~alias ~pid ~cwd + with + | Ok () -> () + | Error reason -> + prerr_string + (managed_kimi_registration_failure_message ~name ~alias + ~broker_root ~reason); + flush stderr); + (* Name/alias distinction is explicit rather than silent: peers + must address the ALIAS, `c2c stop`/`restart` take the NAME. *) + if String.lowercase_ascii alias <> String.lowercase_ascii name then + Printf.eprintf + "note: c2c start kimi: instance name '%s' is not the broker \ + alias — peers must address '%s' (`c2c send %s ...`); \ + `c2c stop`/`c2c restart` still take '%s'.\n%!" + name alias alias name + end); (match thread_id_handoff_path_opt with | Some path -> (* In XML mode the bridge does not start/resume a thread until it sees the first @@ -5186,6 +5281,13 @@ let run_outer_loop ~(name : string) ~(client : string) placeholder would never be recognised and the no-downgrade guard would go inert — letting a correctly-bound daemon be flapped onto a .inbox.json nothing lands in. *) + (* #40 UPDATE: the launcher now writes its own registration + (session_id = [name]) just above, so branch (1) normally + resolves to [name] and the notifier drains .inbox.json — + which is where `c2c send ` puts mail. The hook no longer + mints a competing real-sid identity for managed sessions (it + adopts this row instead), so the real-sid branch survives only + for pre-#40 rows and hook-registered vanilla sessions. *) let real_session_id = resolve_kimi_notifier_session_id ~broker_root ~alias ~cwd:(Sys.getcwd ()) ~fallback:alias () diff --git a/ocaml/c2c_start.mli b/ocaml/c2c_start.mli index 36ba5ba4..8693f67a 100644 --- a/ocaml/c2c_start.mli +++ b/ocaml/c2c_start.mli @@ -85,6 +85,38 @@ val kimi_disabled_for_release : bool val kimi_disabled_notice : string (** List of supported client names. *) +(** [register_managed_kimi_session ~broker_root ~name ~alias ~pid ~cwd] + registers a managed `c2c start kimi` instance on the broker under + [session_id = name] and [alias], recording [cwd] and [pid], then READS the + registry back to confirm the row persisted. [Error reason] on any failure — + the launcher turns that into a loud, actionable operator message + ({!managed_kimi_registration_failure_message}); it never raises. + + Why the launcher and not the SessionStart hook (#40): Kimi Code >= 0.27 + runs sessions inside a SHARED long-lived `kimi server` daemon and spawns + hook commands from that daemon's environment, so the per-instance + [C2C_MCP_SESSION_ID] / [C2C_MCP_AUTO_REGISTER_ALIAS] that {!build_env} puts + in the TUI's env are invisible to `c2c hook kimi`. Before this, every + managed kimi session was registered under a freshly-minted alias against + Kimi's real session id, and `c2c send ` failed with + "alias '' is not registered" — silently, from the operator's side. + One daemon serves many sessions, so its env can never identify any one of + them: the launcher is the only correct identity authority here. *) +val register_managed_kimi_session : + broker_root:string -> + name:string -> + alias:string -> + pid:int -> + cwd:string -> + (unit, string) result + +(** Operator-facing text for a failed managed-kimi registration: names the + unreachable alias, the exact `c2c send` error peers will see, the broker + root, and the recovery command. Pure; exposed so the wording is pinned by + a test rather than drifting. *) +val managed_kimi_registration_failure_message : + name:string -> alias:string -> broker_root:string -> reason:string -> string + (** [pick_live_registration_sid ~alias ~now regs] is the session_id of the MOST-RECENT registration for [alias] that can be corroborated as live (any explicit pid still exists, and a timestamp places it inside the diff --git a/ocaml/cli/c2c_hook_cmd.ml b/ocaml/cli/c2c_hook_cmd.ml index 78dd1b32..d28fd385 100644 --- a/ocaml/cli/c2c_hook_cmd.ml +++ b/ocaml/cli/c2c_hook_cmd.ml @@ -1296,6 +1296,34 @@ let hook_grok : unit Cmdliner.Cmd.t = let kimi_session_events = [ "SessionStart"; "SessionEnd" ] +(* #40: live managed `c2c start kimi` registrations owning [cwd]. + + Kimi Code >= 0.27 runs sessions inside a SHARED long-lived `kimi server` + daemon and spawns hook commands from that daemon's environment, so this hook + CANNOT see the managed session's C2C_MCP_SESSION_ID / + C2C_MCP_AUTO_REGISTER_ALIAS — one daemon serves many sessions, and its env + describes none of them. Minting a fresh alias here therefore produced a + SECOND, competing identity for an already-managed session (and re-keyed its + notifier onto an inbox no mail lands in). The launcher now registers the + managed alias itself; this lookup lets the hook recognise that row by + [cwd] + live [pid] and adopt it instead of minting. + + Match criteria (all required): kimi client_type, NOT hook-registered + (managed rows have no [registered_by]), same [cwd], and a live pid. The pid + is what makes this safe: managed rows always carry one, and a dead pid means + the instance is gone so its row must not be adopted. Pure over [regs] so it + is unit-testable without a broker. *) +let live_managed_kimi_registrations ~(cwd : string) + (regs : C2c_mcp.registration list) : C2c_mcp.registration list = + let pid_alive p = p > 0 && Sys.file_exists (Printf.sprintf "/proc/%d" p) in + List.filter + (fun (r : C2c_mcp.registration) -> + r.client_type = Some "kimi" + && r.registered_by <> Some "kimi-hook" + && r.cwd = Some cwd + && (match r.pid with Some p -> pid_alive p | None -> false)) + regs + let hook_kimi_cmd = let open Cmdliner.Term in const (fun () -> @@ -1352,7 +1380,23 @@ let hook_kimi_cmd = | Some _ -> env_sid | None -> payload_sid in - let session_id = match session_id_opt with Some s -> s | None -> exit 0 in + (* #40: never exit silently here. This bare `exit 0` is what made the + managed-kimi registration gap take a live e2e to find. The hook must + still never fail the host turn, so we log one line and exit 0. *) + let session_id = + match session_id_opt with + | Some s -> s + | None -> + (try + prerr_endline + "c2c hook kimi: no usable session id (payload \ + session_id/sessionId missing or invalid, C2C_MCP_SESSION_ID \ + unset) — skipping broker registration for this SessionStart. \ + This session is unreachable by peers until it registers: run \ + `c2c register` inside it, or launch it with `c2c start kimi`." + with _ -> ()); + exit 0 + in if event = "SessionEnd" then begin let candidates = List.filter_map (fun x -> x) [ env_sid; payload_sid ] in (match @@ -1378,6 +1422,50 @@ let hook_kimi_cmd = leave the session registered-but-DEAF because the alarm guillotined the hook before ensure_daemon ran (#9, A(1)). *) let regs = C2c_mcp.Broker.list_registrations broker in + (* #40: adopt a managed `c2c start kimi` identity when one owns this + workspace, instead of minting a competing alias. See + [live_managed_kimi_registrations] for why the hook cannot simply read + the managed env. Ambiguity (two live managed kimi instances in one + directory) is NOT resolvable from the hook payload — kimi's + session_index only maps session id to workDir — so we bail loudly + rather than guess and hijack the wrong instance's identity. The + managed sessions are already registered by their launchers, so a bail + costs nothing. *) + let hook_cwd = + match payload_string_field payload "cwd" with + | Some c when String.trim c <> "" -> String.trim c + | _ -> + (match payload_string_field payload "workspaceRoot" with + | Some c when String.trim c <> "" -> String.trim c + | _ -> "") + in + let session_id = + if hook_cwd = "" then session_id + else + match live_managed_kimi_registrations ~cwd:hook_cwd regs with + | [ m ] -> + (try + Printf.eprintf + "c2c hook kimi: adopting managed session '%s' (alias '%s') \ + for %s — not minting a new alias.\n%!" + m.session_id m.alias hook_cwd + with _ -> ()); + m.session_id + | _ :: _ as many -> + (try + Printf.eprintf + "c2c hook kimi: %d live managed kimi instances share cwd \ + %s (%s) — cannot tell which one this SessionStart belongs \ + to, so no registration is made here. Those instances are \ + already registered by `c2c start`; to remove the \ + ambiguity run at most one managed kimi per directory.\n%!" + (List.length many) hook_cwd + (String.concat ", " + (List.map (fun (r : C2c_mcp.registration) -> r.alias) many)) + with _ -> ()); + exit 0 + | [] -> session_id + in let already_registered = List.exists (fun (r : C2c_mcp.registration) -> r.session_id = session_id) regs in diff --git a/ocaml/cli/test_c2c_hook_kimi.ml b/ocaml/cli/test_c2c_hook_kimi.ml index 961352c7..0ab5e44f 100644 --- a/ocaml/cli/test_c2c_hook_kimi.ml +++ b/ocaml/cli/test_c2c_hook_kimi.ml @@ -274,6 +274,98 @@ let test_session_start_no_backlog_no_false_nudge () = check bool "no false queued nudge" false (contains ~haystack:skill ~needle:"already queued")) +(* --------------------------------------------------------------------------- + * #40 — the hook and managed sessions + * + * Kimi Code >= 0.27 spawns hooks from the shared `kimi server` daemon, so the + * hook cannot see a managed session's C2C_MCP_SESSION_ID / + * C2C_MCP_AUTO_REGISTER_ALIAS. It must therefore recognise the launcher's + * registration by cwd + live pid and adopt it, rather than minting a second, + * competing identity for the same live session. + * --------------------------------------------------------------------------- *) + +(* Mirrors what `c2c start kimi` writes via + [C2c_start.register_managed_kimi_session]: session_id = instance name, a + live pid, the launch cwd, and NO registered_by (that marks hook rows). *) +let register_managed_session ctx ~alias ~cwd = + let b = C2c_mcp.Broker.create ~root:ctx.broker_root in + C2c_mcp.Broker.register b ~session_id:alias ~alias + ~pid:(Some (Unix.getpid ())) ~pid_start_time:None + ~client_type:(Some "kimi") ~cwd:(Some cwd) ~from_auto_gen:false (); + b + +let test_i40_hook_adopts_managed_session_instead_of_minting () = + with_ctx (fun ctx -> + ignore (register_managed_session ctx ~alias:"zz-i40-managed" ~cwd:"/tmp/proj"); + (* The daemon-spawned hook: real kimi session id in the payload, and NO + managed identity env — exactly the #40 reproduction. *) + let rc, _, stderr = + run_hook ctx + ~payload: + {|{"hook_event_name":"SessionStart","session_id":"session_5f3a2591-0289-4bd3-b214-12c4d6939412","cwd":"/tmp/proj"}|} + in + check int "exit 0" 0 rc; + let regs = list_registrations ctx.broker_root in + check int "no second identity minted" 1 (List.length regs); + let sid, alias = List.hd regs in + check string "managed alias preserved" "zz-i40-managed" alias; + check string "managed session_id preserved" "zz-i40-managed" sid; + check bool "adoption is logged" true + (contains ~haystack:stderr ~needle:"adopting managed session"); + (* Identity skill must name the alias peers can actually address. *) + let skill = read_file (identity_skill_path ctx) in + check bool "skill names the managed alias" true + (contains ~haystack:skill ~needle:"zz-i40-managed")) + +let test_i40_hook_bails_loudly_on_ambiguous_managed_cwd () = + with_ctx (fun ctx -> + ignore (register_managed_session ctx ~alias:"zz-i40-two-a" ~cwd:"/tmp/proj"); + ignore (register_managed_session ctx ~alias:"zz-i40-two-b" ~cwd:"/tmp/proj"); + let rc, _, stderr = + run_hook ctx + ~payload: + {|{"hook_event_name":"SessionStart","session_id":"session_0baa88d1-3c1f-4121-bfb6-676117f52203","cwd":"/tmp/proj"}|} + in + check int "never fails the host turn" 0 rc; + check int "no third identity minted" 2 + (List.length (list_registrations ctx.broker_root)); + check bool "ambiguity is explained, not silent" true + (contains ~haystack:stderr ~needle:"cannot tell which one"); + check bool "names the candidates" true + (contains ~haystack:stderr ~needle:"zz-i40-two-a")) + +(* A dead managed pid means the instance is gone: its row must not be adopted, + so a genuinely new vanilla session still gets its own alias. *) +let test_i40_hook_ignores_dead_managed_registration () = + with_ctx (fun ctx -> + let b = C2c_mcp.Broker.create ~root:ctx.broker_root in + C2c_mcp.Broker.register b ~session_id:"zz-i40-dead" ~alias:"zz-i40-dead" + ~pid:(Some 2147483646) ~pid_start_time:None ~client_type:(Some "kimi") + ~cwd:(Some "/tmp/proj") ~from_auto_gen:false (); + let rc, _, _ = run_hook ctx ~payload:session_start_payload in + check int "exit 0" 0 rc; + let regs = list_registrations ctx.broker_root in + check int "fresh session minted its own identity" 2 (List.length regs); + check bool "the new row is not the dead managed one" true + (List.exists (fun (sid, _) -> sid = session_id) regs)) + +(* The bare `exit 0` on an unresolvable session id is what made #40 invisible. + It must stay exit 0 (never fail the host turn) but say why. *) +let test_i40_unresolvable_session_id_logs_reason_and_exits_0 () = + with_ctx (fun ctx -> + let rc, _, stderr = + run_hook ctx ~payload:{|{"hook_event_name":"SessionStart","cwd":"/tmp/proj"}|} + in + check int "still exits 0" 0 rc; + check bool "logs a reason" true + (contains ~haystack:stderr ~needle:"no usable session id"); + check bool "names the env var it looked for" true + (contains ~haystack:stderr ~needle:"C2C_MCP_SESSION_ID"); + check bool "says what the consequence is" true + (contains ~haystack:stderr ~needle:"unreachable by peers"); + check int "nothing registered" 0 + (List.length (list_registrations ctx.broker_root))) + let () = Random.self_init (); run "c2c_hook_kimi" @@ -291,4 +383,14 @@ let () = ; test_case "SessionStart no backlog no false nudge (#12)" `Quick test_session_start_no_backlog_no_false_nudge ] ) + ; ( "hook_kimi_managed_40" + , [ test_case "adopts managed session instead of minting" `Quick + test_i40_hook_adopts_managed_session_instead_of_minting + ; test_case "ambiguous managed cwd bails loudly" `Quick + test_i40_hook_bails_loudly_on_ambiguous_managed_cwd + ; test_case "dead managed registration is ignored" `Quick + test_i40_hook_ignores_dead_managed_registration + ; test_case "unresolvable session id logs reason, exits 0" `Quick + test_i40_unresolvable_session_id_logs_reason_and_exits_0 + ] ) ] diff --git a/ocaml/test/test_c2c_kimi_notifier.ml b/ocaml/test/test_c2c_kimi_notifier.ml index b0f7aa0d..33e4c922 100644 --- a/ocaml/test/test_c2c_kimi_notifier.ml +++ b/ocaml/test/test_c2c_kimi_notifier.ml @@ -1478,6 +1478,112 @@ let test_b9_resolves_at_spawn_then_rekeys_to_real_sid () = "end state: bound to the real session-id inbox, not the alias" real_sid resolved_now)) +(* --------------------------------------------------------------------------- + * #40 — managed `c2c start kimi` must end up broker-registered under its own + * alias. These model the launcher's real sequence WITHOUT launching kimi: the + * launcher calls [register_managed_kimi_session] with (name, alias, inner pid, + * cwd) and then arms the notifier via [resolve_kimi_notifier_session_id]. Both + * are exactly the calls `c2c start kimi` makes, in order. + * --------------------------------------------------------------------------- *) + +let test_i40_managed_start_registers_instance_name_as_alias () = + with_b9_tmp_dir (fun tmp -> + let broker_root = b9_broker_root tmp in + let name = "zz-i40-managed" in + let cwd = "/proj/i40" in + (* Launcher step 1: register. Our own pid stands in for kimi's inner pid — + it must be a LIVE pid, exactly as at launch time. *) + (match + C2c_start.register_managed_kimi_session ~broker_root ~name ~alias:name + ~pid:(Unix.getpid ()) ~cwd + with + | Ok () -> () + | Error e -> Alcotest.failf "registration failed: %s" e); + let broker = C2c_mcp.Broker.create ~root:broker_root in + let regs = C2c_mcp.Broker.list_registrations broker in + (* This is precisely what `c2c send ` resolves against — the lookup + that returned "alias 'kimi-e2e-b' is not registered" in the #40 e2e. *) + let row = + List.find_opt (fun (r : C2c_mcp.registration) -> r.alias = name) regs + in + (match row with + | None -> + Alcotest.failf "alias %s is not registered after managed start" name + | Some r -> + Alcotest.(check string) "session_id is the instance name" name + r.session_id; + Alcotest.(check (option string)) "cwd recorded for hook adoption" + (Some cwd) r.cwd; + Alcotest.(check (option string)) "client_type kimi" (Some "kimi") + r.client_type); + (* Launcher step 2: arm the notifier. With the registration in place it + must bind to .inbox.json — the inbox `c2c send ` writes. *) + let armed = + C2c_start.resolve_kimi_notifier_session_id ~broker_root ~alias:name + ~cwd:"/nonexistent-workdir" ~fallback:name () + in + Alcotest.(check string) "notifier drains the managed inbox" name armed) + +let test_i40_alias_override_wins_over_instance_name () = + with_b9_tmp_dir (fun tmp -> + let broker_root = b9_broker_root tmp in + let name = "zz-i40-name" and alias = "zz-i40-alias" in + (match + C2c_start.register_managed_kimi_session ~broker_root ~name ~alias + ~pid:(Unix.getpid ()) ~cwd:"/proj/i40" + with + | Ok () -> () + | Error e -> Alcotest.failf "registration failed: %s" e); + let broker = C2c_mcp.Broker.create ~root:broker_root in + let aliases = + List.map (fun (r : C2c_mcp.registration) -> r.alias) + (C2c_mcp.Broker.list_registrations broker) + in + Alcotest.(check bool) "--alias override is the broker alias" true + (List.mem alias aliases); + Alcotest.(check bool) "instance name is NOT also registered" false + (List.mem name aliases)) + +let test_i40_registration_failure_is_loud_and_actionable () = + (* Unwritable broker root: registration must FAIL rather than silently + no-op, and the operator message must name the alias, the exact `c2c send` + error peers hit, and the recovery command. *) + with_b9_tmp_dir (fun tmp -> + let blocked = Filename.concat tmp "no-write" in + Unix.mkdir blocked 0o500; + Fun.protect + ~finally:(fun () -> try Unix.chmod blocked 0o755 with _ -> ()) + (fun () -> + let broker_root = Filename.concat blocked "broker" in + match + C2c_start.register_managed_kimi_session ~broker_root + ~name:"zz-i40-fail" ~alias:"zz-i40-fail" ~pid:(Unix.getpid ()) + ~cwd:"/proj/i40" + with + | Ok () -> + Alcotest.fail + "expected registration to fail on an unwritable broker root" + | Error reason -> + let msg = + C2c_start.managed_kimi_registration_failure_message + ~name:"zz-i40-fail" ~alias:"zz-i40-fail" ~broker_root ~reason + in + let has needle = + let hl = String.length msg and nl = String.length needle in + let rec at i = + i + nl <= hl + && (String.sub msg i nl = needle || at (i + 1)) + in + at 0 + in + Alcotest.(check bool) "names the alias" true (has "zz-i40-fail"); + Alcotest.(check bool) "quotes the peer-visible error" true + (has "is not registered"); + Alcotest.(check bool) "gives a recovery command" true + (has "c2c start kimi -n"); + Alcotest.(check bool) "names the broker root" true + (has broker_root))) + let () = Alcotest.run "c2c_kimi_notifier" [ "notification_id", @@ -1549,4 +1655,9 @@ let () = ; Alcotest.test_case "re-key decision table" `Quick test_b9_rekey_decision_table ; Alcotest.test_case "placeholder is the alias, not the instance name" `Quick test_b9_placeholder_is_alias_not_instance_name ] ) + ; ( "i40-managed-start-registration" + , [ Alcotest.test_case "managed start registers -n as the broker alias" `Quick test_i40_managed_start_registers_instance_name_as_alias + ; Alcotest.test_case "--alias override wins over instance name" `Quick test_i40_alias_override_wins_over_instance_name + ; Alcotest.test_case "registration failure is loud and actionable" `Quick test_i40_registration_failure_is_loud_and_actionable + ] ) ] From cfd2da7ed40f811de4f1c2177cb8921b3cbaff23 Mon Sep 17 00:00:00 2001 From: Max Kaye Date: Sun, 19 Jul 2026 19:49:03 +1000 Subject: [PATCH 2/2] =?UTF-8?q?fix(kimi):=20#40=20review=20follow-ups=20?= =?UTF-8?q?=E2=80=94=20F1=20rekey=20convergence,=20F3=20register=20pre-for?= =?UTF-8?q?k?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — 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/ 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". --- CLAUDE.md | 19 +++- ocaml/c2c_kimi_notifier.ml | 33 +++++-- ocaml/c2c_kimi_notifier.mli | 28 +++++- ocaml/c2c_start.ml | 140 +++++++++++++++++++-------- ocaml/c2c_start.mli | 9 ++ ocaml/cli/c2c_hook_cmd.ml | 74 ++++++++++++-- ocaml/cli/test_c2c_hook_kimi.ml | 40 ++++++++ ocaml/test/test_c2c_kimi_notifier.ml | 70 ++++++++++++-- 8 files changed, 343 insertions(+), 70 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6373745a..c85eba6f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -192,10 +192,21 @@ app-server, OpenCode the plugin API). That is an upstream ask, not our bug. environment, so `c2c hook kimi` cannot see a managed instance's `C2C_MCP_SESSION_ID` / `C2C_MCP_AUTO_REGISTER_ALIAS`. `c2c start kimi` therefore registers the alias itself (`register_managed_kimi_session`, - session_id = instance name, cwd + live pid recorded) and the hook *adopts* - that row by cwd + live pid instead of minting a competing alias. Two managed - kimi instances in one directory are indistinguishable to the hook — it bails - loudly rather than guess. + **before the fork** — the hook can fire the moment the child is up), + session_id = instance name, recording the launch cwd and the OUTER pid + + pid_start_time; the hook *adopts* that row by normalized cwd + live pid pair + instead of minting a competing alias. A failed registration is both printed + and appended to broker.log as `managed_registration_failed` — the TUI paints + over the terminal, so a terminal-only message is not "loud". Because the + launcher's sid IS the alias in the default case, it arms the notifier with + `~authoritative:true` so `decide_notifier_rekey` does not mistake the real + binding for a placeholder and strand a leftover daemon on the wrong inbox. + **Known limits:** two managed kimi instances in one directory are + indistinguishable to the hook (it bails loudly rather than guess), and a + *co-located vanilla* kimi TUI in a managed directory is adopted too — it + never registers its own alias and its identity skill names the managed alias. + Delivery is unaffected (the REST layer is workdir-keyed); nothing in the hook + payload can distinguish these cases. Legacy notification-store runbook: `.collab/runbooks/kimi-notification-store-delivery.md` (deprecated). - **OpenCode**: SIGUSR1 to the *inner* OpenCode pid (not the outer wrapper) diff --git a/ocaml/c2c_kimi_notifier.ml b/ocaml/c2c_kimi_notifier.ml index b41fba51..6fdd5421 100644 --- a/ocaml/c2c_kimi_notifier.ml +++ b/ocaml/c2c_kimi_notifier.ml @@ -927,18 +927,37 @@ let running_session_id alias = placeholder is what stops a later placeholder arm (e.g. a supervisor relaunch) from flapping a correctly-bound daemon back onto the alias. [running = None] is a pre-#9 daemon of unknown binding: bind it as soon as - we have a real sid to bind. Pure; exposed for unit tests. *) -let decide_notifier_rekey ~alias ~requested_sid ~running_sid = + we have a real sid to bind. Pure; exposed for unit tests. + + [authoritative] (#40) breaks the "sid == alias means placeholder" overload. + Since #40 the managed launcher registers session_id = the instance name and + arms the notifier on it, so in the DEFAULT managed case + (alias == name == session_id) the *authoritative* binding is byte-identical + to what this function used to treat as a placeholder. Without the flag a + leftover live notifier for the same alias bound to some other sid — after a + SIGKILLed outer loop or a failed `c2c restart` teardown — would never + converge onto : it falls through to the stale-binary branch and + Skip_current's on an unchanged binary, leaving the session deaf while + `c2c send` reports success (i.e. #40's own symptom, re-introduced). + Callers that KNOW the sid is a real binding rather than a t≈0 guess pass + [~authoritative:true]; the placeholder guard is then skipped and only the + "differs from what is running" test applies. Default [false] preserves the + pre-#40 behaviour for every other caller. *) +let decide_notifier_rekey ~alias ~requested_sid ?(authoritative = false) + ~running_sid () = (* Placeholder compare is case-insensitive, matching how aliases are compared everywhere else (alias comparisons are case-insensitive per B112, and pick_live_registration_sid matches the same way). *) - if String.lowercase_ascii requested_sid = String.lowercase_ascii alias then - false + if + (not authoritative) + && String.lowercase_ascii requested_sid = String.lowercase_ascii alias + then false else match running_sid with | None -> true | Some cur -> cur <> requested_sid -let ensure_daemon ~alias ~broker_root ~session_id ~tmux_pane ?(interval = 2.0) () = +let ensure_daemon ~alias ~broker_root ~session_id ?(authoritative = false) + ~tmux_pane ?(interval = 2.0) () = let pidfile = pidfile_path alias in (* Identity gate FIRST (B145 PID-reuse guard). Treat the pidfile pid as the running notifier ONLY if it is alive AND comm-matches. A dead pid OR a @@ -958,8 +977,8 @@ let ensure_daemon ~alias ~broker_root ~session_id ~tmux_pane ?(interval = 2.0) ( (* nothing of ours running (incl. a just-cleaned stale pidfile) → fresh *) start_daemon ~alias ~broker_root ~session_id ~tmux_pane ~interval () | Some pid when - decide_notifier_rekey ~alias ~requested_sid:session_id - ~running_sid:(running_session_id alias) -> + decide_notifier_rekey ~alias ~requested_sid:session_id ~authoritative + ~running_sid:(running_session_id alias) () -> (* #9 B: live daemon of ours, but bound to the WRONG session_id — it is draining an inbox no mail lands in. Re-key by cycling it onto the requested sid. [pid] is a CONFIRMED-ours notifier (identity-gated diff --git a/ocaml/c2c_kimi_notifier.mli b/ocaml/c2c_kimi_notifier.mli index 04f83436..23f83564 100644 --- a/ocaml/c2c_kimi_notifier.mli +++ b/ocaml/c2c_kimi_notifier.mli @@ -77,9 +77,25 @@ val running_session_id : string -> string option Re-keys iff [requested_sid] differs from what is running AND is not the alias placeholder — refusing to downgrade back to the placeholder is what prevents a later placeholder arm from flapping a correctly-bound daemon. - Pure; exposed for unit tests. *) + Pure; exposed for unit tests. + + [authoritative] (#40) opts out of the placeholder guard. Since #40 the + managed launcher registers [session_id = ] and arms the + notifier on it, so in the default managed case + ([alias = name = session_id]) an AUTHORITATIVE binding is byte-identical to + a placeholder. Without this flag a leftover live notifier bound to a + different sid (SIGKILLed outer loop, failed `c2c restart` teardown) never + converges onto [] — it reaches the stale-binary branch and + [Skip_current]s on an unchanged binary, leaving the session deaf while + `c2c send` reports success. Pass [true] only when the sid is a real + binding rather than a t≈0 guess. Defaults to [false]. *) val decide_notifier_rekey : - alias:string -> requested_sid:string -> running_sid:string option -> bool + alias:string -> + requested_sid:string -> + ?authoritative:bool -> + running_sid:string option -> + unit -> + bool (** [already_running alias] returns [true] iff the notifier pidfile for [alias] names a process that is alive AND whose comm matches the daemon's @@ -125,11 +141,17 @@ val decide_notifier_start : SHA sources can be overridden for tests via [C2C_KIMI_NOTIFIER_FIXTURE_RUNNING_SHA] / - [C2C_KIMI_NOTIFIER_FIXTURE_INSTALLED_SHA]. *) + [C2C_KIMI_NOTIFIER_FIXTURE_INSTALLED_SHA]. + + [authoritative] is forwarded to {!decide_notifier_rekey}: pass [true] when + [session_id] is a known-real binding (the managed launcher's own + registration) rather than a t≈0 placeholder guess, so a leftover daemon + bound elsewhere is re-keyed onto it (#40). *) val ensure_daemon : alias:string -> broker_root:string -> session_id:string -> + ?authoritative:bool -> tmux_pane:string option -> ?interval:float -> unit -> diff --git a/ocaml/c2c_start.ml b/ocaml/c2c_start.ml index 9ffc1e84..cda4dcad 100644 --- a/ocaml/c2c_start.ml +++ b/ocaml/c2c_start.ml @@ -2236,6 +2236,17 @@ let register_managed_kimi_session ~(broker_root : string) ~(name : string) | Ok true -> Ok () | Ok false -> Error "registry write did not persist the alias") +(* #40 F1: may the launcher tell [ensure_daemon] its sid is AUTHORITATIVE (a + real binding) rather than a t≈0 placeholder guess? Only when our own + registration landed AND the notifier resolver actually picked it — if + [resolved_sid] came from the kimi session_index or the bare alias fallback, + it is exactly the placeholder case the #9 no-downgrade guard exists for. + Pure so the condition is pinned by a test instead of living inline in the + launch path. *) +let kimi_notifier_arm_is_authoritative ~(registered_ok : bool) + ~(resolved_sid : string) ~(name : string) : bool = + registered_ok && resolved_sid = name + let instance_dir name = instances_dir // name let per_agent_managed_heartbeats ~(name : string) : managed_heartbeat list = managed_heartbeats_from_toml_path (instance_dir name // "heartbeat.toml") @@ -5034,6 +5045,59 @@ let run_outer_loop ~(name : string) ~(client : string) SigIgn=0x11000 on the client process produced PostToolUse ECHILD on roughly every non-trivial tool call. Forking manually lets us reset the disposition in the child between fork and exec. *) + (* #40 F3: register the managed kimi alias BEFORE forking the client. + Kimi's SessionStart hook can fire as soon as the child is up, so + registering after the fork left a window where the hook saw no managed + row for this cwd, minted a competing alias and armed a second + notifier (a phantom row + duplicate daemon, not deafness — routing + recovers once our row lands). Registering first closes it. + + The pid recorded is the OUTER wrapper's, not the inner client's. That + is deliberate and is the better liveness signal: the outer loop lives + for the whole managed instance and survives `c2c restart` cycling the + inner child, so the row stays adoptable across a restart, whereas an + inner pid would go dead mid-instance. Consumers only ever ask "is this + instance still alive" (hook adoption, pick_live_registration_sid). *) + let kimi_registration_authoritative = ref false in + (if client = "kimi" then begin + let alias = Option.value alias_override ~default:name in + let cwd = try Sys.getcwd () with _ -> "" in + (match + register_managed_kimi_session ~broker_root ~name ~alias + ~pid:(Unix.getpid ()) ~cwd + with + | Ok () -> kimi_registration_authoritative := true + | Error reason -> + let msg = + managed_kimi_registration_failure_message ~name ~alias + ~broker_root ~reason + in + prerr_string msg; + flush stderr; + (* #40 F5: the outer loop's stderr is about to be painted over by + the client's full-screen TUI, so a terminal-only message is + effectively invisible. Also record it durably where + `c2c dev tail-log` / doctor can find it after the fact. *) + (try + Broker_log.append_json ~broker_root + ~json:(`Assoc + [ ("event", `String "managed_registration_failed") + ; ("ts", `Float (Unix.gettimeofday ())) + ; ("client", `String "kimi") + ; ("instance", `String name) + ; ("alias", `String alias) + ; ("reason", `String reason) + ; ("detail", `String (String.trim msg)) ]) + with _ -> ())); + (* Name/alias distinction is explicit rather than silent: peers must + address the ALIAS, `c2c stop`/`restart` take the NAME. *) + if String.lowercase_ascii alias <> String.lowercase_ascii name then + Printf.eprintf + "note: c2c start kimi: instance name '%s' is not the broker \ + alias — peers must address '%s' (`c2c send %s ...`); \ + `c2c stop`/`c2c restart` still take '%s'.\n%!" + name alias alias name + end); let child_pid_opt = try (* S10 (#482): compute pre-deliver hook path for needs_deliver @@ -5135,36 +5199,9 @@ let run_outer_loop ~(name : string) ~(client : string) ~pid ~client_type:"codex-headless" () with _ -> ())); - (* #40: register the managed kimi alias from the LAUNCHER. The - SessionStart hook cannot do it — Kimi Code >= 0.27 spawns hooks - from the shared `kimi server` daemon, whose env has no - C2C_MCP_AUTO_REGISTER_ALIAS for this instance. See - [register_managed_kimi_session]. Registering here also makes - `-n ` the broker alias (unless --alias/role overrides it), - and must happen BEFORE the notifier is armed below so - [resolve_kimi_notifier_session_id] binds the daemon to - .inbox.json — the inbox `c2c send ` writes to. *) - (if client = "kimi" then begin - let alias = Option.value alias_override ~default:name in - let cwd = try Sys.getcwd () with _ -> "" in - (match - register_managed_kimi_session ~broker_root ~name ~alias ~pid ~cwd - with - | Ok () -> () - | Error reason -> - prerr_string - (managed_kimi_registration_failure_message ~name ~alias - ~broker_root ~reason); - flush stderr); - (* Name/alias distinction is explicit rather than silent: peers - must address the ALIAS, `c2c stop`/`restart` take the NAME. *) - if String.lowercase_ascii alias <> String.lowercase_ascii name then - Printf.eprintf - "note: c2c start kimi: instance name '%s' is not the broker \ - alias — peers must address '%s' (`c2c send %s ...`); \ - `c2c stop`/`c2c restart` still take '%s'.\n%!" - name alias alias name - end); + (* #40: the managed kimi alias is registered BEFORE the fork above + (F3) — the SessionStart hook can fire the moment the child is up. + See [register_managed_kimi_session]. *) (match thread_id_handoff_path_opt with | Some path -> (* In XML mode the bridge does not start/resume a thread until it sees the first @@ -5280,18 +5317,42 @@ let run_outer_loop ~(name : string) ~(client : string) role-provided c2c_alias) [name] <> [alias], so a [name] placeholder would never be recognised and the no-downgrade guard would go inert — letting a correctly-bound daemon be - flapped onto a .inbox.json nothing lands in. *) - (* #40 UPDATE: the launcher now writes its own registration - (session_id = [name]) just above, so branch (1) normally - resolves to [name] and the notifier drains .inbox.json — - which is where `c2c send ` puts mail. The hook no longer - mints a competing real-sid identity for managed sessions (it - adopts this row instead), so the real-sid branch survives only - for pre-#40 rows and hook-registered vanilla sessions. *) + flapped onto a .inbox.json nothing lands in. + + #40 SUPERSEDES most of the above for the managed path. The + launcher now writes its OWN registration (session_id = [name]) + before the fork, so branch (1) resolves to [name] and the + notifier drains .inbox.json — exactly where + `c2c send ` puts mail. The hook adopts that row instead + of minting a competing real-sid identity, so the real-sid + branch now only serves pre-#40 rows and hook-registered vanilla + sessions. + + That makes the placeholder reasoning above ACTIVELY WRONG for + the default managed case: with no [alias_override], + [alias = name = session_id], so our authoritative sid is + byte-identical to what decide_notifier_rekey treats as a + placeholder. A leftover live notifier for this alias bound to + some other sid (SIGKILLed outer loop, failed `c2c restart` + teardown) would therefore never converge onto — it would + fall through to the stale-binary branch and Skip_current on an + unchanged binary, leaving the session deaf while `c2c send` + reports success. [~authoritative] says "this sid is a real + binding, not a t≈0 guess", and we only claim it when our + registration actually landed. *) let real_session_id = resolve_kimi_notifier_session_id ~broker_root ~alias ~cwd:(Sys.getcwd ()) ~fallback:alias () in + (* Only authoritative when our registration succeeded AND the + resolver actually picked it: otherwise [real_session_id] came + from the session_index or the bare fallback, which is exactly + the placeholder case the #9 guard exists for. *) + let authoritative = + kimi_notifier_arm_is_authoritative + ~registered_ok:!kimi_registration_authoritative + ~resolved_sid:real_session_id ~name + in (* B145: ensure_daemon (not start_daemon) so a stale notifier left over from a previous binary is cycled onto the new one even on a bare start with no clean stop. Capture the pid + pidfile so the @@ -5299,7 +5360,8 @@ let run_outer_loop ~(name : string) ~(client : string) stop/restart. *) match C2c_kimi_notifier.ensure_daemon - ~alias ~broker_root ~session_id:real_session_id ~tmux_pane () + ~alias ~broker_root ~session_id:real_session_id ~authoritative + ~tmux_pane () with | Some p -> notifier_pid := Some p; diff --git a/ocaml/c2c_start.mli b/ocaml/c2c_start.mli index 8693f67a..3e4dd8b8 100644 --- a/ocaml/c2c_start.mli +++ b/ocaml/c2c_start.mli @@ -110,6 +110,15 @@ val register_managed_kimi_session : cwd:string -> (unit, string) result +(** [kimi_notifier_arm_is_authoritative ~registered_ok ~resolved_sid ~name] — + may the launcher mark its [ensure_daemon] arm AUTHORITATIVE (#40 F1)? + True only when {!register_managed_kimi_session} succeeded AND the notifier + resolver picked that row ([resolved_sid = name]); a sid from the kimi + session_index or the bare alias fallback is the placeholder case the #9 + no-downgrade guard exists for. Pure; exposed for unit tests. *) +val kimi_notifier_arm_is_authoritative : + registered_ok:bool -> resolved_sid:string -> name:string -> bool + (** Operator-facing text for a failed managed-kimi registration: names the unreachable alias, the exact `c2c send` error peers will see, the broker root, and the recovery command. Pure; exposed so the wording is pinned by diff --git a/ocaml/cli/c2c_hook_cmd.ml b/ocaml/cli/c2c_hook_cmd.ml index d28fd385..d011ab17 100644 --- a/ocaml/cli/c2c_hook_cmd.ml +++ b/ocaml/cli/c2c_hook_cmd.ml @@ -1309,19 +1309,69 @@ let kimi_session_events = [ "SessionStart"; "SessionEnd" ] [cwd] + live [pid] and adopt it instead of minting. Match criteria (all required): kimi client_type, NOT hook-registered - (managed rows have no [registered_by]), same [cwd], and a live pid. The pid - is what makes this safe: managed rows always carry one, and a dead pid means - the instance is gone so its row must not be adopted. Pure over [regs] so it - is unit-testable without a broker. *) + (managed rows have no [registered_by]), same [cwd], and a live pid whose + start-time matches the one recorded at registration. + + KNOWN LIMITATION (#40 F2) — this identifies the managed *instance owning the + directory*, NOT the specific Kimi session that fired this hook. Nothing in + the payload can do the latter: kimi's session_index maps session id to + workDir only, and the hook cannot see the managed env. So a **co-located + vanilla** kimi TUI — a bare `kimi` started in a directory that already has a + managed instance — is adopted too: it never registers its own alias and its + identity skill names the managed alias. Delivery is unaffected (the REST + layer is workdir-keyed either way), and the ">= 2 managed" bail below does + not cover this 1-managed + 1-vanilla case. Documented rather than fixed: + 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 a real deafness — not a trade worth + making without knowing when kimi re-mints. + + The pid + pid_start_time pair is an anti-PID-REUSE guard, not proof of + identity: it establishes that the registering instance is still alive, so a + row left behind by a dead instance is never adopted. [pid_start_time] is + corroborated because the launcher records it (via + [Broker.capture_pid_start_time]) and a bare `/proc/` existence check + would happily match an unrelated process that reused the pid. No recency + window is applied — unlike [C2c_start.registration_is_adoptable], whose + 300s bound guards a *notifier binding* — because managed sessions + legitimately run for days and a time bound would stop the hook adopting a + perfectly live instance. Liveness here comes from the pid pair, which does + not decay. Pure over [regs] so it is unit-testable without a broker. *) let live_managed_kimi_registrations ~(cwd : string) (regs : C2c_mcp.registration list) : C2c_mcp.registration list = - let pid_alive p = p > 0 && Sys.file_exists (Printf.sprintf "/proc/%d" p) in + (* #40 F7: normalize both sides. The launcher writes [Sys.getcwd ()] (already + canonical) but kimi's payload cwd is whatever the client passes, so a + trailing slash or a symlinked path would silently defeat the match and + resurrect the competing-alias bug. realpath is best-effort: on failure + fall back to a trailing-slash strip rather than dropping the match. *) + let normalize p = + let p = String.trim p in + let stripped = + let n = String.length p in + if n > 1 && p.[n - 1] = '/' then String.sub p 0 (n - 1) else p + in + try Unix.realpath stripped with _ -> stripped + in + let want = normalize cwd in + let pid_is_live p start_time = + p > 0 + && Sys.file_exists (Printf.sprintf "/proc/%d" p) + && + match start_time with + | None -> true (* pre-#40 row: pid existence is all we have *) + | Some recorded -> ( + match C2c_mcp.Broker.capture_pid_start_time (Some p) with + | Some now -> now = recorded + | None -> false (* unreadable now but recorded then → fail closed *)) + in List.filter (fun (r : C2c_mcp.registration) -> r.client_type = Some "kimi" && r.registered_by <> Some "kimi-hook" - && r.cwd = Some cwd - && (match r.pid with Some p -> pid_alive p | None -> false)) + && (match r.cwd with Some c -> normalize c = want | None -> false) + && (match r.pid with + | Some p -> pid_is_live p r.pid_start_time + | None -> false)) regs let hook_kimi_cmd = @@ -1466,6 +1516,16 @@ let hook_kimi_cmd = exit 0 | [] -> session_id in + (* #40 F6: on adoption this is necessarily true (we adopted an existing + row's session_id), so the whole block below — including + [write_session_statefile] — is skipped. That differs from every other + hook path, which writes a statefile for a session it registered. + Benign here and deliberate: the statefile is a fallback identity hint + for surfaces that have no registration to read, and a managed session + always has one (written by the launcher before the fork), so there is + nothing to fall back to. Writing one would also duplicate identity + state the launcher already owns and would have to be kept in sync + with `c2c rename`. *) let already_registered = List.exists (fun (r : C2c_mcp.registration) -> r.session_id = session_id) regs in diff --git a/ocaml/cli/test_c2c_hook_kimi.ml b/ocaml/cli/test_c2c_hook_kimi.ml index 0ab5e44f..573a4b55 100644 --- a/ocaml/cli/test_c2c_hook_kimi.ml +++ b/ocaml/cli/test_c2c_hook_kimi.ml @@ -349,6 +349,42 @@ let test_i40_hook_ignores_dead_managed_registration () = check bool "the new row is not the dead managed one" true (List.exists (fun (sid, _) -> sid = session_id) regs)) +(* #40 F4: a row whose recorded pid_start_time no longer matches the live + process is a PID-REUSE hit, not our instance — it must not be adopted. + Uses a live pid (our own) with a deliberately wrong start-time, so a bare + `/proc/` existence check would wrongly match. *) +let test_i40_hook_rejects_pid_reuse_row () = + with_ctx (fun ctx -> + let b = C2c_mcp.Broker.create ~root:ctx.broker_root in + C2c_mcp.Broker.register b ~session_id:"zz-i40-reused" ~alias:"zz-i40-reused" + ~pid:(Some (Unix.getpid ())) ~pid_start_time:(Some 1) (* never a real jiffy count *) + ~client_type:(Some "kimi") ~cwd:(Some "/tmp/proj") ~from_auto_gen:false (); + let rc, _, stderr = run_hook ctx ~payload:session_start_payload in + check int "exit 0" 0 rc; + check bool "no adoption of a pid-reuse row" false + (contains ~haystack:stderr ~needle:"adopting managed session"); + let regs = list_registrations ctx.broker_root in + check int "session minted its own identity" 2 (List.length regs); + check bool "the new row is this session" true + (List.exists (fun (sid, _) -> sid = session_id) regs)) + +(* #40 F7: the launcher writes a canonical cwd but kimi's payload cwd is + whatever the client passes. A trailing slash must not defeat the match and + resurrect the competing-alias bug. *) +let test_i40_hook_adoption_normalizes_cwd () = + with_ctx (fun ctx -> + ignore (register_managed_session ctx ~alias:"zz-i40-slash" ~cwd:"/tmp/proj"); + let rc, _, stderr = + run_hook ctx + ~payload: + {|{"hook_event_name":"SessionStart","session_id":"session_5f3a2591-0289-4bd3-b214-12c4d6939412","cwd":"/tmp/proj/"}|} + in + check int "exit 0" 0 rc; + check bool "trailing slash still matches the managed row" true + (contains ~haystack:stderr ~needle:"adopting managed session"); + check int "no second identity minted" 1 + (List.length (list_registrations ctx.broker_root))) + (* The bare `exit 0` on an unresolvable session id is what made #40 invisible. It must stay exit 0 (never fail the host turn) but say why. *) let test_i40_unresolvable_session_id_logs_reason_and_exits_0 () = @@ -390,6 +426,10 @@ let () = test_i40_hook_bails_loudly_on_ambiguous_managed_cwd ; test_case "dead managed registration is ignored" `Quick test_i40_hook_ignores_dead_managed_registration + ; test_case "pid-reuse row is rejected (F4)" `Quick + test_i40_hook_rejects_pid_reuse_row + ; test_case "adoption normalizes cwd (F7)" `Quick + test_i40_hook_adoption_normalizes_cwd ; test_case "unresolvable session id logs reason, exits 0" `Quick test_i40_unresolvable_session_id_logs_reason_and_exits_0 ] ) diff --git a/ocaml/test/test_c2c_kimi_notifier.ml b/ocaml/test/test_c2c_kimi_notifier.ml index 33e4c922..93072d77 100644 --- a/ocaml/test/test_c2c_kimi_notifier.ml +++ b/ocaml/test/test_c2c_kimi_notifier.ml @@ -1385,18 +1385,50 @@ let test_b9_prefers_most_recent_live_registration () = let test_b9_rekey_decision_table () = let d = C2c_kimi_notifier.decide_notifier_rekey ~alias:"zz-kimi-a" in Alcotest.(check bool) "placeholder -> real sid re-keys" true - (d ~requested_sid:"real-sid" ~running_sid:(Some "zz-kimi-a")); + (d ~requested_sid:"real-sid" ~running_sid:(Some "zz-kimi-a") ()); Alcotest.(check bool) "unknown binding + real sid re-keys" true - (d ~requested_sid:"real-sid" ~running_sid:None); + (d ~requested_sid:"real-sid" ~running_sid:None ()); Alcotest.(check bool) "real -> different real re-keys" true - (d ~requested_sid:"real-sid-2" ~running_sid:(Some "real-sid-1")); + (d ~requested_sid:"real-sid-2" ~running_sid:(Some "real-sid-1") ()); Alcotest.(check bool) "same sid does not re-key" false - (d ~requested_sid:"real-sid" ~running_sid:(Some "real-sid")); + (d ~requested_sid:"real-sid" ~running_sid:(Some "real-sid") ()); Alcotest.(check bool) "never downgrades a real sid to the alias placeholder" false - (d ~requested_sid:"zz-kimi-a" ~running_sid:(Some "real-sid")); + (d ~requested_sid:"zz-kimi-a" ~running_sid:(Some "real-sid") ()); Alcotest.(check bool) "placeholder arm over unknown binding is a no-op" false - (d ~requested_sid:"zz-kimi-a" ~running_sid:None) + (d ~requested_sid:"zz-kimi-a" ~running_sid:None ()) + +(* #40 F1 — the managed convergence case the placeholder guard used to eat. + Post-#40 the DEFAULT managed binding is alias == name == session_id, so an + authoritative arm is byte-identical to a placeholder. A leftover live + notifier bound elsewhere (SIGKILLed outer loop, failed `c2c restart` + teardown) must still converge onto , or `c2c send` succeeds into an + inbox nothing drains — #40's own symptom. *) +let test_i40_authoritative_rekeys_managed_alias_binding () = + let alias = "zz-i40-managed" in + let d = C2c_kimi_notifier.decide_notifier_rekey ~alias in + (* The regression: without ~authoritative the request looks like a + placeholder and is refused, so the stale binding survives. *) + Alcotest.(check bool) + "pre-#40 semantics: an alias-shaped request is treated as a placeholder" + false + (d ~requested_sid:alias ~running_sid:(Some "session_stale-0baa88d1") ()); + (* The fix: the launcher knows this sid is its own registration. *) + Alcotest.(check bool) + "authoritative arm re-keys a leftover daemon onto the managed inbox" true + (d ~requested_sid:alias ~authoritative:true + ~running_sid:(Some "session_stale-0baa88d1") ()); + Alcotest.(check bool) + "authoritative arm over an unknown binding also binds" true + (d ~requested_sid:alias ~authoritative:true ~running_sid:None ()); + (* Still idempotent: a correctly-bound daemon is never cycled for nothing. *) + Alcotest.(check bool) "authoritative arm on an already-correct binding is a no-op" + false + (d ~requested_sid:alias ~authoritative:true ~running_sid:(Some alias) ()); + (* And the #9 guarantee is untouched for non-authoritative callers: the hook + must never flap a real binding back onto the alias placeholder. *) + Alcotest.(check bool) "non-authoritative downgrade still refused" false + (d ~requested_sid:alias ~running_sid:(Some "session_real-5f3a2591") ()) (* The no-downgrade guard recognises a placeholder by comparing the requested sid against the ALIAS. With `c2c start kimi -n foo --alias bar` (or a @@ -1410,18 +1442,18 @@ let test_b9_placeholder_is_alias_not_instance_name () = Alcotest.(check bool) "alias placeholder IS recognised (no downgrade)" false (C2c_kimi_notifier.decide_notifier_rekey ~alias ~requested_sid:alias - ~running_sid:(Some "real-sid")); + ~running_sid:(Some "real-sid") ()); Alcotest.(check bool) "alias placeholder recognised case-insensitively" false (C2c_kimi_notifier.decide_notifier_rekey ~alias ~requested_sid:(String.uppercase_ascii alias) - ~running_sid:(Some "real-sid")); + ~running_sid:(Some "real-sid") ()); (* Guard against the regression: were the instance NAME used as the placeholder, it would not be recognised and would flap the daemon. *) Alcotest.(check bool) "instance name is NOT a placeholder — why fallback must be the alias" true (C2c_kimi_notifier.decide_notifier_rekey ~alias ~requested_sid:name - ~running_sid:(Some "real-sid")) + ~running_sid:(Some "real-sid") ()) (* THE ORDERING TEST the fix actually has to pass: reproduce production sequencing rather than seeding the registration up front. @@ -1468,7 +1500,7 @@ let test_b9_resolves_at_spawn_then_rekeys_to_real_sid () = Alcotest.(check bool) "t1: hook's real sid re-keys the placeholder-bound notifier" true (C2c_kimi_notifier.decide_notifier_rekey ~alias - ~requested_sid:real_sid ~running_sid:(Some armed_sid)); + ~requested_sid:real_sid ~running_sid:(Some armed_sid) ()); (* End state: the notifier drains the REAL session-id inbox. *) let resolved_now = C2c_start.resolve_kimi_notifier_session_id ~broker_root ~alias @@ -1544,6 +1576,22 @@ let test_i40_alias_override_wins_over_instance_name () = Alcotest.(check bool) "instance name is NOT also registered" false (List.mem name aliases)) +(* #40 F1: the launcher may only claim AUTHORITATIVE for its own registration. + Claiming it for a session_index-derived or fallback sid would disable the #9 + no-downgrade guard for exactly the case it was written for. *) +let test_i40_authoritative_claim_is_narrow () = + let f = C2c_start.kimi_notifier_arm_is_authoritative in + Alcotest.(check bool) "our registration resolved → authoritative" true + (f ~registered_ok:true ~resolved_sid:"zz-i40" ~name:"zz-i40"); + Alcotest.(check bool) "registration failed → never authoritative" false + (f ~registered_ok:false ~resolved_sid:"zz-i40" ~name:"zz-i40"); + Alcotest.(check bool) + "resolver picked a real kimi sid (session_index) → not our binding" false + (f ~registered_ok:true ~resolved_sid:"session_5f3a2591" ~name:"zz-i40"); + Alcotest.(check bool) + "resolver fell back to an --alias override → not our binding" false + (f ~registered_ok:true ~resolved_sid:"zz-i40-alias" ~name:"zz-i40") + let test_i40_registration_failure_is_loud_and_actionable () = (* Unwritable broker root: registration must FAIL rather than silently no-op, and the operator message must name the alias, the exact `c2c send` @@ -1653,11 +1701,13 @@ let () = ; Alcotest.test_case "dead pid registration not adopted" `Quick test_b9_dead_pid_registration_not_adopted ; Alcotest.test_case "prefers most-recent live registration" `Quick test_b9_prefers_most_recent_live_registration ; Alcotest.test_case "re-key decision table" `Quick test_b9_rekey_decision_table + ; Alcotest.test_case "#40 F1: authoritative arm re-keys managed binding" `Quick test_i40_authoritative_rekeys_managed_alias_binding ; Alcotest.test_case "placeholder is the alias, not the instance name" `Quick test_b9_placeholder_is_alias_not_instance_name ] ) ; ( "i40-managed-start-registration" , [ Alcotest.test_case "managed start registers -n as the broker alias" `Quick test_i40_managed_start_registers_instance_name_as_alias ; Alcotest.test_case "--alias override wins over instance name" `Quick test_i40_alias_override_wins_over_instance_name ; Alcotest.test_case "registration failure is loud and actionable" `Quick test_i40_registration_failure_is_loud_and_actionable + ; Alcotest.test_case "authoritative claim is narrow" `Quick test_i40_authoritative_claim_is_narrow ] ) ]