feat(adapters,engine,devcontract): deterministic missing-marker catch + repair#277
feat(adapters,engine,devcontract): deterministic missing-marker catch + repair#277pbean wants to merge 5 commits into
Conversation
Second of four harness-side mechanisms hardening the #224 missing-marker fallback. A new `_observe_tick(handle, spec)` no-op hook on `CodingCLIAdapter` is called each heartbeat tick from both dev wait loops (generic + opencode). `_DevSynthesisMixin` implements it: sample the snapshotted spec's frontmatter and record the FIRST status this session drives off its launch state to a live, non-terminal value (in practice `in-review`) into `_fm_transition_obs`, with a `spec-status-transition-observed` lifecycle crumb. A recorded transition is deterministic proof the terminal frontmatter the spec later carries is this session's own write, so `_frontmatter_fallback` widens its synthesis predicate to `dead_window or transition_proven or (stable and obs >= FM_FALLBACK_MIN_OBS)` — a single terminal sighting harvests, live or dead, and the synthesized crumb gains a `transition` flag. The M1 hash gate still outranks it: bytes reverted exactly to the launch snapshot refuse even with a transition recorded. A transition that flips between two ticks is missed and falls back to the conservative 2-observation fingerprint. Sampling-only: no snapshot or a torn read is a silently skipped sample, and terminal/blank/unchanged statuses record nothing.
…276 M4) Final mechanism of the deterministic missing-marker catch+repair (#276): a targeted, once-per-session tmux nudge asking the bmad-dev-auto skill to append the '## Auto Run Result' section it owed when a Stop finds a marker-less terminal-frontmatter spec, so the omission is repaired at its source (a compliant append is harvested by the ordinary marker scan; frontmatter synthesis stays the backstop). - policy: LimitsPolicy.dev_contract_nudge (bool, default true) + parse + template + core.toml [limits] switch entry (default_ref, sync tests pass bidirectionally). - adapters/generic.py: CONTRACT_NUDGE_TEXT; _configure_dev_knobs gains the never-cleared _contract_nudge_sent set + _contract_nudge_enabled; the _frontmatter_fallback record-obs branch fires the nudge on the first pending observation (marked BEFORE the send so a raising transport still counts; MultiplexerError swallowed; touches no stall counters, so #149's refill hazard cannot apply). _DevSynthesisMixin declares send_text as a bare annotation. - docs (FEATURES.md, tui-guide.md) + consolidated CHANGELOG #276 entry finalized with all four mechanisms, the knob, and the never-mutate-launch-frontmatter hard constraint. - tests: 9 adapter nudge tests (sent/exactly-once/transition/ambiguous/dead- window/unmodified/send-failure/disabled/e2e-append) + policy knob test.
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe review adds launch-time spec snapshots, heartbeat frontmatter observation, deterministic fallback refusal, optional contract nudging, and guarded engine-side repair of missing ChangesMissing-marker recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/bmad_loop/data/settings/core.toml`:
- Line 144: Remove the empty duplicate [[section.field]] declaration immediately
before max_tokens_per_story in the settings configuration, leaving only
field-table entries that define their key and widget metadata.
In `@src/bmad_loop/engine.py`:
- Around line 1947-1956: Update the exception handler around
devcontract.append_auto_run_result in the spec-marker repair flow to catch both
OSError and UnicodeDecodeError. Preserve the existing spec-marker-repair-failed
journal entry and return behavior so undecodable specs are handled like other
append failures and the run can complete cleanly.
🪄 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: c8a501cd-575b-42b4-8cf0-f2e09c737bf7
📒 Files selected for processing (15)
CHANGELOG.mddocs/FEATURES.mddocs/tui-guide.mdsrc/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/opencode_http.pysrc/bmad_loop/data/settings/core.tomlsrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/policy.pytests/test_devcontract.pytests/test_engine.pytests/test_generic_tmux.pytests/test_opencode_http.pytests/test_policy.py
…elper DRY Addresses CodeRabbit review on #277: - engine._repair_spec_marker widened its best-effort guard from OSError to (OSError, UnicodeDecodeError): append_auto_run_result reads the spec's raw bytes and, by contract, raises UnicodeDecodeError (a ValueError, not an OSError) on an undecodable spec — a torn-mid-write between the frontmatter read and the append. Unguarded it would crash a run whose result was already synthesized and recorded. Matches _post_kill_reconcile's torn-read doctrine. New engine test covers the UnicodeDecodeError path. - test-helper cleanup: fold _transition_crumbs/_nudge_crumbs into one parameterized _lifecycle_events(adapter, event) so both read uniformly (and black stops collapsing only the shorter of the two). The other CodeRabbit finding (core.toml line 144 'empty duplicate section.field') is a false positive: line 144's [[section.field]] is the header for max_tokens_per_story (key on the next line); the settings-schema sync tests confirm every field has a key.
Comprehensive review findingsReviewed Blocking findings
Verification gap
Suggested resolution plan
Verification performed
Recommendation: hold merge until findings 1-3 are addressed and the review-path coverage is added. |
Review addendum: lower-priority findingsThe main review comment contains all blocking findings and the medium verification gap. For completeness, these additional hardening findings were raised during the independent review layers and should remain visible even though they are not merge blockers:
These do not change the prior recommendation: findings 1-3 remain the merge blockers, finding 4 is the required verification gap, and findings 5-7 are lower-priority hardening work. |
1 participant
Hardens the #224/#271 missing-marker fallback (PR #273), whose attribution was heuristic — a 2-stable-Stops fingerprint, or a single sighting post-kill — and which left the offending spec non-compliant on disk. PR #273 shipped with three documented accepted risks; this PR retires all three by making catch and fix deterministic, across four harness-side mechanisms delivered in four single-session phases on one branch.
The problem
The
bmad-dev-autoreview HALT intermittently finalizes a spec's frontmatter to a terminalstatus:(done/blocked) without appending the## Auto Run Resultsection our harvest scan keys on. A finished story then readsno-artifacton every Stop, the stall nudges re-invoke an already-exited workflow (#149), and aftermax_review_cyclesthe verify-passing work is rolled back (#224). PR #273's synthesis-from-frontmatter rescued this, but its attribution was a heuristic and it never repaired the on-disk spec.Mechanisms
Launch-state snapshot + content-hash gate (M1). Right after the [BUG] The #109 stall-grace fix cannot reach the review leg: a review pass is killed on its first Stop by the dev pass's stale
## Auto Run Result#160 marker strip, the engine captures aSpecSnapshot(content sha256, mtime, frontmatter status) and threads it onto the reviewSessionSpec. The fallback refuses to synthesize from a candidate whose bytes still hash-equal the snapshot — provably untouched by this session (adonespec re-opened for review, never re-written) — in every mode, including the dead-window post-kill reconcile. This kills the documented dead-window false positive (a review killed after an mtime-only bump but before thein-reviewflip, previously scoreddonewithout having run). New verdictunmodified-since-launch; dead-window crumbfrontmatter-unmodified-refused. Snapshot is process-transient (crash-resume degrades to the conservative path).Mid-session status-transition observation (M2). On each heartbeat tick the generic (and OpenCode) dev adapter samples the snapshotted spec and records the first status it observes this session drive off its launch state to a live, non-terminal value (in practice
in-review), crumbspec-status-transition-observed. A recorded transition is deterministic proof the terminal frontmatter the spec later carries is this session's own write, so the fallback synthesizes on one terminal sighting instead of the 2-observation fingerprint (synth crumb gainstransition=). The M1 hash gate still outranks it.Artifact repair (M3). On synthesis the engine appends the owed
## Auto Run Resultmarker onto the on-disk spec (newdevcontract.append_auto_run_result, the inverse of the [BUG] The #109 stall-grace fix cannot reach the review leg: a review pass is killed on its first Stop by the dev pass's stale## Auto Run Result#160 strip), so the once-invisible spec re-enters the normal marker scan and the next review launch strips it exactly like a skill-written marker. Best-effort at thesession-synthesized-from-frontmattersite (covers live-Stop, crash-path, and post-kill synthesis); refused for a spec outside the orchestrator-owned roots or whose fresh frontmatter no longer agrees with the synthesized status, so it can never author a marker that disagrees (spec-marker-repaired/-repair-failed/-repair-skipped).Targeted contract nudge (M4), gated by new knob
limits.dev_contract_nudge(bool, defaulttrue). On the firstterminal-frontmatter-pendingStop — one candidate, not the hash-gate refusal, transition not yet proven — the adapter sends one nudge asking the skill to append the section itself and end its turn, repairing the omission at its source; a compliant append is harvested by the ordinary scan (no synthesis flag). Fires exactly once per session — marked before the send (a raising transport still counts), never refilled, touching no stall counters — so an mtime bump that resets the observation counter can never re-nudge (Claude adapter: Stop hook + idle detection re-entrant loop prevents session from ending #149's refill hazard structurally cannot apply).contract-nudge-sentcrumb;MultiplexerErrorswallowed.Decision ladder (
_frontmatter_fallback, final shape)scan → 0 candidates:
no-artifact→ >1:ambiguous-frontmatter→ read candidate (mtime, fm status, sha256 when the snapshot path matches; OSError → torn-read degrade) → hash gate (M1, all modes) → evidence:transition_proven(M2) + fingerprint/stable/observations → synthesize whendead_window or transition_proven or (stable and obs >= 2)→ else record-obs branch: fingerprint +terminal-frontmatter-pending+ contract nudge (M4).HARD CONSTRAINT
The spec's
status:at review launch is load-bearing routing input to the upstream skill (adonespec routes to a fresh step-04 review pass). The review-launch frontmatter is therefore never mutated — all four mechanisms are observation or a prose-append, never a status write.Upstream
The omission originates upstream:
bmad-dev-autointermittently skips the## Auto Run Resultappend at the review HALT while finalizing frontmatter to a terminal status. Filed as bmad-code-org/BMAD-METHOD#2616 with the resultless-Stop evidence pattern and a request to make the HALT append unconditional-and-verified. This PR is the downstream harness-side mitigation regardless of the upstream fix.Tests
Full
uv run pytest -qgreen — 2887 passed, the only 2 failures the pre-existingtest_module_skills_sync.pybmad-loop-setupmodule.yaml drift (unrelated). Fulltrunk check --no-fixclean across all 15 modified files. New coverage: adapter hash-gate refusal (live/dead-window/wait-false/other-path), transition single-sighting harvest + missed-transition-stays-conservative + hash-gate-beats-transition, heartbeat-drives-observe-tick,append_auto_run_resultroundtrip + guards, engine snapshot threading +_repair_spec_marker(repaired/failed/skipped/fm-mismatch), 9 contract-nudge cases (sent-once / exactly-once-across-mtime-reset / not-sent on transition/ambiguous/dead-window/unmodified / send-failure-marks-sent / disabled-by-policy / e2e nudge-then-skill-appends-marker), and the policy/settings-schema knob sync.Closes #276.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation