feat(runner): add lifecycle execution and environment checks (Task 2.1) - #7
Conversation
executeScenario runs one scenario through prerequisites, install, reset, launch, optional setup, steps and cleanup, each phase bounded and abortable, and emits an event per phase without ever claiming a phase that did not run. Outcomes: a missing prerequisite (not a designated test environment, a dirty environment, the wrong OS or architecture, a missing display or audio capability) is blocked and nothing is installed; only an explicit AssertionFailure (or node's AssertionError) is failed; cancellation is cancelled; a timeout, a broken hook or an unexpected error is interrupted, so infrastructure trouble is never blamed on the candidate. Ownership: anything a run creates is recorded in a ledger under the test root before it is used. Cleanup removes only what the ledger lists: a process only if its recorded start time still matches (a reused pid is never signalled, an unidentifiable live process is left alone), a path only if its resolved location is strictly inside the test root (links, junctions and ".." cannot redirect a delete, the root itself is never removed). A cleanup that cannot finish leaves the environment dirty and refuses the next run until reset. Nothing runs anywhere that lacks an explicit test-root marker, and the home directory, the filesystem root and their ancestors are never accepted. Built test-first: 57 failing tests against stubs, then 392 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…est root (Task 2.1 review) Addresses the review of the first executor: - every wait is bounded and abortable: prerequisites, probes, the event sink, waitFor conditions - hooks that outlive their phase are tracked, refused new ownership, and leave the environment dirty if they never stop - one run at a time per test root (lock file with process identity, stale locks taken over) - a dirty marker that cannot be written is remembered in memory; an unreadable marker or ledger blocks the run - cleanup event-sink failures are reported in the result and mark the environment dirty - escalation to SIGKILL re-checks the process identity first; failed signals keep the resource as still-running - spawnOwned stops a child it cannot record, and tolerates commands that exit at once - probes that throw synchronously count as absent capabilities - test fixtures no longer leak processes or directories, and no longer assume an x86_64 host Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…entity read The first lock in a process reads its own identity, which on Windows starts PowerShell and can take seconds on a busy runner. Tests now read it before any deadline starts, the test timeout allows for it, and a failed read is no longer remembered for the life of the process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/qa/test/fixtures/execution.ts">
<violation number="1" location="packages/qa/test/fixtures/execution.ts:10">
P2: This fixture duplicates helpers that already live in execute.test.ts, and that file still defines and uses its own copies instead of importing them: `never`, `lifecycleOf`, `arrange`, `scenarioOf`, and `started` at lines 25–54 of execute.test.ts are identical in shape to this new file. Because execute.test.ts does not import from fixtures/execution.ts, the warmup added here (`acquireTestRoot(testRoot)` + release in `arrange`) never runs for that suite: its first `acquireTestRoot` still happens inside executeScenario's prerequisites with the 2000 ms phaseMs deadline, so the Windows first-identity-read timing out that this commit is meant to fix remains possible on the largest suite. Migrate execute.test.ts to these shared helpers, or add the same warmup to its local `arrange`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| export const never = (): Promise<void> => new Promise<void>(() => {}); | ||
| export const sleep = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms)); | ||
|
|
||
| export function lifecycleOf(calls: string[], overrides: Partial<Lifecycle> = {}): Lifecycle { |
There was a problem hiding this comment.
P2: This fixture duplicates helpers that already live in execute.test.ts, and that file still defines and uses its own copies instead of importing them: never, lifecycleOf, arrange, scenarioOf, and started at lines 25–54 of execute.test.ts are identical in shape to this new file. Because execute.test.ts does not import from fixtures/execution.ts, the warmup added here (acquireTestRoot(testRoot) + release in arrange) never runs for that suite: its first acquireTestRoot still happens inside executeScenario's prerequisites with the 2000 ms phaseMs deadline, so the Windows first-identity-read timing out that this commit is meant to fix remains possible on the largest suite. Migrate execute.test.ts to these shared helpers, or add the same warmup to its local arrange.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/qa/test/fixtures/execution.ts, line 10:
<comment>This fixture duplicates helpers that already live in execute.test.ts, and that file still defines and uses its own copies instead of importing them: `never`, `lifecycleOf`, `arrange`, `scenarioOf`, and `started` at lines 25–54 of execute.test.ts are identical in shape to this new file. Because execute.test.ts does not import from fixtures/execution.ts, the warmup added here (`acquireTestRoot(testRoot)` + release in `arrange`) never runs for that suite: its first `acquireTestRoot` still happens inside executeScenario's prerequisites with the 2000 ms phaseMs deadline, so the Windows first-identity-read timing out that this commit is meant to fix remains possible on the largest suite. Migrate execute.test.ts to these shared helpers, or add the same warmup to its local `arrange`.</comment>
<file context>
@@ -0,0 +1,45 @@
+export const never = (): Promise<void> => new Promise<void>(() => {});
+export const sleep = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms));
+
+export function lifecycleOf(calls: string[], overrides: Partial<Lifecycle> = {}): Lifecycle {
+ const record = (name: string) => async () => { calls.push(name); };
+ return { install: record('install'), reset: record('reset'), launch: record('launch'), cleanup: record('cleanup'), ...overrides };
</file context>
There was a problem hiding this comment.
Fixed in 4514367. execute.test.ts now imports the shared fixtures, so the warm-up runs there too and the duplicate helpers are gone. That was the cause of the remaining Windows failure in the previous CI run.
…remove timing fragility - execute.test.ts uses the shared fixtures (and so the identity warm-up) instead of its own copies - the longer test timeout applies only to the files that read process identity, not the whole package - the late-lock test waits for the late acquisition and polls for the release instead of sleeping - the process-identity test no longer assumes two processes started together have different start times - remove an unused constant Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…alling it unidentifiable On Linux a child that has just exited is a zombie until it is reaped: it still answers "alive" but its identity cannot be read. Cleanup reported such a process as identity-unknown, and spawnOwned killed and rejected a short-lived command whose exit had not been reported yet. Both now wait briefly for the process to finish leaving; one that is still there and unidentifiable is left alone and reported as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/qa/src/runner/resources.ts">
<violation number="1" location="packages/qa/src/runner/resources.ts:220">
P2: When a root has several processes whose identities cannot be read, cleanup waits 500 ms for each entry serially and can exceed `cleanupMs` by an unbounded amount. Bound the aggregate resource cleanup by the phase deadline or use a shared deadline for these waits.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * its parent reaps it) or cannot be identified at all. Only the second is left alone and reported, so the first is | ||
| * given a moment to finish leaving. | ||
| */ | ||
| const unidentifiable = async (): Promise<CleanupFailure | undefined> => ((await waitUntilGone(500)) ? undefined : { resource, reason: 'identity-unknown' }); |
There was a problem hiding this comment.
P2: When a root has several processes whose identities cannot be read, cleanup waits 500 ms for each entry serially and can exceed cleanupMs by an unbounded amount. Bound the aggregate resource cleanup by the phase deadline or use a shared deadline for these waits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/qa/src/runner/resources.ts, line 220:
<comment>When a root has several processes whose identities cannot be read, cleanup waits 500 ms for each entry serially and can exceed `cleanupMs` by an unbounded amount. Bound the aggregate resource cleanup by the phase deadline or use a shared deadline for these waits.</comment>
<file context>
@@ -217,6 +212,18 @@ async function cleanProcess(resource: Extract<OwnedResource, { kind: 'process' }
+ * its parent reaps it) or cannot be identified at all. Only the second is left alone and reported, so the first is
+ * given a moment to finish leaving.
+ */
+ const unidentifiable = async (): Promise<CleanupFailure | undefined> => ((await waitUntilGone(500)) ? undefined : { resource, reason: 'identity-unknown' });
+
+ if (!isAlive(pid)) return undefined;
</file context>
There was a problem hiding this comment.
Fixed in f60e38d, and the problem was wider than the 500 ms wait: the executor bounded the cleanup hook by cleanupMs but the reaping of owned resources after it had no deadline at all (a grace period per process, one after another). Reaping now has its own cleanupMs deadline. When it is cut off, the failure is reported in cleanup.failures, the resources it did not get to stay on the ledger, and the environment is marked dirty. Test: execute-cleanup-bound.test.ts, which hangs the reaper and fails (30 s timeout) without the change.
…line Reaping waits for each owned process in turn (a grace period each), and had no deadline of its own, so a ledger with several stubborn processes could hold a run up for as long as the sum of their waits. It is now bounded by cleanupMs; what it does not finish stays on the ledger, is reported in cleanup.failures, and keeps the environment dirty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… ledger Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Task 2.1 of the plan. Exit: prerequisite failure is
blocked, assertion failure isfailed, infrastructure interruption is explicit, and cleanup does not target unrelated state.This is the first code with real OS side effects (spawning, killing, deleting), so the design is built around what it must never touch.
executeScenario(context, scenario)Runs prerequisites → install → reset → launch → (setup) → steps → cleanup. Every phase has a deadline and can be aborted; a hook that ignores its signal cannot hold the run up. An event is emitted per phase and never for a phase that did not run.
display/audioblocked(nothing installed)AssertionFailure, or node'sAssertionErrorfailedcancelledinterruptedOnly an explicit assertion blames the candidate. A launch that throws is
interrupted, notfailed; a hook can throwAssertionFailureto say the candidate really is broken.Owning only what the run created (
resources.ts)ctx.own,ctx.spawn), so a crash still leaves a record./procstarttime, WindowsStartTime) still matches. A reused pid is never signalled; a live process that cannot be identified is left alone and reported. POSIX: SIGTERM, then SIGKILL after a grace period..., symlinks and junctions cannot redirect a delete; the root itself and links are refused.blockeduntilresetDirtyEnvironmentsucceeds completely.designateTestRoothas not put a marker. The home directory, the filesystem root and any directory containing the home directory are refused, marker or not.How it was built, and two things that went wrong
Test-first: 57 tests failed against stubs (I strengthened the ones that passed by coincidence), then 434 pass, 1 skipped (Linux-only). The rules were then broken one at a time (33 in the first round, 21 more in the review round, each caught by a test).
designateTestRoot(homedir())for real. When I mutated the safety check on purpose, the broken code did exactly what a broken check does and wrote.release-qa-test-rootintoC:\Users\<you>. I found it because the test then failed on every run. I verified the file was byte-for-byte my marker (69 bytes, nothing else matched.release-qa*), deleted only that file, and rewrote the tests so no test can write to a real home:checkTestRoot/designateTestRoottake ahomeoption and the tests use a throwaway one. A mutation run afterwards confirmed nothing was written to the real home or the drive root.Test hygiene: every scratch directory the tests create is registered for removal after the test even when it fails; two full runs leave no temp directories, no helper processes and no marker.
What is not verified
spawnOwnedhalf is only exercised by the racy "command that exits at once" test on Linux CI; I could not reproduce the race on Windows.display/audioare heuristics, not proof:DISPLAY/WAYLAND_DISPLAYor a non-ServicesSESSIONNAME;/proc/asound/cardsor theAudiosrvservice. They do not check for a real device or distinguish Xvfb from a real desktop (Task 2.2's Linux preflight).ctx.own/ctx.spawn) and, if it has not stopped withinabandonedGraceMs, the environment is marked dirty; a late filesystem write from such a hook is detected only through that marker, not prevented.spawnand the ledger write (milliseconds) would orphan the process.Decisions to review
resetruns afterinstalland beforelaunch, so the app starts from a known state.interrupted, notfailed(see above). Say if launch failures should befailed.ScenarioResultcarries an outcome, not anAttempt; turning it into one (and into journal events) is Task 2.2.Test plan
npm ci,npm run typecheck,npm test: 434 passed, 1 skipped (Windows 11, Node 24.13); repeated and concurrent runs leak nothingubuntu-24.04andwindows-2025(the first runs found a Windows timeout on the first identity read and a Linux zombie race; both fixed)🤖 Generated with Claude Code
Summary by cubic
Implements Task 2.1 of the release-qa plan:
executeScenariomeasures the environment, gates on prerequisites, runs a scenario through install → reset → launch → optional setup → steps → cleanup, and cleans up only what the run itself created.Outcomes and ownership
blockedand install nothing; only an explicitAssertionFailuregivesfailed; a throwing launch hook or any other error isinterrupted, never blamed on the candidate..refused)cleanup.failures, and keep the environment dirtyCaveats
/proc/asound/cards) and doesn't verify real devicesresetruns afterinstalland beforelaunch; launch failures currently classify asinterrupted— confirm whether they should befailedWritten for commit 796ab26. Summary will update on new commits.