fix(hierarchy): polish + critical fixes for #165 unification

[aterrylu]

Why this PR exists

PR #165 (the Agent + Session unification) auto-merged before the post-ship /polish review completed. The reviewers surfaced 3 critical bugs plus a handful of issues that warrant a fast follow-up. None block normal usage today, but two of the three create silent data-loss paths that would bite during the next migration or restart.

Critical fixes

1. Migration sentinel — no more silent partial-migration data loss

Files: packages/server/src/agents/store.ts, packages/server/src/agents/migrate.ts

The original idempotency check used agentsDirExists(). But the dir is created by the first per-agent write inside the loop, so:

```

1. migrateIfNeeded() called
2. agentsDirExists() → false → proceed
3. insertAgent(agent[0]) → writes ~/.autonomos/agents/.json (creates dir as side effect)
4. insertAgent(agent[5]) → throws (disk full / EACCES / corrupt JSON value / etc)
5. sessions.json STILL on disk (rename hasn't run)
6. process restart → migrateIfNeeded() → agentsDirExists() === true → SKIP
7. Agents 6..N from sessions.json: permanently orphaned, no warning
   ```

Fix: write a .migration-complete sentinel file ONLY after all writes + the source rename succeed. Idempotency now checks for the sentinel, not the dir. Mid-crash retries cleanly because insertAgent overwrites by id.

2. writeAgentFile throws on lastReadFailed — no more cache desync

File: packages/server/src/agents/store.ts

The function logged a warning and silently returned when the disk-load failure flag was set. But every caller (saveAgent, insertAgent, setManager, markExited, markRunning, patchAgent) updated the in-memory cache on the line after writeAgentFile. Net effect: dashboard saw the change, restart wiped it. Now throws so the error propagates and the cache stays consistent with disk.

3. PTY-identity guard in onExit — no more restart-race state corruption

File: packages/server/src/agents/runtime.ts

node-pty.onExit is async. During restartAllAttachments the kill→spawn cycle for the same agent.id can leave the killed PTY's onExit to fire AFTER the new attachment is registered. The stale handler then called markExited(persisted.id, reason) and live.delete(persisted.id) on the freshly-spawned record. Now the closure captures the pty reference and no-ops when live.get(id)?.pty !== capturedPty.

Other reviewer findings addressed

File	Fix
index.ts	Migration call wrapped in try/catch; process.exit(2) on failure so pm2/systemd flag the unhealthy boot
agents/migrate.ts	Source-rename failure throws (was warn) — sentinel doesn't get written if source can't be moved
routes/agents.ts DELETE-with-reassignTo	Pre-aborts on first failed setManager; returns 409 with reparented list + failedAt detail rather than silent partial-state
routes/agents.ts DELETE	Filesystem-error path now 500s instead of silent 200 after deleteAgentRaw fails
agents/runtime.ts onExit	Missing-store-record case logs warning instead of silently dropping the agent.exited event
ws/agents.ts safeSend	Dead-client removed from set inside catch — prevents slow leak when ws.send throws without firing onClose

Simplifier-applied refactors (kept from the polish run)

• agents/store.ts: extracted buildAgentTree<N>({includeExited, mapNode}) shared helper used by both /api/agents/tree and the get_org_chart MCP tool — dedup of identical tree-building logic
• agents/runtime.ts: collapsed duplicate fresh-spawn vs fork-spawn branches in spawnAgent; extracted respawnAgent(a) helper used by both resumeActiveAgents and restartAllAttachments
• mcp.ts: dead-branch removal in set_manager error check
• routes/agents.ts GET /:id: no longer double-resolves with getAgent ?? resolveAgent

Test plan

• make check clean: 294/294 tests pass, biome lint clean, full TypeScript build clean
• Manual smoke (recommended before next deploy): on a fresh dev instance, run make dev, spawn 2 agents, restart dev, verify both views show the same set
• On forge: backup ~/.autonomos/sessions.json and ~/.autonomos/agents/ before deploying — the new sentinel logic is forward-compatible (adds the marker if missing on next startup)

Follow-ups deliberately deferred

• Round-trip migration unit test + cycle-detection test + restart-race repro (no test coverage on the new modules — were deleted in feat(hierarchy): unify Agent + Session — single source of truth #165)
• titleCache lookup in runtime.ts:resolveAgentId includes non-CC agents whose providerSessionId won't be in any JSONL (wasted work, not incorrect)
• markExited dead-code branch (logic-correct but confusing)
• Typed error classes from runtime.ts to replace string-matching error classification
• UUID-shape validation in getAgentFile() (defense in depth — mode 0700 on parent dir is the actual security boundary)

🤖 Generated with Claude Code

[nox-0x]

The hierarchy/migration polish is generally sound. The idempotency sentinel fix, throw-on-cache-divergence, and tree-builder dedup are good improvements. Two concerns worth flagging: the new stale onExit guard may skip per-spawn resource cleanup (output buffers, listeners) for the killed PTY, and the partial-reparent abort path leaves agents reparented to the new parent without rollback (acknowledged in the error but worth confirming intent). Left 3 inline comments — 2 warnings, 1 suggestion.

[nox-0x]

Automated review timed out — see PR for existing inline comments from manual review. Please check for any additional issues not yet surfaced.

[nox-0x]

Automated review timed out after 10 min — the diff is too large for single-pass analysis with extended thinking enabled. 3 existing inline comments are already on the PR from the manual polish review (stale onExit guard, partial-reparent abort, markMigrationComplete throw). A maintainer should do a hand-review before merging if not already satisfied with the existing review. Size: 256 additions / 141 deletions across 8 files.

[nox-0x]

The three critical fixes (migration sentinel, throw-on-cache-divergence, PTY-identity guard) are solid. One new issue found: the rewritten DELETE /:id fallback path converts a benign race (agent already gone) into a 500. See inline comment on packages/server/src/routes/agents.ts line 374 for detail and suggested fix. Otherwise the PR is good to merge.

[aterrylu]

Addressed all 4 inline review comments in 8bba87c

File:Line	Comment	Resolution
runtime.ts:289	Stale-handler early-return resource cleanup	Added doc-comment confirming no captured resources need disposal today (outputBuffer GC's with the dropped ManagedAttachment, node-pty owns the fds). Documents the contract for future spawn-closure changes.
routes/agents.ts:348	Partial-state rollback on reparent failure	Implemented option (a) — capture each child's originalManagerId up-front; on failure walk reparented children in reverse and restore. Surfaces rollbackFailures in the 409 payload as defense-in-depth when a rollback step itself fails.
migrate.ts:204	markMigrationComplete() throw masks success	Wrapped in try/catch with warn. Migration success no longer turns into fatal startup exit when the marker write hits ENOSPC/EPERM — next startup reconciles via the no-source path.
routes/agents.ts:374	DELETE escalates benign race to 500	Implemented option (b) — when both removes return false, re-check getAgent(id): if absent (race resolved), fall through to 200; only 500 if the record genuinely still exists. Distinguishes desired-state-achieved from real fs error.

Verified: 306/306 tests pass, biome clean, full TS build clean. CI re-running on the push.

[nox-0x]

Automated Review — Comment

Verdict: COMMENT — 5 issues found (2 warnings, 3 suggestions)

The PR addresses real gaps from #165 with thoughtful comments and good rollback ergonomics. Main concerns are around the reparent-failure HTTP semantics (409 misrepresents an inconsistent-tree state), event ordering during reparent rollback that leaves WS clients with confusing transient state, and the removal of the GET /:id fast-path lookup which makes UUID hits go through name resolution.

Issues posted inline:

1. 🟡 409 misused for inconsistent-tree state — rollbackFailures.length > 0 should return 500, not 409
2. 🟡 WS delta ordering during rollback — emit forward events before failure is known, then emit reverse events; UI flicker
3. 🟢 GET /:id lost O(1) fast-path — getAgent(id) ?? resolveAgent(id) simplified to just resolveAgent(id), potential regression
4. 🟢 writeAgentFile throw inside rollback — new throwing behavior not caught in rollback loop, would crash handler
5. 🟢 claudeSessionId field renamed semantically — repurposed from provider session id to agent.id alias; name is misleading

Reviewed by Nox 🤖 using Claude Code (print mode, single-turn analysis of diff only).

[aterrylu]

Round 2 — addressed 6 follow-up review comments in 31d432c

File:Line	Severity	Resolution
migrate.ts:74	🟡 Warning	Wrapped markMigrationComplete() in the fresh-install branch in try/catch + warn (symmetric with post-rename branch). No more misleading fatal exit on first boot when marker write hits ENOSPC/EPERM.
routes/agents.ts:388	🟡 Warning	Reparent deltas buffered into pendingDeltas[] and flushed only on full-loop success. On rollback, no events emitted (clients see no transition rather than flicker through forward+reverse).
routes/agents.ts:396	🟡 Warning	Status code split: 409 for clean rollback (safe retry), 500 for rollbackFailures > 0 (tree mutated, must not retry). Body wording updated to match.
agents/store.ts:184	🟢 Suggestion	Doc-comment on resolveAgent confirms the O(1) cache.get fast-path runs before the O(N) name scan — /api/agents/:id stays constant-time on UUID hits.
routes/agents.ts (rollback loop)	🟢 Suggestion	Each setManager in the rollback loop wrapped in try/catch. writeAgentFile throws on lastReadFailed; throws now feed into rollbackFailures with reason "throw" rather than crashing the handler.
dashboard/store.ts:805	🟢 Suggestion	Added providerSessionId? field to SessionInfo so callers needing the actual CC provider session id (e.g. for claude --resume) have access. claudeSessionId documented as a stable lookup alias for agent.id.

Verified: 306/306 tests pass, biome clean, full TS build clean. CI re-running on the push.

[nox-0x]

Automated Review — Comment

1 new issue found beyond the 10 already on the PR from a previous pass.

The three critical fixes (migration sentinel, throw-on-cache-divergence, PTY-identity guard) are solid, and the buffered-reparent-with-structured-rollback pattern is well-designed. The one remaining gap: the forward setManager() call in the reparent loop is unwrapped, so a throw from writeAgentFile (per this PR's own store.ts change) mid-loop would skip the structured rollback entirely and produce a generic 500 with mutated disk state.

👉 Inline comment posted on packages/server/src/routes/agents.ts:355.

Reviewed by Nox 🤖 using Claude Code (print mode, single-turn analysis of diff only).

[aterrylu]

Round 3 — addressed in 3d51d8d

routes/agents.ts:355 🟡 — wrapped the forward setManager call in the reparent loop in try/catch, symmetric with the rollback loop. A throw on lastReadFailed mid-loop now feeds into the existing updated === undefined rollback branch instead of crashing past the structured 409/500 response. 306/306 tests pass.

[aterrylu]

Round 10: CachePoisonedError contract fidelity 🔴

Critical contract bug — fixed.

routes/agents.ts:415 🔴 — Forward + rollback catches now honor CACHE_POISONED

You're absolutely right. The DELETE-with-children path was the only CachePoisonedError surface in the handler that didn't respect the 503/CACHE_POISONED/retryable:false contract — runtimeDeleteAgent, deleteAgentRaw, and the router-onError fallback all do, but the children loop silently downgraded it to 409 + "safe to retry." Clients keyed off the contract would burn cycles retrying forever.

Two fixes restore parity:

Fix 1: Forward fast-path

catch (forwardErr) {
  ...
  if (forwardErr instanceof CachePoisonedError) cachePoisonedErr = forwardErr;
}

if (typeof updated === "string" || updated === undefined) {
  if (cachePoisonedErr) {
    for (const d of pendingDeltas.values()) emitAgentDelta(d);
    return c.json({
      error: cachePoisonedErr.message,
      code: cachePoisonedErr.code,
      retryable: false,
      ...(reparented.length > 0 && { reparented }),
    }, 503);
  }
  // existing rollback path...
}

Skips the rollback entirely (every iter would throw the same way — writeAgentFile checks lastReadFailed unconditionally), flushes pendingDeltas so WS state matches disk.

Fix 2: Rollback escalation

Records the first CachePoisonedError mid-rollback without aborting (each remaining iter's predictable throw correctly emits its forward delta in the existing failure branch). After the loop, escalates 500 → 503 with the standard CACHE_POISONED envelope plus rolledBack / rollbackFailures so the operator sees what completed.

Contract parity matrix (post-fix)

Throw site	Code	Status
runtimeDeleteAgent	503 + CACHE_POISONED + reparented?	✓
deleteAgentRaw fallback	503 + CACHE_POISONED + reparented?	✓
Forward setManager (children loop)	503 + CACHE_POISONED + reparented?	✓ NEW
Rollback setManager (children loop)	503 + CACHE_POISONED + rolledBack/rollbackFailures	✓ NEW
POST/PATCH/PUT (router onError)	503 + CACHE_POISONED	✓

Every surface now produces the same envelope. No client-visible divergence between paths.

Status

• 306 tests pass, biome+TS clean
• Diff: +66 / -0 in routes/agents.ts

Latest commit: 31f02d7. Continuing the loop.

[nox-0x]

Automated Review — Changes Requested ❌

Verdict: REQUEST_CHANGES — 4 new issues found (1 critical, 3 warnings)

The CachePoisonedError plumbing is broadly sound — the global onError, throw-conversion in writeAgentFile, and the local DELETE catches all cohere. However, the rollback-poisoned escape path misattributes the forward failure (hard-codes reason="throw"), the forward-poisoned fast-path is missing the failedAt field, the buildAgentTree filter silently widens from "running" to "non-exited" (API contract change), and the MCP set_manager tool has no CachePoisonedError catch despite now calling a throw-capable writeAgentFile.

Issues posted inline:

1. 🔴 Critical — routes/agents.ts:487 — Rollback-poisoned escape path hard-codes failedAt.reason="throw" even when forward returned "cycle"/"not-found"
2. 🟡 Warning — routes/agents.ts:420 — Forward CachePoisonedError fast-path missing failedAt field (contract drift from all other DELETE error responses)
3. 🟡 Warning — store.ts:392 — buildAgentTree filter widened from status==="running" to status!=="exited" — user-visible API change for MCP + REST consumers
4. 🟡 Warning — mcp.ts:330 — MCP set_manager calls setManager() unwrapped, but writeAgentFile now throws CachePoisonedError

Reviewed by Nox 🤖 via Claude Code (print mode, single-turn diff analysis, all tools disabled).

[aterrylu]

Round 11: failedAt fidelity, contract revert, MCP poison handler

All four issues addressed.

1. routes/agents.ts:487 🔴 — Rollback-poisoned escape misattribution

You're right — hardcoding failedAt.reason="throw" on the rollback-poisoned escape path was wrong when the forward had returned "cycle" or "not-found" cleanly. Hoisted forwardReason above both CachePoisonedError branches so it's computed once and reused. Forward failure mode now reflects the actual cause regardless of whether rollback poisoned afterward.

2. routes/agents.ts:420 🟡 — Forward fast-path missing failedAt

Added the failedAt: { id, name, reason: forwardReason, message? } structure to the forward CachePoisonedError fast-path so every DELETE error response — 409, 500, 503 (CachePoisonedError forward fast-path), 503 (CachePoisonedError rollback escape) — shares one envelope. No more shape divergence between failure modes.

3. store.ts:392 🟡 — Reverted filter contract

Fair point — round 9 widened status === "running" → status !== "exited" per (one of) your earlier suggestions, but that's a user-visible contract change for existing MCP/REST consumers (starting agents would suddenly appear). Reverted to strict running and updated the docstring honestly:

only agents with status === "running" are visible. Exited AND transient states (starting, etc.) are both filtered out. ... widening to status !== "exited" would be a user-visible API change for existing MCP/REST consumers.

Pre-refactor behavior preserved.

4. mcp.ts:330 🟡 — MCP set_manager unwrapped

Wrapped the MCP set_manager setManager() call in try/catch:

try { result = setManager(agent.id, managerId); }
catch (err) {
  if (err instanceof CachePoisonedError) {
    return {
      content: [{ type: "text", text: `CACHE_POISONED: ${err.message} (...retry pointless until operator restart).` }],
      isError: true,
    };
  }
  throw err;
}

MCP clients (agents) now see the same stable CACHE_POISONED signal HTTP clients see via 503. Generic "Failed to set manager" no longer mistakenly implies a transient retryable failure.

Status

• 306 tests pass, biome+TS clean
• Diff: +73 / -22 across store.ts + routes/agents.ts + mcp.ts

Latest commit: 9340c7a. Looping.

[nox-0x]

Automated Review — 2 new issues found

Reviewed this PR after a prior commit (SHA 9340c7a4). The diff addresses the same concerns from #165: migration sentinel, throw-on-cache-divergence, PTY-identity guard, structured DELETE rollback, and tree-builder dedup.

30+ existing comments from previous passes cover the major concerns (race-to-500 regression, 409/503 semantics, WS delta ordering, partial-reparent rollback gaps, CachePoisonedError handling, type safety, etc.).

2 new issues found:

1. 🟡 agents.ts:59 — onError re-throws instead of returning Response. throw err; from an onError handler relies on Hono undocumented behavior; in some versions this surfaces as an unhandled rejection or connection drop instead of a 500-with-stack. Should return c.json(...) explicitly.
2. 🟢 store.ts:294 — No-op short-circuit skips manager existence validation. The if (existing.managerId === managerId) return existing; check runs before cache.has(managerId), allowing set_manager to silently "succeed" against a tombstoned/dangling manager pointer.

Reviewed by Nox 🤖 (Claude Code, print mode, --tools "" single-turn diff analysis).

[aterrylu]

Round 12: explicit onError 500 + dangling-managerId catch

Both issues addressed.

1. routes/agents.ts:59 🟡 — onError re-throw replaced with explicit Response

Good catch — throw err from a Hono onError handler is undocumented and version-dependent (can surface as unhandled rejection or connection drop instead of a structured 500). Replaced with an explicit response:

const message = err instanceof Error ? err.message : String(err);
console.error(`[agents] unhandled ${...} on ${...}: ${message}`, err.stack);
return c.json({ error: message }, 500);

Stack is logged server-side; response carries enough context for the dashboard.

2. store.ts:294 🟢 — Dangling-managerId silently re-affirmed by no-op short-circuit

You're right — the no-op short-circuit ran before the existence validation. Idempotent set_manager retries against a deleted-but-not-yet-cache-evicted manager would silently "succeed."

Reordered so existence is checked FIRST:

// Validate manager existence FIRST — even before the no-op short-circuit.
if (managerId !== null && !cache.has(managerId)) return undefined;
if (managerId === id) return "cycle";
// No-op: now safe — managerId has been validated above.
if (existing.managerId === managerId) return existing;
// ... ancestor cycle walk for actual changes ...

Dangling refs now return undefined regardless of whether the caller's argument matches the existing field. Self-loop check hoisted above the short-circuit for symmetry.

Status

• 306 tests pass, biome+TS clean
• Diff: +26 / -6 across store.ts + routes/agents.ts

Latest commit: 7c0546d. Continuing the loop.

[nox-0x]

Requesting changes — the dashboard's ConversationView reads activeSession.claudeSessionId, but this PR repurposes claudeSessionId to mean agent.id, which silently breaks /api/conversation/<id> lookups for any freshly-spawned (non-migrated) agent.

The migration sentinel, cache-poisoned 503 contract, deferred WS flush, and PTY-identity guard all look solid. Reviewer threads from rounds 4–12 are addressed. The blocker is a single new field-mapping regression introduced by this PR.

The bug

`SessionInfo.claudeSessionId` previously equaled the CC provider session id; this PR remaps it to `agent.id` (store.ts:614, store.ts:822) and adds a separate `providerSessionId` field with a comment noting that "callers that … read CC's JSONL" should use the new field. `ConversationView.tsx:412` is exactly such a caller and was not updated:

```tsx
const claudeSessionId = activeSession?.claudeSessionId; // now agent.id, not CC's session id
// ...
fetch(`/api/conversation/${claudeSessionId}`)
```

`/api/conversation/:sessionId` (routes/conversation.ts:14) scans `~/.claude/projects//.jsonl`. CC writes that JSONL keyed on its own session UUID. For Option-A migrated agents the bug is invisible (id ≡ providerSessionId by construction). For any agent spawned post-merge, agent.id is a fresh `crypto.randomUUID()` (runtime.ts:155), distinct from the CC session id (runtime.ts:154) — so the file does not exist and the conversation panel surfaces "Session not found" 404.

The fix

One line in `packages/dashboard/src/components/conversation/ConversationView.tsx:412`:

```diff

• const claudeSessionId = activeSession?.claudeSessionId;

• const claudeSessionId = activeSession?.providerSessionId;
  ```

(Variable rename optional but worth doing; the new `providerSessionId` field on `SessionInfo` was added precisely for this.) Worth grepping for any other `claudeSessionId` consumers that hand the value to provider-side APIs (telemetry, log scrubbers, copy-for-support) — most other call sites I traced just use it as a stable lookup key and are correct.

Once that's in, this is good to merge.

[aterrylu]

Round 13: ConversationView — providerSessionId regression fix

Excellent catch — real ship-blocking bug. The agent-unification PR remapped SessionInfo.claudeSessionId from "CC's session UUID" to "agent.id" (a dashboard-internal stable lookup key), but ConversationView.tsx still passed it to /api/conversation/:id which expects the CC-side identifier.

Why it escaped review

Type-checking can't catch a semantic remapping when the field name stays the same — the compiler sees string flowing into string, but the runtime sees agent.id flowing into a CC-keyed file lookup. This is exactly the kind of regression that demands either a rename or a runtime check.

The fix

- const claudeSessionId = activeSession?.claudeSessionId;
+ // /api/conversation/:id reads ~/.claude/projects/<cwd>/<id>.jsonl, which
+ // CC writes keyed on its own session UUID. After the agent-unification
+ // PR, SessionInfo.claudeSessionId is repurposed to mean `agent.id` ...
+ const providerSessionId = activeSession?.providerSessionId;

Plus the dependent variable rename in the useEffect body and dependency array.

Audit of other consumers

Verified that /api/conversation/:id has only one consumer in the dashboard (single grep hit on api/conversation). All other claudeSessionId references (store.ts resumeSession, mergeOrgWithSessions, HierarchyPanel) use it as a stable lookup key, which post-refactor IS agent.id — correct semantics. No other provider-keyed callers exist.

For migrated vs spawned agents

• Migrated (Option-A): id === providerSessionId by construction → no observable break ever
• Freshly spawned (post-merge): id === crypto.randomUUID() while providerSessionId === CC's spawn id → the bug

Status

• 306 tests pass, biome+TS clean
• Diff: +12 / -4 in ConversationView.tsx

Latest commit: a7e2bcd. Should be the final blocker per your verdict.

[nox-0x]

(diagnostic test by reviewer-bot — ignore)

[nox-0x]

(diagnostic test by reviewer-bot — ignore)

[nox-0x]

(diagnostic test by reviewer-bot — ignore)

[nox-0x]

🟡 Warning — markExited is now throw-capable. writeAgentFile now throws CachePoisonedError, so the markExited call on line 314 can throw too — and the call is unwrapped. Under cache-poisoned state, node-pty receives an unhandled throw from the onExit callback, AND live.delete + the agent.exited delta (lines 315–322) never run, leaving a phantom-running entry in the live map and an inconsistent dashboard view. Same shape applies to killAttachment line 349 and spawnAgents markRunning / insertAgent on lines 226–232 — in the spawn case the PTY child process is already alive when the throw fires, so each retry under poisoned state spawns another orphan claude process. Only manifests after a prior loadAll failure, so not blocking. Worth a follow-up to wrap these store-mutating calls and at minimum still run live.delete + emit a delta when the disk write fails.

[nox-0x]

🟢 Suggestion — no-op setManager still emits agent.reparented. The new no-op short-circuit in setManager (store.ts line 306) returns the unchanged existing record when managerId === existing.managerId. Both this REST handler (line 295) and the MCP set_manager tool (mcp.ts line 361) unconditionally emit agent.reparented afterward, so an idempotent re-issue (drag-and-drop UI re-firing the same parent, idempotent MCP retry) broadcasts a redundant delta with the un-bumped version. Not a correctness bug — clients converging on (id, managerId, version) see no change — just noise on the WS bus. Easy fix: have setManager return a distinct sentinel for the no-op case, or detect it at the callsite via pre-call version capture.

[nox-0x]

Approving — clean polish PR; the DELETE state machine, migration sentinel, PTY-identity guard, and CachePoisonedError surfacing all hold up under tracing, no critical issues in the current code.

The 33 prior review threads are all resolved and the response shapes / rollback logic are mutually consistent. Two minor follow-ups worth a separate ticket, neither blocking:

• The new writeAgentFile throw widens the blast radius for CachePoisonedError. pty.onExit → markExited, killAttachment → markExited, and spawnAgent → insertAgent / markRunning still call those throw-capable functions unwrapped, so a poisoned cache could leak phantom live entries (and in spawnAgent the throw fires after spawn(), so the PTY child becomes an orphan on each retry). Manifests only post-loadAll-failure.
• The new no-op short-circuit in setManager (store.ts:306) still triggers agent.reparented broadcasts at the REST and MCP callsites. Harmless noise — version does not bump, clients converge — but worth a sentinel-return to skip the emit.

Dashboard claudeSessionId → providerSessionId split for /api/conversation/:id is correct and the field comment in store.ts documents the legacy alias clearly.

[nox-0x]

🔴 Critical — PTY leak when markRunning / insertAgent throws CachePoisonedError

This guard correctly defangs stale onExit handlers, but the mirror race on the spawn side is uncovered. The PR makes writeAgentFile throw on lastReadFailed, so both insertAgent (line 232) and markRunning (lines 227-231) are now throw-capable. They run after spawn(binary, args, …) at line 212, with no try/catch around them.

If the throw fires, the spawned PTY child process is alive but never registered in live and never killed. The router-level onError maps the exception to a 503, but the orphan claude process keeps running with no record. For the resume path, the existing record stays in exited state on disk → next resumeActiveAgents may try to spawn another claude for the same agent id; for the fresh-spawn path there is no record at all so the orphan is invisible until manual ps cleanup.

This is a new gap the PR introduces (silent on the happy path, compounds with each restart cycle once the cache is poisoned).

Fix: wrap the markRunning / insertAgent call in try/catch, pty.kill() (best-effort) on throw, then re-throw so the router still surfaces 503.

[nox-0x]

🟡 Warning — runtimeDeleteAgent returns true on rmSync failure when the agent was live, masking the failure as a 200 success

runtime.deleteAgent (runtime.ts:364-380) ends with return removed || wasLive. If wasLive=true and deleteAgentRaw returns false (rmSync threw and was caught at store.ts:364-367), the function still returns true. The route handler then sees removed=true, skips the if (!removed) recovery branch (with its rawRemoved try/catch and the disk-state-check at line 731), and falls through to return c.json({ ok: true, id }) at line 759.

Net result on rmSync EBUSY/EPERM with a live agent:

• PTY killed and removed from live ✓
• Agent file still on disk, cache entry still present ✗
• agent.deleted event NOT emitted (only fired when removed=true inside runtime.deleteAgent) ✗
• Forward reparent deltas DO emit ✓
• Client gets 200 ok:true ✗ — but the next /api/agents refresh shows the agent reappear

Fix: in runtime.deleteAgent, distinguish "PTY-killed but raw-removal-failed" from "fully gone" — either propagate the rmSync error (rather than deleteAgentRaw swallowing it), or have it return a richer status the route can branch on. The route-level if (!rawRemoved && getAgent(id) !== undefined) recovery exists; runtime.deleteAgent just needs to surface the failure so it runs.

[nox-0x]

🟢 Suggestion — setManager returns undefined for two distinct failures: "agent id not found" AND "managerId not found"

This new cache.has(managerId) check correctly closes the dangling-ref window, but it overloads the undefined return that already meant "id (the agent) not found" (line 289). MCP set_manager (mcp.ts:355-359) only branches on !result || typeof result === "string" and returns a generic Failed to set manager. — the human/agent client cannot tell which arg was bad and may keep retrying with the same wrong managerId.

The REST handler at routes/agents.ts:277-279 says "Agent or managerId not found" which is at least informative, but still ambiguous.

Fix: add a "missing-manager" discriminant to the union return so callers can produce a specific 404. Cheap; setManager already returns string discriminants ("cycle" / "stale") for other distinguishable failure modes.

[nox-0x]

Approving — the three headline fixes (migration sentinel, throw-on-poisoned-cache, PTY-identity guard in onExit) are sound and the DELETE-with-reassignTo rollback path is well-thought-through. None of the findings below are merge-blockers; they're all reachable only after the cache is already poisoned (a degraded state) and have safe surrounding behavior.

Findings (see inline comments for details):

• 🔴 Critical follow-up: spawnAgent PTY leak on markRunning / insertAgent throw — symmetric mirror of the onExit fix. Wrap and pty.kill() on throw before re-raising. (runtime.ts:226-232)
• 🟡 Warning: runtime.deleteAgent return removed || wasLive returns true on rmSync failure when wasLive, so the route returns 200 while the agent file remains on disk and agent.deleted is never emitted. (routes/agents.ts:658)
• 🟢 Suggestion: setManager overloads undefined for both "agent not found" and "managerId not found" — MCP surface returns generic "Failed to set manager." Add a "missing-manager" discriminant. (store.ts:302)

Two prior threads (markExited throw-propagation in onExit, no-op setManager still emitting agent.reparented) remain unresolved — both already accepted as deferred follow-ups in the PR description / by the reviewer themselves. Leaving them open as tracked follow-ups; not blocking this merge.