From 9f05e07811daee948b029a045aee2ac5d7b0a34c Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 11:31:37 -0700 Subject: [PATCH 1/2] fix(verify): subtract baseline untracked snapshot + read the skill's baseline key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent verify-gate bugs found by the PR #86 final review, both living in the same shared gate block. #88: has_changes_since counted every untracked file, so an intent-gap halt's saved patch — untracked residue the resets deliberately protect — let a from-scratch re-drive that produced nothing but a spec status flip pass the proof-of-work gate on the file's mere presence. The gate now passes task.baseline_untracked and subtracts it. A None snapshot still counts every untracked file: the opposite of attempt_dirty's ignore-all, because a proof-of-work gate fails open toward "work happened" while a rollback gate fails open toward "nothing to remove". Docstring pins the asymmetry. #89: the baseline-match gate read the spec's baseline_commit and skipped itself when the key was absent, but bmad-dev-auto stamps baseline_revision — so the check never fired in production and a spec claiming a foreign baseline sailed through. It now reads either key, the idiom devcontract already used. conftest's write_spec stamps baseline_revision like the real skill, which makes the reader load-bearing across all its call sites: the two pre-existing lying-baseline tests fail without the fix, where before they passed vacuously. Closes #88, Closes #89 --- CHANGELOG.md | 16 ++++++++ src/bmad_loop/verify.py | 41 +++++++++++++++++-- tests/conftest.py | 12 +++++- tests/test_verify.py | 87 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36c25ad3..ea8e4c50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,22 @@ breaking changes may land in a minor release. ### Fixed +- **Baseline-era untracked residue no longer vacuously satisfies the proof-of-work gate.** + `has_changes_since` counted every untracked file. After an intent-gap halt the saved patch is + untracked residue under the artifact dirs every reset deliberately protects, so a from-scratch + re-arm — which never learns the patch's path — let a re-driven session that produced nothing but a + spec status flip pass the gate on that file's mere presence, and `finalize_commit`'s `add -A` swept + it into the story commit. The gate now subtracts the task's `baseline_untracked` snapshot. A `None` + snapshot (a pre-upgrade run) still counts every untracked file — deliberately the opposite of + `attempt_dirty`'s ignore-all, because a proof-of-work gate has to fail open toward "work happened". + (closes #88) +- **The baseline-match verify gate was dead code for generic dev sessions.** The gate read the spec's + `baseline_commit` and skipped itself when that key was absent — but `bmad-dev-auto` stamps + `baseline_revision`; `baseline_commit` exists only in the orchestrator's synthesized `result.json`. + In production the check never fired, so a spec claiming a stale or foreign baseline sailed through. + The gate now reads either key, the idiom `devcontract` already used. The test fixture stamps + `baseline_revision` like the real skill does, so it can no longer fabricate the key that hid this. + (closes #89) - **Unit keys with git-ref-illegal characters no longer break worktree runs.** `unit_branch_name` built `bmad-loop//` from the raw ids, so a key or `--run-id` carrying `:`, `..`, `@{`, a space or a trailing `.lock` cleared the (already-sanitized) worktree dir only to die at diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 2346f629..ba25ac47 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -138,7 +138,13 @@ def same_commit(a: str, b: str) -> bool: return a.startswith(b) or b.startswith(a) -def has_changes_since(repo: Path, baseline: str, exclude: tuple[str, ...] = ()) -> bool: +def has_changes_since( + repo: Path, + baseline: str, + exclude: tuple[str, ...] = (), + *, + baseline_untracked: list[str] | None = None, +) -> bool: """True if tracked changes since baseline OR untracked files exist. `exclude` is repo-relative posix dir prefixes whose changes don't count — @@ -146,11 +152,27 @@ def has_changes_since(repo: Path, baseline: str, exclude: tuple[str, ...] = ()) BMAD artifact folders (see `artifact_relpaths`), so a session that only rewrites its own spec (e.g. the frontmatter-status reconcile) under those folders doesn't register as real implementation work. Mirrors - `attempt_dirty`'s exclusion. Default `()` keeps the unscoped behavior.""" + `attempt_dirty`'s exclusion. Default `()` keeps the unscoped behavior. + + `baseline_untracked` is the untracked-file snapshot taken when the baseline + was recorded; when given, those files already existed before the session ran + and are subtracted, so pre-session residue (e.g. an earlier halt's saved + intent-gap patch, which `_protected_relpaths` shields from every reset) can + never masquerade as this session's work. + + `None` means count EVERY untracked file — deliberately the *opposite* of + `attempt_dirty`'s `None` = ignore-all, and not an oversight. The two gates + fail open in opposite directions: a proof-of-work gate must fail open toward + "work happened" (a pre-snapshot run must not have its gate silently + weakened into never seeing new files), while a rollback gate must fail open + toward "nothing to remove" (never delete a file it cannot prove this attempt + created). Keep it that way.""" rc, _ = _git(repo, "diff", "--quiet", baseline, "--", ".", *_exclude_specs(exclude)) if rc != 0: return True created = untracked_files(repo) + if baseline_untracked is not None: + created -= set(baseline_untracked) created = {p for p in created if not _path_under_any(p, exclude)} return bool(created) @@ -1091,7 +1113,13 @@ def _verify_shared_gates( f"spec status is {status!r}, expected {expected_status!r}: {spec_path}" ) - claimed_baseline = str(fm.get("baseline_commit", "")).strip() + # The generic bmad-dev-auto skill stamps `baseline_revision`, never + # `baseline_commit` — that name exists only in the result.json devcontract + # synthesizes, which this gate does not consult (it re-reads frontmatter). + # An absent key skips the check below, so reading `baseline_commit` alone + # made this gate dead code for every generic-skill session. Read both, the + # same idiom as `devcontract.synthesize_result`. + claimed_baseline = str(fm.get("baseline_commit", fm.get("baseline_revision", ""))).strip() if task.baseline_commit and claimed_baseline not in ("", "NO_VCS"): if not same_commit(claimed_baseline, task.baseline_commit): return VerifyOutcome.retry( @@ -1101,7 +1129,12 @@ def _verify_shared_gates( if proof_exclude is not None and task.baseline_commit: try: - if not has_changes_since(paths.project, task.baseline_commit, exclude=proof_exclude): + if not has_changes_since( + paths.project, + task.baseline_commit, + exclude=proof_exclude, + baseline_untracked=task.baseline_untracked, + ): return VerifyOutcome.retry("no changes in worktree since baseline commit") except GitError as e: return VerifyOutcome.escalate(str(e)) diff --git a/tests/conftest.py b/tests/conftest.py index ac20eb4b..bd3efc71 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -183,9 +183,14 @@ def set_sprint(paths: ProjectPaths, key: str, status: str) -> None: def write_spec(path: Path, status: str, baseline: str, *, prose_status: str | None = None) -> None: + """Write a spec the way the real bmad-dev-auto skill does. The skill's step-03 + stamps `baseline_revision` and NEVER `baseline_commit` (that name exists only + in the orchestrator's synthesized result.json), so this fixture stamps the + same key — a reader that only knows `baseline_commit` must fail a test here, + not sail through production (issue #89).""" body = ( f"---\ntitle: 'test'\ntype: 'feature'\nstatus: '{status}'\n" - f"baseline_commit: '{baseline}'\n---\n\n## Intent\n\ntest spec\n" + f"baseline_revision: '{baseline}'\n---\n\n## Intent\n\ntest spec\n" ) if prose_status is not None: # mirror bmad-dev-auto's terminal finalize: it appends a `## Auto Run @@ -291,8 +296,11 @@ def effect(spec: SessionSpec) -> SessionResult: def _spec_baseline(path: Path) -> str: + """Read back whichever baseline key a spec carries: `write_spec` stamps + `baseline_revision` like the real skill, but hand-rolled fixture specs (and + re-arm's re-stamp) may carry either.""" for line in path.read_text().splitlines(): - if line.startswith("baseline_commit:"): + if line.startswith(("baseline_commit:", "baseline_revision:")): return line.split(":", 1)[1].strip().strip("'\"") return "" diff --git a/tests/test_verify.py b/tests/test_verify.py index 00c8a0c7..6afeeb16 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1092,6 +1092,36 @@ def test_has_changes_since_excludes_artifact_only_edit(project): ) +def test_has_changes_since_subtracts_baseline_untracked(project): + """Untracked files already on disk when the baseline snapshot was taken are + not this session's work. `None` deliberately keeps counting all of them — + the opposite of `attempt_dirty`'s ignore-all — so a pre-snapshot run's + proof-of-work gate is never silently weakened.""" + baseline = verify.rev_parse_head(project.project) + (project.project / "residue.txt").write_text("left by an earlier halt\n") + + assert verify.has_changes_since(project.project, baseline) is True + assert verify.has_changes_since(project.project, baseline, baseline_untracked=None) is True + assert ( + verify.has_changes_since(project.project, baseline, baseline_untracked=["residue.txt"]) + is False + ) + + # a file created after the snapshot is this session's + (project.project / "fresh.txt").write_text("this session\n") + assert ( + verify.has_changes_since(project.project, baseline, baseline_untracked=["residue.txt"]) + is True + ) + # and a tracked edit counts regardless of how complete the snapshot is + (project.project / "fresh.txt").unlink() + (project.project / "src.txt").write_text("real\n") + assert ( + verify.has_changes_since(project.project, baseline, baseline_untracked=["residue.txt"]) + is True + ) + + def test_verify_dev_exclude_relpaths_is_file_granular(project): """Unlike artifact_relpaths (whole-folder), this excludes only the sprint-status ledger and the session's own claimed spec file — sibling @@ -1145,6 +1175,63 @@ def test_verify_dev_latched_restore_patch_is_not_proof_of_work(project): assert not out.ok and "no changes" in out.reason +def test_verify_dev_baseline_era_untracked_is_not_proof_of_work(project): + """#88: the from-scratch case the T4 latch exclusion cannot reach. After an + intent-gap halt the saved patch is untracked residue under the protected + artifact dirs, and a from-scratch re-arm (`restore_patch=None`) never learns + its path — but the re-arm's snapshot captured it. Subtracting + `baseline_untracked` is the only mechanical close: a re-driven session that + produced nothing but a status flip must not pass the gate on that residue.""" + write_sprint(project, {"1-1-a": "review"}) + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-review", "NO_VCS") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "baseline") + task = make_task(project) + residue = project.implementation_artifacts / "attempt.patch" + residue.write_text("stale intent-gap diff\n", encoding="utf-8") + rel = residue.relative_to(project.project).as_posix() + + # no latch (from-scratch re-arm) — only the snapshot can rule the residue out + assert task.restore_patch is None + task.baseline_untracked = [rel] + out = verify.verify_dev(task, project, dev_result(sp)) + assert not out.ok and "no changes" in out.reason + + # None (a pre-snapshot run) still counts every untracked file: the gate fails + # open toward "work happened", never toward a silently disabled gate + task.baseline_untracked = None + assert verify.verify_dev(task, project, dev_result(sp)).ok + + # real work passes with the snapshot in place + task.baseline_untracked = [rel] + (project.project / "src.txt").write_text("real work\n") + assert verify.verify_dev(task, project, dev_result(sp)).ok + + +def test_verify_dev_baseline_gate_reads_the_skills_baseline_revision_key(project): + """#89: the generic bmad-dev-auto skill's step-03 stamps `baseline_revision`; + `baseline_commit` exists only in the orchestrator's synthesized result.json. + Reading just the latter made the baseline-match gate dead code in production + (an absent key skips the check), so a spec claiming a foreign baseline sailed + through. Masked for years by fixtures that stamped the key the skill never + writes.""" + write_sprint(project, {"1-1-a": "review"}) + task = make_task(project) + sp = spec_path(project, "1-1-a") + + write_spec(sp, "in-review", "0" * 40) # a foreign baseline, skill-style key + body = sp.read_text() + assert "baseline_revision:" in body and "baseline_commit:" not in body + out = verify.verify_dev(task, project, dev_result(sp)) + assert not out.ok and "does not match" in out.reason + + # matching baseline + real work → the gate passes + write_spec(sp, "in-review", task.baseline_commit) + (project.project / "src.txt").write_text("real work\n") + assert verify.verify_dev(task, project, dev_result(sp)).ok + + def test_verify_dev_exclude_relpaths_normalizes_dotdot_segments(project): """A spec_path with a lexical '..' hop (as an un-normalized session-reported spec_file could produce) must resolve to the same exclude entry as the plain From 86972e96d4d053a7ede7c42e56be151afbd8889d Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 11:47:18 -0700 Subject: [PATCH 2/2] fix(verify): key-agnostic baseline-mismatch message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The value can come from either `baseline_commit` or the skill's `baseline_revision` — naming the former in the retry message is misleading for exactly the generic-skill sessions #89 un-deadened. (CodeRabbit review nitpick on #105) --- src/bmad_loop/verify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index ba25ac47..ecd26059 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -1123,7 +1123,7 @@ def _verify_shared_gates( if task.baseline_commit and claimed_baseline not in ("", "NO_VCS"): if not same_commit(claimed_baseline, task.baseline_commit): return VerifyOutcome.retry( - f"spec baseline_commit {claimed_baseline[:12]} does not match " + f"spec baseline {claimed_baseline[:12]} does not match " f"orchestrator-recorded baseline {task.baseline_commit[:12]}" )