[JacobStephens2]

Summary

When a failure unwinds out of run, spec_run or fix, dropping the Worktree adds a cleanup stage to the Command record before CommandRecord::finish runs. Two places use the record's stages to say where a Run stopped: finish, when there is no Recovery or its stage records nothing (Stage::BaseFix), and Where runs failed, for older or died records. Both took the last stage, so they reported cleanup instead of the stage that actually failed.

Both now use one rule, CommandRecord::stopped_stage, which replaces last_stage:

 stages: implement → CI watch → cleanup
-stopped_at = last stage                                   → "cleanup"
+stopped_at = last stage that isn't cleanup, else last stage → "CI watch"

CommandRecord::finish                  recovery.stage().recorded()  (unchanged, wins)
                                         else stopped_stage()
metrics::failures::read (fallback)     stopped_stage()

If cleanup is the only stage, it is still reported. docs/guide.md (regenerated into site/docs/index.html), site/architecture/index.html and the where_runs_failed OpenAPI description now describe the new fallback.

Evidence

Execution evidence

The host has no C linker, so Rust tests ran in the local node:22 Debian image, using the pinned 1.99.1 toolchain from the host.

Cause confirmed. impl Drop for Worktree (src/worktree.rs:544) calls logs::stage(Stage::Cleanup) before Started::finish reaches CommandRecord::finish. The two seams below fail only when a trailing cleanup is present.

• Before: cargo test --bin thirdshift a_command_records_its_version_at_start_and_where_a_failure_stopped_at_its_end (stages implement, CI watch, cleanup, recovery None)

  panicked at src/logs/recording/tests.rs:770:9:
  assertion `left == right` failed: None
    left: (String("cleanup"), String("stage_failed"))
   right: (String("CI watch"), String("stage_failed"))
  test result: FAILED. 0 passed; 1 failed
  

  After: same command

  test logs::recording::tests::a_command_records_its_version_at_start_and_where_a_failure_stopped_at_its_end ... ok
  

  The test also covers Stage::BaseFix, checks that an explicit SelfMerge Recovery still wins, and checks that a cleanup-only record still reports cleanup.
• Before: cargo test --test metrics where_runs_failed_groups_recorded_endings (an unrecorded failed record with stages pull request, Self-merge, cleanup, and a died record with only cleanup)

  left:  [..., ("cleanup", "died", false, "acme/widgets#16", ...), ("cleanup", "failed", false, "acme/widgets#15", ...), ...]
  right: [..., ("Self-merge", "failed", false, "acme/widgets#15", ...), ..., ("cleanup", "died", false, "acme/widgets#16", ...), ...]
  test result: FAILED. 0 passed; 1 failed
  

  After: same command

  test where_runs_failed_groups_recorded_endings_and_falls_back_for_older_and_died_commands ... ok
  

Wider checks (scripts/affected-tests main selects the full Rust suite, the site checks and the Console browser tests; it was run by hand because neither cargo-nextest nor a linker is installed on the host):

• cargo test --locked --no-fail-fast: 4453 passed, 2 failed. Both failures also happen at the merge base built in its own target dir, so they come from the container, not this change:
  • muse_sessions::muse_runs_unattended_… fails because its no-warning: check sees could not read process …'s environment: Permission denied.
  • trial_scope::trial_sessions_keep_… fails because systemctl is missing.
• npm run docs:check --prefix site: OK. npm test --prefix site: 68/68 passed.
• npm test --prefix console (Playwright, run as root in a container): 198/205 passed. The same 7 host-name tests (32, 74, 124) fail on the merge-base binary as well, because the container hostname is random.
• cargo fmt --check: OK.

Visual evidence

Only wording changes. Each capture is the changed paragraph at a 1440×900 viewport, served over HTTP.

site/architecture/ (1440×900):

site/docs/ (user manual, 1440×900):

Review

Standards: 3 findings. Spec: 3 findings. Fixed:

• S1: the duplicated failed Ended construction in the test is now one shared closure.
• S3: the guide now says a Command enters cleanup when its Worktree is dropped.

The review did not read site/docs/index.html. Both reviewers excluded it as generated output: its source is docs/guide.md, its generator is site/generate-docs.mjs, and npm run docs:check --prefix site exits 0.

Unaddressed findings

Standards

• S2 (judgement call): the ending test closure now takes four positional arguments. Declined. The stage list is each case's scenario input: a trailing cleanup is the condition under test. The reviewer also judged it "acceptable" in a test.

Spec

• P1: last_stage was renamed to stopped_stage, though the issue asked for no rename. No change. last_stage had exactly two callers on main, the two fallbacks this issue targets (src/logs/command_record.rs:481, src/logs/metrics/failures.rs:115). Its old meaning, "the stage entered last", is no longer what either caller wants.
• P2: records already saved with stopped_at: "cleanup" and a failure_category keep it. No change. The issue's Desired behavior limits the fallback to records "without stopped_at" and says "explicit recovery.stage().recorded() values continue to take precedence unchanged". src/logs/metrics/failures.rs:113 reads a saved stopped_at as recorded. So the existing v0.30.0 cleanup rows stay, and new records are attributed correctly from this version on.
• P3: no failed (not died) cleanup-only case in the metrics test. No change. Failed and died records take the same branch, src/logs/metrics/failures.rs:114-121 → stopped_stage(), and only Ending differs. A failed cleanup-only record is covered through finish in src/logs/recording/tests.rs, and a died one by metrics fixture 16.

CI notes

