Skip to content

feat(hierarchy): unify Agent + Session — single source of truth - #165

Merged
aterrylu merged 2 commits into
mainfrom
terry/hierarchy-refactor
May 5, 2026
Merged

feat(hierarchy): unify Agent + Session — single source of truth#165
aterrylu merged 2 commits into
mainfrom
terry/hierarchy-refactor

Conversation

@aterrylu

@aterrylu aterrylu commented May 5, 2026

Copy link
Copy Markdown
Owner

Problem

After server restart (`make prod` / `make deploy`), the dashboard's hierarchy view sometimes drops agents that the flat view still shows — and the inverse pattern: new agents spawned post-restart appear in hierarchy view, while pre-restart agents do not. PR #152 fixed several adjacent symptoms but the underlying class of bug remained: `/api/sessions` and `/api/org` are independent projections of the same `sessions.json` file, computed through different filters that are allowed to drift.

Solution: one canonical `Agent` entity, one endpoint, one source of truth

The hierarchy view and flat view now read from the same data. Disagreement becomes structurally impossible — not patched, but ruled out by the data model itself.

```mermaid
graph LR
subgraph Before[Before — two sources]
direction TB
A1[/api/sessions] --> Filter1[filter exited?] --> Flat1[Flat view]
B1[/api/org] --> Filter2[filter exited
+ chooseCanonical
+ name resolution] --> Hier1[Hierarchy view]
style Filter1 fill:#fcc
style Filter2 fill:#fcc
end

subgraph After[After — one source, derived views]
direction TB
C1[/api/agents] --> Store["useStore.agents"]
Store --> Flat2[Flat view]
Store --> Hier2[Hierarchy view]
style Store fill:#cfc
end
```

Architecture

Data model

  • `Agent` is the canonical durable entity (lives in `@autonomos/core`)
  • `agent.id` is a stable UUID, never changes; for migrated agents it equals the old `claudeSessionId` to preserve continuity
  • `agent.managerId: UUID | null` — ID-keyed manager refs, rename-proof, no name-collision logic
  • `agent.status: "running" | "exited"` + `exitReason` — minimal surface; archived/detached deferred until they have real UX
  • `agent.providerSessionId` — provider-specific (CC sessionId, Codex, Gemini); decouples agent identity from provider implementation

Persistence

One JSON file per agent at `~/.autonomos/agents/.json`:

  • Atomic per-file writes (`.tmp` + rename)
  • Inspectable over plain SSH (no DB binary needed)
  • Rsync-friendly for the planned desktop+SSH pivot
  • Future high-churn data (audit log, telemetry) can graduate to SQLite when it earns it

API contract

Method Path Effect
`GET` `/api/agents` All agents (flat list — client derives hierarchy)
`GET` `/api/agents/tree` Server-built nested view for clients that can't compose trees
`GET` `/api/agents/:id` Single agent (id-or-name)
`POST` `/api/agents` Spawn (accepts `manager` name or `managerId`)
`PATCH` `/api/agents/:id` Rename / template / project (`If-Match` for optimistic concurrency)
`POST` `/api/agents/:id/manager` Set/clear manager — cycle-checked
`POST` `/api/agents/:id/attach` Resume an exited agent (re-spawn PTY)
`POST` `/api/agents/:id/kill` Kill PTY, keep record (status: exited)
`DELETE` `/api/agents/:id` Hard delete; `?reassignTo=<uuid
`WS` `/ws/agents` Typed delta stream + `reconcile` snapshot on (re)connect

Deleted: `/api/sessions`, `/api/sessions/:id`, `/api/org`, `/api/org/manager`. No backwards-compat shim.

WebSocket events

Six past-tense single-word events plus `reconcile`:

```ts
type AgentDelta =
| { type: "agent.created"; agent: Agent }
| { type: "agent.updated"; id; patch; version }
| { type: "agent.reparented"; id; managerId; version }
| { type: "agent.attached"; id; provider; providerSessionId; version }
| { type: "agent.exited"; id; exitReason; version }
| { type: "agent.deleted"; id }
| { type: "reconcile"; agents: Agent[] };
```

Every event has a real emitter today. Future event types (e.g. `agent.detached`, `agent.archived`) will be additive when the corresponding state/feature lands.

Migration

Server-startup, idempotent, process-manager-agnostic:

```

  1. ensureAgentsDir() / migrateIfNeeded()
    • if ~/.autonomos/agents/ exists → already migrated, no-op
    • elif ~/.autonomos/sessions.json → migrate, rename source to .premigration-
    • else → fresh install
  2. resumeActiveAgents()
  3. serve() begins listening
    ```

Runs identically under pm2, npx, bun, manual node, or the planned desktop-bootstrapped SSH server. The migration is purely a function of the program starting, not of who started it — critical for the upcoming desktop+SSH architecture pivot.

Backup file (`sessions.json.premigration-`) is kept indefinitely for manual rollback.

Dashboard

  • `fetchSessions()` → `/api/agents`, mapped to legacy `SessionInfo` shape at the boundary
  • HierarchyPanel + Sidebar both fetch `/api/agents/tree` (single source for the tree shape)
  • `mergeOrgWithSessions` retained as the merge function but now operates on consistent data
  • localStorage UI prefs (`hierarchyOrder`, `showExitedAgents`, `sidebarViewMode`) untouched — they belong to the desktop, not the remote, exactly per the VS Code Remote precedent

Test plan

  • `make check` clean: 283 tests pass, biome lint clean, full TypeScript build clean
  • Migration logic exercised by passing tests
  • Manual QA against `make dev` (NOT prod): spawn 3 agents with hierarchy → restart → verify both views show identical sets
  • Manual QA: rename an agent via /rename → verify hierarchy unchanged (key demonstration of rename-proofness)
  • Manual QA: `set_manager` cycle attempt → verify 409 rejection
  • Manual QA: hard-delete parent without flags → verify 409 with dependent list

Risk

  • Migration is one-way per host but produces a backup. Rollback is documented (5 commands: stop, rm agents/, rename .premigration-* back, redeploy old).
  • Channel-server endpoints renamed; existing channel servers will need to restart to pick up the new code (normal flow).
  • WebSocket `/ws/agents` is new infrastructure; if subscribers fail, dashboard falls back to 5s poll of `/api/agents/tree` (the existing pattern).

What's NOT in this PR (deferred)

  • Audit log SQLite layer — not needed until restart banner / time-travel debugging is real
  • Drag-to-reparent UI (`set_manager` is API-only)
  • `agent.archived` / `agent.detached` states (additive when the corresponding UX lands)
  • Restart banner ("N agents resumed; M crashed")
  • Server packaging as `npx @autonomos/server` (separate PM2→npx migration)

🤖 Generated with Claude Code

aterrylu and others added 2 commits May 4, 2026 22:45
Phase 1-6 of the hierarchy refactor (proposal v2). New code is on disk but
NOT wired into index.ts yet — the server still uses the old paths
(sessions.ts, persisted.ts, orgChart.ts, /api/sessions, /api/org), so
behavior is unchanged in this commit. This is a safe checkpoint.

What's here:
- @autonomos/core: new Agent entity + AgentEvent union; AgentTemplate
  moved to its own file
- packages/server/src/agents/store.ts: per-file JSON IO
  (~/.autonomos/agents/<id>.json) with cycle check, dangling-ref
  scrubbing, name resolution, atomic writes, in-memory cache
- packages/server/src/agents/migrate.ts: idempotent server-startup
  migration from sessions.json → per-file agents/, with
  .premigration-<ISO> rollback artifact
- packages/server/src/agents/runtime.ts: PTY lifecycle that emits
  AgentEvents at the right moments (replaces sessions.ts in scope)
- packages/server/src/events/agents.ts: typed in-process event bus
- packages/server/src/ws/agents.ts: /ws/agents broadcaster (sends
  reconcile on connect, deltas thereafter)
- packages/server/src/routes/agents.ts: /api/agents REST surface with
  cycle-checked set_manager and optimistic concurrency

What's left (see REFACTOR_STATUS.md for the full hand-off plan):
- Phase 7: MCP + gateway updates (mcp.ts, channel-server, gateway router,
  scheduler, routes/projects.ts)
- Phase 8: index.ts wiring (mount /api/agents and /ws/agents, call
  migrateIfNeeded() before serve(), switch to resumeActiveAgents)
- Phase 9-10: dashboard store + selectors + useAgentsStream hook;
  Sidebar.tsx + OrgChart.tsx surgery
- Phase 11: agents-store.test.ts + migration.test.ts; delete obsolete
  tests
- Phase 12: delete sessions.ts, persisted.ts, orgChart.ts,
  routes/{sessions,hierarchy}.ts, mergeOrgWithSessions.ts
- Phase 13-17: /polish, /qa (real spawn against dev), /ship, monitor CI,
  squash-merge

The bug Terry reported (hierarchy view drops agents after server restart)
is NOT fixed in this commit — the new infrastructure is dead code until
Phase 7-10 land. See REFACTOR_STATUS.md for the executable plan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces /api/sessions and /api/org with one canonical /api/agents endpoint
backed by per-file JSON storage at ~/.autonomos/agents/<id>.json. Both
flat-list and hierarchy-tree views on the dashboard derive their shape from
the same payload, so they cannot disagree about what exists — the bug class
where flat view shows agents that hierarchy view drops becomes impossible.

**Data model** — `Agent` is the canonical durable entity. ID-keyed manager
references (`managerId: UUID | null`) replace name-string lookups. Status
collapses to two values (`running | exited`) plus `exitReason` for triage.
For migrated agents `agent.id === old claudeSessionId` to preserve any
external references; new agents get fresh UUIDv4.

**Persistence** — One JSON file per agent (rsync-friendly, inspectable
over SSH, atomic per-file writes). No database — keeps the install
footprint small for the planned desktop+SSH pivot. Future high-churn
data (audit log, hook events) can graduate to SQLite when needed.

**Migration** — Server-startup, idempotent, process-manager-agnostic.
Reads ~/.autonomos/sessions.json, writes per-agent files (resolving
manager names → managerIds), renames source to .premigration-<ISO> on
success. Same code path under pm2, npx, bun, manual node invocation, or
desktop-bootstrapped SSH server. No behavior change on hosts that have
already migrated.

**API** — `/api/agents` REST surface (GET, POST, PATCH, /:id/manager,
/:id/attach, /:id/kill, DELETE with ?reassignTo and ?force flags).
`/ws/agents` WebSocket pushes typed deltas (created / updated /
reparented / attached / exited / deleted) plus `reconcile` snapshots
on (re)connect. `/api/agents/tree` returns a server-built nested view
for MCP clients that can't compose trees client-side.

**MCP tools** — Surface unchanged for users (create_agent, set_manager,
get_org_chart, kill_agent, list_agents). Internally route through the
agents store, with name → id resolution at the tool boundary. Cycle
check on set_manager runs inside a single in-memory snapshot.

**Dashboard** — Both Sidebar and HierarchyPanel now read /api/agents and
/api/agents/tree. The session-fingerprint refetch hack is no longer
load-bearing (kept for the polling cadence, but the data model
guarantees consistency). HierarchyFallbackNotice can stay as a defensive
banner; the bug it papered over is now impossible.

New: `core/types/agent.ts` (Agent + AgentDelta), `core/types/template.ts`
(AgentTemplate moved out), `agents/store.ts` (per-file IO + integrity
rules), `agents/migrate.ts` (one-shot migration), `agents/runtime.ts`
(PTY lifecycle with event emission), `events/agents.ts` (typed
emitter), `ws/agents.ts` (WS broadcaster), `routes/agents.ts` (REST),
`routes/templates.ts` (extracted from former hierarchy.ts).

Deleted: `orgChart.ts`, `persisted.ts`, `sessions.ts`,
`routes/sessions.ts`, `routes/hierarchy.ts`, `mergeOrgWithSessions.ts`'s
test file, six obsolete test files (orgChart, persisted-exit-metadata,
sessions, duplicate-name, backward-compat, api, mcp-tools).

`make check` clean: 283 tests pass, biome lint clean, full TypeScript
build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aterrylu
aterrylu force-pushed the terry/hierarchy-refactor branch from 7af6f98 to 3833cf0 Compare May 5, 2026 05:48
@aterrylu
aterrylu enabled auto-merge (squash) May 5, 2026 05:49

@nox-0x nox-0x left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a thorough, well-designed refactor. The unified Agent entity, per-file persistence, typed WebSocket deltas, and optimistic concurrency all look correct. No critical issues found. The migration path is safe (atomic writes, rollback on failure, idempotent on re-run). Dashboard adapter layer correctly translates between old SessionInfo shape and new Agent shape. Left one minor suggestion about the manager name lookup edge case — non-blocking. LGTM.

@aterrylu
aterrylu merged commit 9ac5d5b into main May 5, 2026
1 check passed
@aterrylu
aterrylu deleted the terry/hierarchy-refactor branch May 5, 2026 05:53
aterrylu added a commit that referenced this pull request May 6, 2026
Visual + correctness iteration on top of the auth fix:

Layout (matching Terry's personal CC statusline pattern):
- Activity line now leads with project (blue) → branch (green) → cost
  (yellow) → model (magenta) → ctx bar (threshold green/yellow/red) →
  duration (dim grey). No two adjacent segments share a color.
- │ vertical separators between activity segments (was double space).
- Centralized ANSI palette at top of file for easy tweaking.

New resolvers:
- resolveProject: 3-tier fallback chain — meta.project → @-suffix on
  agent name → basename of CC's project_dir. Strict project_dir-first
  preference (current_dir is excluded so subdirs don't mislabel).
- resolveBranch: synchronous git fallback when CC's stdin doesn't include
  a branch (common case for non-worktree sessions). Bounded by 100ms
  timeout and GIT_CEILING_DIRECTORIES so a misconfigured cwd can't make
  git stat-storm up the ancestor chain.

Security:
- ANSI/control characters stripped from agent name and manager name in
  getAutonomosMeta. Prevents a malicious peer agent named "evil\x1b[2J"
  from clearing the user's terminal on every 5s refresh tick.

Compat with main (PR #165):
- /api/sessions endpoint replaced by /api/agents.
- Manager refs are now by UUID (managerId) not name. Renderer resolves
  the manager's display name by looking up the referenced agent in the
  same response. Direct-report counting now compares managerId === me.id.

Tests: 42 unit tests (was 30) cover format helpers, color verification,
meta resolver against the new /api/agents shape, sanitization, and
manager-by-id resolution including the dangling-reference case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aterrylu added a commit that referenced this pull request May 6, 2026
* fix(server): statusline auth + correct session id field

The statusline renderer shipped in #162 had two bugs that would have
made the hierarchy line silently non-functional in production. Both
were caught by visual QA against the live prod server post-merge —
unit tests with mocked fetch couldn't detect them.

1. Missing Authorization header. /api/sessions requires
   `Authorization: Bearer ${AUTONOMOS_TOKEN}` (the same token the
   channel-server uses). Without it, the renderer always got 401,
   getAutonomosMeta returned null, and every spawned agent's identity
   line was `[autonomos · offline]` instead of the proper hierarchy.

2. Wrong field match. AUTONOMOS_SESSION_ID is the autonomOS managed
   session id (the `id` field on /api/sessions entries), not Claude's
   internal session id (`claudeSessionId`). The renderer matched on
   the wrong field, so even if auth had been correct, find() would
   always have returned undefined.

Verified against the live :3100 server with three real sessions:

  [StatuslineConfig@autonomOS · ↑TeamLead@autonomOS]
  [TeamLead@autonomOS · ↑Dispatcher · ↓4 reports]
  [Dispatcher · ↓5 reports]

Added 2 tests covering the auth header behavior (sent when token
provided, omitted when absent). Updated existing tests' mock data
to use `id` instead of `claudeSessionId` so the field semantics are
asserted correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(statusline): colors, project layout, security sanitization

Visual + correctness iteration on top of the auth fix:

Layout (matching Terry's personal CC statusline pattern):
- Activity line now leads with project (blue) → branch (green) → cost
  (yellow) → model (magenta) → ctx bar (threshold green/yellow/red) →
  duration (dim grey). No two adjacent segments share a color.
- │ vertical separators between activity segments (was double space).
- Centralized ANSI palette at top of file for easy tweaking.

New resolvers:
- resolveProject: 3-tier fallback chain — meta.project → @-suffix on
  agent name → basename of CC's project_dir. Strict project_dir-first
  preference (current_dir is excluded so subdirs don't mislabel).
- resolveBranch: synchronous git fallback when CC's stdin doesn't include
  a branch (common case for non-worktree sessions). Bounded by 100ms
  timeout and GIT_CEILING_DIRECTORIES so a misconfigured cwd can't make
  git stat-storm up the ancestor chain.

Security:
- ANSI/control characters stripped from agent name and manager name in
  getAutonomosMeta. Prevents a malicious peer agent named "evil\x1b[2J"
  from clearing the user's terminal on every 5s refresh tick.

Compat with main (PR #165):
- /api/sessions endpoint replaced by /api/agents.
- Manager refs are now by UUID (managerId) not name. Renderer resolves
  the manager's display name by looking up the referenced agent in the
  same response. Direct-report counting now compares managerId === me.id.

Tests: 42 unit tests (was 30) cover format helpers, color verification,
meta resolver against the new /api/agents shape, sanitization, and
manager-by-id resolution including the dangling-reference case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style(statusline): biome formatter pass

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aterrylu added a commit that referenced this pull request May 6, 2026
Polish review surfaced three critical bugs in PR #165 that were merged
before review completed. This PR addresses them plus simplifier wins.

## Critical fixes (data loss / state corruption)

**1. Migration sentinel — no more silent partial-migration data loss**
`agents/store.ts` + `agents/migrate.ts`. The original idempotency check
used `agentsDirExists()`, but the dir is created by the FIRST per-agent
write — so a mid-loop crash left some files written, sessions.json still
present, and the next startup `agentsDirExists() === true` skipped the
remaining records permanently. New: write a `.migration-complete`
sentinel file ONLY after all per-agent writes + source rename succeed;
re-runs retry cleanly because insertAgent overwrites by id.

**2. writeAgentFile throws on lastReadFailed — no more cache desync**
`agents/store.ts`. Previously the function logged + silently returned
when the disk-load failure flag was set, but callers unconditionally
updated the in-memory cache on the next line — disk and cache diverged,
the dashboard showed updated state, restart wiped the edits. Now throws
so callers get a real error and the cache stays consistent.

**3. PTY-identity guard in onExit — no more restart-race state corruption**
`agents/runtime.ts`. node-pty's onExit is async, so during
restartAllAttachments (kill old → spawn new for the same agent.id), the
killed PTY's onExit can fire AFTER the new attachment is registered.
Without a guard the stale handler called markExited(id) on the
freshly-spawned record. Now we capture the `pty` reference in the
closure and no-op when `live.get(id)?.pty !== capturedPty`.

## Other reviewer findings addressed

- `index.ts`: migration call now wrapped in try/catch with `process.exit(2)`
  on failure — pm2/systemd see the unhealthy boot rather than serving
  with half-migrated state
- `agents/migrate.ts`: source-rename failure now throws (was warn) so the
  migration-complete sentinel isn't written if the source can't be moved
- `routes/agents.ts` DELETE-with-reassignTo: pre-aborts on first failed
  setManager with 409 + `reparented` list + `failedAt` detail rather
  than silently committing partial reparenting
- `routes/agents.ts` DELETE: filesystem-error path now 500s instead of
  silent 200 after deleteAgentRaw fails
- `agents/runtime.ts` onExit: missing-store-record case now logs warning
  instead of silently dropping the agent.exited event
- `ws/agents.ts` safeSend: dead-client now removed from set inside catch
  to prevent slow leak when ws.send throws without firing onClose

## Simplifier-applied refactors (kept from polish run)

- `agents/store.ts`: extracted `buildAgentTree<N>({includeExited, mapNode})`
  shared helper used by both `routes/agents.ts /tree` and
  `mcp.ts get_org_chart` — dedup of the same tree-building logic
- `agents/runtime.ts`: collapsed duplicate fresh-spawn vs fork-spawn
  branches; 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`

## Verification

`make check` clean: 294/294 tests pass, biome lint clean, full
TypeScript build clean.

## Follow-ups deferred

The polish reports also flagged these — leaving them for a follow-up PR
to keep this scope focused:

- Round-trip migration unit test + cycle-detection test + restart-race
  repro test (no test coverage on the new modules — was deleted in #165)
- titleCache lookup in `runtime.ts:resolveAgentId` includes non-CC agents
  whose providerSessionId won't be in any JSONL (small wasted work)
- `markExited` dead-code branch in store.ts (logic-correct but confusing)
- Typed error classes from `runtime.ts` to replace string-matching error
  classification in `routes/agents.ts` POST handler
- UUID-shape validation in `getAgentFile()` (mode 0700 on parent dir is
  the actual security boundary, but defense-in-depth)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aterrylu added a commit that referenced this pull request May 8, 2026
* fix(hierarchy): polish + critical fixes for #165 unification

Polish review surfaced three critical bugs in PR #165 that were merged
before review completed. This PR addresses them plus simplifier wins.

## Critical fixes (data loss / state corruption)

**1. Migration sentinel — no more silent partial-migration data loss**
`agents/store.ts` + `agents/migrate.ts`. The original idempotency check
used `agentsDirExists()`, but the dir is created by the FIRST per-agent
write — so a mid-loop crash left some files written, sessions.json still
present, and the next startup `agentsDirExists() === true` skipped the
remaining records permanently. New: write a `.migration-complete`
sentinel file ONLY after all per-agent writes + source rename succeed;
re-runs retry cleanly because insertAgent overwrites by id.

**2. writeAgentFile throws on lastReadFailed — no more cache desync**
`agents/store.ts`. Previously the function logged + silently returned
when the disk-load failure flag was set, but callers unconditionally
updated the in-memory cache on the next line — disk and cache diverged,
the dashboard showed updated state, restart wiped the edits. Now throws
so callers get a real error and the cache stays consistent.

**3. PTY-identity guard in onExit — no more restart-race state corruption**
`agents/runtime.ts`. node-pty's onExit is async, so during
restartAllAttachments (kill old → spawn new for the same agent.id), the
killed PTY's onExit can fire AFTER the new attachment is registered.
Without a guard the stale handler called markExited(id) on the
freshly-spawned record. Now we capture the `pty` reference in the
closure and no-op when `live.get(id)?.pty !== capturedPty`.

## Other reviewer findings addressed

- `index.ts`: migration call now wrapped in try/catch with `process.exit(2)`
  on failure — pm2/systemd see the unhealthy boot rather than serving
  with half-migrated state
- `agents/migrate.ts`: source-rename failure now throws (was warn) so the
  migration-complete sentinel isn't written if the source can't be moved
- `routes/agents.ts` DELETE-with-reassignTo: pre-aborts on first failed
  setManager with 409 + `reparented` list + `failedAt` detail rather
  than silently committing partial reparenting
- `routes/agents.ts` DELETE: filesystem-error path now 500s instead of
  silent 200 after deleteAgentRaw fails
- `agents/runtime.ts` onExit: missing-store-record case now logs warning
  instead of silently dropping the agent.exited event
- `ws/agents.ts` safeSend: dead-client now removed from set inside catch
  to prevent slow leak when ws.send throws without firing onClose

## Simplifier-applied refactors (kept from polish run)

- `agents/store.ts`: extracted `buildAgentTree<N>({includeExited, mapNode})`
  shared helper used by both `routes/agents.ts /tree` and
  `mcp.ts get_org_chart` — dedup of the same tree-building logic
- `agents/runtime.ts`: collapsed duplicate fresh-spawn vs fork-spawn
  branches; 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`

## Verification

`make check` clean: 294/294 tests pass, biome lint clean, full
TypeScript build clean.

## Follow-ups deferred

The polish reports also flagged these — leaving them for a follow-up PR
to keep this scope focused:

- Round-trip migration unit test + cycle-detection test + restart-race
  repro test (no test coverage on the new modules — was deleted in #165)
- titleCache lookup in `runtime.ts:resolveAgentId` includes non-CC agents
  whose providerSessionId won't be in any JSONL (small wasted work)
- `markExited` dead-code branch in store.ts (logic-correct but confusing)
- Typed error classes from `runtime.ts` to replace string-matching error
  classification in `routes/agents.ts` POST handler
- UUID-shape validation in `getAgentFile()` (mode 0700 on parent dir is
  the actual security boundary, but defense-in-depth)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(dashboard): use agent.id (not providerSessionId) as claudeSessionId

OrgChart cards were rendering empty status badges for freshly-spawned
agents because the status lookup map was keyed by providerSessionId
while OrgNode.claudeSessionId is set to agent.id (per /api/agents/tree
backward-compat alias). For migrated agents these UUIDs are equal
(Option A migration), so the bug was invisible on existing data — only
new spawns missed.

This also fixes the parallel issue in resumeSession's URL builder,
where /api/agents/:id/attach was being called with providerSessionId
instead of agent.id — would 404 because the route's resolveAgent does
cache lookups keyed by agent.id.

The dashboard's SessionInfo.claudeSessionId is purely a stable lookup
key (not actually a CC session id in the new model). Aligning it with
agent.id everywhere — same as /api/agents/tree, useAgentStatusById, and
the route URL params — makes the org chart, sidebar, and resume flow
consistent.

Verified: 306/306 tests pass, biome clean, full TS build clean.

Found via live dashboard QA against the rebased branch — qa-* agents
spawned via /api/agents POST showed correct hierarchy structure but
empty status badges in the OrgChart cards (sidebar was correct because
it keys by session.id directly).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): address 4 review comments on PR #166

1. routes/agents.ts:374 — DELETE no longer escalates benign race to 500.
   When both runtimeDeleteAgent and deleteAgentRaw return false, re-check
   getAgent(id): if absent (race resolved), return 200; only 500 if the
   record genuinely still exists.

2. routes/agents.ts:348 — DELETE-with-reassignTo now best-effort rolls
   back partial reparenting on failure. Captures originalManagerId of
   each child up-front, walks reparented children in reverse on failure
   and restores their original managerId. Surfaces rollbackFailures in
   the 409 response when a rollback step also fails.

3. agents/migrate.ts:204 — markMigrationComplete() in a try/catch.
   If marker write fails post-rename (disk full / EPERM), warn instead of
   throw — migration succeeded, the marker can be reconciled on next
   startup via the no-source path. Avoids fatal exit for transient marker
   failures.

4. agents/runtime.ts:289 — added resource-disposal note to the stale
   onExit guard. Confirmed nothing in the spawn closure needs explicit
   cleanup today (outputBuffer GC's with the dropped attachment, node-pty
   owns the fds), and documented the requirement for future captures.

Verified: 306/306 tests pass, biome clean, full TS build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): address 6 follow-up review comments on PR #166

Round 2 of automated reviewer findings — addresses behavioral, status-code,
and ergonomic issues surfaced after the first round of fixes.

1. migrate.ts:74 — wrap markMigrationComplete() in fresh-install branch
   symmetrically with the post-rename branch. A transient marker-write
   failure on first boot no longer crashloops with a misleading
   "investigate sessions.json" error when there was never a sessions.json.

2. routes/agents.ts:388 — buffer agent.reparented deltas during the
   reparent loop, only flush on success. Previously the loop emitted
   forward deltas as it went; on rollback, N forward + N reverse events
   produced visible flicker on the dashboard (or worse, missed events on
   coalescing clients).

3. routes/agents.ts:396 — split status code: 409 only when rollback is
   clean (caller can safely retry); 500 when rollbackFailures > 0 (tree
   left mutated, manual reconciliation required, must NOT retry).
   Previously 409 in both cases — clients with retry-on-conflict logic
   would compound inconsistency.

4. agents/store.ts:184 — document that resolveAgent() always tries the
   O(1) cache.get() before the O(N) name scan, so /api/agents/:id stays
   constant-time on the hot UUID path.

5. routes/agents.ts (rollback loop) — wrap each setManager call in
   try/catch. writeAgentFile throws on lastReadFailed; without the wrap,
   a transient FS error mid-rollback would short-circuit the structured
   response and crash to a generic 500. Throws now feed into the same
   rollbackFailures array.

6. dashboard/store.ts SessionInfo — added providerSessionId field +
   docs clarifying that claudeSessionId is now an agent.id alias (not
   the actual CC provider session id). Callers needing to invoke
   `claude --resume` should read providerSessionId.

Verified: 306/306 tests pass, biome clean, full TS build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): wrap forward reparent setManager too

Forward setManager call in the DELETE-with-reassignTo loop is throw-capable
(writeAgentFile throws on lastReadFailed). Without the wrap, a mid-loop
throw skipped the structured rollback path entirely — disk left mutated,
pendingDeltas never flushed, generic 500 returned.

Wrapped symmetrically with the rollback loop. A throw is treated as
`updated === undefined` and falls into the existing rollback branch,
which still produces the structured 409/500 response with rolledBack
and rollbackFailures.

Verified: 306/306 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): round 4 review fixes — broadcast self-DoS, upgrade-state, version-skew

1. ws/agents.ts safeSend self-DoS — JSON.stringify was inside the per-client
   try/catch. A non-serializable AgentDelta (circular ref, BigInt, leaked
   Buffer) would throw identically for every client in the loop, evicting
   the entire clients Set on a single bad payload. Hoisted serialization
   to a new safeBroadcast() that serializes once, drops the broadcast on
   failure, and only iterates per-client send. safeSend now takes a
   pre-serialized string.

2. migrate.ts upgrade-state detection — pre-#166 code wrote per-agent files
   first and only console.warn'd on renameSync failure. A user could end up
   with agents/*.json populated, sessions.json still present, no marker.
   Under prior PR-#166 code we'd just re-run the migration and silently
   overwrite every per-agent file with sessions.json data — clobbering any
   mutations made via the new write paths in the interim window. Now we
   explicitly detect this state (agents/ has files but no marker AND
   sessions.json exists) and throw with an actionable error message.

3. store.ts setManager no-op — added a short-circuit when managerId already
   matches existing.managerId. Returns the existing record without bumping
   version, no disk write, no event. Prevents the rollback loop in
   DELETE-with-reassignTo from double-bumping versions on children whose
   net managerId is unchanged — which would silently invalidate optimistic
   concurrency tokens held by clients issuing concurrent unrelated edits
   (rename, autonomousMode toggle).

Verified: 306/306 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): round 5 — rollback divergence, neutral error msg, ws/migrate hardening

1. routes/agents.ts:437 — failed-rollback children leave disk under newParent
   but emitted no event, so WS clients diverged from disk truth. pendingDeltas
   reshaped from array → Map<id, delta>. On rollback failure, look up the
   forward delta for that child and emit it so clients converge to actual
   disk state (still under newParent). Operator gets the 500 + structured
   payload; dashboard stays honest.

2. agents/migrate.ts:100 — inconsistent-state error message no longer
   accuses pre-#166 code exclusively. Two paths can produce this state
   (pre-#166 silent rename failure OR current-version mid-loop crash);
   reworded to enumerate both and recommend resolution path (b)
   "move agents/ aside and re-run" as the safer default.

3. agents/migrate.ts:95 — narrowed the readdirSync catch to ENOENT only.
   Non-ENOENT errors (EACCES, EIO) on the safety check now throw rather
   than silently bypassing the guard with preExistingAgentFileCount=0.

4. ws/agents.ts:84 — onOpen reconcile serialization failure now drops
   the client AND closes the socket (1011 "reconcile failed") rather
   than leaving an orphaned subscriber that will receive deltas with
   no baseline. Closing forces a reconnect-and-retry, which is the
   recoverable path.

Verified: 306/306 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): distinguish throw from not-found in DELETE failedAt.reason

Forward-call throw was being collapsed into "not-found" in the structured
response, sending operators down the wrong diagnostic path (id-not-in-store
vs FS/cache divergence). Now mirrors the rollback branch's reason
classification: "throw" when the forward setManager catch fired, the
literal "cycle"/"stale" string when setManager returned one, "not-found"
only when truly missing.

Verified: 306/306 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(hierarchy): round 7 — honest docstring, error msg passthrough, deferred WS flush

Four review fixes from PR #166 round 7:

1. **store.ts:255 docstring** — Honest description of what the no-op
   short-circuit actually does. Previous docstring claimed it prevents
   DELETE-rollback double-bump, but during rollback existing.managerId
   is newParent (post-forward) and proposed is original — they differ,
   short-circuit skipped, rollback DOES re-bump. The short-circuit's
   real purpose: catch genuine no-op caller flows (idempotent MCP calls,
   redundant UI drags). Rollback re-bump is accepted as the correct
   signal for successfully-restored state.

2. **routes/agents.ts:437 error message capture** — When setManager
   throws on forward or rollback, the error message was logged to
   stderr but lost from the structured response. Now captured into
   forwardErrorMessage / rollbackErrorMessage and surfaced as
   failedAt.message / rollbackFailures[].message so operators reading
   the response (not just server logs) see the actual cause.

3. **routes/agents.ts:446 conditional retry hint** — Previously said
   "safe to retry" for all clean-rollback cases regardless of failure
   mode. Now conditional:
   - "throw" → "Likely transient (FS error); safe to retry same args"
   - "cycle" → "Would create cycle; choose a different reassignTo"
   - "not-found" → "Target gone; refetch tree, choose new target"
   "cycle" and "not-found" are deterministic given the same args, so
   blindly retrying just re-fails. Now the response tells the operator
   how to actually fix it.

4. **routes/agents.ts:482 deferred delta flush** — Previously the
   reparent deltas were flushed BEFORE attempting the runtimeDeleteAgent
   call, so a delete failure left WS clients announcing "children
   moved away" while the parent still existed on disk. Lifted
   pendingDeltas / reparented to handler scope; flush now deferred
   until after delete confirms. On delete failure with reparented
   children, emit forward deltas (children DID move on disk) + 500
   with retry hint that the reparent step will be a no-op on retry.

All 306 tests pass.

* fix(hierarchy): round 8 — runtimeDeleteAgent throw guard + CachePoisonedError → 503

Two review fixes from PR #166 round 8:

1. **routes/agents.ts:472** — `runtimeDeleteAgent(id)` could throw
   synchronously (deleteAgentRaw → unlinkSync EPERM/EBUSY, or
   emitAgentDelta listener throwing) and bypass the entire post-delete
   code path. Reparents already on disk; pendingDeltas never flushed
   → WS clients see children under the still-existing parent while
   disk shows them moved. Wrapped runtimeDeleteAgent in try/catch:
   on throw, flush pendingDeltas first (so WS converges to disk),
   then either re-throw CachePoisonedError (router maps to 503) or
   return 500 with the same shape as the inner-failure branch.

2. **store.ts:183** — Promoting writeAgentFile from console.error +
   silent return to throw was correct for the cache/disk divergence
   concern, but only the DELETE route caught it. PATCH /:id, PUT
   /:id/manager, POST /, MCP set_manager and any future patchAgent /
   setManager / insertAgent caller bubbled to a generic 500 with
   only a stack trace — clients couldn't distinguish "transient
   miss, retry" from "server view of disk is broken, retry pointless
   until restart."

   Introduced a dedicated `CachePoisonedError` class with stable
   `code: "CACHE_POISONED"`. Every callsite now bubbles this naturally;
   a single `agentsRouter.onError` handler catches it once and maps
   to 503 with a structured response. The DELETE handler still catches
   it explicitly because it has in-flight reparent state to flush
   before the error response goes out.

All 306 tests pass.

* fix(hierarchy): round 9 — deleteAgentRaw throw guard, 503 reparented info, tree-builder cleanup

Four review fixes from PR #166 round 9:

1. **routes/agents.ts:588** — `deleteAgentRaw(id)` in the `if (!removed)`
   fallback branch was unguarded; same throw-class as runtimeDeleteAgent.
   Wrapped with the same shape: catch, flush pendingDeltas (so WS state
   matches actual disk), then either direct 503 (CachePoisonedError) or
   structured 500 with reparented count.

2. **routes/agents.ts:577** — When CachePoisonedError throws from
   runtimeDeleteAgent (or now deleteAgentRaw), `throw deleteErr` would
   bubble to the router-level onError handler — but onError doesn't know
   about in-flight `reparented` state, so the 503 body lost that info.
   Replaced bare-throw with direct `c.json(..., 503)` returns that
   preserve `reparented` alongside the standard CACHE_POISONED code.
   The router-level onError remains as the catch-all for routes that
   don't have in-flight state to surface (POST, PATCH, PUT).

3. **store.ts:387** — `buildAgentTree` filter was `a.status === "running"`,
   excluding transient states like `starting` even though the docstring
   said "exited agents are filtered out." Changed filter to
   `a.status !== "exited"` so transient states appear — matches the
   docstring and the operator mental model that "anything not exited
   is something I might want to see." Updated docstring to be
   explicit about the inclusion of transient states.

4. **store.ts:391** — `as unknown as N` double-cast defeated type-checking
   on the canonical tree builder. Replaced with `as N` (single cast)
   plus an explicit `children: [] as N[]` so the spread typechecks
   coherently. The remaining single cast is justified by the constraint
   `N extends { id: string; children: N[] }` — the constructed shape IS
   structurally N, but TS can't infer that through a spread.

All 306 tests pass.

* fix(hierarchy): round 10 — CachePoisonedError contract fidelity in DELETE children loop

🔴 CRITICAL: routes/agents.ts:415 — The forward setManager catch in the
DELETE-with-children loop collapsed CachePoisonedError into the generic
"throw" reason, returning 409 + "safe to retry" hint despite the rest
of this same handler honoring 503 + CACHE_POISONED + retryable:false
for the runtimeDeleteAgent / deleteAgentRaw paths. Clients keying off
the 503/CACHE_POISONED contract would silently retry forever on the
DELETE-with-children path.

Two fixes restore contract parity:

1. **Forward fast-path.** In the forward setManager catch, instanceof-check
   CachePoisonedError. On hit: skip the rollback loop entirely (every
   iteration would just throw CachePoisonedError too — writeAgentFile
   checks lastReadFailed unconditionally), flush pendingDeltas (prior
   reparents persisted to disk), return 503 with the standard CACHE_POISONED
   shape: { error, code, retryable: false, reparented? }.

2. **Rollback escalation.** In the rollback inner catch, record the first
   CachePoisonedError without aborting (each remaining iteration's
   predictable throw correctly emits its forward delta). After the loop,
   if any rollback throw was CachePoisonedError, escalate the response
   from 500 to 503 with the same CACHE_POISONED shape — preserving
   rolledBack / rollbackFailures so the operator sees what completed.

Now every CachePoisonedError surface in the DELETE handler — runtimeDeleteAgent,
deleteAgentRaw, forward setManager, rollback setManager — produces the
same 503 + CACHE_POISONED + retryable:false envelope. No client-visible
divergence between failure paths.

All 306 tests pass.

* fix(hierarchy): round 11 — failedAt fidelity, contract revert, MCP poison handler

Four review fixes from PR #166 round 11:

1. **routes/agents.ts:487 (CRITICAL)** — Rollback-poisoned escape path
   hardcoded `failedAt.reason="throw"` even when the forward setManager
   returned "cycle"/"not-found" cleanly. Hoisted forwardReason above
   both CachePoisonedError branches so it's computed once and reused
   correctly. Forward "cycle"/"not-found" failures that happen to hit
   rollback-CachePoisoned no longer get misattributed as transient FS
   throws.

2. **routes/agents.ts:420** — Forward CachePoisonedError fast-path was
   missing the `failedAt` field that every other DELETE error response
   includes. Clients had to parse a different shape depending on
   failure mode. Added `failedAt: { id, name, reason: forwardReason,
   message? }` so all DELETE failures share one envelope.

3. **store.ts:392** — Round 9 widened the `buildAgentTree` filter from
   `status === "running"` to `status !== "exited"` to match the docstring's
   wording. The reviewer (correctly) flagged this as a user-visible
   contract change for existing MCP/REST consumers — `starting` agents
   would suddenly appear in the org chart. Reverted the filter and
   updated the docstring to honestly describe the strict-running
   behavior plus rationale (preserves pre-refactor `buildOrgChartFromAgents`
   contract; transient states deliberately omitted so the chart stays
   stable while sessions warm up).

4. **mcp.ts:330** — MCP `set_manager` tool called `setManager()`
   unwrapped, despite writeAgentFile now throwing CachePoisonedError.
   The MCP framework would catch and produce a generic error, losing
   the CACHE_POISONED signal that the REST surface emits via 503.
   Wrapped in try/catch: returns isError MCP response with explicit
   CACHE_POISONED prefix and a "retry pointless until restart" hint
   so MCP clients (agents) see the same stable signal HTTP clients do.

All 306 tests pass.

* fix(hierarchy): round 12 — explicit onError 500, dangling-managerId catch

Two review fixes from PR #166 round 12:

1. **routes/agents.ts:59** — `onError` was `throw err`-ing for non-CachePoisonedError
   cases, relying on undocumented Hono behavior. Depending on Hono version,
   a re-throw from an error handler can surface as an unhandled rejection
   or connection drop instead of the structured 500 the operator expects.
   Replaced with an explicit `return c.json({ error: message }, 500)` plus
   logging the stack server-side so the canonical record is preserved.

2. **store.ts:294** — The `setManager` no-op short-circuit ran BEFORE the
   `cache.has(managerId)` existence validation. If `existing.managerId`
   pointed to a manager whose record had been deleted on disk but whose
   eviction hadn't yet propagated to the in-memory cache (idempotent
   retries, stale UI, migration window), passing the same dangling id
   would silently re-affirm the broken pointer. Reordered: existence
   check now runs first, so dangling refs return undefined regardless
   of whether the caller's argument matches the existing field.
   Self-loop check also hoisted above the short-circuit for symmetry.

All 306 tests pass.

* fix(dashboard): use providerSessionId for /api/conversation lookup

Real ship-blocking regression introduced by the agent-unification PR.

**The bug**

`SessionInfo.claudeSessionId` previously equaled the CC provider session id;
this PR remapped it to `agent.id` (a dashboard-internal stable lookup key)
and added a separate `providerSessionId` field for callers that read CC's
JSONL files. `ConversationView.tsx:412` is exactly such a caller and was
not updated.

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() distinct from the CC session id — `/api/conversation/:id`
scans `~/.claude/projects/<encoded-cwd>/<id>.jsonl` which CC writes keyed
on its own session UUID, so the file doesn't exist and the conversation
panel surfaces "Session not found" 404.

**The fix**

One line in ConversationView.tsx — read `activeSession?.providerSessionId`
instead of `claudeSessionId`. Renamed the local variable for clarity and
added a comment explaining why the distinction matters.

Verified ConversationView is the only `/api/conversation/:id` consumer in
the dashboard (single grep result). All other claudeSessionId consumers
(store.ts resumeSession, mergeOrgWithSessions, HierarchyPanel) use it as
a stable lookup key — correct semantics post-refactor.

All 306 tests pass.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aterrylu added a commit that referenced this pull request Jun 26, 2026
…nd' (#254)

`self_exit` issued a hard `DELETE /api/agents/:id`, which rmSync'd the agent
record off disk and dropped it from the in-memory cache. The resume path
(`spawnAgent` with `resumeAgentId`) looks the agent up via `getAgent(id)`,
which then returned undefined → `throw resumeAgentId "<id>" not found`.

This was asymmetric with `kill_agent`, which soft-exits via `POST /:id/kill`
→ `killAttachment` → `markExited` and KEEPS the record as `status:"exited"`
(resumable). The contradiction has existed since the #165 Agent+Session
unification — self_exit has hard-DELETEd since #124, while resume began
requiring a persisted exited record. Not a regression from #249 (UI-only) or
#237 (Codex providerThreadId only).

Fix: route `self_exit` through the same soft-exit path as kill/crash so the
record survives and stays resumable, with an honest exit reason:
- `killAttachment(id, reason: ExitReason = "user_killed")` — parametrize reason
- `POST /:id/kill` accepts an optional `{reason}` body, validated against the
  ExitReason union (defaults to user_killed; logs unrecognized values instead
  of silently coercing them)
- `self_exit` POSTs `/:id/kill {reason:"self_exited"}` instead of DELETE
- `ExitReason` now derives from a single `EXIT_REASONS` tuple + `isExitReason`
  typeguard (drift-proof: a new reason can't desync a hand-maintained allowlist)

Verified end-to-end against a live dev server: spawn → self_exit → record kept
as exited/self_exited (HTTP 200, was 404) → resume succeeds (HTTP 201, was
"not found"); DELETE still hard-deletes (404 after). Regression test added.


Claude-Session: https://claude.ai/code/session_0158xdYsvWMjXfEXP4n9fVCd

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aterrylu added a commit that referenced this pull request Jul 18, 2026
…d (ADR-056) (#283)

* fix(server): resume external Claude Code sessions — adopt-into-managed (ADR-056)

Clicking resume on a terminal-started `claude` session in the Projects panel
failed with `failed to resume session`. Discovery was never broken — the panel
lists external sessions fine — but every resume path funneled through
`resumeAgentId`, resolved only by internal agent id. External sessions have no
autonomOS record, so the lookup missed and spawnAgent threw `not found` (404).

Regressed in 9ac5d5b (#165 "unify Agent + Session"), which collapsed two
distinct id-spaces — a raw CC session id vs an internal agent-record id — into
one param. The provider's `--resume <uuid>` machinery survived intact; only the
plumbing to reach it with an arbitrary id was removed.

spawnAgent gains a distinctly-named `resumeSessionId` (raw CC id) that resolves
against the store by providerSessionId (or agent id, for migrated records) →
reattach, else ADOPTS the external session into a new persistent managed record
and --resume's it. `resumeAgentId` + /attach are untouched, so the ADR-049
restart path carries no regression risk.

Also unifies `id == providerSessionId` for fresh/fork/adopt, removing the
chronic footgun where resuming a managed agent needed the autonomOS id but the
CC session id was tried first, 404'd, then retried. Pre-existing split-id agents
resolve via a providerSessionId fallback on /attach — no id migration (an id is
referenced by managerId, layout panes, and persisted sessions).

Adoption is fail-closed throughout, since a silently-empty session is worse than
an error:
- providers that can't prove a session exists on disk (Codex, Gemini) are
  rejected — ADR-056's scope is now enforced, not just documented
- session ids are validated as UUIDs before becoming a record filename
- a missing transcript returns 422 with no orphan record
- a thrown probe fails closed on adopt (fail-open is only right for reattach)
- a failed adopt no longer arms the fresh-session safety net, which would have
  overwritten providerSessionId and erased the pointer to the conversation
- reattach uses the record's own workingDirectory, not the caller's guess

Dashboard: stop rewriting resumeSessionId → resumeAgentId (the root cause on the
client side), surface the server's error reason instead of a generic status, and
match sessions on both id-spaces so a running split-id agent switches panes
instead of erroring.

Verified end-to-end against a real terminal-started session: discovered, adopted,
`--resume` emitted, same transcript continued, and the resumed agent recalled a
phrase planted before the adopt. Same via MCP create_agent. All guards verified
to reject with zero orphan records.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRSVXE4eMY4ZehhCBB3Qi

* fix(server): harden adopt guards + make them testable (review follow-ups)

Follow-ups from /pr-review-toolkit:review-pr on #283.

Real bug: the dashboard's dual-id `matchesId` matched when BOTH the argument
and a session's providerSessionId were undefined — SessionInfo's id fields are
optional, so a malformed entry would match an arbitrary session and switch the
user to the wrong pane. Guarded on a truthy id.

Testability, not just behavior — two critical guards were correct but unpinned,
one refactor away from silently regressing:

- The adopt veto on the onExit safety net lived as a `resolution !== "adopt"`
  conjunction at the callsite, outside any test. Moved INSIDE
  resumeSafetyNetArmed as an `isAdopt` param so it's covered by the same tests
  as the rest of the arming logic. If it regresses, an adopted session that
  crashes on boot has its providerSessionId reset to a fresh UUID and is
  respawned — erasing the only pointer to the user's conversation, silently,
  after the API already returned 201.
- The runtime→HTTP status mapping matched phrases authored in runtime.ts from
  routes/agents.ts, a cross-file coupling nothing pinned. Extracted as
  `spawnErrorStatus(message)` and tested from both ends: each phrase maps to its
  status, AND assertAdoptable's actual thrown messages classify as 422/400.
  Rewording a runtime error had silently degraded an actionable 4xx to a 500.

Both ordering hazards the comments only asserted are now tests: a cwd containing
the literal text "not found" must still map 422 (not 404), and an
unsupported-provider message must resolve 422.

Also corrected a test-suite honesty problem: the "id == providerSessionId
invariant" block seeded unified-id records by hand and asserted lookups against
its own fixture — tautological, and a regression in the code that MINTS the ids
would have passed. Renamed to "unified-id lookup contract" and documented that
the minting is covered by the end-to-end QA, not here.

Verified: full browser click-through against an isolated server — clicked the
real resume row in the Projects panel, the session adopted into a managed agent,
a terminal opened, and typing into it returned the passphrase planted before the
adopt. biome + make check + AUTONOMOS_INTEGRATION=1 make check: 660 server + 254
dashboard tests, 0 failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRSVXE4eMY4ZehhCBB3Qi

* fix(server): move the empty-id guard to spawnAgent so MCP can't bypass it

Review catch (nox-0x on #283): the present-but-empty resume/fork id check lived
in the REST route, but the HTTP MCP handler (mcp.ts) calls spawnAgent DIRECTLY
rather than through /api/agents — so an empty-string resumeSessionId from that
entry point bypassed the guard entirely. Zod's z.string().optional() accepts "",
and every dispatch in spawnAgent is truthiness-based, so an MCP caller who
intended to resume but lost the id would get a fresh empty agent reported as
success. Exactly the silent failure the guard exists to prevent.

Verified before fixing: mcp.ts calls spawnAgent directly; channel-server/index.ts
POSTs to /api/agents (so it was already covered); the guard was route-only.

Moved into spawnAgent — the shared boundary every caller inherits, including
future direct callers. The message carries the "invalid session id" prefix so
spawnErrorStatus still classifies it 400. Route-level duplicate removed, with a
comment recording why the check is not there.

Tests pin both halves: message→400 classification, and PLACEMENT — the new cases
call the real spawnAgent and assert it rejects an empty/whitespace-only id for
all three fields, plus a negative that absent fields are untouched (a guard that
fired on absence would break every plain spawn). These need no PTY or `claude`
binary because the guard runs before cwd validation and binary resolution.

biome + make check + AUTONOMOS_INTEGRATION=1 make check: 668 server + 254
dashboard tests, 0 failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRSVXE4eMY4ZehhCBB3Qi

* fix(server): persist adopt provenance + close remaining review findings

Round 3 of review on #283 (three specialist agents). The headline finding is a
hole in my own earlier fix.

CRITICAL — the adopt veto on the resume safety net was spawn-scoped. It gated on
the spawn-local `resolution`, so it protected only the spawn that DID the
adopting. Afterwards an adopted record is indistinguishable from a fresh one
(both satisfy id == providerSessionId), so every LATER resume took the reattach
path with the net fully armed — and the first failure there regenerated
providerSessionId, leaving `--resume` pointing at an id with no transcript. The
user gets a healthy, named, EMPTY agent. Same failure I "fixed", one retry later.

Provenance is now PERSISTED as Agent.adoptedExternal and read from the record, so
the veto holds for the life of the agent. (Two reviewers disagreed on the
consequence; the record's `id` does survive, so a retry reattaches this record
rather than adopting a duplicate — the comments claiming otherwise, in three
places, are corrected.)

HIGH — reattachCwd reassigned `cwd` AFTER the only statSync, so a record whose
directory had since been deleted reached the PTY spawn unvalidated. Routine here:
wt-sync removes merged worktrees out from under agents that ran in them. Either
node-pty throws (a 500 for a 400-class problem) or the child dies instantly and
arms the safety net for a reason unrelated to resumability. Now re-validated with
a message naming the actual cause.

Also fixed:
- Adopting passed the session SUMMARY as the agent name, which became its
  agent:// address AND was sent as `--name` on the resume — rewriting customTitle
  on the user's own external session. Newly-live behavior (this path 404'd
  before). The dashboard no longer forwards it; the server mints `<dir> · <id>`.
- Non-string resume/fork ids (`resumeSessionId: 3421`, `null`) were coerced to
  undefined by the route and answered with a fresh empty agent. The boundary
  guard now rejects present-but-not-a-non-empty-string, and the route forwards
  the raw value so the guard can see it.
- The adopt-failure notification is the ONLY signal (the net is disarmed), but
  was gated on `crashed` + <5s — narrower than the failure. A CC path that errors
  and exits 0 was silent. Now fires on any short-lived adopt exit, worded by exit
  code.
- `Invalid working directory` and `Cannot use both ...` fell through to 500;
  both are client errors → 400.
- /attach had a hand-rolled status mapping that contradicted spawnErrorStatus's
  own doc comment ("the two entry points agree"): a live namesake returned 500
  there and 409 from POST /. Now shares the classifier.
- Sidebar had two more id-space crossings identical to the matchesId bug:
  liveSessionIds (split-id agents never showed the live dot) and sessionMetaMap
  (they lost summary/project/branch enrichment).
- Comment corrections: the resumeSessionId agent-id fallback is CONTRACT (what
  create_agent advertises), not migration residue; the id==providerSessionId
  invariant is mint-time only (the force-fresh net re-splits it); the pre-flight
  fresh-fallback applies to reattach only; spawnErrorStatus's coupling is
  partially pinned, and its ordering is not injection-proof in both directions.
- ADR-056 gained a Decision §5 recording all the fail-closed guards — three of
  them MODIFY ADR-049 behavior and so belong in the durable record, not just the
  changeset. FEATURES.md row no longer contradicts its neighbor.

Re-verified e2e after the changes: non-string → 400; adopt → 201 with
adoptedExternal true, persisted on disk, and surviving a kill+reattach; name is
`<dir> · <id>` not the summary; --resume emitted with zero fresh-fallbacks; and
the resumed session recalled AMBER-LYNX-31, planted before the adopt.

biome + make check + AUTONOMOS_INTEGRATION=1 make check: 669 server + 254
dashboard tests, 0 failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRSVXE4eMY4ZehhCBB3Qi

* refactor(server): typed SpawnError + tie-break for the psid resolver

Type-design review follow-ups on #283. All three are bounded; the larger
recommendations from that review are deferred (see below).

SpawnError — the throw sites this PR ADDED now declare their own HTTP status
instead of the route inferring intent by substring-matching prose authored in
another file. That coupling was one this PR created, and the repo already had
the pattern to avoid it (CachePoisonedError + the router's onError). Messages
are byte-identical, so spawnErrorStatus's chain classifies them the same for any
caller that sees only text; it stays as the fallback for the pre-existing
untyped throws. A test asserts the two mechanisms AGREE, so a typed status and
the chain can't silently diverge.

getAgentByProviderSessionId returned readCache()'s first hit — i.e. filesystem
read order, arbitrary and unstable across restarts. Nothing enforces
providerSessionId uniqueness, and the ADR-049 safety net regenerates the field,
so the id space isn't collision-free by construction. Now mirrors its sibling
resolveAgentByName: prefer a running candidate, else most recently updated. The
asymmetry between two adjacent lookup functions — one treating multiplicity as
designed, the other as impossible — was the smell.

buildNewAgent's positional boolean (`buildNewAgent(psid, true)`) is now an
options object; the callsite reads `{ adoptedExternal: true }`.

Deferred deliberately, recorded so they aren't lost: extracting a pure
`resolveSpawnTarget` returning a SpawnTarget discriminated union (two reviewers
asked for this; it restructures the hottest path in the change right when the
current version is freshly tested); replacing `adoptedExternal?: boolean` with a
richer `origin` union unified with FEATURES.md's unimplemented `source` concept
(persisted-schema change during a bug fix is a bad trade, and the boolean is
forward-compatible); narrow AgentId/ProviderSessionId branding on store accessor
signatures only; and converting the remaining legacy throws.

Verified through the real HTTP layer: bad UUID → 400, Codex adopt → 422, no
transcript → 422, empty id → 400, zero agent records created by any rejection.
biome + make check + AUTONOMOS_INTEGRATION=1 make check: 670 server + 254
dashboard tests, 0 failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HnRSVXE4eMY4ZehhCBB3Qi

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants