Multi-architect conversation resume via persisted per-architect session ID (#832) - #1116
Merged
Conversation
…rchitectLaunch helper + jsonl-existence guard
…sert (additive column only)
…oad-bearing assumption verified)
…ord embedded-sessionId finding
… main (eliminates upgrade-boundary loss)
…sionId, sessionFileExists, deleteArchitectSessionFile)
…; prune session jsonl on removeArchitect
…th restart-bake sites)
…sion, sibling isolation, jsonl prune
…tional capture-sessions backfill
… + migration v12 + state setters)
…tureRunningClaudeSession; drop derived-id helpers
…ss, mint+return on fresh
…captureArchitectSessions; simplify removeArchitect
…flag + route + client)
…resolve decision, capture fallback
…t the conversation sessions (which persist on disk); capture-before-deactivation is for lsof on the live process
… script Remove the --capture-sessions flag, the /capture-sessions REST route, the captureArchitectSessions client method + Tower function. Replace with scripts/backfill-architect-sessions.ts (library-only, reads pids from terminal_sessions, writes via new targeted setArchitectSessionId). Keeps the transitional backfill out of the day-to-day CLI/API surface.
…nt HarnessProvider interface The interface now carries only the steady-state pin/resume contract (newSessionArgs/resumeArgs). The backfill script calls captureRunningClaudeSession directly (Claude-specific, transitional), gated on the harness.session capability. Keeps backfill-only concerns out of the core agent abstraction.
Runs the full (read-only) resolution and prints the exact session id each architect would receive, without writing. Re-run without --dry-run to apply.
…tect for clarity (vs the PtySession terminal id)
… architects in one run Enumerates targets via SELECT DISTINCT workspace_path FROM terminal_sessions WHERE type='architect'. Refactors per-workspace logic into backfillWorkspace(); --all and --dry-run compose. Single-workspace path arg still supported.
…Client, script is now a thin client Add PUT /api/workspaces/:ws/architects/:name/session-id (narrow transitional setter -> setArchitectSessionId) + TowerClient.setArchitectSessionId. Rewrite backfill script as a pure Tower client: enumerate via listWorkspaces, read live architect pids via getWorkspaceStatus, capture via lsof, write through the setter. No state.db/config imports -> no cwd footgun; runs from anywhere. Avoids putting session_id on the wire ArchitectState (captures every live architect; idempotent re-write for already-#832 ones).
…ch resume site resolveArchitectLaunch now returns resumed:boolean; the four callers (launchInstance, addArchitect, both restart-bake sites) emit an INFO log when a stored session is resumed, so end-to-end resume is verifiable from Tower logs.
…correlation Claude does not hold its session jsonl open, so the lsof process->open-file correlation in captureRunningClaudeSession never matched — every capture came from the sole-architect mtime fallback, which can't disambiguate siblings. Read --session-id/--resume <uuid> straight off the process subtree's command line instead: exact, and unambiguous for siblings sharing a cwd. Extract a pure extractSessionIdFromCmdline helper (+8 unit tests). Keep the sole-architect mtime fallback for pre-#832 fresh spawns; siblings without the arg self-heal on first #832 revival. Skip message now explains the per-case reason.
… for main The backfill (script + Tower setter route + TowerClient method + live-process capture) could only ever rescue main, never siblings: Claude holds no jsonl fd open, so process->jsonl correlation never worked, and a pre-#832 sibling's id lives only as a jsonl filename with no robust pid bridge. But main was already self-recovering under #830 via sole-architect jsonl discovery -- a path this branch had removed. Restore it instead: launchInstance falls back to findLatestSessionId when getArchitects()<=1, and resolveArchitectLaunch persists the resolved id so it self-migrates into the stored-UUID path. The <=1 check now gates only the legacy fallback, not resume itself -- stored-UUID resume applies regardless of architect count. Deletes backfill-architect-sessions.ts, state.setArchitectSessionId, the PUT session-id route, TowerClient.setArchitectSessionId, captureRunningClaudeSession/ extractSessionIdFromCmdline/processSubtree/execCapture, and their tests. Every architect self-heals in one revival cycle; single-architect main never regresses. Core + codev build; suite 3389 passed | 48 skipped.
…ANGES) Codex's review-phase consult flagged two planned-but-missing tests (migration v12, tower-terminals restart resume) and an overstated coverage claim in the review. - Add pir-832-migration.test.ts: v12 ALTER adds session_id to a real post-v11 table, legacy rows read null, idempotent, duplicate-column swallowed on fresh installs. - Extract the duplicated restart-bake glue at both tower-terminals.ts sites into resolveArchitectRestart (tower-utils.ts) and unit-test it: stored id -> --resume, legacy/no-row -> fresh, per-name no cross-attachment. - Correct the review's Test Results + document the disposition. Full suite 3397 passed | 48 skipped (+8).
Collaborator
Author
Architect Integration ReviewHigh-risk PR (state.db migration, launchInstance / addArchitect / shellper-restart paths, conversation-resume semantics). Independently verified each of #832's six acceptance criteria against the source on PR head, plus the codex-disposition additions in 333e4e0. Acceptance criteria — all present
Codex-disposition additions in 333e4e0
Design quality observations (beyond acceptance criteria)
Risk surface that's now closedPer the issue's expanded body, the named-architect specialisation-loss problem (a
The single-architect workspace's main-resume is also restored (the RecommendationAPPROVE. All acceptance criteria present and tested; the codex disposition is genuine (not papered-over); the design quality is high (clean helper extraction, agent-neutral schema, observable resume path); the documented risk surfaces all close. Strong PR. Architect integration review |
# Conflicts: # codev/resources/lessons-learned.md # packages/codev/src/agent-farm/servers/tower-instances.ts # packages/codev/src/agent-farm/utils/harness.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PIR Review: Multi-architect conversation resume via persisted per-architect session ID
Fixes #832
Summary
Tower now revives every architect —
mainand named siblings (Spec 755) — into its own prior Claude conversation after a reboot, restart, or in-process crash, instead of onlymain. Each architect row persists an agent-neutralsession_idgenerated at spawn; every revive surface reads it back and resumes via--resume. This removes #830's conservativegetArchitects() <= 1guard that disabledmain's resume whenever any sibling existed, and closes the silent-context-loss path where a specialised sibling (reviewer, demos, …) would come back as a generic architect that had lost its first-message brief.Files Changed
git diff --statagainst the merge-base (0065189):codev/plans/832-multi-architect-conversation-r.md(+321 / -0)codev/projects/832-multi-architect-conversation-r/status.yaml(+22 / -0)codev/state/pir-832_thread.md(+294 / -0)packages/codev/src/agent-farm/db/schema.ts(+1 / -0)packages/codev/src/agent-farm/db/index.ts(+20 / -0) — migration v12packages/codev/src/agent-farm/db/types.ts(+2 / -0)packages/codev/src/agent-farm/types.ts(+4 / -0)packages/codev/src/agent-farm/state.ts(+10 / -0)packages/codev/src/agent-farm/utils/harness.ts(+19 / -0) —sessioncapabilitypackages/codev/src/agent-farm/servers/tower-utils.ts(+48 / -0) —resolveArchitectLaunchpackages/codev/src/agent-farm/servers/tower-instances.ts(+108 / -? net) — main + sibling spawn/revivepackages/codev/src/agent-farm/servers/tower-terminals.ts(+41 / -?) — both restart-bake sitespackages/codev/src/agent-farm/__tests__/tower-utils.test.ts(+75 / -0)packages/codev/src/agent-farm/__tests__/state.test.ts(+54 / -0)packages/codev/src/agent-farm/__tests__/harness.test.ts(+17 / -0)packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts(+5 / -0)codev/resources/lessons-learned.md(+1 lesson, this commit)Net: 16 source/test files, +976 / -65. (The plan/thread/status files are process artifacts.)
Commits
git log main..HEAD --oneline(implementation commits; porchchorecommits omitted):274fbdc4[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Drop backfill layer; restore sole-architect jsonl fallback for main6fac1649[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Backfill: read session id off the command line, drop lsof correlation50002398[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Log a 'Resuming architect session …' line at each resume site7dbf0bc6[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Backfill via Tower (Option B): narrow setter route + TowerClienta1079f62[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Persist agent-neutral session_id on architect rows (schema + migration v12 + state setters)c654b185[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Add HarnessProvider.session capability (Claude impl)2305b588[PIR Multi-architect conversation resume: disambiguate via per-architect session UUID #832] Rewrite resolveArchitectLaunch: stored-id resume via harness, mint+return on fresh274fbdc4)Test Results
pnpm build(core + codev): ✓ passpnpm test(from the worktree): ✓ 3389 passed | 48 skippedsession_idround-trip + removal-clears-id + two-siblings-distinct (state.test.ts); migration v12 adds the column with legacy rows reading back null;resolveArchitectLaunchresume / fresh / no-session / sibling-isolation (tower-utils.test.ts);CLAUDE_HARNESS.sessionproduces--session-id/--resumewhile Codex/Gemini omit it (harness.test.ts);findLatestSessionIddiscovery (claude-session-discovery.test.ts).dev-approval): reviewed the running worktree; the live--all --dry-runbackfill run (before that path was removed) empirically confirmed the root-cause finding that drove the final design (Claude holds no jsonl fd open).Architecture Updates
No
arch.mdchange. Architect-revival mechanics live in code (resolveArchitectLaunch+ the spawn/revive sites) and are documented in this review and the plan; the predecessor #830 mechanism was likewise never lifted intoarch.md, so adding it now would be per-spec noise. The change reinforces the existing hot-tier invariant "state lives in state.db (single source of truth)" — the architect's resumable identity is now a column on its row — rather than altering any documented invariant. Thearch-critical.mdhot tier is unchanged (no new always-on system-shape fact; the cap is preserved).Lessons Learned Updates
Routed one lesson to COLD
codev/resources/lessons-learned.md→ Architecture ([From #832]): a recovery mechanism that keys off a process holding a resource open (fd/lock) is only as sound as that assumption — verify it empirically before building on it (Claude closes its session jsonl between writes;lsofagainst a live architect falsified the whole capture subsystem in minutes, and it had passed unit tests only because the tests exercised the fallback). Corollary: don't delete a working self-recovery path to rebuild it as a bridge that can't cover the hard case. Not hot-tier: it's a spec-narrow design lesson, and the hotlessons-critical.mdalready carries the general "captured raw data beats speculation" rule it specialises. Hot tier unchanged (cap preserved).Things to Look At During PR Review
274fbdc4) is the crux. The branch history builds and then removes an entire transitional backfill layer. Confirm the net diff contains no orphans: noscripts/backfill-architect-sessions.ts, nosetArchitectSessionId(state/route/TowerClient), nocaptureRunningClaudeSession/extractSessionIdFromCmdline. A repo-wide grep for those symbols should be empty outsidedist/.getArchitects() <= 1check changed meaning, not just location. In Workspace recover: revive Tower-managed architects after machine reboot #830 it gated resume wholesale (the bug: any sibling → main spawns fresh). Now it gates only the legacy jsonl fallback inlaunchInstance; stored-UUID resume applies regardless of architect count. The issue's acceptance criterion literally says "remove the guard" — we kept a<= 1check but repurposed it. Worth a careful read oftower-instances.tslaunchInstanceagainst the plan's Revision note 2 to confirm the reinterpretation is sound.resolveArchitectLaunchreturns the resolvedsessionId, and every caller persists it. A legacy sole-mainrow therefore resumes via jsonl-discovery and gains a stored id in the same revival. Confirm the persist happens on the resume branch, not only the fresh branch.addArchitectpassesstoredSessionId ?? null) and the shellper-restart sites have no jsonl fallback (matching Workspace recover: revive Tower-managed architects after machine reboot #830). Both intentionally self-heal once for legacy rows — verify this is deliberate, not an omission.ALTER TABLE architect ADD COLUMN session_id TEXTin try/catch (idempotent). Old binaries ignore the nullable column.How to Test Locally
For reviewers pulling the branch:
pir-832→ View Diffafx dev pir-832(from the main workspace root)main+ a named sibling under this code (ids stored at spawn),afx workspace stop+afx workspace start; each resumes its own conversation and the sibling keeps its brief. Watch Tower logs forResuming architect '<name>' session <id8>…at each site.mainbridge: a single-mainworkspace with a pre-Multi-architect conversation resume: disambiguate via per-architect session UUID #832 row (no stored id) resumes viafindLatestSessionId, and the row then carries a stored id (self-migration) so the next restart takes the exact-id path.afx workspace remove-architect <name>then re-add → starts fresh (row deletion clearssession_id).