fix(omp): keep parent identity across task subagent dispose

[mmkzer0]

Summary

Encountered hcom identity drops in omp when subagents were used; on subagent exit they would take the parent identity with them.

• Nested OMP task extensions no longer bind/stop the parent (HCOM_OMP_IDENTITY_OWNER + ownsIdentity; soft --soft stop only for the identity owner).
• Soft-finalize keeps the instance row; recover_process_binding_for_instance rebuilds process/session bindings via HCOM_INSTANCE_NAME after soft-stop.
• Split hooks/omp.rs into hooks/omp/{mod,handlers,plugin,tests} so the hook side is modular, more readable and generally nicer to work with.

Help and suggestions on this welcome, been a bit of a grind to break up the big hooks file, but tests all pass and live usage works.

Out of scope / follow-ups

• Busy-path delivery still uses deliverAs: "followUp" (messages wait until the current turn ends instead of steering mid-run) — separate change.
• Optional polish from review: DRY recover into a shared rebind helper; replace the keepOwner boolean with clearer session-state vs identity-release APIs.

Test plan

• cargo fmt
• cargo clippy -p hcom -- -D warnings
• cargo test (including hooks::omp)
• Live usage over a few days: parent identity no longer drops under multi-task subagent use

[aannoo]

@mmkzer0 thanks for the fix - not sure if this review is valid you can decide what to do:

PR #97 review — fix(omp): keep parent identity across task subagent dispose

Head: c3c489c3 · Verdict: request changes — do not merge as written.

The bug is real and the core policy is right. The chosen mechanism is not: it converts terminal owner shutdown into a soft, recoverable stop, then rebuilds identity from a name. That trades one identity drop for a stale-row regression plus a re-openable ownership window.

The real bug (premise is valid)

OMP task subagents are separate AgentSessions in the same Node process. task/executor.ts forwards the parent's preloadedExtensionPaths into each child createAgentSession() with hasUI:false + taskDepth/agentId/parentAgentId (executor.ts:2564–2591), then emits a fresh session_start. So a child hcom extension re-binds against the parent's HCOM_PROCESS_ID and, on child dispose, can stop the parent's identity. Duplicate in-process extension instances are expected OMP behavior. The fix must make hcom opt out in sub-sessions.

Timing nuance: keepAlive defaults true (executor.ts:2810), so most successful tasks are retained, not immediately disposed. The trigger is child disposal/unwind (abort, one-shot, isolated, park/TTL, shutdown), not every task completion.

Blocking

B1 — Owner shutdown uses soft-stop → stale row + lost PTY life event

session_shutdown now runs omp-stop --soft for the owner (hcom.ts:409–434). soft_finalize_session deletes the process binding (DELETE FROM process_bindings WHERE session_id = ?, common.rs). But normal OMP exit goes through the default PTY cleanup, which decides ownership by reading that binding:

• instance_owns_process_binding() returns false when get_process_binding is Ok(None) (delivery.rs).
• cleanup_pty_exit_default() then skips cleanup_deleted_instance — logs "name reassigned to new process" (delivery.rs:2061–2078).

Net: on ordinary OMP exit the instance row is never deleted and no life stopped(pty) event fires. Normal exit now leaks a stale inactive row and depends on later name reuse to clean up. session_shutdown is documented as "Fired on process exit (SIGINT/SIGTERM)" and carries no sessionId (shared-events.ts:92) — this is the terminal event, and it should stay a hard stop for the owner. Hard-stop is also more robust: PTY cleanup still holds the authoritative process binding if the stop command itself fails.

B2 — Ownership guard is released before child task teardown

AgentSession.#doDispose() awaits session_shutdown handlers first (agent-session.ts:6885–6887), then cancels/settles owned async jobs (#disposeOwnedAsyncJobs, 6910). The root handler soft-stops and calls resetBinding(), which deletes HCOM_OMP_IDENTITY_OWNER (hcom.ts:388–394). The nested-skip is not sticky: bindIdentity skips only while the env latch is present, and it is re-entered from agent_start, input, before_agent_start, tool_call, tool_result, turn_end, agent_end (hcom.ts:457–545). So a child event still in flight after the root cleared the latch re-binds, becomes the new owner, and can stop the identity during its own dispose — reproducing the exact takeover the PR prevents, on a successful shutdown, no stop-failure required. A skipped sub-extension must be permanently non-participating for the process lifetime.

B3 — Recovery reports success after partial/total DB failure

