[JacobStephens2]

Summary

A Review run that ended without Delivery kept its worktree and local branch because of validation logs that its own prompt had told the session to write. The cause: SkillWorktree::on_pull_request started in UntrackedFiles::Preserve, and ReviewProcess only switched to discarding untracked files once delivered was true. So Drop counted .review-logs/ as uncommitted work.

 SkillWorktree::on_pull_request
-  untracked: Preserve                          # any untracked file retains
+  untracked: DiscardNamedValidationArtifacts   # root `.review-logs/` and `.validation-*` are disposable
 ReviewProcess::execute
   if delivered
     checkout.discard_validation_artifacts()    # unchanged: DiscardAllUntracked (renamed)
 uncommitted_changes(untracked)
-  status --porcelain --untracked-files=<all|no>; non-empty => work
+  status --porcelain -z <untracked.status_flag()>
+  any entry where untracked.keeps(entry) => work

Preservation is unchanged. Unpushed commits, tracked or staged edits, and any untracked file outside the two named roots still keep the worktree and log the warning. The Review validation prompt now names .review-logs/ as the place for disposable logs. The user manual and the homepage say that an undelivered Review run frees its worktree when only those files are left.

Evidence

Execution evidence

• Before (fix not applied): cargo test --test review_run an_undelivered_review

  test an_undelivered_review_leaving_only_validation_artifacts_frees_its_worktree_and_branch ... FAILED
  test an_undelivered_review_preserves_an_untracked_source_file_beside_validation_artifacts ... ok
  thirdshift: step review-and-address: 1s failed: claude exited 1 (...)
  thirdshift: cleaning up the worktree and local branch colleague
  thirdshift: warning: retaining checkout /tmp/.tmpmq7bBP/tmp/thirdshift-review-Moumcm and local branch colleague at b65bd94…: the worktree has uncommitted changes
  

  After: same command → test result: ok. 2 passed; 0 failed. The full review_run file gives 37 passed. The existing retry_delivers_commits_retained_after_delivery_validation_failed still shows an undelivered run with an unpushed commit being retained.
• cargo test --bin thirdshift worktree → 197 passed. cargo test --bin thirdshift prompts_page → 14 passed. npm run docs:check --prefix site → exit 0. npm test --prefix site → passed.
• scripts/affected-tests main selected the full suite. The unit tests (1809 passed) and every integration file passed except tests/process.rs, where 1–2 tests in findings:: fail intermittently. Those tests fail at the same rate with this branch's sources reverted to origin/main: cargo test --test process findings:: failed 3 of 3 times on main's sources and 1 of 3 on this branch. That flakiness predates this change. Because of fail-fast, I ran the integration files after process separately (cargo test --no-fail-fast --test process_identity … --test user_config), and all of them passed.

Visual evidence

Homepage, Review-run validation paragraph (fold opened), 1440×900:

Review coverage

The thirdshift-code-review review used refs/remotes/origin/main as the fixed point.

• Standards: 3 judgement-call findings, all addressed in 045741d. The comment on validation_artifact was made accurate. The variant that discards all untracked files was renamed to DiscardAllUntracked. The repeated switch moved into UntrackedFiles::status_flag/keeps.
• Spec: no findings.

Every changed file was read by both reviewers. The generated files were inspected as diffs and confirmed against their sources, so none was left unread: prompts/*.md and the fragment in site/prompts/index.html, regenerated from src/prompt.rs, and site/docs/index.html, regenerated from docs/guide.md.

Unaddressed findings

Standards

None.

Spec

None.

Security review outcome

No unaddressed introduced findings.

Merge Danger

Door: two-way

Reverting restores the old retention. Cleanup deletes only untracked files under root .review-logs/ or .validation-* in a Review checkout. A real file committed to a repository under those names is tracked, so it is unaffected.

Blast Radius: narrow

Only Review run worktrees (SkillWorktree::on_pull_request) change. Skill, Custom process and Ticket run worktrees keep their policies, and so does retained-checkout recovery.

Closes #2049

Built with claude 2.1.314 · opus · default effort