diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e61fd6..ccb9b3bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,19 @@ breaking changes may land in a minor release. ### Added +- **A park travels with its story's commit, so `bmad-loop confirm` works from any clone (#356).** + Each parked story now writes one committed JSON record to `.bmad-loop/operator/.json`, inside + the story's own commit window, so the record rides the park's commit — through the worktree + merge-back, under every merge strategy — and reaches every clone the commit does. A teammate, + fresh clone, or CI can `confirm --list` and `confirm ` a story parked on another machine. + The machine-local index (`.bmad-loop/operator-actions.json`) is retired: still read and pruned so + an in-flight park from an older version stays confirmable where it was written, never written + again. The record carries no commit sha (it cannot name the commit it rides); provenance is + derived from the record's own git history, and an uncommitted record renders without one. A + failed record write never blocks the story — it parks recordless, journaled and reported by + `validate` — and a failed commit restores the record as found. Confirming commits the record's + deletion together with the spec+board flip, and a legacy-only park still gets that commit. + - **`bmad-loop confirm` completes a parked story (#335, part 3 of 4).** Once you have carried out the external actions a story owed, `bmad-loop confirm ` walks you through them one at a time (`--yes` skips the prompts), writes the spec's `## Operator Confirmation` audit section, advances @@ -31,15 +44,11 @@ breaking changes may land in a minor release. disagreement that co-occurs with an unreadable action list — the two have different remedies, and only the first was ever reported. - Parks are indexed in `.bmad-loop/operator-actions.json`, written at park time along with a - notification naming the actions and the command that ends them. The index is **machine-local and - never committed** (it is registered in the repository's local git exclude): it is written from - inside a story's commit window, where committing it would either shift `HEAD` past the commit the - park just stamped or, under worktree isolation, advance the target branch and break an - `scm.merge_strategy = "ff"` merge-back. So a park is confirmed on the machine that ran it. The - committed truth stays the spec and the board; `confirm` refuses an index entry that disagrees with - them, and `validate` reports the drift in both directions (`operator.registry-stale`, - `operator.actions-malformed`). + Parks are recorded at park time along with a notification naming the actions and the command + that ends them — as committed per-story records since #356 (above), which is what lets a clone + that never ran the story confirm it. The committed truth stays the spec and the board: `confirm` + refuses a record that disagrees with them, and `validate` reports the drift in both directions + (`operator.registry-stale`, `operator.actions-malformed`). - **Stories can park at `awaiting-operator` (#335, part 2 of 4).** A dev session whose story needs an action only a human can take outside the repo — buy a domain, publish a DNS record, grant an diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index dd54b222..b6859ee1 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1567,7 +1567,11 @@ def _print_parked(parked: list[operatoractions.ParkedStory]) -> int: else: drift = p.drift() suffix = f" — NOT CONFIRMABLE: {drift}" if drift else "" - print(f" {p.story_key} (parked {p.parked_at}, commit {p.commit[:8]}){suffix}") + # `commit` is derived provenance and legitimately empty for a record not + # yet in any commit (NO_VCS, or a crash before the park's commit landed) + # — render nothing rather than a dangling "commit " fragment. + provenance = f", commit {p.commit[:8]}" if p.commit else "" + print(f" {p.story_key} (parked {p.parked_at}{provenance}){suffix}") for i, action in enumerate(p.actions, 1): print(f" {i}. {action}") print("") @@ -1606,7 +1610,7 @@ def cmd_confirm(args: argparse.Namespace) -> int: Out of band by construction. The run that parked the story is long finished and its task is in a terminal phase with no legal transition, so this touches no run state at all — it writes the two things that DEFINE the story's - completion (the spec and the board) and drops the index entry that pointed at + completion (the spec and the board) and drops the park record that pointed at them. Nothing is re-driven: the agent-doable work was committed at park time, and re-running a session would redo finished work while the human's actions stayed outside the repo. `--reverify` is there for the case that matters — @@ -1636,10 +1640,11 @@ def cmd_confirm(args: argparse.Namespace) -> int: story = next((p for p in parked if p.story_key == key), None) if story is None: print( - f"error: {key} is not awaiting operator actions on this machine. The index " - f"({operatoractions.STORE_REL}) is written by the run that parked the story " - f"and is not committed, so confirm runs where the orchestrator ran. " - f"`bmad-loop confirm --list` shows what is parked here.", + f"error: {key} is not awaiting operator actions in this checkout. A park is " + f"recorded under {operatoractions.RECORDS_REL.as_posix()}/ and travels with " + f"the story's own commit, so if another machine's run parked it, pull the " + f"branch that carries the park commit. `bmad-loop confirm --list` shows " + f"what is parked here.", file=sys.stderr, ) return 1 @@ -1814,7 +1819,7 @@ def _land_confirmation( today: str, ) -> int: """The half of a confirmation that lives OUTSIDE the spec: advance the board, - drop the index entry, commit the pair. + drop the park record, commit the set. Split out because it is exactly what an interrupted confirmation still owes. The spec half is already on disk in that case — audit section appended, status @@ -1835,12 +1840,18 @@ def _land_confirmation( file=sys.stderr, ) return 1 + # Resolve the record path BEFORE dropping it: the drop unlinks the file, and + # a committed record's deletion must ride the confirm commit (#356) — the + # record arrived in the park's commit, so leaving its removal uncommitted + # would dirty the tree the next run's preflight refuses. `commit_paths` + # skips the path when no record was ever committed (a legacy-index park). + record = operatoractions.record_path(project, story.story_key) operatoractions.drop(project, story.story_key) try: verify.commit_paths( paths.repo_root, f"chore(operator): confirm {story.story_key}", - [spec, paths.sprint_status], + [spec, paths.sprint_status, record], ) except verify.GitError: pass # files are written; git history is best effort (as `decisions`) diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 423f6927..83ab0079 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -130,9 +130,12 @@ def confirm_document(parked: list[ParkedStory]) -> dict[str, object]: has to be able to see WHICH reading made the boolean what it is. CONFIRM_SCHEMA_VERSION deliberately stays 1. Evolution here is additive-only - (machine.py), new fields may appear, and every field that already existed - produces the same value for every input — nothing a consumer pinned has - moved.""" + (machine.py), new fields may appear, and no field has changed presence or + type. One value semantic did move with #356, stated honestly: `commit` is now + DERIVED from the park record's git history rather than stored at park time + (the committed record cannot contain its own sha), so it can be `""` for a + record not yet in any commit — a consumer that pinned "always 40 hex chars" + must treat empty as "no provenance", which the field always could express.""" return { "schema_version": CONFIRM_SCHEMA_VERSION, "parked": [ diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index e6306cbb..3d661fac 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -1798,10 +1798,11 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: inside it. ``finalize_commit`` is content-idempotent across both crash states — pre-squash (skill commit chain above baseline) squashes normally; post-squash (squashed commit at HEAD, clean tree) - re-squashes to an identical-content commit, orphaning the pre-crash - squash (harmless). ``commit_sha`` is stamped only here and is - write-only (never routing), so the empty persisted value is - harmless.""" + re-squashes to an equivalent-content commit, orphaning the pre-crash + squash (harmless; "equivalent" rather than "identical" because a park + record regenerated across midnight carries a new ``parked_at``). + ``commit_sha`` is stamped only here and is write-only (never routing), + so the empty persisted value is harmless.""" message = self._commit_message(task) # pre_commit: a plugin may rewrite the commit message or escalate (pause). # A defer/skip veto would have to unwind a COMMITTING task (no legal move @@ -1821,8 +1822,13 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: # the close, before its write, so both failure arms below hold the # pre-close text no matter where in the window a raise lands. snapshot: list[tuple[Path, str]] = [] + park_record: tuple[Path, str | None] | None = None try: self._close_declared_deferred(task, snapshot) + # A parked story's record rides this same commit (#356): written into + # the workspace ahead of the `git add -A`, it reaches every clone the + # story's commit does — including through the worktree merge-back. + park_record = self._write_park_record(task) # bmad-dev-auto commits its own work each iteration; the orchestrator # squashes that chain plus its uncommitted bookkeeping back onto the # pre-dev baseline as one commit carrying `message`. None means there @@ -1837,6 +1843,7 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: task.restore_patch = None except verify.GitError as e: self._restore_deferred_closes(task, snapshot) + self._restore_park_record(park_record) self._escalate(task, f"commit failed: {e}") except BaseException: # A failed commit is not the only way out of this window: the signal @@ -1844,9 +1851,10 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: # thread is standing, and an OSError raised outside `_run_git` (an # FS fault; the spawn class arrives as GitSpawnError since #343 and # takes the arm above) passes through as itself. Either leaves the - # ledger flipped for a commit that does not exist. Restore, then - # re-raise untouched. + # ledger flipped — and the park record written — for a commit that + # does not exist. Restore, then re-raise untouched. self._restore_deferred_closes(task, snapshot) + self._restore_park_record(park_record) raise # Final-phase rule: AWAITING_OPERATOR iff the task carries actions, # otherwise DONE. Derived from PERSISTED task state, never from a local @@ -1864,7 +1872,6 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: commit=task.commit_sha, actions=list(task.operator_actions), ) - self._index_park(task) self._notify_park(task) else: advance(task, Phase.DONE) @@ -1888,8 +1895,8 @@ def _park_spec_relpath(self, task: StoryTask) -> str: `RuntimeError` joins the guard because `Path.resolve` reports a symlink loop that way below 3.13 — the same non-OSError `_ledger_in_repo` already catches, and `requires-python` is 3.11. Catching it HERE rather than only - around the call is what keeps the fallback: the index entry is still - written, with `spec_file` recorded verbatim, and per #356 the entry is + around the call is what keeps the fallback: the park record is still + written, with `spec_file` recorded verbatim, and per #356 the record is load-bearing (a story key does not yield a spec path).""" spec_file = task.spec_file or "" if not spec_file: @@ -1899,39 +1906,65 @@ def _park_spec_relpath(self, task: StoryTask) -> str: except (OSError, RuntimeError, ValueError): return spec_file - def _index_park(self, task: StoryTask) -> None: - """Add the parked story to the project's operator-actions index — the - store `bmad-loop confirm` reads. + def _write_park_record(self, task: StoryTask) -> tuple[Path, str | None] | None: + """Write the parked story's committed record — the store `bmad-loop + confirm` reads (#356) — returning ``(path, prior_text)`` for the restore + arm, or None when nothing was written (not a park, or the write failed). + + Runs inside the commit window, into the WORKSPACE (the unit worktree + under isolation, like the sprint-status mirror and the deferred-close + annotations), so `finalize_commit`'s `git add -A` folds it into the + story's own commit and the merge-back carries it to the target. Rooted + at `self.workspace.paths.project`, NOT `self.paths.project`: the main + root is the wrong tree here — a record written there would sit + untracked beside the commit instead of inside it. Best-effort by design, and the only write on this path that is. The - commit has already landed: the work is safe, the board says - `awaiting-operator`, and the journal and spec both record what is owed. - Raising here would abort a story that genuinely succeeded over its - bookkeeping, so an unwritable index degrades to a journal line — and - `validate` reports the resulting drift (`operator.registry-stale`). - - This runs at the tail of `_finalize_commit_phase`, OUTSIDE every try - there and after `advance(task, Phase.AWAITING_OPERATOR)`, so an escape - skips `_notify_park`, `_emit("post_commit")` and `self._save()` — the - park phase is never persisted, over an index write that was best-effort - by design. `RuntimeError` is in the guard for a second reason beyond the - resolve above: `record_park` -> `_write_store` -> `_exclude_from_git` -> - `install._worktree_local_exclude` has a bare `.resolve()` outside that - function's only try, reachable when `git rev-parse --git-common-dir` - answers with a relative path (the linked-worktree case — exactly this - one).""" + park itself is defined by the spec frontmatter and the board token; + raising here would abort a story that genuinely succeeded over its + bookkeeping, so an unwritable record degrades to a journal line — and + `validate` reports the board-parked-but-recordless drift. The kind + stays `operator-index-failed` so nothing greping journals re-learns a + name. `RuntimeError` stays in the guard for `_park_spec_relpath`'s + reason (symlink-loop `resolve` below 3.13) and as insurance: the cost + of degrading a non-OSError here is a missing record, the cost of NOT + catching it is an aborted commit for a finished story.""" + if not task.operator_actions: + return None + path = operatoractions.record_path(self.workspace.paths.project, task.story_key) try: + prior = path.read_text(encoding="utf-8") if path.is_file() else None operatoractions.record_park( - self.paths.project, + self.workspace.paths.project, task.story_key, actions=list(task.operator_actions), spec_file=self._park_spec_relpath(task), - commit=task.commit_sha or "", run_id=self.state.run_id, parked_at=self._today(), ) except (OSError, RuntimeError) as e: self.journal.append("operator-index-failed", story_key=task.story_key, error=str(e)) + return None + return (path, prior) + + def _restore_park_record(self, record: tuple[Path, str | None] | None) -> None: + """Put the park record back the way `_write_park_record` found it, for + the failure arms of the commit window: a `GitError` escalation or a + pass-through raise must not leave a record — untracked, in a tree the + next story's `git add -A` would sweep — for a commit that does not + exist. Best-effort like `_restore_deferred_closes`: the restore runs + under an exception already in flight and must never replace it.""" + if record is None: + return + path, prior = record + with contextlib.suppress(OSError): + if prior is None: + path.unlink(missing_ok=True) + parent = path.parent + if parent.is_dir() and not any(parent.iterdir()): + parent.rmdir() + else: + path.write_text(prior, encoding="utf-8") def _notify_park(self, task: StoryTask) -> None: """Tell the human a story is waiting on them, and exactly what for. diff --git a/src/bmad_loop/operatoractions.py b/src/bmad_loop/operatoractions.py index a0808d32..19a7c78a 100644 --- a/src/bmad_loop/operatoractions.py +++ b/src/bmad_loop/operatoractions.py @@ -1,49 +1,56 @@ -"""Project-level index of stories parked at ``awaiting-operator`` (#335). +"""Committed, per-story records of stories parked at ``awaiting-operator`` (#335, #356). A park commits everything an agent could do and records what a human still owes in the spec's ``operator_actions:`` frontmatter. That frontmatter, plus the board's ``awaiting-operator`` token, is the *committed truth*. This module adds -an index over it — ``.bmad-loop/operator-actions.json``, keyed by story key — so -``bmad-loop confirm`` can list every outstanding obligation across epics and runs -without re-reading every spec, and can name the run and commit a park came from -(provenance the spec itself does not carry). - -Deliberately machine-local --------------------------- -Unlike ``.bmad-loop/decisions.json``, this store is NOT committed. It is written -from inside a story's commit window, where every way of committing it is wrong: - -- committing it on its own path shifts HEAD past the ``commit_sha`` the park just - stamped; and under ``scm.isolation = "worktree"`` it would advance the *target* - branch while the unit branch is still out, so the merge-back fails outright - under ``scm.merge_strategy = "ff"``; -- folding it into the story's own commit is only possible in the worktree, and - the human reads the index at the project root; -- leaving it merely untracked dirties the tree that the epic-boundary auto-sweep - refuses to run on, and the next story's ``git add -A`` would sweep one story's - bookkeeping into another story's commit. - -So the writer registers the path in the repository's local git exclude instead. -The index stays invisible to git, and none of the above can happen. - -The cost, stated plainly because ``confirm`` has to answer for it: the index -lives on the machine that ran the orchestrator, so that is where a park is -confirmed. A fresh clone has none, and cannot rebuild one — ``spec_file`` is -reported by the dev session in its result JSON and is not derivable from a story -key, so the path to a parked story's spec survives only here. ``confirm`` -therefore refuses rather than guessing, and says why. Confirming from a second -machine needs a *committed* index and the park-commit sequencing to go with it; -that is a follow-up, not something to fake with a glob. - -Because the index can still drift from the committed truth it points at (a +a record over it — one JSON file per parked story under ``.bmad-loop/operator/`` +— because the truth alone is not findable: ``spec_file`` is reported by the dev +session in its result JSON and is not derivable from a story key, so the record +is the only route from ``bmad-loop confirm `` back to the spec. + +Committed, one file per story +----------------------------- +The record is written into the WORKSPACE inside the story's commit window, just +before ``finalize_commit``'s ``git add -A``, so it rides the park's own commit — +and under ``scm.isolation = "worktree"`` it reaches the target branch with the +ordinary merge-back (#355 merges parked units beside DONE). That placement is +what makes a committed record safe where committing a shared index was not +(#356): nothing lands on the *target* branch during the commit window (the +``ff`` merge-back survives, ``branch_per = "run"`` included), the clean tree the +epic-boundary auto-sweep requires is undisturbed (the record is part of the +story's commit, not residue beside it), and a crash re-drives through the same +COMMITTING resume arm that re-derives the park itself. One file per story, +rather than one shared index, means two parks on different branches can never +produce a merge conflict. The payoff is the acceptance criterion of #356: a +fresh clone carries every record, so ``confirm`` works wherever the repository +does. + +The record deliberately carries no commit sha — it is written into the very +commit it rides, so the sha does not exist yet. :func:`resolve` derives it from +``git log`` over the record's own path instead, which under a squash merge names +the commit that actually carries the park on *this* branch; a record not yet in +any commit derives to ``""``. + +The legacy machine-local index +------------------------------ +Before #356 the store was a single ``.bmad-loop/operator-actions.json``, kept +out of git via the repository-local exclude file. :func:`load` still reads it — +a park written by an older version must stay confirmable on the machine that +wrote it — and :func:`drop` prunes it, but nothing writes it anymore. A +per-story record wins its key over a legacy entry. Stale exclude lines on old +machines are left alone: they keep leftover legacy files invisible, which is +exactly right for a file nothing writes. + +Because a record can still drift from the committed truth it points at (a hand-edited spec, a ``git revert``, a story re-driven to ``done``), ``validate`` carries ``operator.registry-stale`` and ``operator.actions-malformed``. Both are -warnings: an index is only safe to keep out of the commit if something reports -its drift and nothing gates on it. +warnings: ``confirm`` refuses drifted entries itself, so nothing gates on the +record. """ from __future__ import annotations +import contextlib import json from dataclasses import dataclass from pathlib import Path @@ -51,10 +58,10 @@ from . import devcontract, sprintstatus, verify from .bmadconfig import ProjectPaths from .frontmatter import operator_actions_of, read_frontmatter, status_of -from .install import _worktree_local_exclude -from .platform_util import atomic_replace +from .platform_util import atomic_replace, safe_segment -STORE_REL = Path(".bmad-loop") / "operator-actions.json" +RECORDS_REL = Path(".bmad-loop") / "operator" +LEGACY_STORE_REL = Path(".bmad-loop") / "operator-actions.json" AWAITING_OPERATOR = verify.AWAITING_OPERATOR # The status a confirmation lands on. Spelled here rather than imported from # `sprintstatus.STATUS_ORDER` because both sides of the join use it — the board @@ -62,48 +69,64 @@ DONE = "done" -def store_path(project: Path) -> Path: - return project / STORE_REL +def records_dir(project: Path) -> Path: + return project / RECORDS_REL + + +def record_path(project: Path, story_key: str) -> Path: + """Where a story's park record lives. `safe_segment` is identity for every + conventional story key; a hostile key gets a legal filename, and the record's + own ``story_key`` field stays authoritative over the mangled stem.""" + return records_dir(project) / f"{safe_segment(story_key)}.json" + + +def legacy_store_path(project: Path) -> Path: + return project / LEGACY_STORE_REL # --------------------------------------------------------------- store I/O def load(project: Path) -> dict[str, dict]: - """The park index, ``{story_key: {actions, spec_file, commit, run_id, - parked_at}}``. Tolerant of a missing or malformed file (returns ``{}``): the - index is a convenience over committed truth, so an unreadable one degrades to - "no index" rather than blocking a human from confirming their own work.""" - path = store_path(project) - if not path.is_file(): - return {} + """Every park record, ``{story_key: {actions, spec_file, run_id, + parked_at}}``, merged over any legacy machine-local entries (a record wins + its key). Tolerant throughout: this is a convenience over committed truth, + so an unreadable side degrades rather than blocking a human from confirming + their own work. An unparseable RECORD file still contributes an empty entry + under its filename stem — visible as drift ("no spec file could be located + for it") rather than silently absent, because the file's presence is the + committed claim that something is owed.""" + data: dict[str, dict] = dict(_load_legacy(project)) + for path in _record_files(project): + record = _read_json(path) + if isinstance(record, dict): + key = str(record.get("story_key") or "") or path.stem + data[key] = record + else: + data.setdefault(path.stem, {}) + return data + + +def _record_files(project: Path) -> list[Path]: try: - data = json.loads(path.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError): - return {} - return data if isinstance(data, dict) else {} - + return sorted(records_dir(project).glob("*.json")) + except OSError: + return [] -def _write_store(project: Path, data: dict) -> None: - path = store_path(project) - path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(".tmp") - tmp.write_text(json.dumps(data, indent=2, sort_keys=True), encoding="utf-8") - atomic_replace(tmp, path) - _exclude_from_git(project) +def _read_json(path: Path) -> object | None: + try: + return json.loads(path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError, UnicodeDecodeError): + return None -def _exclude_from_git(project: Path) -> None: - """Keep the index out of git's view (see the module docstring for why). - Reuses the helper worktree provisioning already excludes its seeded tool - files with; it resolves ``--git-common-dir``, so the pattern lands in the - MAIN repository's exclude file even when the park is committing inside a - linked worktree — which is exactly the case that needs it. Best-effort by - construction (the helper returns quietly when git cannot be queried), and - that is the right failure mode: an index that could not be excluded is still - a correct index.""" - _worktree_local_exclude(project, [STORE_REL.as_posix()]) +def _load_legacy(project: Path) -> dict[str, dict]: + path = legacy_store_path(project) + if not path.is_file(): + return {} + data = _read_json(path) + return data if isinstance(data, dict) else {} # ------------------------------------------------------------ record + drop @@ -115,46 +138,109 @@ def record_park( *, actions: list[str], spec_file: str, - commit: str, run_id: str, parked_at: str, -) -> None: - """Index a story the run just parked. Re-parking the same key overwrites its - entry rather than accumulating: a story owes whatever its latest park says it - owes, and a stale action list is worse than none.""" - data = load(project) - data[story_key] = { +) -> Path: + """Write a story's park record, returning its path. Re-parking the same key + overwrites rather than accumulates: a story owes whatever its latest park + says it owes, and a stale action list is worse than none. + + The write is atomic (temp + replace) and the temp file is unlinked on any + raise: this runs just ahead of ``finalize_commit``'s ``git add -A``, and a + stranded ``.tmp`` would ride the story's own commit.""" + path = record_path(project, story_key) + path.parent.mkdir(parents=True, exist_ok=True) + record = { + "story_key": story_key, "actions": list(actions), "spec_file": spec_file, - "commit": commit, "run_id": run_id, "parked_at": parked_at, } - _write_store(project, data) + tmp = path.with_suffix(".tmp") + try: + tmp.write_text(json.dumps(record, indent=2, sort_keys=True), encoding="utf-8") + atomic_replace(tmp, path) + except BaseException: + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) + raise + return path def drop(project: Path, story_key: str) -> bool: - """Remove a story's entry, returning whether one was there. No write when the - key is absent, so confirming a story the index never knew about (a fresh - clone) leaves no file behind.""" - data = load(project) + """Remove a story's park record — and any legacy machine-local entry — + returning whether anything was removed. No write when the key is absent + everywhere, so confirming a story no store ever knew about (pre-#356 the + fresh-clone case) leaves no file behind. Removal raises on OSError like any + repair write: a drop that silently failed would leave `validate` warning + about a story that was genuinely confirmed.""" + removed = _drop_record(project, story_key) + return _drop_legacy(project, story_key) or removed + + +def _drop_record(project: Path, story_key: str) -> bool: + """Unlink every record file claiming ``story_key`` — matched on the record's + own field, not the filename, so a mangled or hand-renamed file cannot survive + its confirmation (nor can dropping one key ever unlink another's record).""" + removed = False + for path in _record_files(project): + if _record_key(path) == story_key: + path.unlink() + removed = True + return removed + + +def _record_key(path: Path) -> str: + record = _read_json(path) + if isinstance(record, dict): + return str(record.get("story_key") or "") or path.stem + return path.stem + + +def _drop_legacy(project: Path, story_key: str) -> bool: + """Prune a legacy entry. The emptied store is deleted outright rather than + rewritten as ``{}``: nothing writes the legacy file anymore, and an empty + husk would read as "an index with nothing parked" forever. + + The rewrite unlinks its temp on any raise, like `record_park` — but for the + opposite reason. `drop` runs out of band from `confirm`, not inside a commit + window, so the hazard is not a temp RIDING a commit but one OUTLIVING the + failure: `.bmad-loop/` is not ignored (`install` excludes only `runs/`, + `cache/` and `policy.toml`, and the pre-#356 exclude line was the anchored + literal `operator-actions.json`, never its `.tmp` sibling), so a stranded + `.bmad-loop/operator-actions.tmp` is an untracked file to + `verify.worktree_clean` — a dirty tree blocking the next run's preflight and + the epic-boundary auto-sweep, over a prune of a store nothing writes.""" + path = legacy_store_path(project) + data = _load_legacy(project) if story_key not in data: return False del data[story_key] - _write_store(project, data) + if data: + tmp = path.with_suffix(".tmp") + try: + tmp.write_text(json.dumps(data, indent=2, sort_keys=True), encoding="utf-8") + atomic_replace(tmp, path) + except BaseException: + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) + raise + else: + path.unlink(missing_ok=True) return True -# ------------------------------------------------- joining index to truth +# ------------------------------------------------- joining records to truth @dataclass(frozen=True) class ParkedStory: - """One index entry joined back to the committed truth it points at. + """One park record joined back to the committed truth it points at. - The index is what `confirm` can *find*; the spec and the board are what it is + The record is what `confirm` can *find*; the spec and the board are what it is allowed to *believe*. Keeping both readings on one object is what lets the - command refuse precisely — "the index says parked, the board says done" is a + command refuse precisely — "the record says parked, the board says done" is a different message, and a different remedy, from "no such story". ``spec_status`` / ``board_status`` are None when that side could not be read @@ -165,7 +251,12 @@ class ParkedStory: carries the ``## Operator Confirmation`` section `confirm` writes. It is what separates a story a human never signed off from one whose confirmation was interrupted part-way — two states whose spec and board readings otherwise - look identical to a stale entry.""" + look identical to a stale entry. + + ``commit`` is display provenance only, never a predicate input. For a park + record it is DERIVED (``git log`` over the record's path — the record rides + the commit it names, so it cannot store it) and is ``""`` for a record not + yet in any commit; a legacy index entry keeps its stored sha.""" story_key: str actions: tuple[str, ...] @@ -181,7 +272,7 @@ class ParkedStory: def confirmable(self) -> bool: """Whether both sides of the committed truth still describe a park with something owed. Everything else is drift, and `confirm` refuses it rather - than flipping a board on the index's word alone.""" + than flipping a board on the record's word alone.""" return ( bool(self.actions) and self.spec_status == AWAITING_OPERATOR @@ -198,7 +289,7 @@ def resumable(self) -> bool: a raising `sprintstatus.advance`, or one that returns unchanged because the board line is in a shape its line regex cannot rewrite — and what is left on disk is a signed-off spec at `done` with an entry still pointing - at it. That reads to :meth:`drift` as a stale index (arm 3, "its spec now + at it. That reads to :meth:`drift` as a stale entry (arm 3, "its spec now says status: done"), so a re-run refuses the very state a re-run exists to clear, and `validate` nags about it forever. @@ -210,9 +301,9 @@ def resumable(self) -> bool: ⚠️ The board arm accepts `done` as well as `awaiting-operator`. The interruption message tells the human to fix the board by hand; if they do, a strict ``== AWAITING_OPERATOR`` test would drop the entry out of this - predicate and strand it — index entry retained, `validate` warning - forever, and no command that will remove either. `sprintstatus.advance` - is already idempotent at `done`, so resuming from there costs nothing and + predicate and strand it — record retained, `validate` warning forever, + and no command that will remove either. `sprintstatus.advance` is + already idempotent at `done`, so resuming from there costs nothing and finishes the one thing left: dropping the entry.""" return ( self.confirmation_recorded @@ -225,14 +316,14 @@ def committed_drift(self) -> str | None: entry, or None when it does not. Split from :meth:`drift` so a caller that has already reported something - about the *index* side (an unreadable action list) can still report a + about the *record* side (an unreadable action list) can still report a co-occurring disagreement about the committed side. Folding both into one method meant the first cause found was the only one anybody heard about, and the two have different remedies: repair the list, versus discard the entry. Ordered most-fundamental first — a missing spec explains a missing status, so it is reported instead of it.""" if self.spec_path is None: - return "the index records no spec file for it" + return "no spec file could be located for it" if self.spec_status is None: return f"its spec is missing or unreadable ({self.spec_path})" if self.spec_status != AWAITING_OPERATOR: @@ -245,7 +336,7 @@ def committed_drift(self) -> str | None: def drift(self) -> str | None: """Why this entry is not confirmable, phrased for a human, or None when - it is — the committed-side causes, then the index's own. + it is — the committed-side causes, then the record's own. The empty-actions cause comes last because it is the least fundamental: a spec that has moved on to `done` explains its own unreadable list, and @@ -257,16 +348,23 @@ def drift(self) -> str | None: def resolve(project: Path, paths: ProjectPaths) -> list[ParkedStory]: - """Every index entry joined to its spec and board status, sorted by key. + """Every park entry joined to its spec and board status, sorted by key. Reading degrades rather than raises throughout: this backs `confirm --list` and a `validate` warning, and neither may be the thing that crashes on a spec someone deleted. The actions come from the SPEC when it can be read and from - the index only as a fallback — the spec is the committed truth, so a spec + the record only as a fallback — the spec is the committed truth, so a spec edited after the park shows the human what they actually owe now. The confirmation-section reading degrades the same way — an absent or unreadable spec cannot be SHOWN to carry an acknowledgment, so it reads as False and the - entry takes the ordinary path, which reports the real fault.""" + entry takes the ordinary path, which reports the real fault. + + ``commit`` comes from the entry when stored (a legacy index entry) and is + otherwise derived from the record file's own git history — see the module + docstring. The derived sha can legitimately differ from the journal's + ``story-awaiting-operator`` entry: under a squash merge the unit's own park + commit never reaches the target, and the squash commit that did is the + truthful provenance here.""" entries = load(project) out: list[ParkedStory] = [] for key in sorted(entries): @@ -284,7 +382,7 @@ def resolve(project: Path, paths: ProjectPaths) -> list[ParkedStory]: confirmation_recorded=( spec_path is not None and devcontract.has_operator_confirmation(spec_path) ), - commit=str(entry.get("commit") or ""), + commit=str(entry.get("commit") or "") or _derive_commit(project, paths, key), run_id=str(entry.get("run_id") or ""), parked_at=str(entry.get("parked_at") or ""), ) @@ -292,6 +390,13 @@ def resolve(project: Path, paths: ProjectPaths) -> list[ParkedStory]: return out +def _derive_commit(project: Path, paths: ProjectPaths, story_key: str) -> str: + try: + return verify.last_commit_for(paths.repo_root, record_path(project, story_key)) + except verify.GitError: + return "" # no VCS, or a repository with no history yet + + def _spec_path(entry: dict, paths: ProjectPaths) -> Path | None: spec_file = str(entry.get("spec_file") or "") if not spec_file: @@ -322,11 +427,11 @@ def _board_status(paths: ProjectPaths, key: str) -> str | None: def actions_of(entry: dict) -> tuple[str, ...]: - """The actions an index entry declares, read through the *same* normalizer + """The actions a park entry declares, read through the *same* normalizer the spec frontmatter goes through (:func:`frontmatter.operator_actions_of`). - Sharing the reading is the point: the index is written from a spec and + Sharing the reading is the point: the record is written from a spec and compared back against one, so a shape that collapses to ``()`` on the spec side must collapse to ``()`` here too. Two readings would let a hand-edited - index disagree with the spec about what a human owes.""" + record disagree with the spec about what a human owes.""" return operator_actions_of({"operator_actions": entry.get("actions")}) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 7f0f3d3e..1b69b99f 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -143,6 +143,23 @@ def rev_parse_head(repo: Path) -> str: return out +def last_commit_for(repo: Path, path: Path) -> str: + """Sha of the most recent commit touching ``path``, or ``""`` when no commit + does (an untracked or deleted-without-history file) or the path lies outside + the repo. Backs the derived provenance of an operator park record, which is + written into the very commit it rides and so cannot store its own sha. Git + failures raise :class:`GitError` like every sibling; only the path relation + degrades silently, mirroring `commit_paths`' outside-the-repo contract.""" + try: + rel = Path(path).resolve().relative_to(repo.resolve()).as_posix() + except (OSError, RuntimeError, ValueError): + return "" + rc, out = _git(repo, "log", "-n", "1", "--format=%H", "--", rel) + if rc != 0: + raise GitError(f"git log failed in {repo}: {out}") + return out + + def worktree_clean(repo: Path) -> bool: # The orchestrator's own config file (.bmad-loop/policy.toml) is excluded: # the TUI settings editor rewrites it, and a tracked config edit must not @@ -1969,7 +1986,13 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: or staged changes untouched. Unlike commit_story's `add -A`, this is safe to call out of band (e.g. `bmad-loop decisions`) when the tree may hold the user's own uncommitted work. Returns the new HEAD sha, or None when the - given paths had no changes to commit. Paths outside the repo are ignored.""" + given paths had no changes to commit. Paths outside the repo are ignored — + and so is a path git has never seen (absent from both the working tree and + the index): `git add` hard-fails on a pathspec matching nothing, and one + optional path would otherwise sink the whole commit (a swallowed `GitError` + in `confirm` silently losing the spec+board commit over a park record that + was never committed). A missing-but-TRACKED path stays in: that is a + deletion to stage.""" rels: list[str] = [] repo_root = repo.resolve() for p in paths: @@ -1977,6 +2000,13 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: rels.append(str(Path(p).resolve().relative_to(repo_root))) except ValueError: continue + missing = [r for r in rels if not ((repo_root / r).exists() or (repo_root / r).is_symlink())] + if missing: + rc, out = _git_raw(repo, "ls-files", "-z", "--", *missing) + if rc != 0: + raise GitError(f"git ls-files failed: {out}") + tracked = {t for t in out.split("\0") if t} + rels = [r for r in rels if r not in missing or r.replace("\\", "/") in tracked] if not rels: return None rc, out = _git(repo, "add", "--", *rels) diff --git a/tests/test_cli.py b/tests/test_cli.py index 401ab98b..449df185 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4316,18 +4316,18 @@ def test_platform_preflight_notes_forced_selection_provenance(mux_registry, monk assert any("forced by BMAD_LOOP_MUX_BACKEND" in n for n in notes) -# ---------------------------------------- bmad-loop confirm (#335, part 3 of 4) +# ---------------------------------------- bmad-loop confirm (#335 part 3, #356) # # The exit from `awaiting-operator`. Out of band by construction: the run that # parked the story is long finished and its task is terminal, so `confirm` writes # only the two things that DEFINE completion (the spec and the board) and drops -# the index entry that pointed at them. +# the park record that pointed at them. CONFIRM_ACTIONS = ["buy example.com at the registrar", "publish the _acme-challenge TXT record"] def _park_story(paths, key="1-1-a", *, actions=None, spec_status="awaiting-operator", board=None): - """Put the project in the state a park leaves behind: spec, board, index.""" + """Put the project in the state a park leaves behind: spec, board, record.""" from bmad_loop import operatoractions acts = CONFIRM_ACTIONS if actions is None else actions @@ -4339,7 +4339,6 @@ def _park_story(paths, key="1-1-a", *, actions=None, spec_status="awaiting-opera key, actions=list(acts) if isinstance(acts, list) else [], spec_file=sp.relative_to(paths.project).as_posix(), - commit="abcdef1234567890", run_id="run-1", parked_at="2026-07-28", ) @@ -4350,15 +4349,21 @@ def _confirm_argv(paths, *extra): return ["confirm", "--project", str(paths.project), *extra] -def test_confirm_flips_spec_board_index_and_commits(project, capsys, monkeypatch): +def test_confirm_flips_spec_board_record_and_commits(project, capsys, monkeypatch): """The happy path, end to end. All four records move together: the spec gains its audit section and reaches done, the board advances through the sole - writer, the index entry goes, and the pair is committed. + writer, the park record goes, and the set is committed — the record's + DELETION included, because the record arrived in the park's commit (#356) + and leaving its removal uncommitted would dirty the tree the next run's + preflight refuses. Ablation: delete the `sprintstatus.advance` call in `_apply_confirmation` and this fails on the board assertion — a spec saying done over a board still - parked is the false-green the state exists to prevent.""" - from bmad_loop import devcontract, operatoractions, sprintstatus + parked is the false-green the state exists to prevent. For the deletion leg: + drop `record` from `_land_confirmation`'s `commit_paths` list and this fails + on the name-only assertion, leaving a staged-nothing deletion dirtying the + tree.""" + from bmad_loop import devcontract, operatoractions, sprintstatus, verify install_bmad_config(project) sp = _park_story(project) @@ -4376,8 +4381,11 @@ def test_confirm_flips_spec_board_index_and_commits(project, capsys, monkeypatch assert f"- {action}" in text assert sprintstatus.story_status(project.sprint_status, "1-1-a") == "done" assert operatoractions.load(project.project) == {} - # the spec and board moved into history together, and nothing else did + # the spec, board and record deletion moved into history together assert "confirm 1-1-a" in git(project.project, "log", "-1", "--format=%s") + changed = git(project.project, "show", "--name-only", "--format=", "HEAD") + assert ".bmad-loop/operator/1-1-a.json" in changed + assert verify.worktree_clean(project.project) assert "✓ 1-1-a confirmed" in capsys.readouterr().out @@ -4428,16 +4436,18 @@ def _boom(*a, **k): assert action in out # unattended still means auditable -def test_confirm_unknown_story_explains_the_machine_local_index(project, capsys): - """The index is not committed, so "not here" is the common case on a second - machine — the error has to say that rather than implying the story is fine.""" +def test_confirm_unknown_story_explains_the_committed_record(project, capsys): + """Since #356 the record travels with the story's commit, so "not here" means + the checkout does not carry it (or the story was never parked) — the error + points at pulling the branch that does, never at some other machine.""" install_bmad_config(project) write_sprint(project, {"epic-1": "in-progress"}) assert cli.main(_confirm_argv(project, "9-9-z")) == 1 captured = capsys.readouterr() assert captured.out == "" - assert "not awaiting operator actions on this machine" in captured.err + assert "not awaiting operator actions in this checkout" in captured.err + assert ".bmad-loop/operator" in captured.err and "pull the branch" in captured.err assert "--list" in captured.err @@ -4529,7 +4539,9 @@ def test_confirm_json_is_a_pure_document(project, capsys): assert entry["story_key"] == "1-1-a" assert entry["actions"] == CONFIRM_ACTIONS assert entry["confirmable"] is True and entry["drift"] is None - assert entry["commit"] == "abcdef1234567890" and entry["run_id"] == "run-1" + # `commit` is DERIVED provenance since #356 — "" for this uncommitted record, + # a real sha once the park's commit exists. Presence and type are stable. + assert entry["commit"] == "" and entry["run_id"] == "run-1" def test_confirm_json_carries_both_sides_of_a_disagreement(project, capsys): @@ -4641,6 +4653,77 @@ def boom(*a, **k): assert sprintstatus.story_status(project.sprint_status, "1-1-a") == "done" +def test_confirm_commits_even_without_a_committed_record(project, capsys, monkeypatch): + """A pre-#356 park has a legacy machine-local entry and NO record file — so + `_land_confirmation` hands `commit_paths` a record path git has never seen. + `git add` hard-fails on a pathspec matching nothing, the `GitError` is + swallowed as best-effort, and the spec+board commit would silently vanish + behind a "✓ confirmed". + + Ablation: revert `commit_paths`' missing-and-untracked filter and this fails + on the log assertion — confirm reports success, but nothing was committed.""" + from bmad_loop import operatoractions, sprintstatus, verify + + install_bmad_config(project) + sp = _park_story(project) + operatoractions.record_path(project.project, "1-1-a").unlink() # old versions wrote none + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "park (pre-#356)") + # the machine-local entry, written AFTER the commit the way the old version did + store = operatoractions.legacy_store_path(project.project) + store.write_text( + json.dumps( + { + "1-1-a": { + "actions": CONFIRM_ACTIONS, + "spec_file": sp.relative_to(project.project).as_posix(), + "commit": "abcdef1234567890", + "run_id": "run-legacy", + "parked_at": "2026-07-01", + } + } + ) + ) + monkeypatch.setattr(cli, "_confirm", lambda _q: True) + + assert cli.main(_confirm_argv(project, "1-1-a")) == 0 + assert sprintstatus.story_status(project.sprint_status, "1-1-a") == "done" + # the commit LANDED despite the never-committed record path in its list + assert "confirm 1-1-a" in git(project.project, "log", "-1", "--format=%s") + assert operatoractions.load(project.project) == {} # legacy entry pruned too + assert verify.worktree_clean(project.project) + + +def test_confirm_works_on_a_fresh_clone_of_a_parked_repo(project, capsys, monkeypatch, tmp_path): + """The #356 acceptance criterion at the CLI layer: the record travels with + the park commit, so a clone that never ran the orchestrator can list AND + confirm the story. Before #356 this exact flow refused with "confirm runs + where the orchestrator ran".""" + from bmad_loop import bmadconfig, sprintstatus + + install_bmad_config(project) + _park_story(project) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "auto: 1-1-a (awaiting operator)") + + clone = tmp_path / "fresh-clone" + git(tmp_path, "clone", "-q", str(project.project), str(clone)) + git(clone, "config", "user.email", "operator@test") + git(clone, "config", "user.name", "operator") + monkeypatch.setattr(cli, "_confirm", lambda _q: True) + + assert cli.main(["confirm", "--project", str(clone), "--list"]) == 0 + listing = capsys.readouterr().out + assert "1-1-a" in listing + for action in CONFIRM_ACTIONS: + assert action in listing + + assert cli.main(["confirm", "--project", str(clone), "1-1-a"]) == 0 + clone_paths = bmadconfig.load_paths(clone) + assert sprintstatus.story_status(clone_paths.sprint_status, "1-1-a") == "done" + assert "confirm 1-1-a" in git(clone, "log", "-1", "--format=%s") + + # ------------------------- confirm: a write that did not land is not a confirmation # # `confirm` used to discard both spec-write results and print "✓ confirmed" diff --git a/tests/test_engine.py b/tests/test_engine.py index 3e9d5f3f..7611857f 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -1399,16 +1399,22 @@ def test_resume_through_committing_re_derives_the_park(project): kinds = [e["kind"] for e in resumed.journal.entries()] assert "resume-commit" in kinds and "story-awaiting-operator" in kinds assert "story-done" not in kinds + # the re-driven commit carries the record (#356): `_write_park_record` is + # regenerated from persisted state inside the same window the resume re-enters + assert ".bmad-loop/operator/1-1-a.json" in git( + project.project, "ls-tree", "-r", "--name-only", "HEAD" + ) -def test_park_indexes_the_story_for_confirm(project): - """The park writes the project-level index `bmad-loop confirm` reads (#335 - part 3). Without it the obligation exists only in a journal nobody greps and +def test_park_commits_a_record_for_confirm(project): + """The park writes the per-story record `bmad-loop confirm` reads — INTO the + story's own commit (#356), so a fresh clone can confirm what this machine + parked. Without it the obligation exists only in a journal nobody greps and a spec nobody re-reads, and there is no way to find the parked story's spec from its key. - Ablation: delete the `_index_park` call in `_finalize_commit_phase` and this - fails — the story parks with nothing to confirm it from.""" + Ablation: delete the `_write_park_record` call in `_finalize_commit_phase` + and this fails — the story parks with nothing to confirm it from.""" from bmad_loop import operatoractions write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) @@ -1426,20 +1432,29 @@ def test_park_indexes_the_story_for_confirm(project): entry = operatoractions.load(project.project)["1-1-a"] assert entry["actions"] == ACTIONS - assert entry["commit"] == engine.state.tasks["1-1-a"].commit_sha assert entry["run_id"] == engine.state.run_id + assert "commit" not in entry # the record rides the commit it would name + # the record is IN the park commit, not untracked beside it: that is what a + # fresh clone receives, and what keeps the tree clean for the next story + assert ".bmad-loop/operator/1-1-a.json" in git( + project.project, "ls-tree", "-r", "--name-only", "HEAD" + ) + assert worktree_clean(project.project) # the recorded spec path resolves from the PROJECT, which is what `confirm` # has — a worktree-absolute path would be dead before a human read it (story,) = operatoractions.resolve(project.project, project) assert story.spec_path is not None and story.spec_path.is_file() assert story.confirmable, story.drift() + # provenance is derived from the record's own history: the park commit + assert story.commit == engine.state.tasks["1-1-a"].commit_sha -def test_park_indexing_failure_never_un_commits_the_story(project, monkeypatch): - """The only best-effort write on the park path. The commit has already - landed, so raising here would abort a story that genuinely succeeded over its - bookkeeping — it degrades to a journal line instead, and `validate` reports - the resulting drift.""" +def test_park_record_failure_never_blocks_the_story(project, monkeypatch): + """The only best-effort write on the park path. Since #356 it runs BEFORE + `finalize_commit` (the record rides the story's commit), so raising here + would now abort the commit of a story that genuinely finished — it degrades + to a journal line instead, the story parks recordless, and `validate` + reports the board-parked-but-recordless drift.""" from bmad_loop import operatoractions write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) @@ -1465,6 +1480,9 @@ def boom(*a, **k): kinds = [e["kind"] for e in engine.journal.entries()] assert "operator-index-failed" in kinds and "story-awaiting-operator" in kinds assert operatoractions.load(project.project) == {} + # the commit landed recordless and CLEAN — no record, no half-written .tmp + assert ".bmad-loop/operator" not in git(project.project, "ls-tree", "-r", "--name-only", "HEAD") + assert worktree_clean(project.project) def _park_engine(project): @@ -1482,77 +1500,96 @@ def _park_engine(project): return engine -def test_an_unresolvable_spec_path_still_indexes_the_park(project, monkeypatch): - """`_park_spec_relpath` resolves inside a try catching `(OSError, ValueError)`, - and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` — - which is neither. It was called as an argument expression inside - `_index_park`'s own try, whose handler was `except OSError` only, so the type - escaped BOTH. +def test_a_failed_commit_restores_the_park_record(project): + """The record is written just BEFORE `finalize_commit` so it rides the + story's own commit — but that commit can still fail, and `_escalate` unwinds + nothing. Left alone, the record claims a park that is in no commit, sitting + untracked in the tree the run-start preflight refuses and the next story's + `git add -A` would sweep into an unrelated commit — the two hazards the old + machine-local index dodged with a git exclude, owed a restore now that the + record is deliberately visible. - Severe because of where: `_index_park` runs at the tail of - `_finalize_commit_phase`, outside every try there and after - `advance(AWAITING_OPERATOR)`, so an escape skipped `_notify_park`, - `_emit("post_commit")` and **`self._save()`** — the park phase was never - persisted, over an index write that is best-effort by design. + Ablation: delete the `_restore_park_record` calls in `_finalize_commit_phase` + and this fails — the record survives a commit that does not exist.""" + from bmad_loop import operatoractions - Guarding it inside `_park_spec_relpath` rather than only around the call is - what preserves the documented fallback: `spec_file` is recorded verbatim and - the entry still exists. Per #356 that matters — a story key does not yield a - spec path, so losing the entry loses the only route back to the spec. + engine = _park_engine(project) + _reject_commits(project) + + summary = engine.run() + + assert summary.awaiting_operator == 0 and summary.paused # the commit really did fail + assert engine.state.tasks["1-1-a"].phase == Phase.ESCALATED + assert operatoractions.load(project.project) == {} + # emptied on the way out: no record, no .tmp, no husk of a directory + assert not operatoractions.records_dir(project.project).exists() + + +def test_an_unresolvable_spec_path_still_records_the_park(project, monkeypatch): + """`_park_spec_relpath` resolves inside a try catching `(OSError, ValueError)`, + and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` — + which is neither. Guarding it INSIDE `_park_spec_relpath` rather than + leaving it to `_write_park_record`'s outer `(OSError, RuntimeError)` guard + is what preserves the documented fallback: the record is still written, with + `spec_file` recorded verbatim, instead of degrading to no record at all. + Per #356 that matters — a story key does not yield a spec path, so losing + the record loses the only committed route back to the spec. ⚠️ The fault is INJECTED. 3.13/3.14 resolve real symlink loops without raising, so a loop-based test is green on this box and red only on the 3.11/3.12 CI legs. Ablation: revert `_park_spec_relpath`'s except tuple to - `(OSError, ValueError)` and this fails — and run it on 3.11 too, since a - real-loop version would not bite here at all. - - Scoped to the `_index_park` window rather than the whole run because the spec - is resolved on the verify path too: a fault live from the start stops the - story before it ever parks, which tests nothing about this guard. + `(OSError, ValueError)` and this fails — the RuntimeError then lands in + `_write_park_record`'s outer guard, which journals `operator-index-failed` + and writes nothing. Run it on 3.11 too, since a real-loop version would not + bite here at all. + + Scoped to the `_write_park_record` window rather than the whole run because + the spec is resolved on the verify path too: a fault live from the start + stops the story before it ever parks, which tests nothing about this guard. `_park_spec_relpath` has exactly one caller, so the window IS its resolve.""" from bmad_loop import operatoractions engine = _park_engine(project) real_resolve = Path.resolve - real_index = engine._index_park + real_write = engine._write_park_record def unresolvable(self, *a, **kw): if self.name.startswith("spec-"): raise RuntimeError(f"Symlink loop from {str(self)!r}") return real_resolve(self, *a, **kw) - def index_with_the_fault_live(task): + def write_with_the_fault_live(task): with monkeypatch.context() as m: m.setattr(Path, "resolve", unresolvable) - real_index(task) + return real_write(task) - monkeypatch.setattr(engine, "_index_park", index_with_the_fault_live) + monkeypatch.setattr(engine, "_write_park_record", write_with_the_fault_live) summary = engine.run() task = engine.state.tasks["1-1-a"] assert task.phase == Phase.AWAITING_OPERATOR and task.commit_sha assert summary.awaiting_operator == 1 and not summary.crashed and not summary.paused - # the entry survives with the verbatim fallback path — a wrong path a human + # the record survives with the verbatim fallback path — a wrong path a human # can see beats a missing one they cannot entry = operatoractions.load(project.project)["1-1-a"] assert entry["spec_file"] == task.spec_file and entry["actions"] == ACTIONS - # and the phase was PERSISTED — that is the part an escape broke assert load_state(engine.run_dir).tasks["1-1-a"].phase == Phase.AWAITING_OPERATOR kinds = [e["kind"] for e in engine.journal.entries()] assert "story-awaiting-operator" in kinds and "operator-index-failed" not in kinds -def test_a_runtime_error_writing_the_index_degrades_like_an_oserror(project, monkeypatch): - """The second escape path, and not a duplicate of the guard above: - `record_park` -> `_write_store` -> `_exclude_from_git` -> - `install._worktree_local_exclude` has a bare `.resolve()` outside that - function's only try, reachable when `git rev-parse --git-common-dir` answers - with a relative path — the linked-worktree case, which is exactly when a park - is committing. +def test_a_runtime_error_writing_the_record_degrades_like_an_oserror(project, monkeypatch): + """Not a duplicate of the guard above: this pins the OUTER except tuple in + `_write_park_record`. `RuntimeError` stays in it for `_park_spec_relpath`'s + reason (a symlink-loop `resolve` below 3.13, reachable through any path the + record write touches) — and the stakes moved with #356: the write now runs + INSIDE the commit window, so an escaping non-OSError no longer just skips + the park bookkeeping, it lands in the window's `BaseException` arm and + aborts the commit of a story that genuinely finished. Degrades to the same journal line its `OSError` sibling does. Ablation: - revert `_index_park`'s except tuple to `except OSError` and this fails — the - run crashes and the park phase is never saved.""" + revert `_write_park_record`'s except tuple to `except OSError` and this + fails — the commit is aborted and the story never parks.""" from bmad_loop import operatoractions engine = _park_engine(project) diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 85415d70..a9266cfb 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -241,6 +241,18 @@ def test_worktree_parked_unit_merges_like_a_done_one(project): kinds = journal_kinds(engine) assert "unit-merged" in kinds and "story-awaiting-operator" in kinds assert "unit-closed" not in kinds # the failed-unit teardown arm never ran + # the park record rode the unit's own commit through the merge (#356): it is + # tracked at the target root — written into the WORKTREE, not the main root, + # where it would have sat untracked beside the merge instead of inside it — + # and `confirm` can resolve the story from the project alone + assert ".bmad-loop/operator/1-1-a.json" in git( + project.project, "ls-tree", "-r", "--name-only", "HEAD" + ) + from bmad_loop import operatoractions + + (story,) = operatoractions.resolve(project.project, project) + assert story.confirmable, story.drift() + assert story.commit # derived from the record's history on the target branch def test_worktree_run_dir_is_outside_worktree(project): diff --git a/tests/test_operatoractions.py b/tests/test_operatoractions.py index 6c78fc91..0fdc0370 100644 --- a/tests/test_operatoractions.py +++ b/tests/test_operatoractions.py @@ -1,9 +1,10 @@ -"""The project-level operator-actions index (#335, part 3 of 4). +"""The committed per-story park records (#335 part 3, #356). -The index is what `bmad-loop confirm` can find; the spec and the board are what -it is allowed to believe. These tests pin both halves: the store round-trip, and -the join back onto the committed truth that decides whether an entry is -confirmable or drifted. +The record is what `bmad-loop confirm` can find; the spec and the board are what +it is allowed to believe. These tests pin both halves: the store round-trip +(committed per-story records over a legacy machine-local index), and the join +back onto the committed truth that decides whether an entry is confirmable or +drifted. """ from __future__ import annotations @@ -11,29 +12,46 @@ import json import pytest -from conftest import install_bmad_config, spec_path, write_spec, write_sprint +from conftest import git, install_bmad_config, spec_path, write_spec, write_sprint from bmad_loop import devcontract, operatoractions, verify ACTIONS = ["buy example.com at the registrar", "publish the _acme-challenge TXT record"] -def _index_only(paths, key="1-1-a", **kw): - """Index a story WITHOUT writing anything else, so a test about git's view of - the index is not confounded by the untracked spec/config a park also leaves.""" +def _record_only(paths, key="1-1-a", **kw): + """Write a park record WITHOUT the committed state it points at, so a test + about the store itself is not confounded by the spec/board a park also + leaves.""" operatoractions.record_park( paths.project, key, actions=kw.get("actions", ACTIONS), spec_file=kw.get("spec_file", "spec.md"), - commit="abcdef1234567890", run_id="run-1", parked_at="2026-07-28", ) +def _legacy_entry(paths, key="1-1-a", *, actions=None, commit="abcdef1234567890"): + """Write a pre-#356 machine-local index entry byte-shaped the way the old + version wrote it — including the stored `commit` the new records no longer + carry.""" + store = operatoractions.legacy_store_path(paths.project) + store.parent.mkdir(parents=True, exist_ok=True) + data = json.loads(store.read_text()) if store.is_file() else {} + data[key] = { + "actions": ACTIONS if actions is None else actions, + "spec_file": "spec.md", + "commit": commit, + "run_id": "run-legacy", + "parked_at": "2026-07-01", + } + store.write_text(json.dumps(data, indent=2, sort_keys=True)) + + def _park(paths, key="1-1-a", *, actions=None, status="awaiting-operator", board=None): - """Index a story and write the committed state it points at.""" + """Record a park and write the committed state it points at.""" if not (paths.project / "_bmad" / "bmm" / "config.yaml").is_file(): install_bmad_config(paths) acts = ACTIONS if actions is None else actions @@ -50,7 +68,6 @@ def _park(paths, key="1-1-a", *, actions=None, status="awaiting-operator", board key, actions=list(acts) if isinstance(acts, list) else [], spec_file=spec.relative_to(paths.project).as_posix(), - commit="abcdef1234567890", run_id="run-1", parked_at="2026-07-28", ) @@ -64,10 +81,13 @@ def test_record_and_load_round_trip(project): _park(project) data = operatoractions.load(project.project) assert list(data) == ["1-1-a"] + assert data["1-1-a"]["story_key"] == "1-1-a" assert data["1-1-a"]["actions"] == ACTIONS - assert data["1-1-a"]["commit"] == "abcdef1234567890" assert data["1-1-a"]["run_id"] == "run-1" assert data["1-1-a"]["parked_at"] == "2026-07-28" + # never stored: the record rides the very commit it would name (#356), so + # provenance is derived at read time instead + assert "commit" not in data["1-1-a"] def test_re_parking_replaces_rather_than_accumulates(project): @@ -80,20 +100,19 @@ def test_re_parking_replaces_rather_than_accumulates(project): "1-1-a", actions=["only this one now"], spec_file="spec.md", - commit="c2", run_id="run-2", parked_at="2026-07-29", ) data = operatoractions.load(project.project) assert data["1-1-a"]["actions"] == ["only this one now"] - assert data["1-1-a"]["commit"] == "c2" + assert data["1-1-a"]["run_id"] == "run-2" -def test_drop_removes_the_entry_and_reports_whether_it_was_there(project): +def test_drop_removes_the_record_and_reports_whether_it_was_there(project): _park(project) assert operatoractions.drop(project.project, "1-1-a") is True assert operatoractions.load(project.project) == {} - # a second drop is a no-op, not an error — confirming a story the index never + # a second drop is a no-op, not an error — confirming a story the store never # knew about must not create a file just to delete from it assert operatoractions.drop(project.project, "1-1-a") is False assert operatoractions.drop(project.project, "never-existed") is False @@ -107,72 +126,222 @@ def test_drop_removes_the_entry_and_reports_whether_it_was_there(project): ('"a string"', "a bare JSON scalar"), ], ) -def test_load_degrades_on_an_unusable_store(project, content, why): - """The index is a convenience over committed truth, so an unreadable one - reads as "no index" rather than blocking a human from confirming their own - work.""" - store = operatoractions.store_path(project.project) +def test_load_degrades_on_an_unusable_legacy_store(project, content, why): + """The legacy index was a convenience over committed truth, so an unreadable + one reads as "no index" rather than blocking a human from confirming their + own work.""" + store = operatoractions.legacy_store_path(project.project) store.parent.mkdir(parents=True, exist_ok=True) store.write_text(content) assert operatoractions.load(project.project) == {}, why +def test_a_mangled_record_file_is_still_listed(project): + """A record file that cannot be parsed still NAMES an obligation — its + presence in the tree is the committed claim. It degrades to an empty entry + under its filename stem, which `resolve` reports as drift ("no spec file + could be located for it"), rather than vanishing the way a mangled + legacy store does: the legacy store was one file for every story, so + degrading it wholesale lost nothing attributable; a per-story file IS the + attribution.""" + rp = operatoractions.record_path(project.project, "1-1-a") + rp.parent.mkdir(parents=True, exist_ok=True) + rp.write_text("{ not json") + assert operatoractions.load(project.project) == {"1-1-a": {}} + + def test_load_of_an_absent_store_is_empty(project): assert operatoractions.load(project.project) == {} -def test_the_store_is_written_atomically_and_parsable(project): +def test_the_record_is_written_atomically_and_parsable(project): _park(project) - raw = operatoractions.store_path(project.project).read_text() + raw = operatoractions.record_path(project.project, "1-1-a").read_text() assert json.loads(raw) # valid JSON, not a half-written temp - assert not list(operatoractions.store_path(project.project).parent.glob("*.tmp")) + assert not list(operatoractions.records_dir(project.project).glob("*.tmp")) -# --------------------------------------------------- keeping git out of it +def test_a_failed_record_write_leaves_no_tmp_residue(project, monkeypatch): + """The record is written just ahead of `finalize_commit`'s `git add -A`, so + a stranded `.tmp` would ride the story's own commit — one story's half-write + polluting its own history forever. + Ablation: drop the unlink in `record_park`'s except arm and this fails.""" -def test_writing_the_index_leaves_the_worktree_clean(project): - """The index is written from INSIDE a story's commit window. If git could see - it, the epic-boundary auto-sweep would refuse to run (it requires a clean - tree) and the next story's `git add -A` would sweep one story's bookkeeping - into another story's commit. + def boom(tmp, target): + raise OSError(28, "No space left on device") - Ablation: delete the `_exclude_from_git` call in `_write_store` and this - fails — the untracked index shows up in `git status` and the tree is dirty.""" - assert verify.worktree_clean(project.project) - _index_only(project) - assert operatoractions.store_path(project.project).is_file() # it really is on disk - assert verify.worktree_clean(project.project) + monkeypatch.setattr(operatoractions, "atomic_replace", boom) + with pytest.raises(OSError): + operatoractions.record_park( + project.project, + "1-1-a", + actions=ACTIONS, + spec_file="spec.md", + run_id="run-1", + parked_at="2026-07-28", + ) + records = operatoractions.records_dir(project.project) + assert not list(records.glob("*.tmp")) and not list(records.glob("*.json")) + + +# ------------------------------------------------------- git sees the record -def test_the_exclude_pattern_lands_in_the_repository_exclude_file(project): - _index_only(project) - exclude = (project.project / ".git" / "info" / "exclude").read_text() - assert ".bmad-loop/operator-actions.json" in exclude +def test_a_park_record_is_visible_to_git(project): + """The design premise of #356, INVERTED from the machine-local index this + store replaced: the record is written inside the story's commit window + precisely so `finalize_commit`'s `git add -A` folds it into the story's own + commit — which requires git to SEE it. An excluded record would silently + fall out of every commit and the fresh-clone acceptance would be a lie. + + Ablation: re-add an exclude-from-git call to `record_park` (the legacy + `_worktree_local_exclude` pattern) and this fails — the tree stays clean and + nothing would ever commit the record.""" + assert verify.worktree_clean(project.project) + _record_only(project) + assert operatoractions.record_path(project.project, "1-1-a").is_file() + assert not verify.worktree_clean(project.project) -def test_the_exclude_is_not_appended_twice(project): - _index_only(project) - _index_only(project, "1-2-b") - exclude = (project.project / ".git" / "info" / "exclude").read_text() - assert exclude.count(".bmad-loop/operator-actions.json") == 1 +def test_a_record_does_not_touch_the_repository_exclude_file(project): + """The legacy store registered itself in `.git/info/exclude` at every write. + The committed record must not: an exclude line would hide it from the very + `git add -A` it exists to ride.""" + _record_only(project) + exclude = project.project / ".git" / "info" / "exclude" + if exclude.is_file(): + assert ".bmad-loop/operator" not in exclude.read_text() -def test_the_store_write_survives_a_non_git_project(tmp_path): - """Best-effort by construction: an index that could not be excluded is still - a correct index, so a project with no git at all must not raise.""" +def test_the_record_write_survives_a_non_git_project(tmp_path): + """A project with no git at all must not raise: the record is still a + correct record, and `confirm` still works from the files alone.""" operatoractions.record_park( tmp_path, "1-1-a", actions=["do the thing"], spec_file="spec.md", - commit="", run_id="r", parked_at="2026-07-28", ) assert operatoractions.load(tmp_path)["1-1-a"]["actions"] == ["do the thing"] +# ------------------------------------------- the legacy machine-local index + + +def test_load_merges_legacy_entries_under_per_story_records(project): + """A park written by a pre-#356 version must stay confirmable on the machine + that wrote it, so `load` still reads the legacy store — but a per-story + record wins its key: the record travels with the commit, the legacy entry is + a machine-local cache.""" + _legacy_entry(project, "1-1-a", actions=["the stale legacy copy"]) + _legacy_entry(project, "2-2-b") + _record_only(project, "1-1-a") + data = operatoractions.load(project.project) + assert data["1-1-a"]["actions"] == ACTIONS # the record won + assert data["2-2-b"]["actions"] == ACTIONS # legacy-only key still listed + assert data["2-2-b"]["commit"] == "abcdef1234567890" + + +def test_drop_prunes_the_legacy_entry_too(project): + """Confirming a story must remove it EVERYWHERE it is listed, or `validate` + warns forever about a story that was genuinely confirmed. The emptied legacy + store is deleted outright rather than rewritten as `{}` — nothing writes it + anymore, and an empty husk would linger as bookkeeping nobody owns.""" + _legacy_entry(project, "1-1-a") + _legacy_entry(project, "2-2-b") + _record_only(project, "1-1-a") + assert operatoractions.drop(project.project, "1-1-a") is True + assert "1-1-a" not in operatoractions.load(project.project) + assert operatoractions.legacy_store_path(project.project).is_file() # 2-2-b remains + assert operatoractions.drop(project.project, "2-2-b") is True + assert not operatoractions.legacy_store_path(project.project).is_file() + + +def test_a_failed_legacy_prune_leaves_no_tmp_residue(project, monkeypatch): + """`drop` runs out of band from `confirm`, not inside a commit window, so a + stranded `.bmad-loop/operator-actions.tmp` does not ride a commit — it sits + UNTRACKED (`.bmad-loop/` is not ignored, and the pre-#356 exclude line named + the `.json` only), which `verify.worktree_clean` reads as a dirty tree that + blocks the next run's preflight and the epic-boundary auto-sweep. + + TWO entries, deliberately: with one, the prune takes the `else` arm and + unlinks the store outright, never reaching `atomic_replace`. That is what + the `pytest.raises` is for — it fails LOUDLY on `DID NOT RAISE` rather than + letting the residue assertion pass over a rewrite that never ran. + + Ablation: drop the unlink in `_drop_legacy`'s except arm and this fails.""" + + def boom(tmp, target): + raise OSError(28, "No space left on device") + + _legacy_entry(project, "1-1-a") + _legacy_entry(project, "2-2-b") + store = operatoractions.legacy_store_path(project.project) + before = sorted(p.name for p in store.parent.iterdir()) + assert before == ["operator-actions.json"] # a snapshot with something IN it + + monkeypatch.setattr(operatoractions, "atomic_replace", boom) + with pytest.raises(OSError): + operatoractions.drop(project.project, "1-1-a") + + # Equality against that non-empty snapshot, not `not glob("*.tmp")`: the + # bare negative would pass just as happily on a `.bmad-loop/` that was never + # created. The failed rewrite also left the store itself as it found it, so + # the entry stays confirmable once whatever broke the write is fixed. + assert sorted(p.name for p in store.parent.iterdir()) == before + assert sorted(operatoractions.load(project.project)) == ["1-1-a", "2-2-b"] + + +def test_drop_matches_the_records_own_key_not_its_filename(project): + """The record's `story_key` field is authoritative over the filename stem + (`safe_segment` can mangle a hostile key, a human can rename a file). A drop + keyed only on the computed path would leave a renamed record behind — + resolvable, confirmable, and then impossible to remove.""" + rp = operatoractions.record_path(project.project, "1-1-a") + _record_only(project) + rp.rename(rp.with_name("renamed-by-hand.json")) + assert operatoractions.drop(project.project, "1-1-a") is True + assert operatoractions.load(project.project) == {} + + +# ------------------------------------------------------- derived provenance + + +def test_resolve_derives_the_commit_from_the_record_history(project): + """The record rides the very commit it would name, so it cannot store the + sha — `resolve` derives it from `git log` over the record's own path. + + Ablation: make `_derive_commit` return "" unconditionally and this fails.""" + _park(project) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "auto: 1-1-a (awaiting operator)") + (story,) = operatoractions.resolve(project.project, project) + assert story.commit == git(project.project, "rev-parse", "HEAD") + + +def test_an_uncommitted_record_derives_an_empty_commit(project): + """A record not yet in any commit (NO_VCS park, or read between write and + commit) has no provenance to show — and that must not gate anything: + `commit` is display-only, never a predicate input.""" + _park(project) + (story,) = operatoractions.resolve(project.project, project) + assert story.commit == "" + assert story.confirmable, story.drift() + + +def test_a_legacy_entry_keeps_its_stored_commit(project): + """A legacy entry stored its sha at park time (it was written AFTER the + commit, outside it); derivation is only for records that could not.""" + install_bmad_config(project) + write_sprint(project, {"1-1-a": "awaiting-operator"}) + _legacy_entry(project, "1-1-a") + (story,) = operatoractions.resolve(project.project, project) + assert story.commit == "abcdef1234567890" + + # ------------------------------------------------- reading actions back @@ -189,26 +358,26 @@ def test_the_store_write_survives_a_non_git_project(tmp_path): ], ) def test_actions_of_shares_the_frontmatter_reading(stored, expected, why): - """The index is written from a spec and compared back against one, so a shape - that collapses to () on the spec side must collapse to () here too.""" + """The record is written from a spec and compared back against one, so a + shape that collapses to () on the spec side must collapse to () here too.""" assert operatoractions.actions_of({"actions": stored}) == expected, why -# --------------------------------------------- joining the index to truth +# --------------------------------------------- joining the records to truth -def test_resolve_joins_the_index_to_spec_and_board(project): +def test_resolve_joins_the_record_to_spec_and_board(project): _park(project) (story,) = operatoractions.resolve(project.project, project) assert story.story_key == "1-1-a" assert story.actions == tuple(ACTIONS) assert story.spec_status == "awaiting-operator" assert story.board_status == "awaiting-operator" - assert story.commit == "abcdef1234567890" + assert story.run_id == "run-1" assert story.confirmable and story.drift() is None -def test_resolve_prefers_the_specs_actions_over_the_indexed_copy(project): +def test_resolve_prefers_the_specs_actions_over_the_recorded_copy(project): """The spec is the committed truth. A spec edited after the park must show the human what they owe NOW, not what the run recorded then.""" spec = _park(project) @@ -217,7 +386,7 @@ def test_resolve_prefers_the_specs_actions_over_the_indexed_copy(project): assert story.actions == ("the revised action",) -def test_resolve_falls_back_to_the_indexed_actions_when_the_spec_has_none(project): +def test_resolve_falls_back_to_the_recorded_actions_when_the_spec_has_none(project): """A spec whose declaration was lost still shows what the run recorded — the fallback is what keeps a human from being told they owe nothing.""" spec = _park(project) @@ -245,7 +414,7 @@ def test_resolve_is_sorted_by_story_key(project): ], ) def test_drift_names_the_side_that_disagrees(project, spec_status, board, expect_drift): - """ "The index says parked, the board says done" is a different message and a + """ "The record says parked, the board says done" is a different message and a different remedy from "no such story" — so both sides are reported, and the spec is reported first because a spec that moved on explains everything.""" _park(project, status=spec_status, board=board) @@ -271,13 +440,26 @@ def test_drift_reports_an_entry_with_no_spec_path(project): "1-1-a", actions=ACTIONS, spec_file="", - commit="c", run_id="r", parked_at="2026-07-28", ) (story,) = operatoractions.resolve(project.project, project) assert story.spec_path is None - assert story.drift() == "the index records no spec file for it" + assert story.drift() == "no spec file could be located for it" + + +def test_a_mangled_record_resolves_as_drift_not_absence(project): + """The other half of the load test above: the empty entry a mangled record + degrades to must come out of `resolve` as a reportable drift, so `validate` + and `confirm --list` name the file a human has to look at.""" + install_bmad_config(project) + write_sprint(project, {"1-1-a": "awaiting-operator"}) + rp = operatoractions.record_path(project.project, "1-1-a") + rp.parent.mkdir(parents=True, exist_ok=True) + rp.write_text("{ not json") + (story,) = operatoractions.resolve(project.project, project) + assert story.drift() == "no spec file could be located for it" + assert not story.confirmable def test_drift_reports_a_story_absent_from_the_board(project): @@ -308,12 +490,12 @@ def test_resolve_degrades_on_an_unreadable_board(project): assert not story.confirmable -def test_resolve_tolerates_a_non_dict_entry(project): - """A hand-mangled index must degrade, not raise, on the way to the warning - that reports it.""" +def test_resolve_tolerates_a_non_dict_legacy_entry(project): + """A hand-mangled legacy store must degrade, not raise, on the way to the + warning that reports it.""" install_bmad_config(project) write_sprint(project, {"1-1-a": "awaiting-operator"}) - store = operatoractions.store_path(project.project) + store = operatoractions.legacy_store_path(project.project) store.parent.mkdir(parents=True, exist_ok=True) store.write_text(json.dumps({"1-1-a": "not a record"})) (story,) = operatoractions.resolve(project.project, project) @@ -331,7 +513,7 @@ def test_verify_exports_the_same_token(project): def _interrupt(project, key="1-1-a", *, board="awaiting-operator"): """Leave on disk exactly what `confirm` leaves when it dies between its spec writes and its board write: the audit section appended, the spec at `done`, - the board where the park left it, and the index entry still there.""" + the board where the park left it, and the record still there.""" spec = _park(project, key, status="done", board=board) devcontract.append_operator_confirmation(spec, ACTIONS, date="2026-07-28") return spec @@ -351,7 +533,7 @@ def test_resolve_reads_back_whether_the_confirmation_section_is_there(project): def test_an_interrupted_confirmation_is_resumable(project): - """Spec signed off and at `done`, board still parked, entry still indexed: + """Spec signed off and at `done`, board still parked, record still there: the state `confirm` leaves when `sprintstatus.advance` raises or returns unchanged. `drift()` calls it stale, so without this predicate a re-run refuses the very state a re-run exists to clear.""" @@ -408,7 +590,7 @@ def test_a_fenced_confirmation_heading_does_not_make_a_story_resumable(project): assert story.resumable is False -# ------------------------------------ committed drift, split from the index's own +# ------------------------------------ committed drift, split from the record's own def test_committed_drift_ignores_an_unreadable_action_list(project): @@ -423,7 +605,7 @@ def test_committed_drift_ignores_an_unreadable_action_list(project): def test_committed_drift_is_none_for_a_park_owing_nothing_readable(project): """The one input where the two answers differ: the committed state still - describes a park, and only the index's own list is unreadable.""" + describes a park, and only the record's own list is unreadable.""" _park(project, actions=[]) (story,) = operatoractions.resolve(project.project, project) assert story.committed_drift() is None