recover_process_binding_for_instance (instance_binding.rs) does four writes — clear session id from other rows, update session id, rebind_session, set_process_binding — logs and ignores every error, then unconditionally returns Some(name). Its only failure mode is a missing row. handle_start treats that Some as admission: marks the instance listening, writes tool/session metadata, registers the plugin notify endpoint, returns the name to the extension (handlers.rs:116–176). A notify endpoint can make the launch look ready while process-based lookup, delivery ownership, and PTY cleanup disagree. If kept, it must be one transaction returning Result with handle_start surfacing failure. (In the supported single-root topology its actual trigger is unclear — after a terminal soft-stop there is no same-process re-start, and switch/branch keep the binding — so this reads as unsafe defensive code whose only necessity is B1's soft-stop.)

B4 — Name treated as proof of identity

When no process binding exists, handle_start falls back to HCOM_INSTANCE_NAME then HCOM_NAME (instance_name_from_env, handlers.rs:64–70) and recovers any row with that name — no check that it's OMP, inactive, this OS process, this extension, or unclaimed. HCOM_NAME is a user-configurable export used elsewhere for diagnostics, not an ownership credential. Recover from an hcom-issued lifecycle token with transactional agreement, or not at all. Restoring hard owner shutdown removes the need.

Secondary (real, non-blocking)

• Env latch leaks to child OS processes. HCOM_OMP_IDENTITY_OWNER is described as same-process but is a plain env var; it is not in HCOM_IDENTITY_VARS (shared/constants.rs:54), so a nested hcom-OMP launch inherits it and skips its own legitimate bind. Marginal reachability (requires launching OMP from inside OMP) but the "process-local" claim is false. A globalThis[Symbol.for(...)] registry would actually have the claimed scope.
• Failed/killed soft-stop still releases ownership. resetBinding() runs unconditionally after a non-zero or thrown stop, clearing the latch and re-opening B2's window.
• hcom() helper masks signal kills and has no timeout. code: code ?? 0 maps a signal-terminated child (close code null) to success (hcom.ts:37–52); a killed omp-stop reads as clean, a killed/empty omp-start can parse {} and publish owner "1" with no instance name. OMP caps each session_shutdown handler at 2000 ms (SESSION_SHUTDOWN_HANDLER_TIMEOUT_MS, extensions/runner.ts:88) and abandons — but does not kill — the subprocess on timeout, so a slow stop can mutate the latch after OMP has moved on. Add timeout/kill and treat code === null as failure.
• Reconcile timer leaks; managed API ignored. reconcileTimer is a raw setInterval (hcom.ts:371), started at every session_start including skipped nested ones (hcom.ts:399–403), and never clearInterval'd anywhere — resetBinding clears only the idle timer. OMP provides ctx.setInterval with auto-clear on shutdown (extensions/types.ts:452). Only the owner should reconcile, via the managed timer.

Design

"First binder wins" is an ordering heuristic, not an identity model, and the claim isn't even atomically implemented: bindIdentity reads the env latch before two awaits (notify server + omp-start) and publishes ownership only after, so two instances can both bind. In the current supported topology this is contained — hcom rejects omp acp/--mode/print/--no-extensions, and OMP awaits session_start before a task can launch — so it's a future/embedding hazard, not a supported-path repro. The soft-stop + name-recovery + keepOwner machinery is circular: shutdown is made non-terminal, authoritative bindings are deleted, then reconstructed by name, then a global latch is added to stop children from entering that reconstruction. The information needed already exists upstream — OMP computes agentKind from taskDepth/parentTaskPrefix (sdk.ts:1621) but ExtensionContext exposes only hasUI/sessionManager (extensions/types.ts:410), not taskDepth/agentKind. That API gap is the true root; it does not justify first-binder ownership as a durable abstraction.

Scope

The hooks/omp.rs → mod/handlers/plugin/tests split is a reasonable mechanical refactor, but it should land separately from the lifecycle change: it is most of the +1296/−1032 diff and makes the semantic change hard to isolate or revert. The stray list.rs clippy edit is unrelated noise.

Tests

Insufficient for the behavior change. The ownership/soft-stop/nested logic is asserted almost entirely via PLUGIN_SOURCE.contains("…") against the include_str!'d TS, which is never type-checked or executed (tests.rs). Those pass while B2 ordering, the timer leak, B3 error handling, and env inheritance are all broken. Rust side has a happy-path recover test and a soft-finalize-keeps-row test — no failure injection. Needs an executable extension harness: root→child→child-dispose (root survives); root shutdown with a late/in-flight child event (no re-claim); retained/aborted/isolated tasks; switch/branch with live children; hard shutdown + PTY cleanup deletes exactly once; omp-stop nonzero/signal/>2s/hang; transactional failure injection if any recover remains.

Required revision

1. hcom extension: return immediately for OMP sub-sessions (prefer an OMP-exposed agentKind/taskDepth; interim, a synchronous globalThis registry with a tearing_down tombstone that never re-opens claiming while children can unwind, and permanently-non-participating skipped closures).
2. Keep hard omp-stop on owner session_shutdown; drop --soft from that path.
3. Remove recover_process_binding_for_instance and the HCOM_NAME/HCOM_INSTANCE_NAME recovery fallback (or make it transactional + token-gated).
4. Start notify/reconcile resources only after owner admission; use ctx.setInterval; clear on shutdown.
5. Handle hcom() timeout/kill/code===null explicitly.
6. Add executable multi-session lifecycle tests, not source-string assertions.

Fixing only DB error propagation, env scrubbing, or test coverage would not be enough — the ownership/recovery model itself must change. Once it does (hard root teardown preserved, no name recovery, sticky sub opt-out), the resulting smaller fix should merge.

[mmkzer0]

@aannoo ty, did a bit of digging and follow-up

Some polish:

• OMP soft-stop now shares the agy soft-stopped PTY cleanup path (inactive row survives)
• clearer skip/tombstone logging when PTY cleanup is deferred or skipped (+ helper + test)
• OMP soft-stop keeps process binding for live rebind; agy still clears; tests updated
• hardened OMP identity registry / ownership (sticky nested opt-out, tearingDown, timer + hcom() timeout)

On the review items:

• B1/B2: mechanism partly real, but the defect framing doesn’t hold imo; soft-stop + nested skip look misread
• “hard stop on owner + drop recover” is the wrong rewrite unless owner session_shutdown is proven terminal-only for root
• B3/B4 addressed in polish: recover is fail-closed (omp + inactive), HCOM_NAME fallback dropped

Tests still pass and did some live testing, nothing seems to break and no more subagent id drop on parent

unrelated: found kimi SessionEnd on an OMP-launched pid is a separate identity cycle that can break, can tackle that in another PR

[aannoo]

nice