fix(adapters,engine): pin the read-back to the spec the session owes (#261)#298
Conversation
…261) Closes #261. The generic dev/review read-back located "the artifact this session produced" by scanning the implementation-artifacts dir for the most-recently-modified qualifying *.md, with nothing tying the match to the story being driven. That dir is shared: under worktree isolation the search also covers the main checkout's copy, and with isolation="none" it IS that copy. A foreign story's spec landing there after launch (a concurrent run's merge-back, a human edit, a sweep) won on mtime and became this session's result — a review that produced nothing was scored completed:done and merged unreviewed code, and on the dev leg its followup_review_recommended: false skipped the review entirely. Unlike the rest of this family (#88/#127/#160/#224) it failed toward LANDING bad work. Part 1 — authoritative-path read-back. Where the orchestrator already knows which spec the session owes (every review leg and every dev retry, via StoryTask.spec_file — the path it hands the session in its own prompt), SessionSpec.expected_spec pins the read-back to that one file and the scan is never reached. Fix by subtraction: stories mode already resolved by id rather than mtime, so this closes an asymmetry. The #224 missing-marker fallback (a second mtime-only scan of the same shared dir, added after the issue was filed) goes through the same seam via its new `only` parameter. devcontract keeps its semantics; find_result_artifact / find_frontmatter_candidates are refactored onto per-path predicates (is_result_artifact / is_frontmatter_candidate) so a caller holding the owed path can test one file instead of a directory. Deliberately NOT a filename/story-key rule: spec names come from an LLM-derived slug, so a prefix check risks trading this unsafe failure for a lossy one — and a pinned path needs no exemption for the bmad-dev-auto-result-* fallback or for sweep bundles, which legitimately adopt a differently-named story spec (#161). Withheld from injected plugin-workflow sessions, which owe the completion marker rather than the story spec. A dev attempt 1 has no recorded spec and keeps the scan. Part 2 — proof-of-work gate. A read-back artifact may no longer upgrade a dead session to completed when it shows no evidence it ran at all: no hook event arrived AND its pane log never grew past PROOF_OF_WORK_MIN_LOG_BYTES. The two signals are ORed because each has a blind spot (a misbound pane sink still fires hooks (#254/#217); a hook-less profile still logs), and no signal at all leaves the gate inert (unknown never blocks). Applies to the crash path and to _post_kill_reconcile — the call path the issue's second occurrence took — and journals readback-refused-no-proof-of-work. The floor is not zero: the report's wedged windows left 0-byte AND 2-byte logs. test_tmux_timeout_with_flushed_spec_rescued_post_kill's fake CLI now renders to its pane before writing the spec. Measured, the silent fake produced a 0-byte log with zero hook events — byte-identical to the wedge this issue is about, so it was unfaithful to the #61 scenario it stands for (that run logged 1.4 MB). Its new companion, test_tmux_timeout_silent_session_not_rescued, pins the complementary refusal on the same call path.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe session adapter now uses an explicit expected spec path for read-back and restricts fallback candidates accordingly. Completion upgrades and post-kill rescues require observable hook or pane-log evidence. Artifact predicates and integration tests cover foreign specs, silent sessions, and relative paths. ChangesSession integrity
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…es resume CI py3.11/py3.12 failed test_tmux_timeout_with_flushed_spec_rescued_post_kill (3.13/3.14 and both Windows jobs passed). The fake CLI emitted its pane output as one burst at startup, which can finish before pipe_pane attaches to the freshly created window — leaving a 0-byte log and tripping the new proof-of-work gate. Spread the output across ~2s (40 lines x 50ms, still well inside the 6s session timeout) so some of it always lands after the sink exists. Stable over 5 local repeats. Its silent companion now asserts "at or below the floor" rather than exactly zero: below the floor is the invariant, zero is incidental. Also, two corrections to the expected_spec seam: - The SessionSpec.expected_spec comment claimed the field is process-transient like SpecSnapshot and that a crash-resume falls back to the scan. That is wrong in the direction that matters: StoryTask.spec_file round-trips through state.json and WorktreeFlow.reopen_unit re-absolutizes it before any session launches, so the engine re-derives expected_spec on every launch and a resumed run IS protected. Documented as the property it is. - Rebase a relative expected_spec against spec.cwd, the same idiom the stories read-back uses for BMAD_LOOP_SPEC_FOLDER. The engine always threads an absolute path, but a relative one would resolve against the process CWD, miss, and read as "the session wrote nothing" — turning this guard into exactly the silent work-losing failure it exists to prevent.
…to prove The threshold comment claimed "any session that reached the agent loop echoes at least its own prompt into the pane". That is an inference and it is wrong as stated: the prompt is delivered by send-keys, and a program that never echoes it leaves a 0-byte log (measured with the silent fake CLI). The floor measures the CLI's OWN rendering, not that the session was launched — which is precisely why _produced_work ORs it with the hook-event signal instead of trusting it alone. Restated around the observed separation (1.4 MB working vs 0/2 bytes wedged), and noted that the exact value is therefore not load-bearing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_generic_tmux.py`:
- Around line 3002-3006: Update the foreign spec setup in the concurrent
merge-back test so its modification time is explicitly newer than the launch
timestamp recorded by start_session, or create it through the fake CLI after
launch. Ensure find_result_artifact accepts it and the proof-of-work gate,
rather than the since_ns filter, rejects the foreign result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a815e72-e460-4c74-aa37-81a006405974
📒 Files selected for processing (7)
CHANGELOG.mdsrc/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pytests/test_engine.pytests/test_generic_tmux.py
…e death _log_evidence is declared inert on _ResultFileMixin and overridden on GenericAdapter, which owns the pane log. Both mixins sit ahead of the concrete adapter in the MRO, and this file already documents that shadowing hazard for send_text. C3 puts GenericAdapter first today (it subclasses _ResultFileMixin), but a future base reshuffle that let the inert stub win would silently disable the proof-of-work gate everywhere. Assert both resolutions, including that OpencodeDevAdapter — which has no pane log — keeps the inert one.
#298 review) CodeRabbit caught that test_tmux_timeout_silent_session_not_rescued proved nothing. The foreign spec was written BEFORE adapter.run(), which is where start_session stamps launched_ns — so its mtime sat below the since_ns floor, find_result_artifact discarded it outright, and the read-back never produced a candidate for the proof-of-work gate to refuse. Verified by ablation: the test passed with the gate entirely removed. That is the same vacuously-satisfiable-gate failure mode this whole issue is about, in the test written to prove the fix. The merge-back now lands mid-window from a separate thread (1s into a 6s window), so the foreign spec post-dates launch exactly as the report's did, plus an explicit assertion that it IS a qualifying candidate. Re-verified by ablation: with the gate disabled the test now FAILS with status=completed — the foreign spec adopted through post-kill reconcile, i.e. a genuine end-to-end reproduction of #261 through real tmux. Stable over 6 local repeats.
Comprehensive review findingsI reviewed the full PR diff against issue #261 and the PR’s stated behavior, including parallel adversarial, edge-case, and verification-gap passes. Recommendation: request changes before merge. The authoritative-path mechanism is directionally correct and works for the primary review/retry case, but the new proof-of-work and path-selection seams still contain three high-severity edge cases. Findings
Remediation plan
Validation performed
The main authoritative-path change successfully rejects foreign marker and markerless specs when the session genuinely owes a known path. The items above are about making that authority accurate for every launch route and ensuring the proof gate neither fails open on unsafe shared scans nor fails closed on authoritative task-local results. |
…298 review) The #261 read-back may demand a spec back solely because the session was told to write it. `_run_session` derived the pin from any recorded `task.spec_file`, but knowing a spec exists is not the same as having pointed a session at it: a from-scratch re-drive after an escalation or deferral carries a spec recorded by `_record_dev_spec` and dispatches a bare story key. Pinning there polls a stale path while the re-drive's real output goes unread — trading #261's unsafe failure for a work-losing one, the exact trade this fix exists to avoid. Same shape for a sweep bundle (dispatches intent.md) and stories mode (folder+id). Pin iff the dispatched prompt names the path. One rule, covering all three engines without per-engine plumbing since each already builds its own prompt; it reads the post-gate text so a plugin rewrite cannot desynchronize the pin from the contract that justifies it. A miss falls back to the scan — the pre-#261 behavior, and the safe direction. Also stop recording the skill's `bmad-dev-auto-result-*` no-spec fallback as a story's spec. It is not one, and every consumer misroutes on it: the escalation re-arm flips frontmatter on a marker no re-drive reads, the repair leg re-opens it as the frozen intent contract, and the read-back then pins to it. Ablation-verified: reverting the rule to the old `task.spec_file` presence check fails the routing tests; dropping the `label is None` guard fails the labeled-workflow test on its own; removing the marker filter fails its test.
review) Three defects, all the same shape: the #261 gate was inert exactly where it was needed and over-broad where it was not. 1. Launch metadata was read as proof of work. `session_id`/`transcript_path` are populated by SessionStart and SessionEnd too, both of which a CLI that launched and wedged emits — so only a session with literally ZERO hook events ever reached the gate. Track a `Stop` event (a turn that ENDED) as its own signal instead; `SessionResult.stop_seen` carries it to `_post_kill_reconcile`. The #254/#217 rationale is untouched: a healthy session ends its turn. 2. A missing pane log failed the gate OPEN. `pipe_pane` tolerates a window that already died and then attaches no tee, so a dead-on-arrival session left no log at all — the "this transport has no pane signal" state, which the gate treats as inert. `start_session` now creates the log empty before the window exists, so no-tee-ever-attached reports False (rendered nothing) rather than None. The inert state now means only "a handle this adapter never launched". 3. The gate applied to authoritative task-scoped results. It lived in `_ResultFileMixin._final`, shared by every adapter whose skill writes `tasks/<task_id>/result.json` — a file unique to the task and unlinked at launch, which no foreign writer can reach. Gating it could only ever discard a real completion. Scoped to `_DevSynthesisMixin`, the one read-back that scans a shared directory. The silent-session e2e is also de-raced: its merge-back thread waited a fixed 1s from before `run()`, but the gap to `launched_ns` covers a real `tmux new-session` and a loaded box can outrun any margin picked in advance, silently restoring the vacuous pass. It now waits on the actual launch and asserts qualification against the floor the session recorded, not a permissive 0. Ablation-verified, each mechanism separately: disabling `_produced_work` fails all 7 refusal tests; restoring the any-hook-metadata rule fails the SessionStart-only test; removing the log pre-create fails the dead-on-arrival test; gating the base mixin fails the task-scoped-result test. One new test was itself vacuous on first write (the foreign spec predated launch, so `since_ns` rejected it before the gate saw it) and was caught by exactly this pass.
Round-2 disposition — all six confirmed, all six fixedValidated every finding against HEAD before changing anything. All six reproduce, and #2 is worse than reported. Fixed in a84bc44 (engine) + 91b6b68 (adapters). The through-line is worth naming: findings 2–4 all say the same thing — the proof-of-work gate was inert exactly where it was needed and over-broad where it was not — and finding 1 says the pin introduced a new lossy path. Both are the failure modes this PR exists to eliminate, reproduced inside the fix. 1 —
|
| Ablation | Fails |
|---|---|
_produced_work → True |
all 7 refusal tests, incl. the real-tmux e2e |
| restore any-hook-metadata rule | SessionStart-only test |
| remove the log pre-create | dead-on-arrival + reused-task-log tests |
| gate the base mixin | task-scoped-result + MRO tests |
| revert the pin rule | pin-routing + sweep-bundle tests |
drop the label is None guard |
labeled-workflow test alone |
| remove the marker filter | its test |
Worth recording: one of the new tests was itself vacuous on first write — the foreign spec predated launch, so since_ns discarded it before the gate saw it, and it passed with the gate removed. The same defect CodeRabbit caught in the e2e, in a test written to prove the fix for it. This pass is what caught it.
Full suite 2986 passed, 1 skipped. trunk check (no filter) clean, pyright 0 errors, both real-tmux tests stable over 6 consecutive local runs.
Standing residual
Unchanged and now the only one: dev attempt 1, where no spec exists yet and the scan is genuinely required. Round 2 closed the two holes that had left Part 2 largely inert there, so the gate is now real cover for it — but a session that did work, died, and was outraced on mtime remains theoretically exposed. Still a follow-up rather than a widening of this change.
…ng (#298 review) CI Windows/py3.14 only. The dead-on-arrival test asserts its foreign spec is a candidate the read-back would otherwise adopt — a precondition assertion, so that setup drift fails loudly instead of passing vacuously. It established that by writing the file after `start_session` returned, which is not sufficient on Windows: `launched_ns` comes from `time.time_ns()` (a precise clock) while an NTFS mtime is stamped from the coarse system-time tick (~15.6 ms), so a file written a millisecond later can carry an mtime BELOW the floor. Re-stamp until the precondition actually holds, bounded, instead of assuming the two clocks are comparable. Production is unaffected: no session writes its terminal spec within 15 ms of launch. Re-ablated after the change — with `_produced_work` short-circuited the test still fails, so the fix did not turn it into a test that agrees for free.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
1 participant
Closes #261.
The defect
The generic dev/review read-back located "the artifact this session produced" by scanning the implementation-artifacts dir for the most-recently-modified qualifying
*.md, with nothing tying the match to the story being driven. That dir is shared: under worktree isolation the search also covers the main checkout's copy, and withisolation="none"it is that copy.So a foreign story's spec landing there after launch — a concurrent run's merge-back, a human edit, a sweep — won on mtime and became this session's result. A review that produced nothing was scored
completed:doneand merged unreviewed code; on the dev leg itsfollowup_review_recommended: falseskipped the review entirely. Unlike the rest of this family (#88/#127/#160/#224, all closed) it fails toward landing bad work.Validation at HEAD
Reproduced against current code before changing anything:
find_result_artifactreturns the foreignspec-i-13-*.md, andsynthesize_result(..., story_key="i-12-…")returns{story_key: "i-12-…", spec_file: ".../spec-i-13-….md", status: "done", followup_review_recommended: False}withstatus_consistent=True. The identity mismatch sits inside the returned dict and nothing reads it.## Auto Run Resultre-triggers the #149 livelock, then DEFER-drops a finished story #224 fix (71e18ce) landed after the issue was filed and addedfind_frontmatter_candidates— a secondglob("*.md")over the same shared dir with the same mtime-only floor. Same hole, reproduced.Two findings shaped the fix:
verify_reviewreadstask.spec_file— the correct spec — which is legitimatelystatus: donealready, because_reset_spec_for_reviewdeliberately never mutates frontmatter (it is load-bearing skill routing). Sprint-status isdoneand verify commands pass on the dev's code. The gate is vacuously satisfied; the forged session result is the only signal.task.spec_fileis recorded byverify_dev/verify_dev_bundleon 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.Part 1 — authoritative-path read-back
SessionSpec.expected_specpins the read-back to the one spec the session owes, and the scan is never reached. The pin is taken iff the dispatched prompt names the path — the read-back may demand a file back only because the session was told to write it. Knowing a spec exists is not the same thing: a from-scratch re-drive after an escalation/deferral carries atask.spec_filerecorded by_record_dev_specbut dispatches a bare story key, and pinning there would poll a stale path while the re-drive's real output went unread (see review round 2 below). Fix by subtraction — stories mode already resolved by id rather than mtime, so this closes an existing asymmetry rather than adding a concept. The #224 fallback goes through the same seam via a newonlyparameter.devcontractkeeps its semantics; the two scans are refactored onto per-path predicates (is_result_artifact/is_frontmatter_candidate) so a caller holding the owed path can test one file instead of a directory. Every test pinning the current scan behaviour stays green untouched, including the three the issue names.Deliberately not a filename/story-key prefix rule. Spec names come from an LLM-derived slug (
step-01-clarify-and-route.md:79), so a prefix check risks trading this unsafe failure for a lossy one — the exact trade this repo has repeatedly paid to avoid. A pinned path also needs no exemption for either caveat the issue flags: thebmad-dev-auto-result-*fallback carries no story key by design, and sweep bundles legitimately adopt a differently-named pre-existing story spec (#161) — a filename rule would have broken that.Withheld from injected plugin-workflow sessions (a TEA
pre_commit_gateruns the generic adapter but owes the completion marker, not the story spec) — the same doctrine asStoriesEnginewithholdingBMAD_LOOP_SPEC_FOLDERfrom labeled sessions. A dev attempt 1 has no recorded spec yet and keeps the scan.Part 2 — proof-of-work gate
A read-back artifact may no longer upgrade a dead session to
completedwhen it shows no evidence it ran at all: no turn ever ended and its pane log never grew pastPROOF_OF_WORK_MIN_LOG_BYTES.The hook signal is a
Stopevent specifically —SessionStartandSessionEndpopulate the samesession_id/transcript_pathand are both emitted by a CLI that launched and wedged. The pane log is created empty at launch, so a window that dies beforepipe_paneattaches reports "rendered nothing" rather than "no pane signal here". And the gate is scoped to the shared-directory read-back: a task-scopedresult.jsonis unique to its session and unlinked at launch, so no foreign writer can reach it and it stays authoritative. (All three landed in review round 2 — see below.)The two signals are ORed because each has a known blind spot — a misbound pane sink still fires hooks (#254/#217, where a healthy session logs zero bytes), and a hook-less profile still logs. Requiring both to be absent is what makes it safe to apply to a
completedupgrade. No signal at all (no pane log — the opencode-http transport, and every unit fixture) leaves the gate inert: unknown never blocks, matching the tristate liveness doctrine.Applies to the crash path and to
_post_kill_reconcile(the call path the issue's second occurrence took), and journalsreadback-refused-no-proof-of-work. The floor is not zero: the report's wedged windows left 0-byte and 2-byte logs.One existing test changed, deliberately
test_tmux_timeout_with_flushed_spec_rescued_post_kill's fake CLI now renders to its pane before writing the spec. I measured the original: it produced a 0-byte pane log with zero hook events — byte-identical to the wedge this issue is about. It was an unfaithful stand-in for the #61 scenario it documents (total hook loss), since a CLI that genuinely implemented a story always renders; the run it represents logged 1.4 MB. The test's actual invariant (rescue a finished session whose Stop was lost) is untouched, and its new companiontest_tmux_timeout_silent_session_not_rescuedpins the complementary refusal on the same call path.That companion was itself defective on first submission, and CodeRabbit caught it: the foreign spec was written before
adapter.run()— wherestart_sessionstampslaunched_ns— so its mtime sat below thesince_nsfloor, the scan discarded it, and nothing ever reached the gate. It passed with the gate entirely removed — the same vacuously-satisfiable-gate defect this PR fixes, inside the test written to prove the fix. The merge-back now lands mid-window from a separate thread, plus an explicit assertion that the foreign spec really is a qualifying candidate.Verification
_produced_workshort-circuited toTrue,test_tmux_timeout_silent_session_not_rescuedfails withstatus=completed(the foreign spec adopted through post-kill reconcile — a genuine end-to-end reproduction of Session read-back adopts another story's spec: a review that produced nothing is scored done #261 through real tmux); restored, it passes. Both real-tmux tests stable over 6 consecutive local runs.trunk check(no filter): clean.pyright@1.1.411: 0 errors.devcontractprimitives behave identically (the fix is not there); the adapter unpinned still reproduces the bug, pinned returnsNone.Review round 2 (post-
e880768)A full review pass raised six findings; all six reproduced at HEAD, and one was worse than reported. Fixed in
a84bc44(engine) +91b6b68(adapters):expected_specpinned a re-drive to a spec it was never told to write — a work-losing regression this PR introducedbmad-dev-auto-result-*marker as a story's specSessionStartmetadata counted as proof of work — andSessionEnddid too, so only a session with zero hook events ever reached the gateStopevent as its own signal (SessionResult.stop_seen)start_sessioncreates the log empty before the window existsresult.json_DevSynthesisMixinvia_READBACK_NEEDS_PROOF_OF_WORKlaunched_nsSessionStart-only, dead-on-arrival, task-scoped resultAblation-verified per mechanism, each restored afterwards: disabling
_produced_workfails all 7 refusal tests; restoring the any-hook-metadata rule fails theSessionStart-only test; removing the log pre-create fails the dead-on-arrival test; gating the base mixin fails the task-scoped-result test; reverting the pin rule fails the routing tests; dropping thelabel is Noneguard fails the labeled-workflow test alone; removing the marker filter fails its test. One new test was itself vacuous on first write — the foreign spec predated launch, sosince_nsrejected it before the gate saw it — and this pass is what caught it.Full suite 2986 passed, 1 skipped.
trunk check(no filter) clean;pyright0 errors. Both real-tmux tests stable over 6 consecutive runs.Not addressed
_artifact_dirsleft alone. The issue calls narrowing it the smallest option and notes no test pins it — but the rendered skill bakes an absolute main-checkout spec path, which is plausibly why the "defensive fallback" exists. Dropping it could re-introduce a lossy failure. Parts 1 and 2 make it unnecessary.SessionStart-only and absent-log holes that had left Part 2 largely inert. It is partly covered by_verify_shared_gates' baseline check (vacuous between parallel worktrees cut from the same baseline) and by Part 2 whenever the session produced nothing. A session that did work, died, and was outraced on mtime remains theoretically exposed — worth a follow-up rather than widening this change.Summary by CodeRabbit