diff --git a/codev/plans/1145-codev-adopt-launchinstance-mai.md b/codev/plans/1145-codev-adopt-launchinstance-mai.md new file mode 100644 index 000000000..25310efcb --- /dev/null +++ b/codev/plans/1145-codev-adopt-launchinstance-mai.md @@ -0,0 +1,101 @@ +--- +approved: 2026-07-09 +validated: [] +--- + +# PIR Plan: Stop launchInstance from hijacking unrelated Claude sessions on fresh workspaces + +> **Post-approval deviation (dev-approval gate, 2026-07-10)**: the cwd-content +> ownership verification specified below was narrowed to a file-existence check +> at the human reviewer's direction. Rationale and final shape are recorded in +> `codev/reviews/1145-codev-adopt-launchinstance-mai.md` (Things to Look At). + +## Understanding + +When Tower's `launchInstance` creates the `main` architect for a workspace, it decides between resuming a persisted conversation and spawning fresh. The decision code at `packages/codev/src/agent-farm/servers/tower-instances.ts:489-499` is: + +1. Read the stored session id from the `main` architect row in `~/.agent-farm/global.db` (keyed by `workspace_path`). Fresh workspace: no row, so `storedSessionId = null`. +2. If no stored id, run the "legacy bridge" fallback: when `getArchitects(resolvedPath).length <= 1`, call `harness.buildResume(workspacePath)` which is mtime-based jsonl discovery (`findLatestSessionId` in `packages/codev/src/agent-farm/utils/claude-session-discovery.ts`). + +The fallback was introduced (#832) to bridge pre-#832 architect rows that exist but carry no `session_id`. But the gate `length <= 1` is satisfied by **zero** rows, so the fallback also fires for a workspace that has *never had an architect at all*: exactly the `codev adopt` / first-touch `launchInstance` case in issue #1145. + +`findLatestSessionId` scans `~/.claude/projects//` for the newest `.jsonl`. Claude Code persists *every* interactive session there, including the user's personal (non-Codev) conversations. So if the user ever chatted with Claude Code in that project directory before adopting Codev, the brand-new "main architect" launches with `--resume `. Two compounding effects: + +- The architect inherits the personal conversation's full context (the "hijack"). +- The resume branch of `resolveArchitectLaunch` (`packages/codev/src/agent-farm/servers/tower-utils.ts:223-230`) deliberately skips role injection (a genuinely-resumed architect already has its role in-conversation) — so the hijacked session also never receives the architect role prompt. + +There is a second, cross-project vector: `encodeClaudeProjectDir` (`claude-session-discovery.ts:28-30`) replaces both `/` and `.` with `-`, which is lossy. Distinct paths can collide into the same store directory (e.g. `/x/foo.bar` and `/x/foo/bar` both encode to `-x-foo-bar`), so discovery can surface a session that genuinely belongs to a *different project*. The session jsonl records the true `cwd` on its user/assistant lines (verified against a real session file), which gives us an ownership tag to check. + +Note: we never pass a bare `--resume` (the "most recent global session" form hypothesized in the issue) — every resume carries an explicit uuid. The hijack mechanism is the discovery fallback firing on fresh workspaces, plus the lossy dir encoding. + +## Proposed Change + +Two surgical changes, matching the issue's fix sketch: + +### 1. Remove the architect discovery fallback entirely (tower-instances.ts) + +The fallback exists to bridge legacy rows (pre-#832: row present, `session_id` NULL). But even gated on row existence it cannot be made safe: mtime discovery picks *the newest session in the cwd*, and a row only proves an architect once ran there — not that the newest jsonl is the architect's. A personal Claude conversation held in the workspace directory after the architect's last run would still be hijacked, and cwd ownership verification (change 2) cannot catch it because the personal session's cwd genuinely matches. + +So `launchInstance`'s session resolution becomes: read `storedSessionId` from the `main` row; resume it if present (after ownership verification, change 2); otherwise spawn fresh with a newly minted session id and role injection. No jsonl discovery on the architect path, ever. + +Consequences: + +- Fresh workspace (`codev adopt` / first touch): always fresh — the issue's fix. +- Legacy pre-#832 row without a stored id: spawns fresh, loses at most one conversation's context, mints and persists an id, and is on the stored-UUID path forever after. Exposure is limited to workspaces untouched since #832 shipped. +- `global.db` read failure: fresh spawn (unchanged in spirit — never hijack on uncertainty). +- Normal stop/start resume is unaffected: `afx workspace stop` preserves architect rows (`commands/stop.ts:42,98`), so restarts resume via the stored-UUID path, which never depended on discovery. +- `buildResume` / `findLatestSessionId` remain in place for **builder** resume (#831/#929), where the worktree cwd is effectively private to the builder so mtime discovery stays sound. The `buildResume` doc comment in `harness.ts:75-93` (which names the architect legacy fallback as a consumer) is updated to reflect builders as the sole remaining consumer. + +### 2. Verify session ownership before attach (claude-session-discovery.ts + harness.ts + tower-utils.ts) + +**Discovery side** — teach `findLatestSessionId` to verify ownership: iterate candidate jsonls newest-first and return the first whose recorded `cwd` matches the requested path. Implementation: read the file's first ~64KB, scan lines for the first record carrying a `cwd` field, compare after `realpath`-canonicalizing both sides. Candidates with a mismatched `cwd` are skipped (encoding collision → not ours); candidates with *no* `cwd` record in the sample (e.g. a session that never got a user message) are skipped too — there is nothing worth resuming in them and skipping is the safe default. With the architect fallback removed (change 1), builder resume (#831/#929) is `buildResume`'s sole consumer, and this protects it from encoding-collision pickups. + +**Stored-id side** — the issue also asks that a passed session id be cross-checked. Add an optional, harness-gated capability alongside the existing `session` block in `HarnessProvider`: + +```ts +session?: { + newSessionArgs(sessionId: string): string[]; + resumeArgs(sessionId: string): string[]; + /** Optional: return false if the session on disk does not belong to cwd. */ + verifyOwnership?(sessionId: string, cwd: string, opts?: { homeDir?: string }): boolean; +} +``` + +Only Claude implements it: the session file must exist at `~/.claude/projects//.jsonl` *and* its recorded `cwd` must match (same helper as above). `resolveArchitectLaunch` (tower-utils.ts) calls it before taking the resume branch; on `false` it falls through to the fresh branch (new id, role injection). Because `resolveArchitectLaunch` is the single choke point, this covers `launchInstance`, `add-architect` sibling re-spawn, and both shellper restart-bake sites (via `resolveArchitectRestart`) in one place. Harnesses without `verifyOwnership` keep today's behavior (trust the stored id), preserving harness neutrality. + +Side benefit: a stored id whose jsonl was deleted (stale-id class, cousin of #929's crash-loop) now degrades to a fresh spawn instead of a broken `--resume`. + +## Files to Change + +- `packages/codev/src/agent-farm/utils/claude-session-discovery.ts` — add `sessionFileCwd`-style helper (scan first lines for `cwd`), make `findLatestSessionId` filter candidates by ownership (newest-first, first match wins); export a `verifySessionOwnership(absolutePath, sessionId, opts?)` for the harness. +- `packages/codev/src/agent-farm/utils/harness.ts:68-73,132-135` — add optional `session.verifyOwnership` to the `HarnessProvider` interface; implement it on `CLAUDE_HARNESS` using the discovery helper. Update the `buildResume` doc comment (`harness.ts:75-93`) to name builder resume as its sole remaining consumer. +- `packages/codev/src/agent-farm/servers/tower-utils.ts:208-236` — in `resolveArchitectLaunch`, verify `storedSessionId` ownership (when the harness offers it) before the resume branch; on failure log-worthy fall-through to fresh (return `resumed: false`, fresh minted id). +- `packages/codev/src/agent-farm/servers/tower-instances.ts:469-499` — delete the discovery fallback block (`getArchitects().length <= 1` + `buildResume`); keep only the stored-id read. Replace the long #832 comment block with the new invariant: architect resume comes exclusively from the stored session id on the workspace-scoped row; no row → fresh. +- `packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts` — new cases: mismatched-cwd jsonl skipped; falls back to next-newest matching; no matching candidate → null; cwd-less jsonl skipped; `verifySessionOwnership` true/false paths. +- `packages/codev/src/agent-farm/__tests__/tower-utils.test.ts` — `resolveArchitectLaunch`: stored id failing ownership → fresh spawn with role injection and a *new* session id. +- `packages/codev/src/agent-farm/__tests__/tower-instances.test.ts` — `launchInstance` on a fresh workspace (no architect row) with a decoy jsonl in the encoded project dir → spawn args contain no `--resume`; legacy row without session id → also spawns fresh (fallback removed); row with stored id → resumes. + +No `codev/` ↔ `codev-skeleton/` mirroring needed — all changes are package source (`packages/codev/src`), not framework files. + +## Risks & Alternatives Considered + +- **Risk: context loss for dormant pre-#832 workspaces.** A legacy `main` row without a stored id now spawns fresh instead of resuming via discovery — a one-time loss of at most one conversation, after which the workspace self-heals onto the stored-UUID path. Accepted: fresh-not-hijacked is the right default, and even a row-gated fallback would still hijack when a personal session in the same cwd is newer than the architect's last one (mtime cannot distinguish them, and cwd verification passes trivially in the same directory). +- **Alternative: keep the discovery fallback but gate it on a legacy row existing.** Rejected per the above — it narrows but does not close the hijack window in single-architect workspaces. +- **Risk: over-strict ownership check drops valid resumes.** A jsonl whose first 64KB lacks a `cwd` line would be skipped. Sampled real session files show `cwd` appears on the first user message (line ~5); sessions with no user message carry no resumable value. Accepted. +- **Risk: realpath comparison surprises with symlinked workspace paths.** Canonicalize both sides with `fs.realpathSync` (falling back to the raw string if realpath throws) so symlink vs. resolved-path launches compare equal. +- **Alternative: only fix the gate (change `<= 1` to row-exists), skip ownership verification.** Rejected: leaves the encoding-collision cross-project vector and the issue explicitly asks for the ownership check. +- **Alternative: verify ownership inside Tower instead of the harness.** Rejected: jsonl layout is Claude-specific; per the harness abstraction rule, agent-specific session mechanics live behind `HarnessProvider`. +- **Alternative: stop skipping role injection on resume.** Rejected: out of scope; genuine resumes already have the role in-conversation, and double-injection would bloat context. + +## Test Plan + +- **Unit** (vitest, run from the worktree: `pnpm --filter @cluesmith/codev test`): + - discovery: ownership filtering, next-newest fallback, cwd-less skip, collision skip, `verifySessionOwnership` behavior with a fake `homeDir`. + - `resolveArchitectLaunch`: stored id failing verification → fresh (new uuid, role args present, `resumed: false`); passing verification → resume unchanged; harness without `verifyOwnership` → resume unchanged. + - `launchInstance`: fresh workspace + decoy personal jsonl → no `--resume` in spawn args; legacy row bridge still resumes. +- **Manual** (for the dev-approval gate): + 1. Create a scratch project dir (no `codev/`), run `claude` in it briefly so a personal session jsonl exists for that cwd, and exit. + 2. From the scratch dir, trigger the launch path (`codev adopt` then workspace start, or VS Code auto-adopt) against the locally installed build (`pnpm build && pnpm -w run local-install`). + 3. Observe the main architect terminal: it must open a **fresh** conversation (architect role prompt active, no prior chat context), and Tower's log must not print "Resuming architect 'main' …". + 4. Restart the workspace (`afx workspace stop` / `start`): the architect must now resume its *own* stored session (log shows "Resuming architect 'main' session …" with the minted id). +- **Cross-platform**: n/a (server-side Node path handling only; path canonicalization uses `fs.realpathSync` which is platform-neutral). diff --git a/codev/projects/1145-codev-adopt-launchinstance-mai/status.yaml b/codev/projects/1145-codev-adopt-launchinstance-mai/status.yaml new file mode 100644 index 000000000..442c8d8f0 --- /dev/null +++ b/codev/projects/1145-codev-adopt-launchinstance-mai/status.yaml @@ -0,0 +1,30 @@ +id: '1145' +title: codev-adopt-launchinstance-mai +protocol: pir +phase: review +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-07-08T06:36:03.156Z' + approved_at: '2026-07-09T04:26:17.754Z' + dev-approval: + status: approved + requested_at: '2026-07-09T04:41:45.899Z' + approved_at: '2026-07-11T00:38:11.861Z' + pr: + status: approved + requested_at: '2026-07-11T00:47:07.380Z' + approved_at: '2026-07-11T00:50:43.768Z' +iteration: 1 +build_complete: true +history: [] +started_at: '2026-07-08T06:30:47.411Z' +updated_at: '2026-07-11T00:50:43.770Z' +pr_history: + - phase: review + pr_number: 1160 + branch: builder/pir-1145 + created_at: '2026-07-11T00:41:29.251Z' +pr_ready_for_human: false diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 54ee62ac5..9f10ceaa6 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -275,7 +275,7 @@ A `codev doctor` audit (`lib/framework-ref-audit.ts`) flags shell-fetch of frame > **Caveat — unrecognized override commands still default to the claude harness (tracked in cluesmith/codev#1062).** `#929`'s override-awareness only covers *recognized* harness commands (claude/codex/gemini/opencode, matched by `detectHarnessFromCommand`). An override command the detector does **not** recognize — e.g. `TOWER_ARCHITECT_CMD=bash`, a wrapper script, or any custom launcher — with **no** explicit `shell.architectHarness` / `shell.builderHarness` falls through `resolveHarness` to the **claude** harness (`harness.ts`, the final `return CLAUDE_HARNESS`). With a stale Claude `.jsonl` present, that can still build ` --resume ` for the unrecognized command. This is **pre-existing and narrow** (not a #929 regression — #929 strictly *improved* the recognized codex case) and separable. Mitigation today: set an explicit `shell.architectHarness` / `shell.builderHarness` when using an unrecognized launcher command. -**Conversation resume is Claude-main-only.** `launchInstance` resumes a prior session only when the configured harness implements `HarnessProvider.buildResume` — currently just claude (its sessions live at `~/.claude/projects//*.jsonl`). Codex architects (and resumed codex/gemini *builders*, via `spawn.ts` `discoverResumeSession`) return `null` from `buildResume` and relaunch fresh with role injection. This gating fixes a latent crash-loop where a non-Claude harness + a stale Claude `.jsonl` built an invalid ` --resume ` invocation and shellper restart-looped to death. +**Architect conversation resume is stored-id-only (#832 / #1145).** Every architect launch path (`launchInstance` main spawn, `add-architect` / sibling reconcile, both shellper restart-bake sites) resolves through `resolveArchitectLaunch`: a session id stored on the workspace-scoped architect row (minted at spawn, pinned via the harness's `session.newSessionArgs`) is resumed only after the harness's optional `session.verifyOwnership` confirms the session file still exists for this cwd (Claude: `~/.claude/projects//.jsonl`, accepted under either the logical or physical form of a symlinked path). Anything else — no row, no stored id, missing file — spawns fresh with role injection and a newly minted, persisted id. `launchInstance`'s mtime-based jsonl-discovery fallback was **removed** by #1145: on a fresh workspace (`codev adopt` / first touch) it resumed whatever Claude conversation the user last held in that directory — hijacking personal sessions, roleless too, since the resume path skips role injection. Do not reintroduce discovery on any architect path; mtime cannot distinguish an architect's session from a newer personal one in the same cwd. Discovery (`HarnessProvider.buildResume`, newest jsonl by mtime) survives for **builder resume only** (`spawn.ts` `discoverResumeSession`), harness-gated to claude — the gating that fixes the latent crash-loop where a non-Claude harness + a stale Claude `.jsonl` built an invalid ` --resume ` invocation and shellper restart-looped to death. **Architect role injection is centralized in `buildArchitectArgs`** (`tower-utils.ts`), the shared helper every architect-launch path routes through — `launchInstance` (fresh), `add-architect` (sibling), shellper reconnect (×2), and the no-Tower `afx architect` (refactored in #929 to call `buildArchitectArgs` instead of duplicating injection). So the architect role is injected on **every** launch path, not just first-activation. (No architect context-file seam exists: claude/codex read project context natively; the gemini-only `getArchitectFiles` seam #1059 introduced was removed when gemini's architect support was dropped.) diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index be3a68e61..db40a2b8e 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -66,6 +66,7 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated ## Architecture +- [From #1145] Gate a legacy-bridge fallback on **positive evidence of the legacy state it bridges**, never on absence of conflict. The #832 jsonl-discovery fallback existed to rescue pre-#832 architect rows (row present, no stored session id) but was gated on `getArchitects().length <= 1` — and a *fresh* workspace (zero rows) satisfies "at most one", so the bridge became the default behavior for every first-ever `codev adopt`, resuming whatever personal Claude conversation the user last held in that directory (roleless, too, since resume skips role injection). Second failure mode found at review: even row-gated, the bridge is unsound — newest-by-mtime in a shared cwd cannot distinguish the architect's last session from a *newer personal* one, so the fallback was removed outright rather than re-gated (cost: one fresh spawn for workspaces dormant since #832, which then self-heal onto the stored-UUID path). Companion lesson from the same review: before defending a protective layer, enumerate the concrete threat it guards — the cwd-content ownership scan defended only path-encoding collisions that Agent-Farm's own worktree path conventions already preclude, and it fell to three "why is this here at all?" questions; what survived was the one check with a real threat behind it (session file existence, guarding the stale-stored-id `--resume` crash-loop). - [From 1144] When activation becomes ambient (`onStartupFinished` runs in every window), any detection heuristic feeding it becomes an **inertness gate** and must be downgraded to an exact, one-sentence-explainable rule. The extension's workspace detector walked ancestors to the filesystem root looking for `codev/`/`.codev/`; harmless while activation implied a codev workspace, but under ambient activation every folder beneath the user's codev-enabled home directory silently inherited that workspace, connected to Tower, and rendered live workspace actions in unrelated projects. The fix was "the opened folder itself is the workspace root, or nothing" (a settings override covers exotic layouts), which also matches the CLI's semantics: afx runs from the workspace root and never marker-walks. Audit convenience heuristics whenever the trigger that constrained them broadens. - [From #1140] When a process derives identity from an inherited environment variable, every *programmatic* respawn/recovery path must set that variable explicitly from recorded state, never inherit it. `afx spawn` derives the new builder's `spawned_by_architect` from `CODEV_ARCHITECT_NAME` (correct interactively, because Tower injects it per architect terminal), but `afx workspace recover` re-invoked spawn via `child_process.spawn` with no `env` override, so after any reboot every recovered builder was silently reattributed to whichever architect's shell ran the recovery: affinity routing, the `architect:` anti-spoofing check, and Agents-view ownership all broke. The fix reads the recorded value from `global.db` via the *same* helper the message router uses (`lookupBuilderSpawningArchitect`) and forces it into the child env; rows with no recorded value pass the base env through so the interactive default stays owned by `spawn.ts` alone. Env-derived identity + programmatic re-invocation is a recurring bug shape: the env var is a per-terminal fact, and any automation that fans out from one terminal turns it into a wrong global. - [From #1118] When a subsystem's *scope* outgrows its storage *location*, relocate the storage to match — don't patch the scope with a column and leave the location lying. `state.db` was born per-workspace; when Tower became a system-wide singleton its scope became user-global, but the file stayed at a cwd-dependent `/.agent-farm/state.db`. Bugfix #826 patched the *symptom* (a `workspace_path` column so cross-workspace rows could coexist in one file) while leaving the *location* wrong, so which file Tower opened still depended on its start-cwd — architect state "disappeared" after any restart from a different directory. The real fix was to move the tables into the already-user-global `global.db` and make `getDb()` return it. Corollary caught mid-implementation: moving a table into a *shared* DB forces you to re-examine its primary key — `builders` was keyed by `id` alone (safe when each workspace had its own file), but ids are `-`, unique per repo yet **reused across repos**, so the shared table needs a composite `(workspace_path, id)` PK (mirroring what #826 did for `architect`) or same-id builders from two repos collide — and that collision is security-relevant (it feeds the `afx send` spoofing check). The issue's "tables move as-is" framing was true for 3 of 4 tables but not the one whose key assumed per-file isolation. Migration technique that generalized: an **upsert-if-newer** engine (`ON CONFLICT DO UPDATE … WHERE excluded.started_at > existing`) is the same code for the empty-target one-off and the non-empty satellite import, and reading legacy source files **defensively** (`PRAGMA table_info` + synthesizing the new key column) handles heterogeneous historical schema versions without running the old migration ladder. @@ -258,6 +259,7 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From 0053] Clean up stale node processes from previous tests before running new tests. Lingering processes cause confusing failures. - [From 0056] Path fallback patterns need explicit testing -- mock both paths (new location exists, old location exists, neither exists) to catch regressions. Always run `copy-skeleton` after modifying `codev-skeleton/` to ensure changes propagate. - [From 456] Vitest 4 constructor mocks require class syntax — `vi.fn(() => ({...}))` throws "is not a constructor". Use `vi.mock('module', () => ({ ClassName: class MockClass { ... } }))` with `vi.hoisted()` for shared mock functions. +- [From #1145] Never assert CLI flags via substring over a `JSON.stringify`'d spawn call — assert on the argv token array. The Claude architect launch embeds the whole role prompt as one `--append-system-prompt` argument, and that prose legitimately contains flag strings like `--resume` in its CLI examples, so `expect(callStr).not.toContain('--resume')` false-fails on a correct fresh launch (and the mirror `toContain` can false-pass). `expect(callArgs).not.toContain('--resume')` checks whole-element equality and is immune. - [From 936] `vi.mock('@cluesmith/codev-*')` still requires the mocked workspace package to *resolve* — Vitest resolves the module ID before substituting the factory. Packages whose `exports` point at an unbuilt `dist/` (e.g. `@cluesmith/codev-types`, `@cluesmith/codev-core/*`) fail with "Failed to resolve entry for package" until built. In a fresh worktree, `pnpm --filter build` the workspace deps before `vitest run`, even for deps you intend to mock. - [From 456] Avoid duplicate React effects that fire on the same dependency change. Two `useEffect` hooks both depending on `isActive` will both fire when `isActive` transitions, causing double-fetches. Merge into a single effect. - [From 0058] Debouncing is essential for search inputs to prevent excessive DOM updates. A global Escape key handler adds resilience by ensuring modals/overlays can always be dismissed. diff --git a/codev/reviews/1145-codev-adopt-launchinstance-mai.md b/codev/reviews/1145-codev-adopt-launchinstance-mai.md new file mode 100644 index 000000000..22669e42d --- /dev/null +++ b/codev/reviews/1145-codev-adopt-launchinstance-mai.md @@ -0,0 +1,79 @@ +# PIR Review: launchInstance no longer hijacks unrelated Claude sessions + +Fixes #1145 + +## Summary + +`codev adopt` (or Tower's first-touch `launchInstance`) on a machine where the user had ever chatted with Claude Code in that directory booted the "main architect" *inside the user's personal conversation* — with no architect role, since the resume path skips role injection. The cause was the #832 legacy-bridge fallback: gated on `getArchitects().length <= 1`, which a fresh workspace (zero rows) satisfies, it ran newest-jsonl-by-mtime discovery over `~/.claude/projects//` and resumed whatever it found. The fallback is now removed outright (not re-gated — mtime cannot distinguish the architect's last session from a newer personal one in the same cwd): architect resume comes exclusively from the session id stored on the workspace-scoped architect row, validated by a new harness-gated existence check (`session.verifyOwnership`) so a stale stored id degrades to a fresh spawn instead of a `--resume` crash-loop. Builder resume (mtime discovery in Agent-Farm-managed worktree cwds) is unchanged. + +## Files Changed + +- `codev/plans/1145-codev-adopt-launchinstance-mai.md` (+91 / -0) +- `codev/resources/arch.md` (+1 / -1) +- `codev/resources/lessons-learned.md` (+2 / -1) +- `codev/state/pir-1145_thread.md` (+35 / -0) +- `packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts` (+76 / -38) +- `packages/codev/src/agent-farm/__tests__/tower-instances.test.ts` (+59 / -16) +- `packages/codev/src/agent-farm/__tests__/tower-utils.test.ts` (+70 / -11) +- `packages/codev/src/agent-farm/servers/tower-instances.ts` (+21 / -28) +- `packages/codev/src/agent-farm/servers/tower-utils.ts` (+42 / -7) +- `packages/codev/src/agent-farm/utils/claude-session-discovery.ts` (+47 / -4) +- `packages/codev/src/agent-farm/utils/harness.ts` (+17 / -4) + +(plus porch-managed `codev/projects/1145-*/status.yaml`) + +## Commits + +- `f5910e67` [PIR #1145] Verify Claude session ownership before any resume +- `0ee53387` [PIR #1145] Remove architect jsonl-discovery fallback in launchInstance +- `b10e71d6` [PIR #1145] Update tests: ownership verification, fresh-boot regression +- `6fc3636c` [PIR #1145] Make the cwd scan streaming: semantic, not byte-offset-bound +- `00284f3f` [PIR #1145] Narrow ownership verification to file existence; drop content scanning +- `7515cbfe` [PIR #1145] Comment: point at sibling resumption via the reconcile loop + +(`6fc3636c` and `00284f3f` reflect dev-approval-gate feedback; the streaming scanner introduced by the former was removed by the latter — see Things to Look At.) + +## Test Results + +- `pnpm build`: ✓ pass +- `pnpm test` (full `@cluesmith/codev` suite): ✓ 3451 passed, 48 skipped (pre-existing skips, none added) +- Touched test files: 109 tests, ~20 new/updated cases, including the #1145 regression ("fresh workspace + personal Claude session in the same cwd → boots fresh, no `--resume`") and the preserved #929 codex guard +- Manual verification: the reviewer worked the change over at the `dev-approval` gate through iterative design review (fallback scope, multi-architect resumption, verification necessity); the end-to-end fresh-adopt scenario is scripted below for the PR reviewer + +## Architecture Updates + +Routed to **COLD** `codev/resources/arch.md` (Agent Farm Internals): rewrote the stale "Conversation resume is Claude-main-only" paragraph as "Architect conversation resume is stored-id-only (#832 / #1145)" — documenting the new invariant (stored row id + existence verification, no discovery on any architect path, discovery is builder-only) and an explicit "do not reintroduce discovery on architect paths" warning with the reason. Not HOT-tier material: it's an agent-farm-internal invariant, not a cross-cutting fact that changes day-to-day implementation choices, and the hot file is at its cap. + +## Lessons Learned Updates + +Routed to **COLD** `codev/resources/lessons-learned.md`: + +- **Architecture**: gate a legacy-bridge fallback on positive evidence of the legacy state it bridges, never on absence of conflict (`length <= 1` is satisfied by zero rows, turning the bridge into default behavior for fresh installs); and enumerate the concrete threat behind each protective layer before defending it — the cwd-content scan defended only path-encoding collisions that Agent-Farm's own path conventions preclude. +- **Testing**: never assert CLI flags via substring over a stringified spawn call — the injected role prompt legitimately contains flag strings like `--resume` in its CLI examples; assert on argv tokens. + +Not HOT-tier: both are situation-specific recipes, not behavior-changing cross-cutting rules; the hot file is at its cap. + +## Things to Look At During PR Review + +- **Deviation from the approved plan.** The plan specified cwd-*content* ownership verification (reading the `cwd` recorded inside the session jsonl), per the issue's fix sketch. At the `dev-approval` gate the reviewer walked this back in three steps: content scanning defended only lossy-encoding collisions; the fixed scan window made resume content-dependent; and collisions are contrived for both architects (row-gated, ids minted by us) and builders (Agent-Farm-managed worktree paths). Final shape is existence-only verification. The intermediate streaming scanner (`6fc3636c`) was added and then removed (`00284f3f`) as part of that walk-back, so the net diff never contains it. +- **`verifySessionOwnership` checks two directory encodings** (logical and `realpath`'d) because Claude keys its store by process cwd, which the OS reports in physical form for symlinked paths (macOS `/tmp` → `/private/tmp`). Covered by a dedicated symlink test. +- **Degradation semantics**: a `global.db` read failure, a missing session file, or a throwing harness check all resolve to *fresh spawn* — the deliberate direction is "never resume on uncertainty". Fresh spawns mint and persist a new id, so the row self-heals. +- **Legacy pre-#832 rows** (row exists, no stored id) now spawn fresh once instead of discovery-resuming — a one-time context loss for workspaces dormant since #832, accepted in plan review. +- **The name `session.verifyOwnership`** slightly overstates what it now does (existence check). The reviewer was offered a rename (`isResumable`) and did not take it up; flagging in case the PR reviewer feels differently. + +### Consultation Verdicts and Dispositions (single advisory pass — PIR does not re-review) + +- **Codex: REQUEST_CHANGES** — (1) "implementation drops the approved cwd-based ownership verification; the collision vector remains for builder resume" and (2) "tests for the dropped ownership cases are missing". **Rebutted**: this is the deviation documented above, directed explicitly by the human reviewer at the `dev-approval` gate after a three-step design walk-back; the plan text was deliberately not rewritten to retro-fit the outcome (deviations belong in this review file). The collision vector requires two distinct absolute paths that encode identically AND host codev workspaces/worktrees with matching ids — contrived for Agent-Farm-managed paths, and the reviewer judged the content scan complexity not worth that threat. The dropped tests tested the dropped behavior. (3) "plan file missing approved-plan YAML frontmatter" — **fixed**: frontmatter plus a post-approval deviation note added to the plan file. +- **Claude: COMMENT** — implementation solid; one real finding: an editing error in `lessons-learned.md` merged the unrelated `[From 1144]` lesson into the new `[From #1145]` bullet. **Fixed**: split back into two bullets with the `[From 1144]` attribution restored. +- **Gemini: skipped** (non-blocking) — `agy` CLI not installed on this machine. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder `pir-1145` → **View Diff** +- **Run locally**: `pnpm build && pnpm -w run local-install` (restarts Tower) +- **What to verify** (maps to the plan's Test Plan): + 1. Create a scratch project dir (no `codev/`), run `claude` in it briefly and exit — this plants the "personal session" jsonl for that cwd. + 2. From the scratch dir, run `codev adopt` and start the workspace (or open in VS Code with auto-adopt). + 3. The main architect terminal must open a **fresh** conversation (architect role active, no prior chat context); Tower's log must not print "Resuming architect 'main' …". + 4. `afx workspace stop` then `start`: the architect must now resume its **own** stored session ("Resuming architect 'main' session …" with the minted id). + 5. Optional multi-architect check: `afx workspace add-architect --name reviewer`, stop/start, confirm both `main` and `reviewer` resume their own conversations. diff --git a/codev/state/pir-1145_thread.md b/codev/state/pir-1145_thread.md new file mode 100644 index 000000000..2f60c100d --- /dev/null +++ b/codev/state/pir-1145_thread.md @@ -0,0 +1,39 @@ +# pir-1145 — builder thread + +## 2026-07-08 — Plan phase + +Investigated issue #1145 (main architect hijacks unrelated Claude session on fresh workspaces). + +**Root cause**: the #832 legacy jsonl-discovery fallback in `launchInstance` (tower-instances.ts:493-499) is gated on `getArchitects().length <= 1`, which a *fresh* workspace (0 rows) satisfies. So first-ever launch runs mtime discovery over `~/.claude/projects//` and resumes the user's personal Claude conversation if one exists for that directory — with role injection skipped (resume branch of `resolveArchitectLaunch`). Second vector: the project-dir encoding maps both `/` and `.` to `-` (lossy), so distinct paths can collide into one store dir → genuine cross-project pickup. + +**Plan** (codev/plans/1145-codev-adopt-launchinstance-mai.md): +1. Gate discovery on a legacy `main` row *existing* without a session id (the bridge case it was built for). No row → always fresh. +2. Ownership verification: jsonl candidates must record a `cwd` matching the requested path (discovery side), plus an optional harness-gated `session.verifyOwnership` checked in `resolveArchitectLaunch` before any stored-id resume. + +Confirmed `afx workspace stop` preserves architect rows, so normal stop/start resume is unaffected. Sitting at plan-approval gate. + +## 2026-07-09 — Plan revised, approved; implement phase + +Reviewer challenged the row-gated fallback: even gated, mtime discovery can't distinguish the architect's last session from a *newer personal session in the same cwd* (and the cwd ownership check passes trivially there). Revised plan to drop the architect discovery fallback entirely; approved. + +Implementation: +- `claude-session-discovery.ts`: candidates now verified against the cwd recorded inside the jsonl (realpath-canonicalized both sides); new `readSessionCwd` + `verifySessionOwnership` (checks both logical and physical path encodings — macOS /tmp symlinks). +- `harness.ts`: optional `session.verifyOwnership` capability; Claude implements it. `buildResume` doc updated: builder resume is its sole consumer now. +- `tower-utils.ts`: `resolveArchitectLaunch` verifies stored-id ownership before the resume branch; failure falls through to fresh (new minted id replaces the stale one on persist). `homeDir` test seam threaded through. +- `tower-instances.ts`: discovery fallback deleted; stored row id is the only architect resume source. + +Test gotcha worth remembering: asserting `JSON.stringify(spawnCall).not.toContain('--resume')` false-fails because the injected architect role prompt *text* contains `--resume` in its CLI examples. Assert on argv tokens instead. + +115 tests green on the four touched files; full suite + push next, then dev-approval gate. + +## 2026-07-10 — dev-approval feedback: content scanning removed + +Reviewer walked the design back in three steps at the gate: (1) why scan file contents at all → only real justification was lossy-encoding collisions; (2) the fixed 64KB scan window made resume content-dependent → made it streaming/semantic; (3) collisions are contrived for both architects (row-gated) and builders (Agent-Farm-managed worktree paths) → dropped content scanning entirely. + +Final shape: architect resume = stored row id + file-*existence* check (both logical/physical path encodings, macOS symlink case); builder discovery = plain newest-by-mtime (pre-#1145 behavior). The #1145 fix proper (no discovery fallback in launchInstance) is untouched. Deviation from the approved plan (which specified cwd-content verification) to be recorded in the review file. + +Lesson: when a reviewer asks "why is this here at all", enumerate what each layer actually defends against before defending the layer — two of three protections were against a threat the system's own path conventions already preclude. + +## 2026-07-11 — Review phase: PR #1160, consultation, pr gate + +dev-approval approved. Retrospective written (arch.md resume paragraph rewritten to the stored-id-only invariant; two lessons routed cold). PR #1160 opened with the review as body. Consultation (single pass): codex=REQUEST_CHANGES (flagged the content-verification deviation — rebutted as human-directed at the gate; frontmatter nit fixed), claude=COMMENT (caught a real lessons-learned.md bullet-merge editing error — fixed in ff9855b3), gemini skipped (agy not installed). Rebuttals in codev/projects/1145-*/1145-review-iter1-rebuttals.md. Architect notified with the REQUEST_CHANGES disposition leading. Sitting at the pr gate. diff --git a/packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts b/packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts index c8f2a8557..89c48176a 100644 --- a/packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts +++ b/packages/codev/src/agent-farm/__tests__/claude-session-discovery.test.ts @@ -2,16 +2,19 @@ * Tests for Claude session discovery via on-disk jsonl introspection. * * Issue #829 — conversation resume. + * Issue #1145 — verifySessionOwnership: a stored session id is only resumable + * while its jsonl still exists for the workspace's cwd. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, rmSync, mkdirSync, writeFileSync, utimesSync } from 'node:fs'; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync, utimesSync, realpathSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { encodeClaudeProjectDir, findLatestSessionId, + verifySessionOwnership, } from '../utils/claude-session-discovery.js'; describe('encodeClaudeProjectDir', () => { @@ -36,7 +39,7 @@ describe('encodeClaudeProjectDir', () => { }); }); -describe('findLatestSessionId', () => { +describe('claude session discovery', () => { let fakeHome: string; let projectsRoot: string; @@ -59,44 +62,79 @@ describe('findLatestSessionId', () => { utimesSync(file, t, t); } - it('returns the newest session UUID by mtime', () => { - const worktree = '/Users/x/repo/.builders/pir-1'; - writeSession(worktree, 'old-uuid', 1_000_000_000_000); - writeSession(worktree, 'newest-uuid', 1_700_000_000_000); - writeSession(worktree, 'middle-uuid', 1_400_000_000_000); - expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('newest-uuid'); + describe('findLatestSessionId', () => { + it('returns the newest session UUID by mtime', () => { + const worktree = '/Users/x/repo/.builders/pir-1'; + writeSession(worktree, 'old-uuid', 1_000_000_000_000); + writeSession(worktree, 'newest-uuid', 1_700_000_000_000); + writeSession(worktree, 'middle-uuid', 1_400_000_000_000); + expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('newest-uuid'); + }); + + it('returns null when the project dir does not exist', () => { + expect(findLatestSessionId('/nonexistent/path', { homeDir: fakeHome })).toBeNull(); + }); + + it('returns null when the project dir exists but contains no jsonl files', () => { + const worktree = '/Users/x/repo/.builders/pir-2'; + const dir = join(projectsRoot, encodeClaudeProjectDir(worktree)); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'memory'), 'not a jsonl', 'utf-8'); + expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBeNull(); + }); + + it('ignores non-jsonl files and subdirectories', () => { + const worktree = '/Users/x/repo/.builders/pir-3'; + const dir = join(projectsRoot, encodeClaudeProjectDir(worktree)); + mkdirSync(dir, { recursive: true }); + mkdirSync(join(dir, 'some-uuid'), { recursive: true }); + writeFileSync(join(dir, 'history.txt'), 'text', 'utf-8'); + writeSession(worktree, 'the-uuid', 1_500_000_000_000); + expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('the-uuid'); + }); + + it('returns the single jsonl when only one exists', () => { + const worktree = '/Users/x/repo/.builders/pir-4'; + writeSession(worktree, 'only-uuid', 1_500_000_000_000); + expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('only-uuid'); + }); }); - it('returns null when the project dir does not exist', () => { - expect(findLatestSessionId('/nonexistent/path', { homeDir: fakeHome })).toBeNull(); - }); - - it('returns null when the project dir exists but contains no jsonl files', () => { - const worktree = '/Users/x/repo/.builders/pir-2'; - const dir = join(projectsRoot, encodeClaudeProjectDir(worktree)); - mkdirSync(dir, { recursive: true }); - writeFileSync(join(dir, 'memory'), 'not a jsonl', 'utf-8'); - expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBeNull(); - }); - - it('ignores non-jsonl files and subdirectories', () => { - const worktree = '/Users/x/repo/.builders/pir-3'; - const dir = join(projectsRoot, encodeClaudeProjectDir(worktree)); - mkdirSync(dir, { recursive: true }); - mkdirSync(join(dir, 'some-uuid'), { recursive: true }); - writeFileSync(join(dir, 'history.txt'), 'text', 'utf-8'); - writeSession(worktree, 'the-uuid', 1_500_000_000_000); - expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('the-uuid'); - }); - - it('returns the single jsonl when only one exists', () => { - const worktree = '/Users/x/repo/.builders/pir-4'; - writeSession(worktree, 'only-uuid', 1_500_000_000_000); - expect(findLatestSessionId(worktree, { homeDir: fakeHome })).toBe('only-uuid'); + describe('verifySessionOwnership (Issue #1145)', () => { + it('accepts a session whose jsonl exists for the workspace cwd', () => { + const worktree = '/Users/x/repo/ws-1'; + writeSession(worktree, 'owned-uuid', 1_500_000_000_000); + expect(verifySessionOwnership(worktree, 'owned-uuid', { homeDir: fakeHome })).toBe(true); + }); + + it('rejects a session id with no jsonl on disk (stale stored id)', () => { + const worktree = '/Users/x/repo/ws-2'; + mkdirSync(join(projectsRoot, encodeClaudeProjectDir(worktree)), { recursive: true }); + expect(verifySessionOwnership(worktree, 'gone-uuid', { homeDir: fakeHome })).toBe(false); + }); + + it('rejects a session id whose jsonl lives under a different cwd', () => { + const worktree = '/Users/x/repo/ws-3'; + writeSession('/Users/x/repo/other-ws', 'other-uuid', 1_500_000_000_000); + expect(verifySessionOwnership(worktree, 'other-uuid', { homeDir: fakeHome })).toBe(false); + }); + + it('accepts a session stored under the physical form of a symlinked cwd', () => { + // macOS: os.tmpdir() is /var/... which is a symlink to /private/var/.... + // Claude keys the store by its process cwd (physical form); the caller + // may hold the logical form. Both must verify. + const logicalDir = mkdtempSync(join(tmpdir(), 'csd-sym-')); + try { + const physicalDir = realpathSync(logicalDir); + writeSession(physicalDir, 'sym-uuid', 1_500_000_000_000); + expect(verifySessionOwnership(logicalDir, 'sym-uuid', { homeDir: fakeHome })).toBe(true); + } finally { + rmSync(logicalDir, { recursive: true, force: true }); + } + }); }); }); -// Issue #832: the live-process session-id capture (cmdline-reading backfill) was -// dropped in favour of the sole-architect jsonl-discovery fallback in -// launchInstance + the stored-UUID spawn/revive path. findLatestSessionId (above) -// is the shared discovery helper that remains. +// Issue #832 introduced the stored-UUID spawn/revive path for architects; +// Issue #1145 removed the architect-side jsonl-discovery fallback entirely. +// findLatestSessionId (above) now serves builder resume only. diff --git a/packages/codev/src/agent-farm/__tests__/tower-instances.test.ts b/packages/codev/src/agent-farm/__tests__/tower-instances.test.ts index 597a23a13..f6fb97a96 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-instances.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-instances.test.ts @@ -49,9 +49,10 @@ const { vi.mock('../db/index.js', () => ({ getGlobalDb: () => ({ prepare: mockDbPrepare }), - // state.getArchitects() (used by launchInstance's safeToResume guard) calls - // getDb(), not getGlobalDb — mock it too so the guard doesn't throw and - // default to skip-resume. (Issue #929 resume regression tests depend on it.) + // state.getArchitectByName() (launchInstance's stored-session lookup) calls + // getDb(), not getGlobalDb — mock it too. The default mock has no `.get`, + // so the lookup throws and launchInstance treats the workspace as having no + // stored session (the fresh-boot cases below rely on that). getDb: () => ({ prepare: mockDbPrepare }), closeDb: () => {}, })); @@ -600,16 +601,20 @@ describe('tower-instances', () => { }); // ========================================================================= - // Issue #929 — architect resume is gated on the configured harness + // Issue #929 / #1145 — architect launch never discovery-resumes // - // Regression guard for the crash-loop: a codex/gemini architect with a stale - // Claude jsonl in ~/.claude/projects// must NOT launch - // ` --resume `. Only the Claude harness implements - // buildResume, so codex/gemini relaunch fresh (role-injected) instead. + // #929's crash-loop guard (codex/gemini + stale Claude jsonl must not launch + // ` --resume`) is subsumed by #1145: launchInstance no longer consults + // the jsonl store at all. Its ONLY resume source is the session id stored on + // this workspace's `main` architect row. A fresh workspace with a personal + // Claude conversation in the same cwd (the hijack in #1145) must boot fresh. // ========================================================================= - describe('Issue #929 — architect resume gated on harness', () => { - function writeStaleClaudeSession(fakeHome: string, cwd: string, uuid: string): void { + describe('Issue #929/#1145 — architect resume comes only from the stored row', () => { + // A session in this workspace's own store dir — exactly what the removed + // discovery fallback would have picked up and resumed. The fresh boot + // below proves the fallback is gone. + function writePersonalClaudeSession(fakeHome: string, cwd: string, uuid: string): void { const dir = path.join(fakeHome, '.claude', 'projects', encodeClaudeProjectDir(cwd)); fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(path.join(dir, `${uuid}.jsonl`), `{"sessionId":"${uuid}"}\n`); @@ -649,7 +654,7 @@ describe('tower-instances', () => { JSON.stringify(configJson), ); } - writeStaleClaudeSession(fakeHome, tmpDir, uuid); + writePersonalClaudeSession(fakeHome, tmpDir, uuid); process.env.HOME = fakeHome; await fn(tmpDir, fakeHome, uuid); } finally { @@ -661,7 +666,7 @@ describe('tower-instances', () => { }; } - it('codex architect + stale Claude jsonl → launches fresh, no --resume', withSetup( + it('codex architect + stale Claude jsonl → launches fresh, no --resume (#929)', withSetup( { shell: { architect: 'codex' } }, async (tmpDir, _fakeHome, uuid) => { const { deps, createSession } = makeCapturingDeps(); @@ -677,7 +682,7 @@ describe('tower-instances', () => { }, )); - it('claude architect (default) + stale Claude jsonl → resumes with --resume ', withSetup( + it('fresh workspace + personal Claude session in the same cwd → boots fresh, no --resume (#1145 regression)', withSetup( null, async (tmpDir, _fakeHome, uuid) => { const { deps, createSession } = makeCapturingDeps(); @@ -687,9 +692,47 @@ describe('tower-instances', () => { expect(result.success).toBe(true); expect(createSession).toHaveBeenCalled(); - const callStr = JSON.stringify(createSession.mock.calls[0]); - expect(callStr).toContain('--resume'); - expect(callStr).toContain(uuid); + // Assert on the argv tokens, not the stringified call: the injected + // role prompt legitimately mentions "--resume" in its CLI examples. + const callArgs = (createSession.mock.calls[0][0] as { args: string[] }).args; + expect(callArgs).not.toContain('--resume'); + expect(callArgs).not.toContain(uuid); + // Fresh spawn pins a newly minted session id instead. + expect(callArgs).toContain('--session-id'); + }, + )); + + it('main row with a stored session id + owned jsonl → resumes that id', withSetup( + null, + async (tmpDir, _fakeHome, _uuid) => { + const storedId = 'stored-main-uuid-5678'; + writePersonalClaudeSession(_fakeHome, tmpDir, storedId); + // getArchitectByName reads via db.prepare(...).get — return a main row + // carrying the stored conversation id. + const resolvedTmpDir = fs.realpathSync(tmpDir); + const mockGet = vi.fn().mockReturnValue({ + workspace_path: resolvedTmpDir, id: 'main', pid: 0, port: 0, + cmd: 'claude', started_at: 'x', terminal_id: null, session_id: storedId, + }); + mockDbPrepare.mockReturnValue({ run: mockDbRun, all: mockDbAll, get: mockGet }); + try { + // The stored session must exist under BOTH path forms the launch code + // touches (tmpdir is symlinked on macOS: /var → /private/var). + writePersonalClaudeSession(_fakeHome, resolvedTmpDir, storedId); + + const { deps, createSession } = makeCapturingDeps(); + initInstances(deps); + + const result = await launchInstance(tmpDir); + expect(result.success).toBe(true); + expect(createSession).toHaveBeenCalled(); + + const callArgs = (createSession.mock.calls[0][0] as { args: string[] }).args; + expect(callArgs).toContain('--resume'); + expect(callArgs).toContain(storedId); + } finally { + mockDbPrepare.mockReturnValue({ run: mockDbRun, all: mockDbAll }); + } }, )); diff --git a/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts b/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts index 17d178bef..7db24749d 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts @@ -29,6 +29,18 @@ vi.mock('../state.js', async (importOriginal) => { return { ...actual, getArchitectByName: vi.fn() }; }); import { getArchitectByName } from '../state.js'; +import { encodeClaudeProjectDir } from '../utils/claude-session-discovery.js'; + +/** + * Issue #1145: stored-id resume now requires the session jsonl to exist under + * the (test-pinned) home dir. This writes a minimal session file so ownership + * verification passes; omit it to simulate a stale stored id. + */ +function writeSessionFixture(homeDir: string, cwdPath: string, uuid: string): void { + const dir = path.join(homeDir, '.claude', 'projects', encodeClaudeProjectDir(cwdPath)); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, `${uuid}.jsonl`), `{"sessionId":"${uuid}"}\n`, 'utf-8'); +} describe('tower-utils', () => { describe('isRateLimited', () => { @@ -212,22 +224,27 @@ describe('tower-utils', () => { describe('resolveArchitectLaunch (Issue #832)', () => { let workspace: string; + let fakeHome: string; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; beforeEach(() => { // A bare temp dir: no codev/roles, so buildArchitectArgs returns baseArgs // unchanged (loadRolePrompt → null). Default harness resolves to Claude - // (which has the session capability). + // (which has the session capability). fakeHome pins the session store the + // ownership check (Issue #1145) reads. workspace = fs.mkdtempSync(path.join(tmpdir(), 'ral-ws-')); + fakeHome = fs.mkdtempSync(path.join(tmpdir(), 'ral-home-')); }); afterEach(() => { fs.rmSync(workspace, { recursive: true, force: true }); + fs.rmSync(fakeHome, { recursive: true, force: true }); }); it('resumes the stored id (no role injection) and echoes it back', () => { + writeSessionFixture(fakeHome, workspace, 'stored-abc'); const { args, env, sessionId, resumed } = resolveArchitectLaunch({ - workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: 'stored-abc', + workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: 'stored-abc', homeDir: fakeHome, }); expect(args).toEqual(['--resume', 'stored-abc']); expect(env).toEqual({}); @@ -237,7 +254,7 @@ describe('resolveArchitectLaunch (Issue #832)', () => { it('mints a fresh --session-id when there is no stored id, and returns it', () => { const { args, sessionId, resumed } = resolveArchitectLaunch({ - workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: null, + workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: null, homeDir: fakeHome, }); expect(args).not.toContain('--resume'); expect(args).toContain('--session-id'); @@ -248,25 +265,51 @@ describe('resolveArchitectLaunch (Issue #832)', () => { }); it('mints distinct ids across fresh spawns', () => { - const a = resolveArchitectLaunch({ workspacePath: workspace, name: 'a', baseArgs: [] }).sessionId; - const b = resolveArchitectLaunch({ workspacePath: workspace, name: 'b', baseArgs: [] }).sessionId; + const a = resolveArchitectLaunch({ workspacePath: workspace, name: 'a', baseArgs: [], homeDir: fakeHome }).sessionId; + const b = resolveArchitectLaunch({ workspacePath: workspace, name: 'b', baseArgs: [], homeDir: fakeHome }).sessionId; expect(a).not.toBe(b); }); it('preserves baseArgs ahead of the session flags', () => { + writeSessionFixture(fakeHome, workspace, 'x'); const { args } = resolveArchitectLaunch({ - workspacePath: workspace, name: 'main', baseArgs: ['--foo'], storedSessionId: 'x', + workspacePath: workspace, name: 'main', baseArgs: ['--foo'], storedSessionId: 'x', homeDir: fakeHome, }); expect(args).toEqual(['--foo', '--resume', 'x']); }); it('two siblings with distinct stored ids resume independently (no cross-attachment)', () => { - const reviewer = resolveArchitectLaunch({ workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: 'rev-1' }); - const casa = resolveArchitectLaunch({ workspacePath: workspace, name: 'casa', baseArgs: [], storedSessionId: 'casa-1' }); + writeSessionFixture(fakeHome, workspace, 'rev-1'); + writeSessionFixture(fakeHome, workspace, 'casa-1'); + const reviewer = resolveArchitectLaunch({ workspacePath: workspace, name: 'reviewer', baseArgs: [], storedSessionId: 'rev-1', homeDir: fakeHome }); + const casa = resolveArchitectLaunch({ workspacePath: workspace, name: 'casa', baseArgs: [], storedSessionId: 'casa-1', homeDir: fakeHome }); expect(reviewer.args).toEqual(['--resume', 'rev-1']); expect(casa.args).toEqual(['--resume', 'casa-1']); }); + // Issue #1145: ownership verification ahead of the resume branch. + + it('spawns fresh when the stored id has no jsonl on disk (stale stored id)', () => { + const { args, sessionId, resumed } = resolveArchitectLaunch({ + workspacePath: workspace, name: 'main', baseArgs: [], storedSessionId: 'ghost-id', homeDir: fakeHome, + }); + expect(args).not.toContain('--resume'); + expect(args).toContain('--session-id'); + expect(resumed).toBe(false); + expect(sessionId).toMatch(UUID_RE); // replacement id for the caller to persist + expect(sessionId).not.toBe('ghost-id'); + }); + + it('spawns fresh when the stored session file lives under a different cwd', () => { + writeSessionFixture(fakeHome, '/somewhere/else/entirely', 'foreign-id'); + const { args, resumed, sessionId } = resolveArchitectLaunch({ + workspacePath: workspace, name: 'main', baseArgs: [], storedSessionId: 'foreign-id', homeDir: fakeHome, + }); + expect(args).not.toContain('--resume'); + expect(resumed).toBe(false); + expect(sessionId).not.toBe('foreign-id'); + }); + it('no-session harness → plain fresh, returns null sessionId', () => { // Force a Codex architect harness (no `session` capability) via config. fs.mkdirSync(path.join(workspace, '.codev'), { recursive: true }); @@ -286,21 +329,25 @@ describe('resolveArchitectLaunch (Issue #832)', () => { describe('resolveArchitectRestart (Issue #832 — shellper auto-restart bake)', () => { let workspace: string; + let fakeHome: string; const mockGet = vi.mocked(getArchitectByName); beforeEach(() => { // Bare temp dir → default Claude harness (has the session capability). workspace = fs.mkdtempSync(path.join(tmpdir(), 'rar-ws-')); + fakeHome = fs.mkdtempSync(path.join(tmpdir(), 'rar-home-')); mockGet.mockReset(); }); afterEach(() => { fs.rmSync(workspace, { recursive: true, force: true }); + fs.rmSync(fakeHome, { recursive: true, force: true }); }); it('revives the architect\'s stored session id on restart (--resume, no role injection)', () => { + writeSessionFixture(fakeHome, workspace, 'stored-xyz'); mockGet.mockReturnValue({ name: 'reviewer', cmd: 'claude', startedAt: 'x', sessionId: 'stored-xyz' } as never); - const { args, env, resumed, storedSessionId } = resolveArchitectRestart(workspace, 'reviewer', []); + const { args, env, resumed, storedSessionId } = resolveArchitectRestart(workspace, 'reviewer', [], { homeDir: fakeHome }); expect(mockGet).toHaveBeenCalledWith(workspace, 'reviewer'); expect(args).toEqual(['--resume', 'stored-xyz']); expect(env).toEqual({}); @@ -308,6 +355,16 @@ describe('resolveArchitectRestart (Issue #832 — shellper auto-restart bake)', expect(storedSessionId).toBe('stored-xyz'); }); + it('spawns fresh on restart when the stored session fails ownership verification (#1145)', () => { + // No jsonl fixture written → stale stored id. + mockGet.mockReturnValue({ name: 'reviewer', cmd: 'claude', startedAt: 'x', sessionId: 'stale-xyz' } as never); + const { args, resumed, storedSessionId } = resolveArchitectRestart(workspace, 'reviewer', [], { homeDir: fakeHome }); + expect(args).not.toContain('--resume'); + expect(args).toContain('--session-id'); + expect(resumed).toBe(false); + expect(storedSessionId).toBe('stale-xyz'); // still reported for the caller's log line + }); + it('falls back to a fresh session when the row has no stored id (legacy / self-heal)', () => { mockGet.mockReturnValue({ name: 'reviewer', cmd: 'claude', startedAt: 'x' } as never); // no sessionId const { args, resumed, storedSessionId } = resolveArchitectRestart(workspace, 'reviewer', []); @@ -326,9 +383,11 @@ describe('resolveArchitectRestart (Issue #832 — shellper auto-restart bake)', }); it('looks each architect up by its own name — no cross-attachment between siblings', () => { + writeSessionFixture(fakeHome, workspace, 'rev-1'); + writeSessionFixture(fakeHome, workspace, 'casa-1'); mockGet.mockImplementation((_ws: string, name: string) => (name === 'reviewer' ? { sessionId: 'rev-1' } : { sessionId: 'casa-1' }) as never); - expect(resolveArchitectRestart(workspace, 'reviewer', []).args).toEqual(['--resume', 'rev-1']); - expect(resolveArchitectRestart(workspace, 'casa', []).args).toEqual(['--resume', 'casa-1']); + expect(resolveArchitectRestart(workspace, 'reviewer', [], { homeDir: fakeHome }).args).toEqual(['--resume', 'rev-1']); + expect(resolveArchitectRestart(workspace, 'casa', [], { homeDir: fakeHome }).args).toEqual(['--resume', 'casa-1']); }); }); diff --git a/packages/codev/src/agent-farm/servers/tower-instances.ts b/packages/codev/src/agent-farm/servers/tower-instances.ts index d834a5619..ded1d85c5 100644 --- a/packages/codev/src/agent-farm/servers/tower-instances.ts +++ b/packages/codev/src/agent-farm/servers/tower-instances.ts @@ -13,7 +13,6 @@ import { exec } from 'node:child_process'; import { promisify } from 'node:util'; import { homedir } from 'node:os'; import { encodeWorkspacePath } from '../lib/tower-client.js'; -import { getArchitectHarness } from '../utils/config.js'; import { loadConfig } from '../../lib/config.js'; const execAsync = promisify(exec); @@ -466,37 +465,31 @@ export async function launchInstance(workspacePath: string): Promise<{ success: const cmdParts = architectCmd.split(/\s+/); const cmd = cmdParts[0]; - // Issue #832: resume main's persisted conversation when its row carries a - // session id, else spawn fresh and mint one. The returned `sessionId` is - // stored on the architect row below so the next restart resumes it. A - // state.db read failure degrades to a fresh spawn rather than aborting. + // Issue #832 / #1145: resume main's persisted conversation ONLY when its + // workspace-scoped row carries a session id (which resolveArchitectLaunch + // additionally ownership-verifies against the on-disk session store); + // anything else spawns fresh with a newly minted id, persisted on the + // architect row below so the next restart resumes it. This block only + // handles `main`; sibling architects resume the same way via the + // reconciliation loop at the end of launchInstance → addArchitect, + // which reads each sibling's own row. // - // Legacy bridge (#830/#929): a row from before #832 has no stored id. For it, - // fall back to harness-gated jsonl-discovery — but ONLY when main is the sole - // architect, since a cwd shared with siblings makes newest-by-mtime ambiguous - // (the old `safeToResume` guard, now scoped to just this fallback rather than - // gating resume wholesale). Going through `harness.buildResume` keeps discovery - // harness-gated: only Claude has a jsonl store, so a codex/gemini architect - // returns null → fresh, avoiding the stale-jsonl `--resume` crash-loop (#929). - // Stored-UUID resume applies regardless of architect count, so main resumes in - // multi-architect workspaces once it has an id. resolveArchitectLaunch persists - // whatever it resolves, so a discovered id self-migrates into the stored-UUID - // path on this same revival — no separate backfill step. - // The stored id and the discovery fallback are independent recovery - // sources, so a failed read of one must not disable the other (e.g. a - // state.db hiccup reading the row shouldn't suppress sole-architect - // discovery). Each gets its own try. + // The mtime-based jsonl-discovery fallback that used to run here for + // legacy pre-#832 rows was removed by #1145: on a fresh workspace + // (`codev adopt` / first touch) it resumed whatever Claude conversation + // the user last held in this directory — hijacking personal sessions, + // and roleless too, since the resume path skips role injection. Even + // row-gated, mtime cannot distinguish the architect's last session from + // a newer personal one in the same cwd, so the fallback is gone rather + // than re-gated. A legacy row without an id costs one fresh spawn, then + // self-heals onto the stored-UUID path. Discovery (`buildResume`) + // survives for builder resume only, where the worktree cwd is private. + // A global.db read failure also degrades to a fresh spawn — never + // resume on uncertainty. let storedSessionId: string | null = null; try { storedSessionId = getArchitectByName(resolvedPath, DEFAULT_ARCHITECT_NAME)?.sessionId ?? null; - } catch { /* state.db unreadable — fall through to discovery */ } - if (!storedSessionId) { - try { - if (getArchitects(resolvedPath).length <= 1) { - storedSessionId = getArchitectHarness(workspacePath).buildResume?.(workspacePath)?.sessionId ?? null; - } - } catch { /* discovery unavailable — spawn fresh */ } - } + } catch { /* global.db unreadable — spawn fresh */ } const { args: cmdArgs, env: harnessEnv, sessionId: mainSessionId, resumed } = resolveArchitectLaunch({ workspacePath, name: DEFAULT_ARCHITECT_NAME, diff --git a/packages/codev/src/agent-farm/servers/tower-utils.ts b/packages/codev/src/agent-farm/servers/tower-utils.ts index b5b73e64c..ad5ab9852 100644 --- a/packages/codev/src/agent-farm/servers/tower-utils.ts +++ b/packages/codev/src/agent-farm/servers/tower-utils.ts @@ -14,6 +14,7 @@ import type { RateLimitEntry } from './tower-types.js'; import crypto from 'node:crypto'; import { loadRolePrompt, type RoleConfig } from '../utils/roles.js'; import { getArchitectHarness } from '../utils/config.js'; +import type { HarnessProvider } from '../utils/harness.js'; import { getArchitectByName } from '../state.js'; // ============================================================================ @@ -188,6 +189,27 @@ export function buildArchitectArgs(baseArgs: string[], workspacePath: string): { }; } +/** + * Issue #1145: true when the stored session may be resumed. Harnesses that + * expose `session.verifyOwnership` get the final say; ones that don't are + * trusted (their stored ids are minted by us and have no on-disk store to + * cross-check). A throwing check counts as "not ours". + */ +function sessionIsOwned( + harness: HarnessProvider, + sessionId: string, + workspacePath: string, + homeDir?: string, +): boolean { + const verify = harness.session?.verifyOwnership; + if (!verify) return true; + try { + return verify(sessionId, workspacePath, { homeDir }); + } catch { + return false; + } +} + /** * Issue #832: resolve the args/env to launch (or revive) an architect, choosing * between resuming its persisted conversation and starting a fresh one. The @@ -197,10 +219,15 @@ export function buildArchitectArgs(baseArgs: string[], workspacePath: string): { * Decision order: * 1. Harness has no `session` capability (e.g. Codex/Gemini today) → plain fresh * spawn, `sessionId: null` (nothing to resume next time). - * 2. `storedSessionId` present → resume it (no role injection — the saved - * conversation already holds the role/system prompt); echo the same id back. + * 2. `storedSessionId` present AND the harness confirms the session on disk + * belongs to this workspace (Issue #1145: harnesses without a + * `verifyOwnership` capability are trusted as-is) → resume it (no role + * injection — the saved conversation already holds the role/system + * prompt); echo the same id back. * 3. Else fresh → generate a new id, pin the session to it (with role injection), - * and return it for the caller to persist. + * and return it for the caller to persist. A stored id that fails + * ownership verification (its jsonl no longer exists) lands here, and + * the fresh id the caller persists replaces it. * * The returned `sessionId` is the value the caller writes onto the architect row, * so the column is populated correctly on every spawn. @@ -210,8 +237,10 @@ export function resolveArchitectLaunch(opts: { name: string; baseArgs: string[]; storedSessionId?: string | null; + /** Test seam: pins the home dir the ownership check resolves the session store under. */ + homeDir?: string; }): { args: string[]; env: Record; sessionId: string | null; resumed: boolean } { - const { workspacePath, baseArgs, storedSessionId } = opts; + const { workspacePath, baseArgs, storedSessionId, homeDir } = opts; const harness = getArchitectHarness(workspacePath); // 1. No resumable-session support → plain fresh, nothing to persist. @@ -219,8 +248,11 @@ export function resolveArchitectLaunch(opts: { return { ...buildArchitectArgs(baseArgs, workspacePath), sessionId: null, resumed: false }; } - // 2. Resume the persisted conversation (role injection skipped). - if (storedSessionId) { + // 2. Resume the persisted conversation (role injection skipped) — but only + // when the session still exists on disk for this workspace. A failed or + // throwing check falls through to a fresh spawn (Issue #1145: a stored id + // can outlive its jsonl, and resuming it would crash-loop the restart). + if (storedSessionId && sessionIsOwned(harness, storedSessionId, workspacePath, homeDir)) { return { args: [...baseArgs, ...harness.session.resumeArgs(storedSessionId)], env: {}, @@ -253,9 +285,12 @@ export function resolveArchitectRestart( workspacePath: string, architectName: string, baseArgs: string[], + opts?: { homeDir?: string }, ): { args: string[]; env: Record; sessionId: string | null; resumed: boolean; storedSessionId: string | null } { const storedSessionId = getArchitectByName(workspacePath, architectName)?.sessionId ?? null; - const resolved = resolveArchitectLaunch({ workspacePath, name: architectName, baseArgs, storedSessionId }); + const resolved = resolveArchitectLaunch({ + workspacePath, name: architectName, baseArgs, storedSessionId, homeDir: opts?.homeDir, + }); return { ...resolved, storedSessionId }; } diff --git a/packages/codev/src/agent-farm/utils/claude-session-discovery.ts b/packages/codev/src/agent-farm/utils/claude-session-discovery.ts index b8d505aaf..d2c79c857 100644 --- a/packages/codev/src/agent-farm/utils/claude-session-discovery.ts +++ b/packages/codev/src/agent-farm/utils/claude-session-discovery.ts @@ -5,15 +5,19 @@ // ~/.claude/projects//.jsonl // where the encoding replaces both '/' and '.' in the absolute path with '-'. // We use the newest jsonl by mtime as a stand-in for "the last conversation -// that ran in this directory" so reviving a dead builder (or architect) can -// resume via `claude --resume ` without any spawn-time bookkeeping. +// that ran in this directory" so reviving a dead builder can resume via +// `claude --resume ` without any spawn-time bookkeeping. // // This is intentionally a heuristic — multiple jsonl files in the same // directory mean multiple past sessions, and we pick the most recent. For -// builder worktrees that almost always means the right one; for shared cwds -// the caller should be aware of the ambiguity. +// builder worktrees (Agent-Farm-managed paths, effectively private cwds) that +// almost always means the right one. Architect launch no longer uses discovery +// at all (Issue #1145): it resumes solely from the session id stored on the +// workspace-scoped architect row, after verifySessionOwnership (below) +// confirms the session still exists on disk. import { existsSync, readdirSync, statSync } from 'node:fs'; +import { realpathSync } from 'node:fs'; import { homedir } from 'node:os'; import { join } from 'node:path'; @@ -33,6 +37,15 @@ export function getClaudeProjectDir(absolutePath: string): string { return join(homedir(), '.claude', 'projects', encodeClaudeProjectDir(absolutePath)); } +/** Canonicalize a path for comparison; fall back to the input when realpath fails. */ +function realpathOrSelf(p: string): string { + try { + return realpathSync(p); + } catch { + return p; + } +} + /** * Return the session UUID of the most-recently-modified jsonl in the Claude * project dir for the given cwd, or null if none exists. @@ -68,3 +81,33 @@ export function findLatestSessionId( if (!bestName) return null; return bestName.slice(0, -JSONL_EXT.length); } + +/** + * Verify that a stored session id still has a session file on disk for + * `absolutePath` (Issue #1145). Stored ids are minted by us and persisted + * keyed by workspace path, so existence is the meaningful check: a row can + * outlive its jsonl (`workspace stop` preserves rows; ~/.claude gets pruned + * independently), and resuming a deleted session bakes a broken `--resume` + * into a shellper restart loop (the #929 crash-loop class). A failed check + * degrades to a fresh spawn. + */ +export function verifySessionOwnership( + absolutePath: string, + sessionId: string, + opts?: { homeDir?: string }, +): boolean { + const home = opts?.homeDir ?? homedir(); + // Claude keys the store dir by its process cwd, which the OS may report in + // physical (symlink-resolved) form — e.g. /tmp/ws vs /private/tmp/ws on + // macOS. Accept the session under either encoding of the same directory. + const candidateDirs = new Set([ + encodeClaudeProjectDir(absolutePath), + encodeClaudeProjectDir(realpathOrSelf(absolutePath)), + ]); + for (const dir of candidateDirs) { + if (existsSync(join(home, '.claude', 'projects', dir, `${sessionId}${JSONL_EXT}`))) { + return true; + } + } + return false; +} diff --git a/packages/codev/src/agent-farm/utils/harness.ts b/packages/codev/src/agent-farm/utils/harness.ts index 7e379a4b1..319bb4bdb 100644 --- a/packages/codev/src/agent-farm/utils/harness.ts +++ b/packages/codev/src/agent-farm/utils/harness.ts @@ -12,7 +12,7 @@ * @see codev/specs/591-af-workspace-failure-with-code.md */ -import { findLatestSessionId } from './claude-session-discovery.js'; +import { findLatestSessionId, verifySessionOwnership } from './claude-session-discovery.js'; import { buildWorktreeGuardFiles } from './worktree-write-guard.js'; // ============================================================================= @@ -70,6 +70,14 @@ export interface HarnessProvider { newSessionArgs(sessionId: string): string[]; /** Args to RESUME an existing session by id (caller skips role injection). */ resumeArgs(sessionId: string): string[]; + /** + * Optional: verify that `sessionId` still has a resumable session on disk + * for `cwd` before the caller resumes it (Issue #1145). Returns false when + * the session file is gone (a stored id can outlive its jsonl); callers + * then spawn fresh instead of baking a broken resume into a restart loop. + * Harnesses that omit this are trusted as-is. + */ + verifyOwnership?(sessionId: string, cwd: string, opts?: { homeDir?: string }): boolean; }; /** @@ -82,9 +90,11 @@ export interface HarnessProvider { * cwd-keyed session store → callers fall back to a fresh launch. Only Claude * implements it (store: ~/.claude/projects//.jsonl). * - * Discovery-based (newest jsonl by mtime): used for builder resume (#831/#929) and, - * for architects (#832), only as the harness-gated legacy fallback when `main` has - * no stored session id yet (codex/gemini omit it → no stale-jsonl crash-loop). + * Discovery-based (newest jsonl by mtime): used for builder resume + * (#831/#929) ONLY. Architect launch never discovers — it resumes solely from + * the stored session id on the workspace-scoped architect row, else spawns + * fresh (Issue #1145: discovery on a fresh workspace hijacked whatever Claude + * conversation the user last held in that directory). */ buildResume?(absolutePath: string, opts?: { homeDir?: string }): { sessionId: string; @@ -132,6 +142,9 @@ export const CLAUDE_HARNESS: HarnessProvider = { session: { newSessionArgs: (sessionId) => ['--session-id', sessionId], resumeArgs: (sessionId) => ['--resume', sessionId], + // Issue #1145: a stored id is only resumed when its jsonl still exists + // under this cwd's project dir (stale ids degrade to a fresh spawn). + verifyOwnership: (sessionId, cwd, opts) => verifySessionOwnership(cwd, sessionId, opts), }, };