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!

[pbean]

Triage: kept as a standalone primary, and flagged as the highest-severity open issue.

Two notes from the triage pass, both drawn from the report itself:

1. It is the only open defect that fails toward landing bad work. Every other member of this
family — #88, #127, #160, #224, all closed — loses good work. This one merges unreviewed code and,
on the dev leg, skips the review entirely. That asymmetry is why it is not being nested under the
parallel-execution work even though parallelism is what surfaced it: the report establishes the
single production call site is reached from _result_json on every Stop and every window death,
and that only StoriesEngine exports the guard env var — so sprint and sweep modes both take the
mtime-scan path, with or without concurrent runs. Any writer touching a qualifying .md after
launch wins on mtime.

2. #224's proposed hardening and this issue constrain each other, and must be designed
together. #224's direction — synthesize the result from the authoritative frontmatter status:
— would make this worse, because the whole failure here is that a foreign spec's
status: done is being read as authoritative. Whoever picks either one up needs both open in front
of them.

Also worth preserving from the report, since it is the part that generalizes: the suggested
identity check is not a new concept, it closes an existing asymmetry — stories mode already
rejects a spec whose filename does not start with the story id, and synthesize_result already
receives the story_key it would need. The two caveats a naive filename rule breaks (the
bmad-dev-auto-result-* fallback carries no story key by design; sweep bundles are legitimately
called with story_key=None) are both called out above, as is the smaller alternative — narrowing
the main-checkout fallback in _artifact_dirs, which no test pins.

[pbean]

Re-validated at HEAD (1e6ec9f): still reproduces, and the surface grew since filing.

Reproduced directly against current code before touching anything:

• devcontract.find_result_artifact returns the foreign spec-i-13-*.md, and synthesize_result(..., story_key="i-12-…") returns {story_key: "i-12-…", spec_file: ".../spec-i-13-….md", status: "done", followup_review_recommended: False} with status_consistent=True. The identity mismatch sits inside the returned dict — story_key and spec_file disagree — and nothing reads it.
• All ten mechanism steps in the report are present as described.

Three things worth adding to the record:

1. The #224 fix added a second instance of the same hole. 71e18ce landed after this issue was filed and introduced devcontract.find_frontmatter_candidates — another glob("*.md") over the same shared dir with the same mtime-only floor. A foreign marker-less status: done spec is adopted through it too (reproduced). Under a dead window it synthesizes on a single sighting. So the report's note that #224 and this issue constrain each other was right, and the constraint has already partly materialized.

2. The #276 M1/M2 launch-snapshot gate does not cover this. It computes the content hash only when the candidate is the snapshotted spec, so a foreign candidate skips the gate entirely and reaches synthesis unchallenged.

3. verify_review cannot catch it, so the read-back is the only viable chokepoint. verify_review reads task.spec_file — the correct spec — which is legitimately status: done already, because _reset_spec_for_review deliberately never mutates frontmatter (it is load-bearing skill routing). Sprint-status is done, verify commands pass on the dev's code. The gate is vacuously satisfied and the forged session result is the only signal.

That last point also pointed at the fix: task.spec_file is recorded by verify_dev on dev success and handed to the review session in its own prompt (/bmad-dev-auto {task.spec_file}). The read-back was scanning a shared directory for a file it had already been told the name of — so the fix is to stop scanning where the path is known, rather than to add the filename rule the report suggests. That avoids the two caveats you flagged entirely (a pinned path needs no exemption for bmad-dev-auto-result-*, and none for story_key=None sweep bundles — which, per #161, legitimately adopt a differently-named pre-existing story spec that a filename rule would have rejected).

One correction to the report's "no test pins it" note on _artifact_dirs: that is accurate as far as it goes, but the rendered skill bakes an absolute main-checkout spec path (_bmad/render/bmad-dev-auto/step-01-clarify-and-route.md:79), which is plausibly why the "defensive fallback" is there. Narrowing it looks cheaper than it is, so I left it alone.

PR #298 — authoritative-path read-back plus the proof-of-work gate you listed as the optional second hardening. Details there.