Session read-back adopts another story's spec: a review that produced nothing is scored done

[Polloinfilzato]

Description

A session's read-back locates "the artifact this session produced" by scanning a shared
directory for the most recently modified qualifying *.md, with no constraint that it belongs
to the story being driven. When the session dies having produced nothing, a different story's
spec — written into that directory moments earlier by a concurrent run's merge-back — is adopted
as its result, and its status: done becomes this story's verdict.

Observed twice in one night, on two different call paths. On the review leg it merged unreviewed
code; on the dev leg it caused the review to be skipped entirely.

Unlike the rest of this failure family (#224, #127, #88, #160), this fails in the unsafe
direction: every prior member loses good work; this one lands unverified work.

Evidence — a natural experiment with two controls

Run 20260722-025215-9ffc, story i-12, dev=claude / review=codex, mux=tmux, macOS,
isolation="worktree", four runs launched as separate processes.

(The i-… story keys are this project's own epic-naming convention, carried by a small local patch to the sprint-status key parser. It changes only which keys are accepted — artifacts are located by mtime and heading, never by key shape — so it plays no part in what follows.)

Three review sessions wedged identically that night. Only one was scored a pass:

story	review-1 launched	died	pane log	hook events	qualifying foreign .md present	verdict
i-9	04:14:56	04:58:22	2 B	none	no	crashed ✅
i-13	04:23:07	04:59:20	0 B	none	no	crashed ✅
i-12	04:55:25	05:36:41	0 B	none	yes — spec-i-13-*.md, mtime 05:15:27	completed ❌

The only delta is the presence of a foreign artifact. (Control on the control: the review-2 /
review-3 passes of i-9 and i-13 — same codex adapter — wrote normal SessionStart+Stop
events and multi-MB logs, so the absent events are anomalous, not normal for this adapter.)

What the journal then recorded for i-12:

{"kind":"session-end","task_id":"i-12-…-review-1","status":"completed","tokens":null,"log_pos":0}
{"kind":"spec-reconcile-skipped-out-of-tree","story_key":"i-12-personality-projection",
 "spec":"…/dm20-protocol/_bmad-output/implementation-artifacts/spec-i-13-combat-stateview-projection.md"}
{"kind":"review-result","story_key":"i-12-personality-projection","cycle":1,"status":"done",
 "followup_review_recommended":false}
{"kind":"story-done","story_key":"i-12-personality-projection","commit":"5416fc3"}

Note the adopted spec path: it is the main checkout's, not the worktree's.

Where the foreign artifact came from — reflog and mtime agree to the second:

reflog: 43ef7b0 HEAD@{2026-07-22 05:15:27}: merge bmad-loop/…/i-13-combat-stateview-projection
stat:   _bmad-output/implementation-artifacts/spec-i-13-…md → 2026-07-22 05:15:27

i-13's own run merged its unit branch back into the target branch, which by design happens in
the main repo's working tree (workspace.py module docstring). That write landed inside i-12's
review window.

Mechanism

1. _DevSynthesisMixin._artifact_dirs builds the search list as the worktree-rebased dir plus
   the configured main-checkout dir, the latter labelled in-comment as a "defensive fallback".
   Under worktree isolation that second entry is a directory shared with every concurrent run.
2. _DevSynthesisMixin._synth_result takes the first hit from
   devcontract.find_result_artifact(dir, since_ns=handle.launched_ns).
3. find_result_artifact returns the most-recently-modified *.md carrying an
   ## Auto Run Result heading (or the bmad-dev-auto-result-* fallback, matched by name)
   with mtime >= since_ns. It is a glob("*.md") over a directory holding every story's spec.
   Nothing ties the match to the current story.
4. devcontract.synthesize_result already receives story_key but only stamps it into the
   returned dict — it never checks it against the spec it just read.
5. _ResultFileMixin._final then sets status = "completed" if result_json is not None, with
   accept_result=True by default.
6. escalation.decide_review_session PROCEEDs on completed, and the engine reads the foreign
   spec's status/followup_review_recommended as the review's verdict.

Why the scan reached the shared fallback at all: Engine._reset_spec_for_review strips the prior
## Auto Run Result from the story's own spec before every review launch (the #160 fix), and
the dead session never rewrote it — so the worktree dir held no candidate and the scan fell
through to the main checkout.

Exit path (established by exhaustive elimination, since events/ holds nothing for this
task): window death → _final(fallback="crashed") → upgraded to completed by the artifact.
Not the stall branch (never armed — that requires a result-less Stop), not timeout (2 476 s
elapsed vs session_timeout_min = 180), not the budget branch (gated on a transcript_path that
only a hook event populates), not _post_kill_reconcile (only fires on stalled/timeout/over_budget).

Second occurrence, same batch, different call path

Run …-94a5 (story i-11), dev leg, via _post_kill_reconcile:

05:52:03  session-rescued-post-kill   i-11-…-dev-1
05:52:04  session-end status=completed
05:52:04  spec-reconcile-skipped-out-of-tree  spec=…/spec-i-13-combat-stateview-projection.md
05:52:04  review-not-recommended → review-skipped
05:52:05  story-done

The same foreign spec was adopted, and its followup_review_recommended: false skipped the review
altogether. The repo later had to reopen that story (abb49e7 fix(sprint-status): reopen i-11 — the run marked it done but shipped no code).

Stated, not proven: I cannot show the i-11 code loss was caused by the adoption rather than
merely concurrent with it — that worktree is gone. It is offered as a second independent
occurrence of the mechanism, not as a proven causal chain.

Not limited to parallel runs

The single production call site is reached from _result_json (every Stop, every window death)
and from _post_kill_reconcile. The only guard is if spec.env.get("BMAD_LOOP_SPEC_FOLDER"),
exported only by StoriesEngine — so sprint and sweep modes both take the scan path.
Any writer touching a qualifying .md after launch wins on mtime: a git merge/checkout/rebase, a
human editing another spec, a concurrent sweep. With isolation="none" the search collapses to
one directory, but it is the same shared one — identical exposure.

Suggested fix direction

Stories mode already has this defence; the ask is to bring it to the sprint/sweep read-back.
verify_dev_stories rejects a spec whose filename does not start with the story id, and
stories.resolve_story_spec resolves by id rather than by mtime. Nothing equivalent exists for
sprint/sweep, and synthesize_result already holds the story_key needed to do it — so this
closes an existing asymmetry rather than introducing a new concept.

Two caveats a naive filename rule would break:

1. The no-spec fallback carries no story key in its name. bmad-dev-auto-result-* is matched
   by filename precisely because it has no heading (shipped fixtures:
   bmad-dev-auto-result-unclear-1234.md). A "name must contain the story key" rule breaks that
   documented contract — it needs an explicit exemption.
2. Sweep bundles have no story-shaped key (dw-<name> / dw<N>-<name>), and
   synthesize_result is legitimately called with story_key=None. The check must no-op on a
   falsy key.

A third, smaller option worth putting on the table: the root enabler is the main-checkout fallback
in _artifact_dirs, which under worktree isolation deliberately widens the search into a shared
directory. Narrowing or dropping it is a smaller change, and no test pins it —
test_generic_dev_finds_spec_in_worktree leaves the main dir empty and asserts only that the
cwd-derived dir is used.

Tests that pin the current mtime-only behaviour, for whoever takes this (all in
tests/test_devcontract.py): test_find_artifact_picks_newest_with_heading is the one that
literally pins "newest wins, identity irrelevant"; test_find_artifact_accepts_no_spec_fallback_prefix
and test_synthesize_result_non_utf8_fallback_marker_is_not_terminal are caveat 1. The
adapter-level tests in tests/test_generic_tmux.py all use story_key="3-1" against
spec-3-1-foo.md, so a spec-<key> prefix rule leaves them green — except
test_scan_readback_non_utf8_fallback_marker_returns_none, which is caveat 1 again.

Relationship to existing issues

• Proof-of-work gate: untracked halt residue can vacuously satisfy 'changes since baseline' (from-scratch re-drives) #88 — same defect class ("a gate that can be vacuously satisfied"), filed and fixed, on the
  dev leg. Closest precedent.
• Review HALT that omits ## Auto Run Result re-triggers the #149 livelock, then DEFER-drops a finished story #224 — the inverse: a review that did run is dropped. Its proposed hardening ("synthesize
  the result from the authoritative frontmatter status:") would make this worse; the two
  constrain each other.
• Completed, verify-green, committed story deferred: done-check reads only the self-reported status string #127 — "done-check reads only the self-reported status string": same sentence, dev leg, safe
  direction.
• psmux: pipe_pane targets a bare @N window id — per-server ambiguity binds the sink to the wrong server's pane (empty run logs) #254 / psmux pipe_pane sink captures nothing on 3.3.7: -EncodedCommand transport is stripped upstream, and the CopyTo sink can't flush incrementally #217 — empty pane logs on psmux. Ruled out: tmux backend, and the same run's dev log
  is 1.4 MB.
• Dev session lost to an API/transport failure is charged as a story timeout, exhausting the attempt budget #194 — same wedged-CLI precondition, opposite consequence. Its zero-token heuristic is
  relevant to the hardening below.

Optional second hardening

Independently of the identity check: require positive evidence that the session produced
something (non-empty transcript delta, or non-null token usage) before a read-back artifact can
upgrade a dead session to completed. That is the review-leg counterpart of #88's proof-of-work
gate, and it would catch this class even when no foreign artifact is present.

Not established

• Why three codex windows died silently after 36-43 minutes with no output. Not determinable
  from surviving evidence, and not needed: the bug is how the engine reacts to a dead session.
• The i-11 causal link (see above).

[coderabbitai]

🔗 Related PRs

#53 - fix(adapters): complete dev sessions only on Stop or window death [merged]
#77 - feat(stories): spec-to-loop engine, adapter, CLI + HITL checkpoints (Phase 2 + 3) [merged]
#86 - feat(resolve): intent-gap patch-restore re-drive + inline-review-layer preflight (re-cut) [merged]
#95 - feat(adapters): post-kill reconcile rescues finished stalled/timeout sessions [merged]
#175 - fix(adapters,engine): timely + observable session-timeout teardown [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!