fix(test-env): prove instance identity before reuse and teardown (#898)

[sapersky]

Closes #898

🎯 Goal

Make the test-env launcher act only on the instance this worktree actually started, so a QA run can no longer kill -9 an unrelated process or silently verify a different branch's build.

🔍 Problem

.ai/scripts/test-env-up.sh and .ai/scripts/test-env-down.sh decided both "reuse this instance" and "kill this instance" from liveness signals that never proved the process was theirs: kill -0 $pid plus an HTTP 200 on the recorded URL. .ai/qa/test-env.json is gitignored and never removed — down only rewrote one field — so it outlives reboots and days of process churn, after which the recorded PID number very plausibly belongs to something else. Two failure modes followed, worst first: a kill -9 of an innocent process, which needs only PID recycling and no second coincidence; and a silent false-green QA run, where the recorded port has meanwhile been taken by another worktree's instance and the run tests a different branch's build while reporting "verified" — the worst possible outcome for a gate whose whole job is to be trustworthy, because it passes and leaves evidence claiming it checked.

🔍 Root Cause

The descriptor recorded where (a URL) and what number (a PID), never who. try_reuse() required status: running, kill -0 $pid, curl OK on the health path and on /, TTL freshness, and no source newer than startedAt — every one of which a foreign process satisfies, because kill -0 only asks whether some process currently holds that number and the two HTTP probes only ask whether something answers on that port. teardown_stale() killed whenever startedByThisRepo was true, and write_descriptor() writes that field as the literal true on every boot, so it could never be false; it also never consulted status, so a descriptor left behind by test-env-down.sh (which rewrote status to stopped but kept app.pid) still nominated that PID for kill and then kill -9. test-env-down.sh carried the identical pattern.

The identity signal already existed and was simply unused: GET /api/v1/health returns repoRoot, the checkout path of the answering instance (packages/cezar/src/server/server.ts:1476), and start_app() launches node packages/cezar/dist/index.js … --repo "$REPO_ROOT", a per-worktree argv fingerprint.

What Changed

• .ai/scripts/test-env-up.sh — new is_our_server() reads the live process's argv via ps -ww -o command= -p and requires both packages/cezar/dist/index.js and --repo $REPO_ROOT. The match is anchored at end-of-argv (or a space) deliberately: an unanchored match would let the main checkout claim a worktree's server, since <root> is a prefix of <root>/.ai/cezar/worktrees/<id>. New health_repo_root_matches() parses repoRoot out of the health payload and compares it through realpath — the server derives its root from git (getRepoInfo) while the script derives it from its own path, and the two can disagree on symlinked components; it degrades to a basename comparison for the CEZ_REMOTE-trimmed form, and treats an unreadable or empty answer as a mismatch, never a pass.
• try_reuse() — now also requires the recorded PID to be this worktree's server and the recorded port to be answered by this checkout. Both rejections log a greppable reason in the same shape as the existing source changed since boot (…) line.
• teardown_stale() — gates the kill on is_our_server and on status = running instead of on startedByThisRepo. A PID that is not ours is logged and left alone.
• write_descriptor() — records app.repoRoot, which checkout this instance belongs to, for descriptor consumers and debugging. The guards deliberately do not read it back: the descriptor is the artifact that goes stale, so each derives $REPO_ROOT itself and compares against that — comparing the file against itself would be a no-op. Its startCommand string also regains the --repo flag the process has always carried in argv but the descriptor omitted.
• .ai/scripts/test-env-down.sh — the same guard before signalling (duplicated on purpose: these are deliberately standalone entrypoints), plus app.pid: null alongside status: "stopped", so a cleanly stopped descriptor nominates no PID at all. That is defence in depth, not a substitute — a crashed run still leaves status: running with a dead PID, which is what the identity guard covers.

Where ps is unavailable the guards fail closed: reuse is declined and the kill is skipped, costing a reboot rather than risking a wrong signal.

🧪 Tests

• npm run test:unit — pass; npm run build — pass; npm run test:package — pass; npm run typecheck — pass.
• npm test — 6084 passed / 6 failed. Every one of the 6 reproduces on a clean tree with no changes at all (directional-usage, agents-section, todos watch, two git-changes cases) or is flaky under full-suite load (automations-gate passes in isolation). None is in the changed area. A further 8 failures seen initially were an artifact of TMPDIR pointing inside the git repo, which breaks tests asserting "outside a git repository".
• Three new cases in packages/cezar/test/unit/test-env-launcher.test.ts, all red-green proven — they fail on the unfixed scripts and pass on the fixed ones:
  • a descriptor whose PID was recycled onto a foreign process leaves that process running;
  • an instance from another checkout on the recorded port forces TEST_ENV_REUSED=0 rather than being adopted;
  • a happy-path regression that an instance the fixture booted is still reused (TEST_ENV_REUSED=1), so the guards do not defeat the build cache.
• Two supporting fixture changes were load-bearing rather than cosmetic: ps joins the fixture's symlinked PATH (without it every guard fails closed and the tests would pass vacuously), and the fake server answers /api/v1/health echoing its own --repo (the real server reports repoRoot there; the fixture previously only special-cased /api/health). The survival assertions use ChildProcess.exitCode/signalCode rather than kill -0, which succeeds on a zombie — the first draft of the recycled-PID test passed against the unfixed script for exactly that reason.

💥 Breaking Changes

None. BACKWARD_COMPATIBILITY.md protects the HTTP API surface and the published package; this touches only the generated .ai/scripts/ test-env entrypoints and their unit test. .ai/qa/test-env.json gains one additive field (app.repoRoot) and corrects startCommand — the file is gitignored, per-worktree, and rewritten on every boot, so no consumer needs a migration. Neither descriptor consumer (.ai/scripts/e2e.sh, packages/web/e2e/agent-browser.ts) reads app.pid or app.startCommand.

Notes for the reviewer

• The issue also asked to correct app.healthPath (/api/health → /api/v1/health) and the hard-coded platform: "wsl2". Both already landed on main in bbd77e9b (feat(history): progressively load long sessions #739), after the issue's analysis was written, so they are not in this diff. Only the startCommand omission remained from that family.
• Out of scope, as the issue notes: the bootstrap lock at .ai/qa/test-env.lock has the same kill -0-only shape for stale-owner recovery, but its consequence is only waiting on a foreign PID for up to 300 s rather than signalling it. Worth a follow-up, not worth widening this fix.

[sapersky]

🤖 om-open-pr — 🏷️ label rationale

Labels could not be applied: this PR comes from a fork and its author has read access, so addLabelsToLabelable is rejected outright. Posting the intended set here for a maintainer to apply.

• 🔍 review — the PR is complete and ready for review; it enters the pipeline at the review stage rather than as work in progress.
• 🐛 bug — the scripts did something incorrect rather than something missing: they signalled and reused processes on evidence that did not identify them, so both the kill path and the reuse path could act on the wrong process.
• ⏭️ skip-qa — there is nothing to exercise in a browser. The diff is confined to two generated shell entrypoints under .ai/scripts/ and one existing unit test; no route, component, template, style, or user-visible string is touched, and the behavior it changes is verified by the three red-green unit cases rather than by manual QA.
• 🔹 priority-medium — the worst outcome is a false-green QA run against another branch's build, which is severe for a gate whose whole job is to be trustworthy, but it needs a stale descriptor plus a coincidence to trigger, so it is not release-blocking today.
• 🟢 risk-low — no production code and no API surface. The descriptor change is additive on a gitignored, per-worktree file that is rewritten on every boot, and neither of its two consumers (.ai/scripts/e2e.sh, packages/web/e2e/agent-browser.ts) reads the fields involved. Where ps is unavailable the new guards fail closed, which costs an unnecessary reboot rather than risking a wrong signal.

[sapersky]

🤖 om-open-pr — chain lock moved onto this PR. The om-auto-fix-issue run continues here with om-auto-review-pr; other auto-skills should skip this PR until the lock is released.

The in-progress label and self-assignment could not be applied (fork PR, read access), so this comment is the lock signal.

[sapersky]

🤖 om-auto-review-pr taking over the chain lock on this PR from om-open-pr. Reviewing now; the lock stays held until the chain completes.

[sapersky]

🤖 om-auto-review-pr — automated review. GitHub does not allow approving one's own pull request, so this is posted as a comment; the verdict below is the one this run would have submitted, and a maintainer's approval is still required.

---

🔍 Code Review

🎯 Summary

This PR replaces the test-env launcher's liveness-based decisions with identity-based ones. is_our_server() reads the live process's argv through ps -ww -o command= -p and requires both packages/cezar/dist/index.js and --repo $REPO_ROOT; health_repo_root_matches() requires the health payload's repoRoot to be this checkout. teardown_stale() is re-gated on those signals plus status = running instead of on startedByThisRepo, and test-env-down.sh gains the same guard plus app.pid: null on stop.

The diff is confined to two generated shell entrypoints under .ai/scripts/ and one existing unit test — no production code, no HTTP surface, no state file under .ai/cezar/. Reviewed against CODE_REVIEW.md and BACKWARD_COMPATIBILITY.md in full.

Two details raise the quality of the fix above the issue's own prescription and deserve calling out, because both are places where a plausible implementation would have been silently wrong:

• The argv match is anchored at end-of-argv or a following space. An unanchored *"--repo $REPO_ROOT"* would let the main checkout claim a worktree's server, because <root> is a genuine prefix of <root>/.ai/cezar/worktrees/<id> — that is this repository's own layout, not a hypothetical.
• The repoRoot comparison goes through realpath on both sides. The server derives its root from git (getRepoInfo, packages/cezar/src/index.ts:115) while the script derives it from its own path ($SCRIPT_DIR/../..), and the two disagree on symlinked components — which is exactly what a macOS /var → /private/var temp dir produces. A naive string comparison would have failed closed on every reuse in the test fixture and, worse, looked like it was working.

✅ Verdict: APPROVE

No blockers and no majors. One minor finding (an inaccurate code comment) and one nit, both listed below; neither affects behavior. The local npm test failures are documented in the gate table with the evidence that they are not attributable to this change.

🧪 Validation Gate

Run against the PR head 4ac8c765 in an isolated worktree.

Command	Result	Notes
npm run typecheck	✅ pass
npm test	⚠️ 6084 passed / 6 failed	See the note below — every failure reproduces on a pristine tree.
npm run test:unit	✅ pass	Includes the three new launcher cases.
npm run build	✅ pass	Includes the check:pack tarball gate.
npm run test:package	✅ pass

On the npm test failures. om-code-review rightly refuses "pre-existing" and "flaky" as bare excuses, so here is the evidence rather than the claim. The six are directional-usage, agents-section, todos (watch unsubscribe), automations-gate (background scheduler), and two git-changes cases. Five of them were re-run with every change in this PR stashed — a pristine tree, no diff at all — and failed identically. The sixth, automations-gate, passes in isolation and fails only under full-suite load, which is the signature of a timing-sensitive scheduler test rather than a regression. None is in or downstream of the changed area: this PR touches no TypeScript source, only two shell scripts and one test file that no other test imports.

A further eight failures seen on the first run were an artifact of the local TMPDIR pointing inside the git repository (.ai/cezar/tmp/…), which breaks every test asserting behavior "outside a git repository" — getRepoInfo correctly finds the enclosing repo. Re-running with TMPDIR outside the checkout removed all eight. CI (ubuntu-latest, normal TMPDIR) is the authority here and is the reason the CI follow-up step exists.

📌 Findings

Minor

1. .ai/scripts/test-env-up.sh:428 — the comment on app.repoRoot says the guards compare against it; they do not.

The comment reads "it is what the reuse and teardown guards compare against". Neither guard reads the descriptor field: is_our_server() compares the live argv against the runtime $REPO_ROOT (line 117), and health_repo_root_matches() compares the health payload against the same runtime value (line 137). Nothing calls json_get "$ENV_DESCRIPTOR" app.repoRoot.

This is not a nit about wording — it misdescribes the security property. A future reader could reasonably conclude that tampering with app.repoRoot in the gitignored descriptor would redirect the guards, and try to "simplify" by having the guards read the field. That would reintroduce the exact bug this PR fixes: the descriptor is the untrusted artifact, and comparing it against itself is a no-op, since the same script at the same path always writes the same value. The runtime derivation is load-bearing precisely because it does not come from the file.

Fix: state what the field actually is — an informational record for descriptor consumers and for debugging — and note explicitly that the guards deliberately derive their own value instead of trusting the file. Cite #898 inline while there, per CODE_REVIEW.md ("comments cite the spec or issue that motivated the code").

Nit

2. .ai/scripts/test-env-up.sh:104 — the identity block has no issue citation.

The file's history header cites #898, and test-env-down.sh carries the citation on its own history line, but the is_our_server / health_repo_root_matches block itself — the least obvious code in the diff — does not. Worth one reference so the reasoning is one git log away. Folding it into the fix for finding 1.

💥 Breaking Changes

• Exported APIs — none touched.
• HTTP routes and response shapes — none touched. GET /api/v1/health is consumed, never modified. BACKWARD_COMPATIBILITY.md §2 protects its shape and specifically documents that repoRoot is an absolute path in local mode but a basename under CEZ_REMOTE (security: defense-in-depth hardening bundle (health repoRoot leak, href protocol guard, terminal quoting, git dash-guard) #431); the new comparison honors exactly that, degrading to a basename comparison when the value is not absolute rather than reporting a false mismatch.
• Event names — none touched.
• CLI flags — none touched. --repo is read from argv, not introduced or changed.
• DB schema / migrations — none in this repository's model.
• Config formats — .ai/cezar/ state files (§3) are untouched. .ai/qa/test-env.json is not a protected surface: gitignored, per-worktree, rewritten on every boot. Its change is additive (app.repoRoot) plus a correction to startCommand, which now matches the argv the process has always carried.
• Consumer check — both descriptor consumers were read. .ai/scripts/e2e.sh:45 reads only browser.installed and browser.notes; packages/web/e2e/agent-browser.ts:16 reads baseUrl and the browser block. Neither reads app.pid, app.startCommand, or app.healthPath, so nulling the PID on stop cannot break them.

Graceful degradation (CODE_REVIEW.md priority 2) — checked, and this is the property most at risk in a change like this. Where ps is unavailable or does not support -o command= (busybox, a stripped container), is_our_server() returns non-zero. That declines the reuse and skips the kill: the run boots a fresh instance on a free port instead of attaching or signalling. The degradation costs a rebuild, never a wrong signal or a hard failure — the correct direction. CI is ubuntu-latest with procps, and the flags were verified locally on Darwin.

🧪 Test Coverage

Coverage is the strongest part of this PR, and was verified rather than taken on trust.

• Red-green proven. All three new cases were run against the unfixed scripts (changes stashed) and against the fixed ones: 3 fail → 3 pass. A bug fix without a test that fails without the fix is a major finding under this repo's rules; this one has three.
• The first draft of the recycled-PID case was a false pass, and the author caught it. It asserted survival with process.kill(pid, 0), which succeeds on a zombie — a killed-but-unreaped child of the test runner. The test passed against the unfixed script, i.e. it would have shipped as a test that could never fail. It now asserts on ChildProcess.exitCode/signalCode, which an exit cannot hide from. This is the single most valuable thing in the diff and is exactly the class of defect that makes a suite decorative.
• Two fixture changes are load-bearing, not cosmetic. ps joins the fixture's symlinked PATH — without it every guard fails closed and all three tests would pass vacuously. The fake server now answers /api/v1/health echoing its own --repo, mirroring what the real server reports; previously it special-cased only /api/health, which the script has not polled since HEALTH_PATH was corrected.
• The happy path is guarded too. TEST_ENV_REUSED=1 is asserted for an instance the fixture itself booted, so the guards cannot silently defeat the build cache — the obvious way this fix could regress everyone's iteration time.
• The stopped-descriptor invariant is asserted (app.pid === null after down), which locks in the defence-in-depth half of the change.

No further cases are required. One gap is worth naming rather than fixing: none of this runs on Linux locally, and ps -ww -o command= output is the one platform-dependent input. CI covers it via npm run test:unit on ubuntu-latest.

[sapersky]

🤖 om-auto-review-pr — autofix applied

Both review findings are fixed in 46fe64a0, and the PR body carried the same inaccuracy so it was corrected too.

• Minor 1 — the app.repoRoot comment claimed the guards compare against the descriptor field. They do not, and the distinction is the security property: the descriptor is the artifact that goes stale, so both guards derive $REPO_ROOT at runtime and never read the file back. The comment now says what the field is (an informational record for descriptor consumers and debugging) and states explicitly why the guards derive their own value instead.
• Nit 2 — the identity block now carries its #898 citation inline, per CODE_REVIEW.md.

The change is comment-only in .ai/scripts/test-env-up.sh; no executable line moved. Re-verified with sh -n and the full test-env-launcher suite (3 passed, 1 skipped — setsid is unavailable on this host, which CI covers on ubuntu-latest). Typecheck and build are unaffected by a shell comment and were not re-run.

Verdict is unchanged: approve, pending a maintainer's review since self-approval is not permitted.

[sapersky]

🤖 om-auto-review-pr — CI result

CI has not run and will not run without a maintainer. Both workflow runs for this branch (32032429231, 32033818664) are sitting at action_required: .github/workflows/ci.yml triggers on pull_request, and GitHub holds fork PRs from outside contributors until someone with write access clicks Approve and run workflows. This author has read access, so no amount of waiting changes that state — this agent is not going to poll it, and no further CI follow-up will come from this run. license/cla is the only check reporting, and it passes.

Local gate, run against 4ac8c765 in an isolated worktree, as this run's evidence only:

Command	Result
npm run typecheck	✅ pass
npm test	⚠️ 6084 passed / 6 failed — every one reproduced on a pristine tree with this PR's changes stashed, or flaky under load (automations-gate passes in isolation)
npm run test:unit	✅ pass — includes the three new launcher cases
npm run build	✅ pass — includes the check:pack tarball gate
npm run test:package	✅ pass

The follow-up commit 46fe64a0 is comment-only in a shell script; it was re-verified with sh -n and the full test-env-launcher suite.

This is evidence, not a substitute for branch protection. The required checks still gate the merge, and one platform-dependent input in this change is genuinely worth seeing green on CI rather than on a developer's Mac: ps -ww -o command= was verified on Darwin here, and ubuntu-latest is where its Linux behaviour — and the setsid variant that skips on this host — actually gets exercised.

🏷️ Labels could not be applied at any point in this run (fork PR, read access), ci-monitoring included; the intended set is in the label-rationale comment above.

[sapersky]

🤖 om-auto-review-pr — completed: APPROVE (posted as a comment; self-approval is not permitted). Lock released — the om-auto-fix-issue chain ends here and this PR is free for anyone to act on.

What a maintainer needs to do next: apply the labels listed in the rationale comment, click Approve and run workflows so CI actually executes, and submit the human review this PR still requires.