• console-test: Working identifies live Review and Fix targets locally and across the Fleet failed with 390px remote review-explicit: visible PR target (run 43285536060). This is unrelated to the branch: it changes no Console code and doesn't touch console/tests/working-targets.test.cjs. The same assertion for the remote review-explicit row also fails on unrelated branches: issue-2052 (run 43283026428, 390px) and issue-2018 (run 43054228339, 1440px).

Security review outcome

• Security review incomplete: session failed, ended early or omitted a valid final line; see Session trace

Merge Danger

Door: two-way

Nothing already saved is rewritten. Only newly finished records, and reads of unrecorded ones, change.

Blast Radius: metrics

Only where failed Commands are attributed changes: stopped_at in new Command records and the Where runs failed fallback rows (local, Fleet and Console). Records that recorded nothing and ended with a trailing cleanup move from the cleanup row to the stage before it.

Closes #2107

Built with claude 2.1.322 · claude-opus-5-5 · high

[JacobStephens2]

Delivery completed on agent-authored or reviewed head d6ec591.

[JacobStephens2]

Review run. No fixes to push. Spec axis used the linked closing issues.

Reviewed PR #2140 at d6ec5915, against fixed point 943166dea1ef448c2a50e345bedc6b3ef6257bd4. The PR remains open and non-draft.

Standards

0 findings. No documented-standard violations or actionable baseline smells. The shared fallback keeps stage attribution in one place.

Standards report and final files-read list

Spec

0 findings. The implementation matches linked closing issue #2107: trailing cleanup is skipped, cleanup-only records retain cleanup, and explicit Recovery stages retain precedence.

Spec report and final files-read list

No source fixes were needed. The working tree is clean; no commit was created.

Unaddressed findings

• Standards: none.
• Spec: none.

No findings were declined or deferred; no triage issues were needed.

Review coverage

Both reviewers directly inspected all eight changed files, including generated site/docs/index.html. Generated-output exclusions: none. Unread changed files: none.

Each axis’s final files-read list consists of this shared list plus its additions below:

.agents/skills/codebase-design/SKILL.md
.thirdshift-review-input.md
AGENTS.md
CODING_STANDARDS.md
GLOSSARY.md
docs/adr/0011-thirdshift-keeps-a-per-repository-activity-log.md
docs/adr/0016-the-console-is-a-long-running-reader.md
docs/agents/domain.md
docs/code-navigation.md
docs/guide.md
site/architecture/index.html
site/docs/index.html
site/how-it-works/index.html
site/index.html
src/console/openapi.rs
src/logs/command_record.rs
src/logs/metrics/failures.rs
src/logs/recording.rs
src/logs/recording/tests.rs
src/recovery.rs
src/worktree.rs
tests/metrics.rs

Standards additionally read:

.agents/skills/code-review/SKILL.md
docs/adr/0026-each-console-serves-an-api-that-can-do-what-the-cli-can.md
docs/agents/issue-tracker.md
docs/agents/triage-labels.md
docs/testing.md
site/tests/README.md
src/logs/metrics.rs

Spec additionally read src/command.rs.

Validation

All commands ran in this Review worktree.

Exact command	Result
npm ci --prefix site	Exit 0; dependencies installed
npm ci --prefix console	Exit 0; dependencies installed
scripts/affected-tests main	Exit 0; all selected checks passed
npm test --prefix site	Exit 0; manual generation check and 69 tests passed
cargo build --locked	Exit 0
npm test --prefix console	Exit 0; 205 tests passed
cargo test	Exit 0; 4,470 passed, 199 intentionally ignored, 0 failed
git diff --check 943166dea1ef448c2a50e345bedc6b3ef6257bd4...HEAD	Exit 0

cargo-nextest was unavailable, so the selector used cargo test. Both changed regression tests passed within that full run. No selected validation command failed or was blocked.

Validation log · summary.json, containing {"standards":0,"spec":0}.

Standards: 0 findings, worst issue: none. Spec: 0 findings, worst issue: none.

Security

No unaddressed introduced findings.

CI red at d6ec591

• console-test: https://github.com/JacobStephens2/thirdshift/actions/runs/43292261587/job/131441262875

Inherited failures on main at 943166d:

Repair 1

No branch-owned fix was needed. The working tree is clean, and no commit was created. PR #2140 remains open, ready, and unchanged at d6ec5915.

The failure comes from an existing fixture-write race: rewriting the remote record briefly removes its row, then its return triggers “2 need you” before the third row arrives. Holding that empty interval for 350 ms reproduced the exact CI assertion. The test and involved runtime code are unchanged from main; this branch’s stage-attribution changes are not involved.

Validation against that head:

Exact command	Result
cargo build --locked	Exit 0
node --test --test-name-pattern='Fleet Inbox polls' console/tests/fleet.test.cjs	Exit 0 in all 11 focused runs
node --test --test-name-pattern='Fleet Inbox polls' .review-logs/fleet-truncated-write.test.cjs	Exit 1, expected diagnostic reproduction; temporary probe removed
scripts/affected-tests origin/main	Exit 0; all selected checks passed
npm test --prefix site	Exit 0; generated docs check and 69 tests passed
npm test --prefix console	Exit 0; all 205 tests passed
cargo test	Exit 0; 4,470 passed, 199 existing ignored tests

The selector used cargo test because nextest is unavailable. No required local checks were blocked. Logs are under .review-logs/. Nothing was pushed, and CI was not rerun.

CI passed or absent at d6ec591