diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f640528..3a90c99d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,21 @@ breaking changes may land in a minor release. ### Added +- **Journal-kind and refusal-site coverage gates.** `tests/test_portability_guard.py` gains + three enumerate-vs-declare inventories: the 204 literal journal kinds (`JOURNAL_KINDS`, + fed by a literal-kind emit that also sees kind-only writes, constructor-inline + `Journal(run_dir).append(...)` writes — a receiver spelling the journal scan was blind + to — and every literal reaching a declared dynamic-kind position, by keyword, by + POSITION, or as the parameter default; a `kind` argument such a position cannot read — + a variable, or a `*args` splat over the slot — fails loud rather than passing as "no + literal here", which is the one way an undeclared kind could still reach the journal + with the inventory green), the `_refuse_*`/`_reject_*` helper definitions counted with + multiplicity (`REFUSAL_HELPER_DEFS`), and the eleven #414-family isolation-refusal call + sites counted with multiplicity (`ISOLATION_CONFLICT_CALLERS`). A new kind, refusal + helper, or refusal call site reddens CI until its row lands; the row is the PR-time + decision, and the failure message demands the covering test land beside it. Each new + detector arm carries must-flag and must-stay-silent probe rows. + - **Interactive resolve context names both the BMAD project root and the run's code root.** `bmad-loop resolve` warns before a divergent-root session launches, keeps the session project-rooted, and directs code fixes and commits to the code root. The @@ -51,8 +66,13 @@ breaking changes may land in a minor release. - **`repo_root` in run `state.json`** (#716). A run records the git root its code work happens in, so an out-of-process reader — `bmad-loop resolve`'s re-arm — uses the tree the run measured - instead of re-deriving one. A `state.json` written before the field existed degrades to the - project directory, which is the pre-upgrade behavior. + instead of re-deriving one. `resume` re-stamps that mirror against the `repo_root` it re-reads + from `_bmad/bmm/config.yaml`, so an edit made while the run was paused cannot leave the engine + working in one tree while the out-of-process re-arm advances the attempt baseline in the other, + with no error on either side; a move is announced rather than silent, since the baselines, + preserve refs and branches already recorded name objects in the previous tree. A `state.json` + written before the field existed degrades to the project directory — the pre-upgrade behavior — + and migrates without being reported as a move. - **Atomic writers gain an opt-in `require_writable_target` refusal** (#597). Callers over operator-curated files can ask for the `PermissionError` a plain `Path.write_text` used to @@ -141,8 +161,12 @@ breaking changes may land in a minor release. - **`bmad-loop diagnose` routes the re-arm records by field name** (#640, #716). `spec_file` and `overwritten` are aliased, and `repo` is dropped — an absolute host path that correlates nothing. - Routing is by field name across every entry rather than by kind, so no existing run's dump changes - shape and `SCHEMA_VERSION` is unaffected. + All three arrive only on the re-arm kinds, so no run predating them renders differently on their + account. By-name routing is the default rather than the whole rule: a narrow kind-scoped table is + consulted FIRST, for the fields whose meaning depends on the kind carrying them, and that table + DOES change pre-existing dumps — `target` is aliased on `unit-merge-started`, `unit-merged` and + `resume-unit-merge` (see `### Security`) while the `board-advance-*` family keeps rendering it + verbatim, and `sentinel` is aliased on `sentinel-cleared`. `SCHEMA_VERSION` is unaffected. - **`bmad-loop diagnose`'s default report shows the split code root and the task generation** (#705, #716). Both fields reached `--json` but not the markdown renderer, which samples its fields @@ -156,12 +180,18 @@ breaking changes may land in a minor release. advanced the attempt baseline in the tree the run had left, while the engine that resumed measured in the new one, with no error anywhere. Both now re-stamp through one shared writer, after the confirm — so a cancelled resolve still leaves the divergence for `resume` to report — and each - warns that the run has changed repositories. A config this process cannot read degrades to the - root the run recorded, and says so. The #414 isolation refusal is hoisted alongside it, ahead of + warns that the run has changed repositories, and journals `rearm-code-root-restamped` so the + move leaves a durable record: this re-stamp is what makes resume's own `code_root_changed` + read `false` later in the same gesture, and a stderr line or a TUI toast is not a record. A + config this process cannot read degrades to the root the run recorded, and says so. The #414 isolation refusal is hoisted alongside it, ahead of both writes: under `isolation = "worktree"` beside a `repo_root` override, both surfaces used to re-stamp, advance the attempt baseline and report "re-armed" before resume refused the configuration — spending an escalation `resolve` could no longer re-run, since the story was no - longer escalated. + longer escalated. `resolve` refuses it a second time BEFORE the interactive session, ahead of + the adapter build: the pair is knowable from config, so the late refusal alone let an operator + converse with a full agent and answer the re-arm prompt only to be handed rc 1 for it. The + post-confirm refusal stays the authority — it re-reads the config after a conversation of + unbounded length, and is the only one `--no-interactive` reaches. - **Re-arm refuses a story spec it cannot re-open, instead of re-driving onto a status the session cannot route** (#640). A spec carrying no top-level `status:` failed the flip silently: the @@ -197,7 +227,12 @@ breaking changes may land in a minor release. HOLDS the resume both surfaces fold in behind the re-arm, since its advice is unactionable once the run has resumed, and `--resume` does not override the hold — the re-arm stands, and `bmad-loop resume ` picks the story up once the - fix is committed. + fix is committed. A spec in an artifact directory configured outside the project is exempt + entirely: `ProjectPaths.rebased` leaves such a directory where it is, shared across checkouts + rather than rebased onto each worktree, so the flip lands on the one file every re-drive reads and + there is nothing to commit — a remedy naming a file outside the repository. Containment is decided + on the canonical paths, so a spec spelled out of but resolving back into the worktree still warns, + as does one the host cannot canonicalize. - **The re-arm baseline records reach the TUI operator too** (#640). Each surface carried its own copy of the journal-kind → message routing and they had drifted: the TUI printed only @@ -278,6 +313,66 @@ breaking changes may land in a minor release. unrecoverably, on a gitignored ledger. The mark and append legs now re-anchor only when the preimage they wrote over is still the bytes the run last claimed; the restore skips and journals `ledger-restore-skipped-diverged`. +- **Re-arm hold remedies name the status the re-drive routes on.** All four holding + remedies — correct the spec in the main checkout, commit it on the pinned branch, + restore it at the recorded path, commit the upstream `SPEC.md` / `stories.yaml` — now + name the `status:` the corrected file must carry, so a correction that lands + byte-correct but still terminal no longer burns the escalation the hold just bought. + The restore remedy also stops claiming the re-drive "will see the escalated attempt's + status": that arm fires only when the path holds no readable file, so the re-drive + finds no spec there at all. + +- **The TUI's re-arm hold now names the remedy of the record that actually held.** Its + hold branch printed one hardcoded "commit the corrected spec" for all four holding + records, and two of them cannot be obeyed that way: `rearm-spec-write-unreachable`'s + in-place arm needs the edit made in the MAIN checkout, and `rearm-spec-flip-skipped`'s + holding arm fires only where the recorded spec path is not a readable file — nothing to + commit, and possibly a shared artifact directory outside any repository. + `runs.RearmOutcome` gains `hold_next_step`, captured first-wins from the earliest + holding record so the operator is told the cause they must clear first, and the toast + composes it as "not resuming in this gesture. `` — the run stays paused and + resumable from this screen." The advisory toasts still drop `next_step`; only the hold + line carries it. + +- **A failed `rearm-code-root-restamped` append no longer loses the move record for + good.** `restamp_code_root` committed the new root and journalled afterwards, so an + append that failed once left a retry exiting at "already agrees" with the record never + written and the later `run-resume` line reporting `code_root_changed=False`. The move + now lands with an intent marker (`RunState.code_root_restamp_pending`) in one atomic + state write, the record follows it, and the marker is cleared only once the record is + down — so a retry writes the record the move still owes, and a record can never claim + a move the state write did not make. A plain `resume` consumes the outstanding record + too: it counts the marker as a move, so its `run-resume` line and the code-root warning + fire even though the re-stamped mirror already agrees with config, and clears the + marker on the same write that persists the resume. + +- **An owed code-root re-stamp record survives a second move of the root.** The pending + marker is a bare flag, so the root a failed record was owed for is described only by + the run's own `repo_root`. An operator who re-pointed the root again — restoring the + original or moving to a third tree — before retrying had that value overwritten, and + the earlier move's record could never be written. The retry now discharges the owed + record under the root the marker still names before taking the new one, so every move + keeps its own journal row. + +- **The journal-kind inventory no longer misses a kind passed POSITIONALLY to a declared + dynamic-kind position.** The scan read `node.keywords` only, so + `_close_bundle_ledger_when_spec_status(task, spec, status, "new-kind")` — legal Python, + since `kind` is positional-or-keyword — reached the journal with no `JOURNAL_KINDS` row + anyone had to decide on, while the guard reported itself complete. All three ways a literal + reaches such a position (keyword, positional, parameter default) now feed the same emit, + with the slot resolved from the declared function's own signature and the bound receiver + dropped. A `*args` splat covering the slot yields a kind no row can declare, so an + unreadable position reddens the inventory naming its site instead of passing in silence; a + declared forwarder is exempt, its kind already read by the journal-write emit. + +- Take the re-arm's mount claim and its resume hold from the `rearm-spec-flip-skipped` + record instead of inferring both from `reaches_redrive`. The record now carries the live + `redrive` mode, so the notice no longer tells an ISOLATED run that it "mounts no worktree" — + that arm reaches the re-drive through a spec shared across checkouts, with a worktree very + much mounted. A record written before the field drops the mode clause rather than guessing. + The same leg now HOLDS the resume (`reaches_redrive` and not `refused`), so its "check the + recorded spec path before resuming" imperative stops rendering on the two surfaces that + re-arm and resume in one gesture. - **`bmad-loop clean` no longer aborts the whole sweep when one run's state lock is held.** A busy run used to end the invocation, so later candidates went unprocessed and runs already reclaimed vanished from a report only emitted after the loop. `clean` now @@ -490,7 +585,10 @@ add` failed on the held branch anyway, which is the harm the guard exists to pre undoing the spec beneath it mirrors the same defect. An ORDINARY failure of the abort record's OWN journal write is suppressed whatever its type, so an observation that cannot be made never replaces the fault the operator is being told about — an interrupt still - leaves, since by then the rollback has already run and the operator asked to stop. + leaves, since by then the rollback has already run and the operator asked to stop. That + `finally` is also what makes the residue echo unconditional: the residue is journalled before + the re-stamp that can raise, so an abort would otherwise drop the notices for records already + on disk — the commits warning among them. - Stop an LLM-authored preference escalation from aborting the review leg. `_review_and_commit` splats a review session's own `result.json` escalation entries into `journal.append`, so a @@ -633,26 +731,6 @@ argument` and failed the story; a `ts` key did not raise and instead silently re then mounted a fresh worktree cut from git that could not see it. A change across the session is now reported on stderr. -- **`resume` re-stamps the run's recorded code root** (#716). Resume arms the engine against the - `repo_root` it re-reads from `_bmad/bmm/config.yaml` but left the `state.json` copy at its launch - value, so after an edit the engine worked in one tree while the out-of-process re-arm advanced the - attempt baseline in the other, with no error on either side. The mirror now follows the paths - resume adopts, and a move is announced rather than silent — the baselines, preserve refs and - branches already recorded name objects in the previous tree. A `state.json` from before the field - existed migrates without being reported as a move. - -- **The unreachable-spec-write warning no longer fires on a shared artifact directory** (#640). An - artifact directory configured outside the project is left where it is by `ProjectPaths.rebased`, - shared across checkouts instead of rebased onto each worktree — so the flip lands on the one file - every re-drive reads and there was never anything to commit, yet that layout took the warning on - every re-arm with a remedy naming a file outside the repository. Containment is decided on the - canonical paths, so a spec spelled out of but resolving back into the worktree still warns, as - does one the host cannot canonicalize. - -- **`bmad-loop resolve` still reports abandoned-restore residue when the re-arm aborts** (#640). - The residue is journalled before the re-stamp that can raise, so an abort discarded records - already written — including the commits warning. The echo now runs on both paths. - - **A YAML boolean in a spec's baseline key no longer refuses the attempt** (#716). `no`, `off`, `yes` and `on` parse as booleans, and the shared reader stringified them into `"False"`/`"True"` — non-empty, so they were judged as a claimed sha and outranked a `baseline_commit` naming the @@ -920,6 +998,10 @@ decisions` and the TUI decision modal now also catch the state-root failure that ### Security - **`bmad-loop diagnose` no longer ships a merge record's target branch verbatim** (#640). + The leak PRE-DATES the re-arm work this section is otherwise about: all three producers + and the by-name routing shipped in earlier releases, so any dump of a run that merged a + unit is affected, and the `#640` citation names the work that happened to find it rather + than an issue that reported it. The journal's `target` field carries a branch on `unit-merge-started`, `unit-merged` and `resume-unit-merge` but a sprint status on the `board-advance-*` family, and per-field routing is by field NAME — so the field was left unrouted and an identifier-shaped branch diff --git a/README.md b/README.md index 9a3c86fe..efbdb735 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ bmad-loop tui # …or drive everything from the dashboard | `bmad-loop run` | Drive the dev → review → verify → commit loop. `--epic N`, `--story KEY`, `--max-stories N`, `--dry-run`. `--spec ` forces **stories mode** (folder+id dispatch off `/stories.yaml`), overriding `[stories].source`; `--story` then filters by story id. | | `bmad-loop sweep` | Triage + execute open `deferred-work.md` entries. `--no-prompt`, `--decisions-only`, `--max-bundles N`, `--repeat`, `--max-cycles N`, `--dry-run`. `--archive [--before DATE]` instead moves closed ledger entries to `deferred-work-archive.md`, leaving id-preserving stubs. | | `bmad-loop resume ` | Continue a run paused at a gate, escalation, or interruption. The resume command rendezvouses with delete/archive on the run lifecycle lock; if cleanup removed the run while resume waited, resume reports it missing without recreating files or launching an engine. | -| `bmad-loop resolve ` | Resolve a CRITICAL escalation: open an interactive resolve agent to fix the frozen spec, then re-arm the story and resume. On an _intent gap_ the re-drive can resume review on the attempted change instead of re-implementing it. `--story KEY`, `--no-interactive`, `--restore-patch ` (intent-gap patch-restore), `--resume` / `--no-resume`, `--force` (proceed when engine liveness is unverifiable; a provably-live engine still blocks). | +| `bmad-loop resolve ` | Resolve a CRITICAL escalation: open an interactive resolve agent to fix the frozen spec, then re-arm the story and resume — the resume is held when the correction provably has not reached the tree the re-drive reads (see [Resolving a CRITICAL escalation](#how-a-story-flows)). On an _intent gap_ the re-drive can resume review on the attempted change instead of re-implementing it. `--story KEY`, `--no-interactive`, `--restore-patch ` (intent-gap patch-restore), `--resume` / `--no-resume`, `--force` (proceed when engine liveness is unverifiable; a provably-live engine still blocks). | | `bmad-loop decisions` | Answer deferred-work decisions earlier sweeps left unanswered (skipped by `--no-prompt`, or an abandoned interactive sweep). Recorded so the next sweep acts on them without re-asking. `--list` shows them without answering; `--json` emits them as a stable machine-readable document — id, question, context, recommendation, and every option's key/label/effect/intent/resolution/bundle-name with a derived `recommended` flag. It implies the listing and never prompts, so a script can select an option by policy instead of scraping the text. | | `bmad-loop confirm ` | Complete a story parked at `awaiting-operator` once you have carried out the external actions it owes (buy the domain, publish the DNS record). Acknowledges each action in turn, writes the spec's `## Operator Confirmation` audit section, advances spec and board to `done`, and commits the pair — nothing is re-driven. `--list` shows every parked story and what it owes; `--yes` skips the prompts; `--reverify` re-runs the project's `[verify]` commands first and blocks the confirmation if they fail; `--json` emits the parked set as a stable machine-readable document. Every write is checked and the spec is read back from disk, so a story is never declared done over a write that did not land; a confirmation interrupted before its board write is **finished** by re-running the command, with no second prompt and no second audit section. The index it reads is machine-local, so a park is confirmed on the machine that ran it. | | `bmad-loop list` (`ls`) | List every run/sweep with its short ref, type, and status — the handle you pass to the commands below. `--json` emits a stable machine-readable document instead — one entry per run, oldest first (short ref, run id, type, started-at, liveness-aware status, paused stage); an empty runs dir yields a valid empty document. | @@ -247,6 +247,8 @@ sprint-status.yaml: 1-2-account-mgmt: ready-for-dev **Resolving a CRITICAL escalation:** the escalated story is parked in a terminal `escalated` phase — `resume` skips it. To un-stick it, run `bmad-loop resolve ` (or press `R` in the TUI). That opens an interactive **resolve agent** seeded with the escalation and the frozen spec; you converse with it to disambiguate the spec, it records the resolution, and on your confirmation the orchestrator re-arms the story (`escalated → pending`, spec status reset to `ready-for-dev`) and resumes — a clean rebuild against the corrected spec, then on through the rest of the sprint. Already fixed the spec yourself? `bmad-loop resolve --no-interactive` skips straight to re-arm + resume. +**When the resume is held.** Under `[scm] isolation = "worktree"` the re-drive mounts a fresh worktree cut from the committed target branch, so a correction living only in your working tree never reaches it. Where the re-arm can _prove_ that — the committed spec does not carry the status the re-drive routes on, or, for a pre-planning sentinel, the ref it mounts from does not hold this checkout's `SPEC.md` / `stories.yaml` — the re-arm still stands but the resume stops there, `--resume` notwithstanding, and both surfaces name the branch to commit on. Commit the correction, then `bmad-loop resume `. Every other re-arm warning stays advisory and resumes in the one gesture as before. + **Intent-gap patch-restore.** When review halted on an **intent gap** — the implementation was sound but read the spec differently than intended — `bmad-build-auto` saves the attempted change as a patch before reverting ([BMAD-METHOD#2564](https://github.com/bmad-code-org/BMAD-METHOD/issues/2564)). If the attempted reading was in fact correct, `resolve` re-arms the spec to `in-review` and re-applies that patch onto baseline after every reset, so the re-driven session resumes **review** on the restored diff instead of re-implementing from scratch. The interactive agent supplies the patch automatically via `resolution.json`; on the hand-driven path pass `bmad-loop resolve --no-interactive --restore-patch `. A patch that fails to apply escalates rather than running on a half-restored tree, and deferred-work `sweep` bundles get the same recovery. ## Deferred-work sweeps @@ -525,7 +527,7 @@ Merge-back is always **serialized** — `max_parallel` is a validated knob clamp The settings editor with the [scm] section expanded: isolation, branch_per, merge_strategy, the seed-adapter-configs switch, and the extra-worktree-seed-files field.

-For a monorepo or any layout where the git root differs from the project dir, set an optional `repo_root` key in `_bmad/bmm/config.yaml` — it decouples where git/code work happens from where run state lives (defaults to the project dir). Your `[verify].commands` run there too — the code's root, not the BMAD project dir — while the orchestrator's own artifact reads stay project-rooted. It is **not compatible with `isolation = "worktree"`**: provisioning seeds a worktree from `repo_root` while the preflight probes `project`, so `validate` reports the pair and `run`/`sweep`/`resume` refuse to start. Use one or the other — plumbing both through provisioning is tracked as #443. +For a monorepo or any layout where the git root differs from the project dir, set an optional `repo_root` key in `_bmad/bmm/config.yaml` — it decouples where git/code work happens from where run state lives (defaults to the project dir). Your `[verify].commands` run there too — the code's root, not the BMAD project dir — while the orchestrator's own artifact reads stay project-rooted. It is **not compatible with `isolation = "worktree"`**: provisioning seeds a worktree from `repo_root` while the preflight probes `project`, so `validate` reports the pair and `run`, `sweep`, `resume` and `resolve` refuse to start — `resolve` before it opens the interactive agent, since its re-arm advances the attempt baseline against that same root. Use one or the other — plumbing both through provisioning is tracked as #443. ### Plugins diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 168bb315..b3734bda 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -113,11 +113,19 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w correction is already committed says nothing, and neither does one whose spec sits in an artifact directory configured outside the project: those are shared across checkouts rather than rebased onto each worktree, so the flip lands on the one file every re-drive reads. That record also HOLDS - the resume: both surfaces re-arm and resume in one gesture, which made its own "commit the - corrected spec before resuming" advice unactionable the moment it printed and then burned the - escalation on a session that halts blocked. They now stop after the re-arm — the story stays armed, - `bmad-loop resume ` picks it up once the fix is committed, and `--resume` does not override - it, since the record is written on proof rather than suspicion. The advisory warnings do not hold. + the resume: both surfaces re-arm and resume in one gesture, which made its own "before resuming" + advice unactionable the moment it printed and then burned the escalation on a session that halts + blocked. They now stop after the re-arm — the story stays armed, `bmad-loop resume ` picks + it up once the correction lands, and `--resume` does not override it, since the record is written + on proof rather than suspicion. Each surface names the HELD record's own remedy rather than one + hardcoded literal, because the holding records do not share one. Each remedy also names the + `status:` the re-drive routes on, so a correction that lands byte-correct but still terminal cannot + burn the escalation a second time — the sentinel remedy below renders that clause on the same + contract, and empty, since its leg has no spec status at all. This one asks you to commit, while + its in-place arm — isolation edited to `none` while the story was escalated, so the re-arm's writes + went into the escalated attempt's worktree and the re-drive now reads the main checkout — asks you + to correct the spec in the MAIN checkout, where committing is not the remedy and naming a branch + would be actively wrong. The advisory warnings do not hold. A pre-planning **sentinel** gets the same treatment on its own artifacts. It is cleared by deletion rather than a status flip, so there is no spec write to measure — but the correction that stops it recurring is upstream (`SPEC.md` / `stories.yaml`, where the resolve skill sends the agent @@ -127,6 +135,16 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w the re-drive mounts from does not already hold this checkout's copy of those two files, so a correction already committed there resumes in one gesture, and an in-place re-drive never records at all — it reads the main checkout, which is where the resolve session runs. + A **failed status flip** holds too, on exactly one of its three arms, which is why that one is + keyed on the record's flags rather than on its kind. `rearm-spec-flip-skipped` is journalled with + `refused = spec_path.is_file() and write_reaches_the_redrive`, so the arm that reaches the re-drive + without being refused proves the recorded spec path is not a readable file here — the re-drive + reads that same path and finds no spec there to route on, futile on the same proof as the two + above, and it reaches this arm from BOTH re-drive modes. Its remedy is to restore the recorded + path carrying the status the re-drive routes on, never to commit: the arm's own condition means + there is no corrected spec at that path to commit, and the path can be a shared artifact directory + outside the project that is not a repository at all. The refused arm raises rather than resuming, so there is no gesture left to hold, and the + remaining arm carries no imperative of its own — `rearm-spec-write-unreachable` holds that leg. The whole re-arm is one **transaction**, and what it covers is stated narrowly: the SPEC's BYTES, from the first spec write to `save_state`. That save is the commit point — until it returns the run still calls the story escalated, so any fault escaping the window in between diff --git a/docs/testing.md b/docs/testing.md index 511828ed..e791f0ff 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -158,18 +158,19 @@ test. A slice of the suite tests the **repo** rather than the product. The inventory: -| Guard | Where | Enforces | -| --------------------- | ------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Portability guard | `tests/test_portability_guard.py` | One shared AST scan over every `src/bmad_loop/**/*.py` (data scripts included), carrying thirteen guards: literal `["tmux", ...]` argvs only in the two backend files (the backends' own `[self._BINARY, ...]` spelling is deliberately unmatched, so this tripwire currently flags nothing — #549); sequence-form git argvs (list or tuple, literal or named constant) only as `_run_git`'s argv argument in `verify.py`, with string-form git spawns refused everywhere, `verify.py` included; no bare `/tmp`-class POSIX paths; no `signal.SIGKILL` attribute; `os.kill(pid, 0)` probes only in `process_host.py`; any `os.kill` at all only there too (a second, distinct guard); `start_new_session` only in the detach helpers; `shell=True` only in its two sanctioned files; `BMAD_LOOP_*` env reads only through the `envvars.py` registry, a plugin's own variable family, or the session-protocol vars the two stand-alone hook relays read back; a persisted `spec_file` / `dispatched_spec_file` resolved with a bare `Path(...)` only in the four files that run inside the tree the value was recorded against; `verify_commands_outcome` called only from `verify._verify_review_commands`; its classifier half `verify_command_results_outcome` called only from `verify.verify_commands_outcome` or `Engine._verify_commands_with_results` (a separate guard, because fencing the wrapper alone still lets a gate compose run+classify by hand and pick its own root — #695); plus a scanned-file-count floor so a broken scan root cannot pass vacuously | -| Settings-schema sync | `tests/test_settings_schema.py` | `src/bmad_loop/data/settings/core.toml` stays in lockstep with `policy.py` by reflection, in both directions: every spec maps to a live dataclass field with a matching default wherever one is baked in, every policy field is reachable from exactly one spec (or listed in the explicit `HIDDEN` set), and every `*Policy` dataclass is consciously classified | -| Exit-code allocation | `tests/test_entry_point.py` | `ExitCode` is pinned literally (OK=0, FAILURE=1, USAGE=2, INTERRUPTED=130) **and closed**: the enum's value set equals exactly those four, so codes 3–129/131+ cannot be allocated quietly | -| Extra-less core CLI | `tests/test_entry_point.py` | A fresh interpreter with `pyte`/`rich`/`textual`/`tomlkit` blocked at `find_spec` — the blocker **raises** rather than returning None, so the dev venv's installed copies cannot make it pass vacuously, and an `import pyte` floor proves it bites — imports `bmad_loop.cli` and `bmad_loop.settings_schema`, runs `list` to rc 0, and asserts `tui` degrades to the `bmad-loop[tui]` hint instead of a traceback. Every test job installs `--all-extras`, which is why #650 shipped broken for 23 releases; CI's isolated wheel `list` run is the same floor at install level | -| State-machine table | `tests/test_statemachine.py` | Every `Phase` has a transition row; `TERMINAL_PHASES` (model.py) equals the table's dead ends — a cross-module parity nothing else links; an N×N `parametrize` grid drives every pair (legal pairs land, illegal pairs raise and leave the phase untouched); the awaiting-operator reachability rule is additionally stated independently, because the N² grid reads its expectation out of the table under test | -| Check-id registry | `checks.py` + `tests/test_cli.py` | `ValidationReport.add` asserts its id is in `VALIDATE_CHECKS` at every **executed** call site, and an end-to-end test unions the ids a real passing **and** failing `validate --json` emit and asserts them registered. Both mechanisms are exercised-path enforcement — there is no static call-site scan, so an id on a branch neither reaches can still ship unregistered and raises `AssertionError` only when that branch first executes; a new check site therefore lands together with a test that reaches it | -| Skill-drift guard | `tests/test_module_skills_sync.py` | The seeded forks in `.claude/skills/` and `.agents/skills/` are byte-identical to canonical `src/bmad_loop/data/skills/`. **Documented limitation: CI-inert** — both trees are gitignored and absent in CI, so every parametrization skips there; the guard bites on dev boxes only. (The canonical-existence assertion runs before the skip and is CI-live.) | -| Schema-version parity | `tests/test_tui_app.py` | The TUI renderer's pinned validate schema version equals `documents.VALIDATE_SCHEMA_VERSION` — deliberate duplication, because an import would auto-follow a CLI bump and silently render a v2 document as v1 | -| Installed-copy drift | `tests/test_hook_script.py`, `tests/test_probe_hook.py` | The hook relays' copies match their source: `test_hook_script.py` re-runs `install_into` and text-compares the project copy against the source; `test_probe_hook.py` compares the packaged resource — which only bites in a wheel-installed run, since an editable install resolves both sides to the same file | -| Version sync | `tests/test_release.py` + CI | `scripts/release.py check` runs as the `version-sync` job — `sync_version.check()` in-process, plus the CHANGELOG release contract (the canonical version's section exists; `## [Unreleased]` was reopened; its `compare/v...HEAD` link tracks the bump). `tests/test_release.py` covers the release helpers' pure logic **and** drives `cmd_check`/`cmd_prepare` over fixture changelogs; the version-field comparison itself is still CI-only | +| Guard | Where | Enforces | +| -------------------------- | ------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Portability guard | `tests/test_portability_guard.py` | One shared AST scan over every `src/bmad_loop/**/*.py` (data scripts included), carrying thirteen guards: literal `["tmux", ...]` argvs only in the two backend files (the backends' own `[self._BINARY, ...]` spelling is deliberately unmatched, so this tripwire currently flags nothing — #549); sequence-form git argvs (list or tuple, literal or named constant) only as `_run_git`'s argv argument in `verify.py`, with string-form git spawns refused everywhere, `verify.py` included; no bare `/tmp`-class POSIX paths; no `signal.SIGKILL` attribute; `os.kill(pid, 0)` probes only in `process_host.py`; any `os.kill` at all only there too (a second, distinct guard); `start_new_session` only in the detach helpers; `shell=True` only in its two sanctioned files; `BMAD_LOOP_*` env reads only through the `envvars.py` registry, a plugin's own variable family, or the session-protocol vars the two stand-alone hook relays read back; a persisted `spec_file` / `dispatched_spec_file` resolved with a bare `Path(...)` only in the four files that run inside the tree the value was recorded against; `verify_commands_outcome` called only from `verify._verify_review_commands`; its classifier half `verify_command_results_outcome` called only from `verify.verify_commands_outcome` or `Engine._verify_commands_with_results` (a separate guard, because fencing the wrapper alone still lets a gate compose run+classify by hand and pick its own root — #695); plus a scanned-file-count floor so a broken scan root cannot pass vacuously | +| Kind & refusal inventories | `tests/test_portability_guard.py` | Enumerate-vs-declare gates riding the same AST scan, so a new entry on an enumerable surface is a PR-time decision instead of review-pass archaeology: every literal journal kind is a declared `JOURNAL_KINDS` row (kind-only writes and the `kind=` literals or parameter defaults that reach a declared dynamic-kind position included, via a dedicated literal-kind emit; only the f-string family stays out, governed by the literalness test); every `_refuse_*`/`_reject_*` helper definition is a `REFUSAL_HELPER_DEFS` row; and the #414-family isolation-refusal call sites (`bmadconfig.worktree_isolation_conflict` plus its CLI wrapper `_reject_isolation_conflict`) match `ISOLATION_CONFLICT_CALLERS` with multiplicity, so `cmd_resolve`'s legitimate second call cannot absorb a third. All assertions are exact-set or exact-Counter in both directions — a removed or renamed entry reddens its stale row too — and each new detector emit carries must-flag and must-stay-silent probe rows through `_scan_source` | +| Settings-schema sync | `tests/test_settings_schema.py` | `src/bmad_loop/data/settings/core.toml` stays in lockstep with `policy.py` by reflection, in both directions: every spec maps to a live dataclass field with a matching default wherever one is baked in, every policy field is reachable from exactly one spec (or listed in the explicit `HIDDEN` set), and every `*Policy` dataclass is consciously classified | +| Exit-code allocation | `tests/test_entry_point.py` | `ExitCode` is pinned literally (OK=0, FAILURE=1, USAGE=2, INTERRUPTED=130) **and closed**: the enum's value set equals exactly those four, so codes 3–129/131+ cannot be allocated quietly | +| Extra-less core CLI | `tests/test_entry_point.py` | A fresh interpreter with `pyte`/`rich`/`textual`/`tomlkit` blocked at `find_spec` — the blocker **raises** rather than returning None, so the dev venv's installed copies cannot make it pass vacuously, and an `import pyte` floor proves it bites — imports `bmad_loop.cli` and `bmad_loop.settings_schema`, runs `list` to rc 0, and asserts `tui` degrades to the `bmad-loop[tui]` hint instead of a traceback. Every test job installs `--all-extras`, which is why #650 shipped broken for 23 releases; CI's isolated wheel `list` run is the same floor at install level | +| State-machine table | `tests/test_statemachine.py` | Every `Phase` has a transition row; `TERMINAL_PHASES` (model.py) equals the table's dead ends — a cross-module parity nothing else links; an N×N `parametrize` grid drives every pair (legal pairs land, illegal pairs raise and leave the phase untouched); the awaiting-operator reachability rule is additionally stated independently, because the N² grid reads its expectation out of the table under test | +| Check-id registry | `checks.py` + `tests/test_cli.py` | `ValidationReport.add` asserts its id is in `VALIDATE_CHECKS` at every **executed** call site, and an end-to-end test unions the ids a real passing **and** failing `validate --json` emit and asserts them registered. Both mechanisms are exercised-path enforcement — there is no static call-site scan, so an id on a branch neither reaches can still ship unregistered and raises `AssertionError` only when that branch first executes; a new check site therefore lands together with a test that reaches it | +| Skill-drift guard | `tests/test_module_skills_sync.py` | The seeded forks in `.claude/skills/` and `.agents/skills/` are byte-identical to canonical `src/bmad_loop/data/skills/`. **Documented limitation: CI-inert** — both trees are gitignored and absent in CI, so every parametrization skips there; the guard bites on dev boxes only. (The canonical-existence assertion runs before the skip and is CI-live.) | +| Schema-version parity | `tests/test_tui_app.py` | The TUI renderer's pinned validate schema version equals `documents.VALIDATE_SCHEMA_VERSION` — deliberate duplication, because an import would auto-follow a CLI bump and silently render a v2 document as v1 | +| Installed-copy drift | `tests/test_hook_script.py`, `tests/test_probe_hook.py` | The hook relays' copies match their source: `test_hook_script.py` re-runs `install_into` and text-compares the project copy against the source; `test_probe_hook.py` compares the packaged resource — which only bites in a wheel-installed run, since an editable install resolves both sides to the same file | +| Version sync | `tests/test_release.py` + CI | `scripts/release.py check` runs as the `version-sync` job — `sync_version.check()` in-process, plus the CHANGELOG release contract (the canonical version's section exists; `## [Unreleased]` was reopened; its `compare/v...HEAD` link tracks the bump). `tests/test_release.py` covers the release helpers' pure logic **and** drives `cmd_check`/`cmd_prepare` over fixture changelogs; the version-field comparison itself is still CI-only | Rules for adding or touching a guard: @@ -181,8 +182,10 @@ Rules for adding or touching a guard: `os.kill`), and `verify.py`'s git exemption is narrowed further, to the `_run_git` argv position. Env-read exemptions are scoped **by variable name or family, never by file** — a file-wide pass would let a hook read a core knob unnoticed. -- **The detector itself gets executable coverage.** Four detectors — env-read, git-argv, and the - two verify-composition ones — carry probe matrices: the scan is split (`_scan_source`) so probe fixtures run +- **The detector itself gets executable coverage.** Every detector added since this bar was set + carries a probe matrix — env-read, git-argv, the two verify-composition ones, spec-anchor, + task-artifact, session-task-id, the re-arm call, the journal field / kind / kind-literal + family, refusal-def and isolation-call: the scan is split (`_scan_source`) so probe fixtures run the same code path as the real scan, with a must-flag row per claimed access form and a must-stay-silent row per lookalike. When a new form turns up, add the failing probe row first, then fix the detector. The green "no findings today" assertion cannot grade a diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 6ba5068b..3b9345a7 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -282,15 +282,22 @@ def _reject_isolation_conflict(paths: bmadconfig.ProjectPaths, pol) -> int | Non """Refuse `isolation = "worktree"` under a `repo_root` override (#414). Returns 1 to abort, None to proceed — the `_reject_bad_run_id` shape. - Called from the three :class:`~engine.Engine` construction sites that return an - rc to a human: `cmd_run`, `cmd_sweep`, and `_resume_paused_run` — the shared - helper behind both `resume` and `resolve`'s re-arm. The fourth such site, the - auto-triggered child sweep in `_sweep_factory`, shares the refusal but not this - disposition: it has no rc channel, so it raises (see the comment there). - Keyed on Engine construction rather than on "loads policy.toml", which is a - wider set that does not all provision — `_configure_mux` reads the file on - every command and builds nothing; `cmd_validate` and `cmd_clean` load it and - never mount a worktree. + Called from the four sites that return an rc to a human: `cmd_run`, `cmd_sweep`, + `_resume_paused_run` — the shared helper behind both `resume` and `resolve`'s + re-arm — and `cmd_resolve`, which calls it TWICE: once before the interactive + session and once after the config re-read that authorises the re-arm. A fifth + site, the auto-triggered child sweep in `_sweep_factory`, shares the refusal but + not this disposition: it has no rc channel, so it raises (see the comment there). + + Keyed on provisioning-or-arming a run against the config, NOT on Engine + construction: `cmd_resolve` constructs no Engine and delegates to + `_resume_paused_run` for that, but `runs.rearm_escalation` mutates persisted run + state — advancing the attempt baseline and re-stamping the spec — against the + same `repo_root` this refuses, and it does so BEFORE the delegate is reached. A + refusal keyed on Engine construction alone therefore arrives after the damage. + Both keyings exclude the same wider "loads policy.toml" set, which does not all + provision — `_configure_mux` reads the file on every command and builds nothing; + `cmd_validate` and `cmd_clean` load it and never mount a worktree. `validate` deliberately does not call this — it reports rather than aborts, so it renders the same message as a Finding and keeps running its other gates.""" @@ -305,8 +312,8 @@ def _reject_under_floor_git(project: Path) -> int | None: """Refuse to start against a git older than `verify.GIT_FLOOR`. Returns `ExitCode.FAILURE` to abort, None to proceed — the `_reject_bad_run_id` shape. - Called from the same four Engine-construction sites as - `_reject_isolation_conflict`, with the same split of dispositions: an rc to a + Called from the four Engine-construction sites, with the same split of + dispositions as `_reject_isolation_conflict`: an rc to a human from `cmd_run`, `cmd_sweep` and `_resume_paused_run`, and a raise from the auto-triggered child sweep in `_sweep_factory`, which has no rc channel. @@ -2707,7 +2714,17 @@ def _prepare_resume_locked(project: Path, run_dir: Path): # The `bool(state.repo_root)` guard is what keeps a legacy state.json — written # before the field existed, and read back as "" — out of the comparison: it is a # missing value, not a divergent one, and the re-stamp migrates it silently. - code_root_changed = bool(state.repo_root) and state.repo_root != str(paths.repo_root) + # + # The `code_root_restamp_pending` half is a move `runs.restamp_code_root` already + # persisted whose `rearm-code-root-restamped` record never landed: the mirror then + # already agrees with config, so the compare alone would read "no move" on the one + # gesture that still owes the operator its record and its warning. This resume + # CONSUMES that outstanding re-stamp — the retry the marker keeps possible may + # arrive through plain `resume` rather than through `resolve`, and a run that + # finishes from here would otherwise leave the move unrecorded for good. + code_root_changed = ( + bool(state.repo_root) and state.repo_root != str(paths.repo_root) + ) or state.code_root_restamp_pending fields: dict[str, object] = { # Scalars only, per the note above: a bool records THAT the pinned surface # moved without journaling a command, a binary path or a plugin name. @@ -2797,6 +2814,10 @@ def _prepare_resume_locked(project: Path, run_dir: Path): # is the tree `runs.rearm_escalation` must read back. Unconditional, so it also # migrates a pre-field state.json onto the root it was already using. state.repo_root = str(paths.repo_root) + # The `run-resume` record above IS the record an outstanding re-stamp owed, so the + # marker clears on the same write that persists the resume — never a separate + # one, which could land without it and leave the run owing a record it has. + state.code_root_restamp_pending = False state.clear_pause() runs.write_pid(run_dir) # Persist before the engine starts: status, the TUI and diagnose only ever @@ -3145,18 +3166,43 @@ def cmd_resolve(args: argparse.Namespace) -> int: # gesture. resolution_recorded = False if args.interactive: - adapters = _make_adapters(project, run_dir, pol) - model = pol.adapter.resolved("dev").model # The interactive session uses the CURRENT CLI project as cwd. Its code root # must come from the CURRENT config too: both can have moved since state.json # was written. This is best-effort observation only; the mandatory config # re-read after the human conversation remains the authority for re-arm. + # + # Read BEFORE `_make_adapters` so the refusal below can precede it. Ordering + # only, no new failure mode: `load_paths` is a read, and the arm that cannot + # read degrades exactly as it did when it sat lower. try: pre_session_paths = bmadconfig.load_paths(project) except (bmadconfig.BmadConfigError, OSError): pre_session_code_root = state.code_root else: pre_session_code_root = pre_session_paths.repo_root + # Refuse the unsupported config BEFORE the interactive session, not only + # after it. Both inputs are already in hand here — `pol` was loaded at the + # top of this function and is being read for `isolation` two calls below — + # so the late refusal alone let an operator build adapters, converse with a + # full agent session and answer the re-arm prompt, only to be handed rc 1 + # for a configuration knowable before any of it. `cmd_run` and `cmd_sweep` + # refuse the same config before provisioning anything; this restores the + # parity, and honours the rule the restore latch states one screen down + # ("validate before the interactive resolve session, not after a whole + # agent conversation the abort would throw away"). Ahead of the adapter + # build for the same reason `cmd_run` puts it ahead of the queue and + # worktree-clean gates: this one says the configuration cannot run at all, + # so an adapter fault reported first would send the operator at the wrong + # problem — and would be refused again anyway. + # + # It does NOT replace the refusal after the confirm: that one re-reads the + # config, which is the authority for the re-arm and is the only check the + # `--no-interactive` path reaches. This is a strictly earlier exit on the + # same predicate, so an operator who declines still gets no config lecture. + if (rc := _reject_isolation_conflict(pre_session_paths, pol)) is not None: + return rc + adapters = _make_adapters(project, run_dir, pol) + model = pol.adapter.resolved("dev").model _ctx_path, withheld, unreadable = resolve.build_context( state, run_dir, diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 93790bc2..dc9df9b2 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -696,6 +696,18 @@ class RunState: # `project`, which is exactly the pre-upgrade behavior and the correct answer # for every run without the override. repo_root: str = "" + # Intent marker for `runs.restamp_code_root`: True from the atomic state write + # that moved `repo_root` until the `rearm-code-root-restamped` journal record + # for that move has landed. state.json and the journal are two files with no + # transaction across them, so the move and its record cannot be made durable + # in one step; this flag rides the state write itself, which is what lets a + # retry tell "moved and recorded" from "moved, record still owed" — the one + # distinction that keeps the record retryable without ever asserting a move + # that was not persisted. Consumed by whichever surface retries first: a second + # `restamp_code_root` writes its own record, and a plain `resume` folds the + # move into its `run-resume` line. Deliberately absent from `documents.py`'s `--json` + # projection (schema 1), like `rearmed` / `resolved_redrive`. + code_root_restamp_pending: bool = False policy_snapshot: dict[str, Any] = field(default_factory=dict) # SECONDARY copy of the host-exec baseline (#498) — runsetup.config_digest over # the agent-writable config that reaches HOST code execution: verify commands, @@ -828,6 +840,7 @@ def to_dict(self) -> dict[str, Any]: "run_id": self.run_id, "project": self.project, "repo_root": self.repo_root, + "code_root_restamp_pending": self.code_root_restamp_pending, "started_at": self.started_at, "policy_snapshot": self.policy_snapshot, "trusted_config_digest": self.trusted_config_digest, @@ -859,6 +872,7 @@ def from_dict(cls, d: dict[str, Any]) -> "RunState": run_id=d["run_id"], project=d["project"], repo_root=str(d.get("repo_root", "")), + code_root_restamp_pending=bool(d.get("code_root_restamp_pending", False)), started_at=d["started_at"], policy_snapshot=d.get("policy_snapshot", {}), trusted_config_digest=str(d.get("trusted_config_digest", "")), diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index ade875f1..b6429179 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -4102,20 +4102,75 @@ def restamp_code_root(run_dir: Path, repo_root: Path) -> str | None: with state_lock(run_dir): state = load_state(run_dir) new = str(repo_root) - if state.repo_root == new: + if state.repo_root == new and not state.code_root_restamp_pending: return None - moved = bool(state.repo_root) - state.repo_root = new - save_state(run_dir, state) - if not moved: + if state.repo_root != new: + # Discharge an OWED record before the root it names is overwritten. + # The marker is a bare bool, so the only surviving description of the + # root an unlanded record was owed for is `state.repo_root` itself: + # once this call re-points it, a record for the previous tree can + # never be written again. An operator who re-points the root a SECOND + # time before retrying would otherwise lose that record silently. + # Ordering is the same at-least-once bargain the append below keeps: + # nothing has been written or cleared yet, so an append that fails + # here leaves the root and the marker exactly as the retry needs them. + if state.code_root_restamp_pending: + Journal(run_dir).append( + "rearm-code-root-restamped", + repo=state.repo_root, + code_root_changed=True, + ) + moved = bool(state.repo_root) + state.repo_root = new + # The move and its intent marker land in ONE atomic state write: a + # save that fails here changes nothing on disk, so the retry simply + # redoes it, and a save that succeeds has durably recorded that a + # record is now owed. The migration of an empty (pre-field) root is + # not a move and owes nothing. The marker carries forward rather than + # clearing: the discharge above settled the PREVIOUS root's debt, and + # this write opens the new one's. + state.code_root_restamp_pending = moved + save_state(run_dir, state) + # Either this call moved the root, or an earlier call moved it and its + # record never landed — the marker is what tells those apart from the + # ordinary "already agrees" return above. + if not state.code_root_restamp_pending: return None - return ( - f"run {run_dir.name}: the code root in _bmad/bmm/config.yaml has changed since " - "this run started — the re-drive works in the tree configured now, while the " - "baselines, preserve refs and branches this run already recorded name objects " - "in the previous one. Restore the previous `repo_root:` value if you did not " - "intend the move." + # Journalled under resume's own field name. This re-stamp aligns the mirror + # that `cli._resume_paused_run` later compares against config, so by the + # time `run-resume` computes `code_root_changed` the two necessarily agree + # and it records `false` — on the one gesture where the root DID move. The + # ephemeral stderr/toast the caller prints from the return value is not a + # record; without this line the move leaves no durable trace on the re-arm + # surfaces while plain `resume` still writes one. `repo` is dropped by the + # diagnose registry (`diagnostics._JOURNAL_DROP_FIELDS`), so the path never + # reaches a dump. + # + # AFTER the persisted move, never before it: a record written first would + # assert a completed move that a failed save then never made. And the + # marker is cleared only once the append has returned: an append that + # fails leaves it set, so the retry re-enters here and writes the record + # the move still owes — or, when the operator runs plain `resume` instead, + # `cli._prepare_resume_locked` reads the marker as a move, journals it on + # its own `run-resume` line and clears it on the same write that persists + # the resume. The one residual is a clearing save that fails after + # a successful append, which costs a duplicate — true — record on the + # retry; a duplicate is recoverable from the journal, a missing record and a + # false one are not. + Journal(run_dir).append( + "rearm-code-root-restamped", + repo=new, + code_root_changed=True, ) + state.code_root_restamp_pending = False + save_state(run_dir, state) + return ( + f"run {run_dir.name}: the code root in _bmad/bmm/config.yaml has changed since " + "this run started — the re-drive works in the tree configured now, while the " + "baselines, preserve refs and branches this run already recorded name objects " + "in the previous one. Restore the previous `repo_root:` value if you did not " + "intend the move." + ) @dataclass(frozen=True) @@ -4134,6 +4189,16 @@ class RearmOutcome: story_key: str notices: tuple[RearmNotice, ...] hold_resume: bool + # The `next_step` of the FIRST record that held, so a surface which folds the + # resume into the same gesture can say what to actually do. `hold_resume` alone + # forced that surface to hardcode ONE remedy for four holding records, and the + # hardcoded one ("commit the corrected spec") is impossible on two of them: the + # in-place arm of `rearm-spec-write-unreachable` needs an edit in the main + # checkout, and `rearm-spec-flip-skipped`'s holding arm fires only when the spec + # path is NOT a readable file — there is nothing at that path to commit, and the + # path may sit in a shared artifact directory outside any repository. Empty when + # the holding record renders no step, which keeps the caller's fallback honest. + hold_next_step: str = "" class _RearmJournal(Journal): @@ -4143,6 +4208,7 @@ def __init__(self, run_dir: Path): super().__init__(run_dir) self.notices: list[RearmNotice] = [] self.hold_resume = False + self.hold_next_step = "" def append(self, kind: str, **fields: Any) -> None: # Capture only after the durable append succeeds. The synthetic entry contains @@ -4150,11 +4216,20 @@ def append(self, kind: str, **fields: Any) -> None: # self-minted timestamp/log fields are not part of either contract. super().append(kind, **fields) entry = {"kind": kind, **fields} - self.hold_resume = rearm_holds_the_resume(entry) or self.hold_resume + holds = rearm_holds_the_resume(entry) rendered = rearm_event_notice(entry) if rendered is not None: severity, message, next_step = rendered self.notices.append(RearmNotice(severity, message, next_step)) + # FIRST-WINS, and deliberately: a re-arm can journal more than one holding + # record, and the earliest is the cause the operator has to clear first — + # the later ones are read from a tree the first remedy changes. Guarded on + # `hold_resume` being still-false rather than on the step being empty, so a + # holding record that renders no step does not silently hand the surface a + # LATER record's imperative for a different file. + if holds and not self.hold_resume: + self.hold_next_step = next_step + self.hold_resume = holds or self.hold_resume def rearm_escalation( @@ -4656,12 +4731,36 @@ def _rearm_escalation_locked( # ("add a top-level `status:`") for a re-arm that COMPLETED sends # the human to repair a file nothing will read. refused = spec_path.is_file() and write_reaches_the_redrive + # `refused` is False for TWO disjoint reasons and the operator + # surfaces cannot re-derive which: the write would have reached + # the re-drive but the file is gone, or the file is there but the + # re-drive discards that copy. Carrying the second half of the + # conjunction is what lets the renderer stop asserting worktree + # behaviour on a run that has no worktree. Absent on records + # written before this field existed, where the renderer keeps its + # previous wording. + # The live re-drive mode, recorded for the same reason + # `refused` is: the renderer reads this OUT OF PROCESS and + # cannot re-derive it. `reaches_redrive` does NOT imply it — + # its isolated arm answers True for a spec in an artifact dir + # configured outside the project tree + # (`_spec_is_shared_with_the_redrive`), which is reachable + # precisely BECAUSE it is shared across checkouts, with a + # worktree very much mounted. Inferring "no worktree" from + # reachability asserted the opposite of the truth on that shape. + # Absent on records written before this field existed, where the + # renderer drops the mount clause rather than guessing: unlike + # the sibling `rearm-spec-write-unreachable`, whose in-place arm + # is newer than the field, this kind was journalled from BOTH + # modes before it, so an absent value here is genuinely unknown. journal.append( "rearm-spec-flip-skipped", story_key=key, spec_file=str(spec_path), status=target_status, refused=refused, + reaches_redrive=write_reaches_the_redrive, + redrive="isolated" if isolated_redrive else "in-place", ) # ...and then ABORT — but only for a spec that IS a readable file # here AND is the copy the re-drive reads. The first half is the same @@ -5009,7 +5108,7 @@ def _rearm_escalation_locked( baseline=task.baseline_commit or "", restore=bool(restore_patch), ) - return RearmOutcome(key, tuple(journal.notices), journal.hold_resume) + return RearmOutcome(key, tuple(journal.notices), journal.hold_resume, journal.hold_next_step) def journal_entries_or_none(run_dir: Path) -> list[dict[str, Any]] | None: @@ -5062,6 +5161,35 @@ def _journal_sequence(value: Any) -> tuple[Any, ...]: return () if value is None else (value,) +def _redrive_status_clause(entry: dict[str, Any]) -> str: + """The `status:` a holding record's remedy must leave on the spec, as a next_step + clause — `""` for a record that carries no status. + + Three of the four holding remedies end in a spec the re-drive has to ROUTE on, and + routing is decided by the frontmatter status alone: step-01 halts blocked on + `unrecognized status in existing story file`, and a spec still carrying the + escalated attempt's terminal status routes to "ingest as context, do not resume". + So an operator who restores or commits the file the record names, byte-correct but + still terminal, has obeyed the remedy and burned the escalation anyway — the resume + the hold bought them is spent on a session that cannot route. Naming the target + here is what makes the remedy sufficient rather than merely necessary. + + All FOUR arms call this, so the remedies stay one uniform contract across both + operator surfaces. `rearm-upstream-write-unreachable` is the fourth and renders + `""` today, since the sentinel leg it fires on has no spec status at all — see the + comment at that arm before concluding the call is dead. + + Read off the record rather than recomputed: the producer writes the very value it + tried to flip to (`target_status`, `in-review` after a restore and `ready-for-dev` + otherwise), and this renderer runs out of process, from a journal line alone. A + record predating the field yields `""` and the clause is dropped, on the same + principle the `target_branch` clause follows — a remedy that names no value beats + one that names a guess. + """ + status = str(entry.get("status", "") or "") + return f" with `status: {status}`" if status else "" + + def rearm_event_notice( entry: dict[str, Any], ) -> tuple[Literal["note", "warning"], str, str] | None: @@ -5085,9 +5213,17 @@ def rearm_event_notice( kind = entry.get("kind", "") if kind == "stale-restore-excluded": files = ", ".join(str(f) for f in _journal_sequence(entry.get("files"))) + # "this re-arm computed" rather than a bare completed past tense, because + # `_stale_restore_residue` journals BEFORE the advance and the re-stamp, and + # `save_state` runs once at the very end. Both surfaces echo this from an abort + # path on purpose (the residue matters most there), and after an abort nothing + # was persisted: the task is still ESCALATED, `restore_patch` is still latched + # and `baseline_untracked` is unchanged. The sibling `stale-restore-commits` + # needs no such hedge — it reports where commits SIT, which stays true. return ( "note", - f"excluded the abandoned restore's new files from the re-drive baseline: {files}", + "excluded the abandoned restore's new files from the re-drive baseline this " + f"re-arm computed: {files}", "", ) if kind == "stale-restore-unparseable": @@ -5150,6 +5286,7 @@ def rearm_event_notice( # an ISOLATED one: that was the only shape the producer could journal before the # in-place arm existed, so the absent field is a known value, not an unknown. spec = entry.get("spec_file", "?") + to = _redrive_status_clause(entry) if str(entry.get("redrive", "isolated") or "isolated") == "in-place": # The mirror shape: `isolation` was edited to `"none"` while the escalation # was paused, so the writes went into the mount the escalated attempt @@ -5164,7 +5301,7 @@ def rearm_event_notice( "escalated attempt's worktree while the re-drive now runs in the main " "checkout — re-apply the correction to the main checkout's copy of the " "spec or the story re-wedges on the escalated attempt's status", - "Correct the spec in the main checkout before resuming", + f"Correct the spec in the main checkout{to} before resuming", ) # The branch is the half an operator cannot infer: the re-drive cuts its fresh # worktree from the run's PINNED target branch, so a correction committed on @@ -5179,7 +5316,7 @@ def rearm_event_notice( f"spec writes ({spec}) land in a tree it discards — the re-driven session " "reads the COMMITTED spec, so commit the corrected " f"spec{where} or the story re-wedges on the escalated attempt's status", - f"Commit the corrected spec{where} before resuming", + f"Commit the corrected spec{where}{to} before resuming", ) if kind == "rearm-upstream-write-unreachable": # The sentinel counterpart, and ONE remedy rather than the two above: the @@ -5195,6 +5332,24 @@ def rearm_event_notice( root = str(entry.get("stories_root", "?")) base = str(entry.get("target_branch", "") or "") where = f" on `{base}`" if base else "" + # The status clause is rendered here for UNIFORMITY with the other three + # holding arms, and on the leg the producer actually emits it renders EMPTY. + # That is intended, not an oversight, and it is not dead code: the append at + # `rearm-upstream-write-unreachable`'s site carries no `status`, because this + # arm fires only on the sentinel path — `_clear_sentinel` DELETES the spec and + # the re-dispatch re-plans from PENDING, so `target_status` is not even in + # scope there, and `stories.yaml` REJECTS a `status` key outright (a story's + # status lives in its story spec). `_redrive_status_clause` answers `""` for a + # record carrying no status, so every remedy this arm renders today is + # byte-identical to the one it rendered before. + # + # It stays because the four holding remedies are ONE contract an operator + # reads across surfaces, and a reader comparing them must not have to work out + # which arm was left out; should this record ever come to carry a status, the + # remedy names it without a second fix. Do NOT "simplify" it back out, and do + # NOT add a `status` field to the producer to make it fire — that would put a + # status on a leg that has none. + to = _redrive_status_clause(entry) return ( "warning", f"the sentinel was cleared, but the re-drive of this story will mount a " @@ -5202,7 +5357,7 @@ def rearm_event_notice( f"correction in {root} (`SPEC.md` / `stories.yaml`) is uncommitted there, " f"so the re-plan reads the same intent that wedged and mints the sentinel " "again", - f"Commit the corrected SPEC.md / stories.yaml{where} before resuming", + f"Commit the corrected SPEC.md / stories.yaml{where}{to} before resuming", ) if kind == "rearm-spec-flip-skipped": # ONE kind, TWO outcomes, told apart by the flag the producer writes rather @@ -5227,11 +5382,42 @@ def rearm_event_notice( "on the status it reads", "Add a top-level `status:` to the spec, then re-run resolve", ) + if entry.get("reaches_redrive"): + # The write DID address the copy the re-drive reads; the flip skipped + # because that path is not a readable file from this process — a spec moved + # or renamed by the resolve session, or an absolute path this `--project` + # invocation cannot see. Either way the re-drive reads that same path, so + # the worktree wording below would tell the operator the failed flip is + # harmless at precisely the moment it is not. + # + # The MOUNT half is read off the record, never inferred from reachability: + # the isolated arm reaches the re-drive through a spec shared across + # checkouts, so a worktree is mounted there and "this run mounts no + # worktree" was simply false. A record predating the `redrive` field says + # nothing about the mode, and this kind was written from both modes before + # the field existed — so drop the clause rather than guess. The rest of the + # sentence holds in every case. + mode = str(entry.get("redrive", "") or "") + if mode == "in-place": + mount = ", and this run mounts no worktree" + elif mode == "isolated": + mount = ", and the re-drive reads it from outside the worktree it mounts" + else: + mount = "" + return ( + "warning", + f"the recorded spec for this story ({spec}) could not be re-opened to " + f"`{status}` — it is not a readable file from here{mount}, so the " + "re-drive reads that same path and finds no spec there to route on", + f"Restore the recorded spec path{_redrive_status_clause(entry)} before " "resuming", + ) # No next_step, and deliberately: on this leg there is nothing to do to THIS # file. Whether anything is left to do at all is decided by the committed spec, # and `rearm-spec-write-unreachable` — journalled from the same block, on # exactly the legs where the committed spec is not already at the target — - # carries that imperative, and holds the resume behind it. + # carries that imperative, and holds the resume behind it. Reached for a + # worktree-local copy the re-drive discards, and for a pre-`reaches_redrive` + # record, which keeps the wording it was written under. return ( "warning", f"the recorded spec for this story ({spec}) could not be re-opened to " @@ -5343,16 +5529,20 @@ def rearm_holds_the_resume(entry: dict[str, Any]) -> bool: tree — so a surface that re-arms and resumes in ONE gesture must stop after the re-arm and leave `bmad-loop resume` to the operator. - TWO kinds qualify, and the discriminator is PROOF, not urgency. + THREE records qualify, and the discriminator is PROOF, not urgency. Two of them + qualify by KIND; the third qualifies by its FLAGS, because one kind there covers + outcomes that answer this question differently. `rearm-spec-write-unreachable` is written only once `_redrive_spec_status` has established that the committed spec does NOT carry the status the re-drive routes on, and only for a spec the working-tree flip cannot reach. Resuming on it is not risky, it is futile: the re-drive discards the worktree, mounts a fresh one from git, and step-01 reads a status it cannot route — `unrecognized status in existing story file` halts it blocked, and the escalation is spent. The record's own - next_step already said "commit the corrected spec before resuming"; both default - surfaces then resumed in the same breath, which made the imperative unactionable at - the moment it rendered. The interactive resolve agent cannot close that gap either + next_step already said "commit the corrected spec ... before resuming"; both + default surfaces then resumed in the same breath, which made the imperative + unactionable at the moment it rendered. It names the target `status:` as well as + the branch, because a spec committed there still carrying the escalated attempt's + terminal status re-wedges exactly as an uncommitted one does. The interactive resolve agent cannot close that gap either — its skill forbids it from committing. `rearm-upstream-write-unreachable` earns it the same way on the sentinel path, @@ -5374,13 +5564,35 @@ def rearm_holds_the_resume(entry: dict[str, Any]) -> bool: LESS than the answer — it proves nothing about whether the re-drive can route, only that one advisory could not be computed. + `rearm-spec-flip-skipped` earns it on ONE of its three arms, which is why this is + keyed on the record's flags rather than on the bare kind. Its producer writes + `refused = spec_path.is_file() and write_reaches_the_redrive`, so + `reaches_redrive and not refused` isolates exactly the leg where the flip addressed + the copy the re-drive reads and that path is NOT a readable file here — so the + re-drive reads the same path and finds no spec there to route on. Futile, on the + same proof as the two above, and that arm's next_step says "restore the recorded + spec path ... before resuming"; without this it rendered on surfaces that resumed + in the same gesture, which is the defect the two kinds above were fixed for. The + remedy names the target `status:` too: a file put back at that path still carrying + the escalated attempt's terminal status is unroutable for the same reason a missing + one is. The other two arms must NOT hold: the `refused` arm raises `RearmError` from + the producer, so no resume happens at all and holding would be meaningless, and the + remaining arm carries no next_step because the imperative on that leg belongs to + `rearm-spec-write-unreachable`, which holds the resume itself. + Not folded into `rearm_event_notice`'s tuple, because they are different questions asked of the same entry: that table answers "what do I tell the operator", this answers "may this gesture still resume". Both surfaces ask both, in one walk. """ - return isinstance(entry, dict) and entry.get("kind") in ( - "rearm-spec-write-unreachable", - "rearm-upstream-write-unreachable", + if not isinstance(entry, dict): + return False + kind = entry.get("kind") + if kind in ("rearm-spec-write-unreachable", "rearm-upstream-write-unreachable"): + return True + return ( + kind == "rearm-spec-flip-skipped" + and bool(entry.get("reaches_redrive")) + and not entry.get("refused") ) diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 4f9fc686..7214a276 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -1100,11 +1100,26 @@ def _do_rearm( # The half of the gesture that still worked is kept: the story IS re-armed # and persisted. What stops is the resume this surface folds in behind it, # because the warning above proved the re-drive would read a spec it cannot - # route on and burn the escalation. Worded for a surface that drops - # `next_step`, and worded as an instruction the operator can finish here — - # the run stays paused and resumable from this same screen. + # route on and burn the escalation. + # + # The step is PROPAGATED from the record that held (`hold_next_step`) rather + # than hardcoded. Four records hold and their remedies differ — commit the + # spec, commit `SPEC.md` / `stories.yaml`, correct the spec in the MAIN + # checkout, or restore the recorded spec PATH — and the one literal this + # branch used to print was impossible to follow on two of them. The literal + # survives only as the fallback for a holding record whose render carries no + # step at all. + # + # Composed rather than concatenated: every `next_step` ends in "before + # resuming", which is true here (this hold is what stops the fold-in) but + # reads as a contradiction next to an imperative to resume. So the resume's + # absence leads, the step follows as the operator's own sentence, and the + # tail says the run is still there to resume from this screen once it is + # done — instead of ordering a resume in the same breath as a hold. + step = outcome.hold_next_step or "Commit the corrected spec before resuming" self.notify( - "not resuming: commit the corrected spec, then resume this run", + f"not resuming in this gesture. {step} — the run stays paused and " + "resumable from this screen.", severity="warning", ) return diff --git a/tests/test_cli.py b/tests/test_cli.py index 041063b8..74ac416c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2871,6 +2871,57 @@ def test_resolve_refuses_worktree_isolation_before_it_mutates_anything( assert state.tasks["s1"].phase == Phase.ESCALATED # still armed for a corrected config +def test_resolve_refuses_worktree_isolation_before_the_interactive_session( + project, monkeypatch, capsys +): + """The sibling above passes `--no-interactive`, so it pins the refusal only against + the WRITES. Nothing pinned it against the agent conversation, and that is the half an + operator pays for: `cmd_run` and `cmd_sweep` refuse this config before doing any + work, while `resolve` built adapters, ran a full interactive session, and handed back + rc 1 for a pair knowable before any of it — throwing the conversation away. + + Graded on `run_session` never being reached, not on the exit code: the post-confirm + refusal returns the same 1 from the same predicate, so an rc assertion alone passes + with the hoist deleted. `_make_adapters` is failed rather than stubbed for the same + reason — it is the first thing the interactive arm does, so it fails EARLIER than + `run_session` if the refusal is merely moved down a few lines rather than removed. + + The post-confirm refusal stays the authority and is deliberately not disturbed: it + re-reads the config after a conversation of unbounded length, and it is the only one + the `--no-interactive` path reaches. + + Ablation: delete the `_reject_isolation_conflict` call from the `else:` branch of + `pre_session_paths` and this reddens on `_make_adapters` while the sibling row above + stays green. + """ + from bmad_loop import resolve, runs + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + run_dir, _moved, recorded = _resolve_run_with_a_moved_code_root(project, monkeypatch) + _write_policy(project.project, ISOLATION_WORKTREE_POLICY) + monkeypatch.setattr( + cli, "_make_adapters", lambda *a, **k: pytest.fail("built adapters for a refused config") + ) + monkeypatch.setattr( + resolve, "run_session", lambda *a, **k: pytest.fail("conversed under a refused config") + ) + monkeypatch.setattr( + runs, + "rearm_escalation", + lambda *a, **k: pytest.fail("re-armed under a configuration the run refuses"), + ) + + # no --no-interactive: this is the path the sibling row cannot reach + argv = ["resolve", "--project", str(project.project), "r1", "--resume"] + assert cli.main(argv) == 1 + + assert REFUSAL in capsys.readouterr().err + state = load_state(run_dir) + assert state.repo_root == str(recorded) # nothing was written on the way out + assert state.tasks["s1"].phase == Phase.ESCALATED + + def test_resolve_degrades_when_the_config_cannot_name_the_code_root(tmp_path, monkeypatch, capsys): """Reading config.yaml to learn the tree is an OBSERVATION, so it degrades: without it this process cannot name the code root, and re-pointing the mirror at a guess is @@ -2953,7 +3004,8 @@ def fake_rearm( err = capsys.readouterr().err ordered_messages = ( - "excluded the abandoned restore's new files from the re-drive baseline: new.txt", + "excluded the abandoned restore's new files from the re-drive baseline this " + "re-arm computed: new.txt", "could not read the abandoned restore patch (b.patch)", "1 commit(s) sit below the re-drive's new baseline (ffffffffffff..)", ) @@ -3288,7 +3340,10 @@ def test_resolve_holds_the_resume_when_the_correction_cannot_reach_the_redrive( `rearm-spec-write-unreachable` fires only once the re-arm has established that the committed spec does not carry the status the re-drive routes on, and its own - next_step reads "Commit the corrected spec before resuming". This command printed + next_step reads "Commit the corrected spec with `status: ` before resuming". + It names the target status because a spec committed there still carrying the + escalated attempt's terminal one re-wedges the re-drive exactly as an uncommitted + correction does. This command printed that and then resumed two lines later, so the imperative was already unactionable when it rendered — and the interactive resolve agent cannot close the gap either, since its skill forbids it from committing. The fresh worktree then checked out the @@ -3342,7 +3397,7 @@ def fake_rearm( cli.main(["resolve", "--project", str(tmp_path), "r1", "--no-interactive", "--resume"]) == 0 ) out, err = capsys.readouterr() - assert "Commit the corrected spec before resuming" in err + assert "Commit the corrected spec with `status: ready-for-dev` before resuming" in err assert "NOT resuming in this gesture" in out assert "bmad-loop resume r1" in out # the escape hatch, reachable once it is committed assert resumed == [] # the gesture stopped; the story stays armed and resumable @@ -5682,6 +5737,38 @@ def test_resume_reports_no_code_root_change_when_the_config_did_not_move( assert "code root" not in capsys.readouterr().err +def test_resume_consumes_an_outstanding_code_root_restamp(project, monkeypatch, capsys): + """A move `runs.restamp_code_root` persisted whose record never landed reaches + resume with the mirror ALREADY agreeing with config — the compare alone reads "no + move" on the one gesture that still owes the operator its record and its warning. + + The intent marker exists so a retry writes that record; the retry may arrive + through plain `resume` rather than `resolve`, and a run that finished from here + would leave the move unrecorded for good — the audit gap the marker closes. So the + marker counts as a move for the `run-resume` line and the stderr warning, and is + consumed on the same state write that persists the resume. + + Ablation: drop the `or state.code_root_restamp_pending` half of the compare and this + reddens on the journal field (`assert False is True`); drop the clearing line + instead and it reddens on the persisted marker. + """ + from bmad_loop.journal import load_state + + run_dir = _paused_run_for_resume( + project, + monkeypatch, + repo_root=str(Path(project.project).resolve()), + code_root_restamp_pending=True, + ) + monkeypatch.setattr(cli, "Engine", _StubEngine) + + assert cli._resume_paused_run(project.project, run_dir) == 0 + + assert _resume_entry(run_dir)["code_root_changed"] is True + assert load_state(run_dir).code_root_restamp_pending is False + assert "the code root in _bmad/bmm/config.yaml has changed" in capsys.readouterr().err + + def test_resume_migrates_a_legacy_state_without_calling_it_a_move(project, monkeypatch, capsys): """A state.json written before `repo_root` existed reads back "" — a MISSING value, not a divergent one. The `bool(state.repo_root)` guard is what keeps it out of the diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index 3ae4ed62..9902368e 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -85,6 +85,7 @@ def _seed_run( extra_journal=None, sweeps_triggered=(), sweeps_refused=None, + repo_root="", ): """Build a run dir loaded with canaries in every readable sink. @@ -129,6 +130,7 @@ def _seed_run( state = RunState( run_id=run_id, project=f"{HOME_PATH}", + repo_root=repo_root, started_at="2026-06-27T12:00:00", run_type="story", target_branch=BRANCH, @@ -2394,8 +2396,20 @@ def test_diag_repo_root_diverges_is_false_for_the_ordinary_layout(project): Without this the assertion above passes for a hardcoded `True`, and the field stops carrying the one bit it exists to carry. + + Seeds `repo_root` EQUAL to `project`, which is what makes this the ordinary + layout rather than the legacy one. `repo_root_diverges` is + `bool(state.repo_root) and Path(state.repo_root) != Path(state.project)`, so a + run with no recorded root short-circuits on the first term and the equality arm + is never evaluated — the row would be named for a layout it does not build. + `runsetup` writes the field unconditionally on every run, equal to `project` + unless a `repo_root:` override exists, so this is the modal shape an operator's + dump carries. + + Ablation: drop the `repo_root=` argument and the row still passes, on the legacy + guard instead of the comparison. """ - run_dir = _seed_run(project.project) + run_dir = _seed_run(project.project, repo_root=f"{HOME_PATH}") diag, _pseudo, _combined = _render_all([run_dir]) (run,) = diag.runs @@ -2468,8 +2482,13 @@ def test_the_markdown_report_carries_the_split_root_and_the_generation(project): def test_the_markdown_report_says_no_for_the_ordinary_layout(project): - """The rendered line distinguishes; a hardcoded "yes" would pass the test above.""" - run_dir = _seed_run(project.project) + """The rendered line distinguishes; a hardcoded "yes" would pass the test above. + + Seeds `repo_root` equal to `project` for the same reason as its JSON twin: an + unset root answers `no` through the legacy guard without ever reaching the + comparison. + """ + run_dir = _seed_run(project.project, repo_root=f"{HOME_PATH}") pseudo = sanitize.Pseudonymizer() diag = diagnostics.collect([run_dir], pseudo=pseudo, project=ANY_PROJECT) md = diagnostics.render_markdown(diag, pseudo=pseudo) diff --git a/tests/test_model.py b/tests/test_model.py index fbe7060d..6ff377ef 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -41,6 +41,20 @@ def test_run_state_stories_fields_default_and_round_trip(): assert back.spec_folder == "_bmad-output/epic-1" +def test_run_state_code_root_restamp_pending_round_trips_and_defaults_false(): + """The intent marker `runs.restamp_code_root` sets between the moved root and its + journal record survives the state round trip, and a state.json from before the + field existed reads back False — a pre-upgrade run owes no record.""" + state = _state(repo_root="/code") + assert state.code_root_restamp_pending is False + state.code_root_restamp_pending = True + back = RunState.from_dict(state.to_dict()) + assert back.code_root_restamp_pending is True + d = state.to_dict() + del d["code_root_restamp_pending"] + assert RunState.from_dict(d).code_root_restamp_pending is False + + def test_run_state_repo_root_round_trips_and_backs_code_root(): """The git root a run's code work happens in, persisted because `runs.rearm_escalation` runs OUT OF PROCESS from the engine and had only diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index 8e6dae3c..28b747f1 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -14,8 +14,8 @@ calls go through the ``_run_git`` chokepoint in ``verify.py``" — see ``test_no_git_invocation_outside_verify``. -Three later invariants ride the same machinery, each one previously held by -docstring prose alone: +Later invariants ride the same machinery, each one previously held by docstring +prose alone (or by nothing): * the task-directory artifact names are ``journal.TASK_CYCLE_ARTIFACTS`` and not a literal repeated per reader/writer — ``test_task_cycle_artifacts_named_only_through_the_constant`` @@ -29,6 +29,12 @@ the two names ``Journal.append`` mints itself, which no call site spells. * ``runs.rearm_escalation`` is called from exactly two places, each of which consults liveness first — ``test_rearm_escalation_called_only_behind_a_liveness_gate``. +* every literal journal KIND is a declared ``JOURNAL_KINDS`` row — + ``test_journal_kind_inventory_is_complete``. +* every ``_refuse_*``/``_reject_*`` helper definition and every #414-family + isolation-refusal call site is enumerated — + ``test_refusal_helper_inventory_is_complete`` and + ``test_isolation_conflict_refusal_sites_are_enumerated``. If this test flags something unexpected, fix the source (route it through the seam / a platform helper) rather than widening an allowlist. @@ -223,6 +229,52 @@ ("tui/app.py", "_do_rearm"), } +# The two refusal surfaces review iteration 6 kept re-finding by hand, enumerated so +# a NEW one reddens CI until its row lands — the row being the PR-time decision +# whose failure message demands the covering test land beside it (the journal-kind +# inventory below is the third surface of that shape). +# +# Every `_refuse_*` / `_reject_*` helper DEFINITION in the tree, as +# `(file, def name)`. The prefix pair is the tree's whole refusal-helper naming +# convention today; a helper named outside it is invisible here — a stated bound, +# not coverage. The guard forces the decision only on names that claim to be +# refusals, and deliberately adds no runtime abstraction (no RefusalError, no +# registry): the inventory is the test file's, not the product's. +REFUSAL_HELPER_DEFS = { + ("cli.py", "_reject_bad_run_id"), + ("cli.py", "_reject_isolation_conflict"), + ("cli.py", "_reject_under_floor_git"), + ("engine.py", "_refuse_gated_story"), + ("platform_util.py", "_refuse_unwritable_target"), + ("platform_util.py", "_refuse_unwritable_target_at"), + ("resolve.py", "_reject_json_constant"), + ("runs.py", "_refuse_live_session"), + ("runs.py", "_refuse_uncontained_run_dir"), + ("workspace.py", "_refuse_foreign_checkout"), +} + +# Every #414-family call site — `bmadconfig.worktree_isolation_conflict`, sole +# producer of the isolation-under-repo-root refusal text, plus its rc-returning CLI +# wrapper `_reject_isolation_conflict` — as `(file, enclosing function) -> count`. +# The `REARM_ESCALATION_CALLERS` idiom WITH multiplicity, because `cmd_resolve` +# legitimately calls the wrapper twice: post-confirm is the authority, and the +# pre-session arm spares the operator a full interactive session on a pair knowable +# from config — the `96aa09a9` fix, which landed with no structural gate naming it. +# Accepted cost (human decision 2026-09-02): every future caller of the predicate +# touches a row here in the same PR. +ISOLATION_CONFLICT_CALLERS = { + ("cli.py", "_reject_isolation_conflict"): 1, # the wrapper's own predicate call + ("cli.py", "cmd_run"): 1, + ("cli.py", "cmd_sweep"): 1, + ("cli.py", "cmd_resolve"): 2, # pre-session + post-confirm re-read + ("cli.py", "cmd_validate"): 1, # reports a Finding rather than aborting + ("cli.py", "_prepare_resume_locked"): 1, # behind both `resume` and the re-arm + ("cli.py", "_warn_preflight_would_abort"): 1, # the dry-run honesty banner + ("cli.py", "factory"): 1, # `_sweep_factory`'s closure: raises — no rc channel + ("tui/app.py", "_guarded"): 1, # the pre-launch toast guard + ("tui/app.py", "_do_rearm"): 1, +} + # What counts as consulting liveness, matched as a substring of the callee's name # because the two sites legitimately spell it differently and neither spelling is more # correct: the CLI calls ``runs.engine_liveness`` directly, the TUI goes through @@ -354,6 +406,7 @@ "errors", "expired_clock", "failed", + "fallback", "field", "finished", "fired_at", @@ -396,6 +449,7 @@ "original", "owed_after_implement", "phase", + "pid", "platform", "plugin", "plugins", @@ -409,6 +463,7 @@ # turned out to be wrong. "rc", "re_review_capped", + "reaches_redrive", "rearmed", "record", "redrive", @@ -609,16 +664,282 @@ ("plugins/bus.py", "_log"), } -# The receivers a ``.append(...)`` call must hang off to be a journal write. Matched -# on the trailing name so `self.journal`, a bare `journal` parameter and -# `self._journal` (the plugin bus's optional handle) all resolve — the three -# spellings in the tree. +# Every literal journal KIND written today: a declared inventory, not a per-kind +# audit — `JOURNAL_BENIGN_FIELDS`' claim, made for the kind axis. Kind #205 cannot +# appear without someone deciding, in the same PR, what covers the record it +# introduces: a routing row in `diagnostics` if any field carries an identifier, a +# path or free text, and a test row asserting the record at the layer that reads it +# — the decision review iteration 6 kept discovering had been skipped. +# +# Generated from the scan, hand-reviewed, grouped by producer module; a kind two +# modules write sits under a shared heading. A deleted or renamed kind reddens the +# staleness direction too — PROVIDED no other producer still writes it: the +# staleness arm sees the union of producers, so removing ONE writer of a shared +# kind (the shared headings below, `run-stop` included) reddens nothing by itself. +# +# A declared dynamic-kind position (`JOURNAL_DYNAMIC_KIND_ALLOW`) writes a +# parameter, not a literal, so its kinds enter here through the literals that reach +# it from outside: the `kind="..."` a caller hands `engine._skip_review_and_commit` +# or `sweep._close_bundle_ledger_when_spec_status`, and each one's parameter +# default (`review-skipped`, `sweep-bundle-closed`) — a second `journalkindliteral` +# arm reads both, keyed by the same `(file, name)` as the position. +# +# ⚠️ STATED BOUNDS. Truly dynamic kinds — the f-string family in +# `recovery_flow.prune_preserve_refs` — are NOT rows here: the position is declared +# and governed by the literalness test, and the kinds it mints (e.g. +# `attempt-preserve-pruned`) never enter this inventory. And every receiver shape +# the journal scan cannot see is a hole in this emit too (`JOURNAL_RECEIVERS`' +# bound): a locally aliased handle, a `Journal` SUBCLASS constructed inline +# (`_RearmJournal(run_dir).append(...)`), the `super().append(...)` inside such a +# subclass's override, and a constructor reached through an import alias. On +# today's tree the only subclass instance is bound to the `journal` name and its +# override forwards its parameter kind, so no literal is missed — but the bound is +# the scan's, not the tree's. +JOURNAL_KINDS = frozenset( + { + # cli.py + "run-resume", + # engine.py + "board-advance-carried", + "board-advance-carry-failed", + "board-advance-carry-foreign-dirt", + "board-advance-carry-uncommitted", + "console-ctrl-ignored", + "defer-ledger-restore-diverged", + "deferred-artifacts-stashed", + "deferred-close-duplicate-id", + "deferred-close-external-ledger", + "deferred-close-ledger-unavailable", + "deferred-close-malformed", + "deferred-close-reopen-unmatched", + "deferred-close-rollback-failed", + "deferred-close-rolled-back", + "deferred-close-skipped-out-of-tree", + "deferred-close-unmatched", + "dev-decision", + "epic-boundary", + "fix-decision", + "fix-harvest-failed", + "harvest-carried", + "harvest-carry-uncommitted", + "isolation-flip-orphaned-worktree", + "ledger-baseline-probe-failed", + "ledger-restore-failed", + "ledger-restore-skipped-diverged", + "ledger-scope-probe-failed", + "ledger-snapshot-missing", + "ledger-tracked-probe-failed", + "legacy-ledger-attribution-failed", + "max-stories-reached", + "notify-desktop-unavailable", + "operator-index-failed", + "park-proof-of-work-skipped", + "park-record-rollback-failed", + "plugin-veto", + "plugins-active", + "preference-escalation", + "resume-defer", + "resume-ledger-carry", + "resume-review", + "resume-unit-merge", + "resume-verify", + "review-budget-committed", + "review-followup-damped", + "review-not-recommended", + "review-result", + "review-retry", + "review-skipped", + "review-skipped-awaiting-operator", + "review-timeout-salvage", + "review-timeout-salvage-failed", + "review-verify-failed", + "run-complete", + "run-crash", + "run-paused", + "run-stop-finalize-error", + "session-end", + "session-rescued-post-kill", + "session-start", + "session-synthesized-from-frontmatter", + "spec-deferral-sighting-stale", + "spec-deferrals-harvested", + "spec-deferrals-malformed", + "spec-deferrals-skipped-out-of-tree", + "spec-marker-repair-failed", + "spec-marker-repair-skipped", + "spec-marker-repaired", + "spec-read-failed", + "spec-reconcile-skipped-out-of-tree", + "spec-status-reconciled", + "sprint-status-unknown-keys", + "stop-request-discarded", + "story-awaiting-operator", + "story-deferred", + "story-deferred-close-carried", + "story-deferred-close-carry-uncommitted", + "story-deferred-closed", + "story-done", + "story-gate-unreadable", + "story-gated", + "story-skipped", + "story-start", + "sweep-auto-failed", + "sweep-auto-finished", + "sweep-auto-not-started", + "sweep-auto-skipped-dirty", + "sweep-auto-suppressed", + "sweep-auto-trigger", + "token-budget-exceeded", + "verify-command-result", + "workflow-end", + "workflow-start", + # engine.py + runs.py (runs.py's writer is the constructor-inline spelling) + "run-stop", + # engine.py + sweep.py + "resume-commit", + "resume-restart", + # engine.py + worktree_flow.py + "story-escalated", + # plugins/bus.py + "plugin-hook", + "plugin-hook-error", + # plugins/bus.py + plugins/registry.py + "plugin-error", + # plugins/loader.py + "plugin-skipped", + # plugins/registry.py + "plugin-loaded", + "plugin-untrusted", + # recovery_flow.py + "attempt-commits-preserved", + "attempt-preserve-enumerate-failed", + "attempt-preserve-failed", + "attempt-restore-failed", + "attempt-restored", + "attempt-worktree-preserve-failed", + "attempt-worktree-preserved", + "rollback-auto", + "rollback-dirty-check-failed", + "rollback-manual-required", + "rollback-owned-spec-baseline-read-failed", + "rollback-owned-spec-baseline-status-failed", + "rollback-owned-spec-manual-required", + "rollback-owned-spec-normalized", + "rollback-owned-spec-restored", + "rollback-owned-spec-snapshot-missing", + "rollback-owned-spec-unavailable", + "rollback-owned-spec-unpreservable", + "rollback-owned-spec-unreadable", + "rollback-reset-failed", + "rollback-skipped-clean", + # runs.py + "rearm-aborted", + "rearm-baseline-advance-failed", + "rearm-baseline-restamp-skipped", + "rearm-baseline-restamped", + "rearm-code-root-restamped", + "rearm-commits-probe-failed", + "rearm-spec-flip-skipped", + "rearm-spec-write-unreachable", + "rearm-upstream-write-unreachable", + "run-stop-undelivered", + "sentinel-cleared", + "stale-restore-commits", + "stale-restore-excluded", + "stale-restore-unparseable", + "story-escalation-resolved", + # runsetup.py + "composition-unwind-failed", + "run-start", + # stories_engine.py + "checkpoint-pause", + "checkpoint-resume", + "checkpoint-skip-last", + "deferred-close-declaration-unreadable", + "plan-halt", + "plan-halt-proof-of-work-skipped", + "sentinel-detected", + "stories-escalation-unresolved", + "stories-manifest-unreadable", + "stories-selector-unknown", + "stories-validated", + "stories-wedged", + # sweep.py + "bundle-start", + "decision-answered", + "decision-pending", + "decision-preanswered", + "decision-preanswers-pruned", + "decision-skipped-unattended", + "migrate-decision", + "migrate-duplicate-ids", + "sweep-bundle-close-carried", + "sweep-bundle-close-carry-uncommitted", + "sweep-bundle-closed", + "sweep-bundle-name-discarded", + "sweep-bundle-name-normalized", + "sweep-bundle-reclosed", + "sweep-bundle-reopened", + "sweep-bundle-skipped", + "sweep-bundles-truncated", + "sweep-cycle", + "sweep-decisions-only", + "sweep-inflight-redrive", + "sweep-inflight-stranded", + "sweep-intent-regenerated", + "sweep-ledger-commit", + "sweep-migrated", + "sweep-migration-restore-diverged", + "sweep-nothing-open", + "sweep-repeat-done", + "sweep-resolved-closed", + "sweep-return-no-client", + "sweep-returned-after-decisions", + "sweep-triage-reload-failed", + "sweep-triage-result", + "triage-decision", + # worktree_flow.py + "isolation-flip-orphan-preserved", + "merge-preflight-refused", + "merge-target-cleaned", + "merge-target-tolerated", + "scm-failed-diff-unlimited", + "target-branch", + "target-branch-checkout", + "target-branch-created", + "unit-closed", + "unit-merge-started", + "unit-merged", + "worktree-exclude-degraded", + "worktree-kept", + "worktree-module-skills-dropped", + "worktree-open-failed", + "worktree-opened", + "worktree-seed-dropped", + "worktree-seed-skipped", + "worktree-teardown-degraded", + } +) + +# The NAMED-HANDLE receivers a ``.append(...)`` call must hang off to be a journal +# write. Matched on the trailing name so `self.journal`, a bare `journal` parameter +# and `self._journal` (the plugin bus's optional handle) all resolve. The tree's +# fourth spelling — the constructor-inline `Journal(run_dir).append(...)` that +# runs.py's stop/restamp records use — is a Call receiver, not a name, and is +# matched structurally in `_is_journal_write` rather than through this set. # # ⚠️ STATED BOUND: a LOCALLY ALIASED handle is invisible. `j = self.journal` followed # by `j.append(kind, customer_email=x)` produces no finding (verified by running it -# through `_scan_source`). No such site exists in the tree today, and resolving the -# binding would be `_call_aliases`' shape rather than a new idea — but the -# guard does not do it, and a reader must not assume it does. +# through `_scan_source`), and a handle bound from the constructor — +# `j = Journal(run_dir)` then `j.append(...)` — is the same shape. So is anything +# the constructor arm's bare-name anchor does not spell: a SUBCLASS constructed +# inline (`_RearmJournal(run_dir).append(...)` — `runs._RearmJournal(Journal)` +# exists), the `super().append(kind, **fields)` inside that subclass's override +# (runs.py's fifth receiver spelling, a `super` Call), and `Journal` reached +# through an import alias. None of these carries a literal the tree misses today +# (the subclass's one instance is bound to `journal`; its override forwards a +# parameter kind), and resolving them would be `_call_aliases`' shape rather than a +# new idea — but the guard does not do it, and a reader must not assume it does. JOURNAL_RECEIVERS = {"journal", "_journal"} # Files that may name a bare POSIX path, each on a line carrying a `# portability:` @@ -1108,10 +1429,96 @@ def _mint_candidates(node: ast.expr, depth: int = 0): yield from _mint_candidates(arg, depth + 1) +def _kind_param_default(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> str | None: + """The string-literal default of ``fn``'s ``kind`` parameter — positional-or- + keyword or keyword-only — or None when there is no such parameter or its default + is not a string literal. The declared dynamic-kind positions mint their fallback + kind here (``review-skipped``, ``sweep-bundle-closed``), and nothing else in + the scan reads a parameter default.""" + args = fn.args + positional = args.posonlyargs + args.args + padded: list[ast.expr | None] = [None] * (len(positional) - len(args.defaults)) + padded.extend(args.defaults) + for arg, default in [*zip(positional, padded), *zip(args.kwonlyargs, args.kw_defaults)]: + if arg.arg == "kind": + if isinstance(default, ast.Constant) and isinstance(default.value, str): + return default.value + return None + return None + + +def _kind_param_index(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> int | None: + """Index of ``fn``'s ``kind`` parameter within a CALL's positional argument + list, or None when it cannot arrive positionally — keyword-only, or absent. + + A leading ``self``/``cls`` is dropped: every declared dynamic-kind position is a + method reached as ``self._log(...)``, where the receiver is bound and never + occupies a slot in ``Call.args``. The unbound spelling (``Bus._log(bus, kind)``) + would shift by one and is deliberately out of scope — it appears nowhere on this + tree, and reading the receiver where a kind was expected yields a non-literal and + therefore no finding, which is a miss rather than a false alarm. + + The sibling of :func:`_kind_param_default`, and needed for the same reason: a + literal reaches a dynamic-kind position three ways — a caller's keyword, a + caller's POSITIONAL argument, and the parameter default — and reading only two of + them leaves the third ungraded while the inventory reports itself complete.""" + positional = fn.args.posonlyargs + fn.args.args + if positional and positional[0].arg in ("self", "cls"): + positional = positional[1:] + for index, arg in enumerate(positional): + if arg.arg == "kind": + return index + return None + + +# A `kind` argument at a declared dynamic-kind position that the scan could not +# resolve: a `*args` splat covering the parameter's slot, or a non-literal expression +# in that slot or in a `kind=` keyword. Emitted AS a kind so the inventory arm reddens +# naming the site: no row can declare it, and an unreadable argument must not share +# its silence with "this call passed no literal". +UNRESOLVED_DYNAMIC_KIND = "" + + +def _positional_kind_literal(node: ast.Call, index: int) -> str | None: + """The string literal a call hands a declared dynamic-kind position + POSITIONALLY; :data:`UNRESOLVED_DYNAMIC_KIND` when that slot is OCCUPIED by + something the scan cannot read — a ``*args`` splat covering it, or a non-literal + expression in it; None only when the slot is EMPTY, which is the parameter default + the definition arm reports instead. + + Empty and occupied-but-unreadable are different answers and must not share one + return value. ``sweep.py``'s own ``_close_bundle_ledger_when_spec_status(task, + str(spec_file), success_status)`` omits ``kind`` and relies on the default, so + folding the empty slot into the sentinel reddens the clean tree. + + A non-literal in an occupied slot is FLAGGED, not skipped. Deferring it to the + literalness test — the rationale this arm used to carry, and the keyword arm with + it — holds only at the declared FORWARDER (``plugins/bus.py::_log``), whose + enclosing function at the CALL is not in ``JOURNAL_DYNAMIC_KIND_ALLOW``, so a + variable there reddens + ``test_journal_kinds_are_literal_or_the_position_is_declared`` anyway. At the two + non-forwarder positions (``engine._skip_review_and_commit``, + ``sweep._close_bundle_ledger_when_spec_status``) the allow set waives exactly that + test for the write INSIDE the position, so nothing else grades the slot: a caller + handing it a variable would reach the journal with a kind no row declares while + every arm stayed green.""" + if any(isinstance(arg, ast.Starred) for arg in node.args[: index + 1]): + return UNRESOLVED_DYNAMIC_KIND + if index >= len(node.args): + return None + arg = node.args[index] + if isinstance(arg, ast.Constant) and isinstance(arg.value, str): + return arg.value + return UNRESOLVED_DYNAMIC_KIND + + def _is_journal_write(node: ast.AST, rel: str) -> bool: """Whether this node writes a journal entry — a ``.append(...)`` call in - each of the three receiver spellings the tree uses (see ``JOURNAL_RECEIVERS``), - or a call to one of this file's declared ``JOURNAL_FORWARDERS``. + each of the four receiver spellings the scan reads: the three named handles (see + ``JOURNAL_RECEIVERS``) and the constructor-inline ``Journal(run_dir).append(...)`` + — or a call to one of this file's declared ``JOURNAL_FORWARDERS``. The tree's + fifth spelling, ``super().append(...)`` inside ``runs._RearmJournal``'s override, + is a stated bound (``JOURNAL_RECEIVERS``), not a receiver. The forwarder half is not a convenience. ``plugins/bus.py::_log`` takes its own ``**fields`` and hands them to ``self._journal.append``, so its four call sites @@ -1129,11 +1536,18 @@ def _is_journal_write(node: ast.AST, rel: str) -> bool: return False if (rel, name) in JOURNAL_FORWARDERS: return True - return ( - isinstance(node.func, ast.Attribute) - and name == "append" - and _called_name(node.func.value) in JOURNAL_RECEIVERS - ) + if not (isinstance(node.func, ast.Attribute) and name == "append"): + return False + receiver = node.func.value + if _called_name(receiver) in JOURNAL_RECEIVERS: + return True + # The constructor-inline spelling: `Journal(run_dir).append(...)`. The receiver + # is an ast.Call, so the named-handle match above can never see it — runs.py's + # stop/restamp records (and their kinds and fields) went unscanned exactly this + # way. Name-anchored on `Journal` like the handle arm, so a lookalike + # constructor stays silent — and so, by the same anchor, does a subclass + # constructor or an import alias (the stated bound on `JOURNAL_RECEIVERS`). + return isinstance(receiver, ast.Call) and _called_name(receiver.func) == "Journal" def _dict_literal_keys(value: ast.expr) -> set[str] | None: @@ -1267,6 +1681,23 @@ def _names_rearm_escalation(func: ast.expr, aliases: frozenset[str] = frozenset( return _names_guarded_verify_call(func, "rearm_escalation", aliases) +def _names_isolation_refusal( + func: ast.expr, + predicate_aliases: frozenset[str] = frozenset(), + wrapper_aliases: frozenset[str] = frozenset(), +) -> bool: + """True when ``func`` spells a #414-family refusal entry point: the + ``bmadconfig.worktree_isolation_conflict`` predicate or its rc-returning CLI + wrapper ``_reject_isolation_conflict``. Both names are guarded because a new + surface can reach the refusal through either — the ``96aa09a9`` site did so + through the wrapper — and each resolves its own alias set. Same reach and + computed-name bound as the sibling detectors, and the same trade: an unrelated + ``x.worktree_isolation_conflict(...)`` is a review prompt, not a miss.""" + return _names_guarded_verify_call( + func, "worktree_isolation_conflict", predicate_aliases + ) or _names_guarded_verify_call(func, "_reject_isolation_conflict", wrapper_aliases) + + def _block_exits(body: list[ast.stmt]) -> bool: """Whether this simple guard body cannot fall through to the re-arm below it.""" return bool(body) and isinstance(body[-1], (ast.Return, ast.Raise)) @@ -1362,6 +1793,8 @@ def _scan_source(src: str, rel: str): verify_command_aliases = _call_aliases(tree, "verify_commands_outcome") verify_classifier_aliases = _call_aliases(tree, "verify_command_results_outcome") rearm_aliases = _call_aliases(tree, "rearm_escalation") + isolation_aliases = _call_aliases(tree, "worktree_isolation_conflict") + isolation_wrapper_aliases = _call_aliases(tree, "_reject_isolation_conflict") # First positional args of `_run_git(...)` calls — the one position where a # git argv literal feeds the chokepoint instead of bypassing it. Collected up @@ -1613,6 +2046,14 @@ def line_at(lineno: int) -> str: ) if kind is None: findings.append(("journalkind", rel, node.lineno, line_at(node.lineno), fn_name)) + else: + # The literal-kind twin, and the KIND inventory's only feed. NOT + # derivable from the `journalfield` rows below, although each of + # those carries the kind: a kind-only write (`run-complete` and + # three siblings) has no keyword row to ride on. + findings.append( + ("journalkindliteral", rel, node.lineno, line_at(node.lineno), kind) + ) for kw in node.keywords: if kw.arg is not None: findings.append( @@ -1865,6 +2306,100 @@ def record_mint(value: ast.expr, *, bare_at_depth: bool) -> None: ) ) + # The literal kinds that reach a declared dynamic-kind POSITION from outside it: + # a `kind="..."` keyword at a call to one of this file's + # `JOURNAL_DYNAMIC_KIND_ALLOW` functions, and that function's own `kind` + # parameter default. The write inside such a position spells a parameter, so + # the journal-write arm above reports it as `journalkind` and nothing more — + # which is how `review-skipped-awaiting-operator` and its three siblings + # reached the journal with no inventory row anyone had to decide on (review + # pass 2). Same `journalkindliteral` finding, same inventory; keyed `(file, + # name)` exactly like the position it serves, so a same-named callee in a file + # that declares no such position stays silent. + # Where each declared position keeps its `kind`, so a caller that spells the + # kind POSITIONALLY is read too. Keyed by name within this file, exactly like + # the allow set it is derived from. + kind_positions = { + node.name: _kind_param_index(node) + for node in ast.walk(tree) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and (rel, node.name) in JOURNAL_DYNAMIC_KIND_ALLOW + } + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and (rel, _called_name(node.func)) in JOURNAL_DYNAMIC_KIND_ALLOW + ): + for kw in node.keywords: + if kw.arg != "kind": + continue + # A spelled-but-unreadable `kind=` is unresolvable, not absent, for + # the reason `_positional_kind_literal` states: the write inside a + # declared position spells a parameter and the literalness test is + # waived there, so skipping it lets an undeclared kind reach the + # journal with nothing red. + findings.append( + ( + "journalkindliteral", + rel, + node.lineno, + line_at(node.lineno), + ( + kw.value.value + if isinstance(kw.value, ast.Constant) + and isinstance(kw.value.value, str) + else UNRESOLVED_DYNAMIC_KIND + ), + ) + ) + # A declared FORWARDER (`plugins/bus.py::_log`) is itself a journal + # write, so the main emit above already read its positional kind; this + # arm exists for the declared positions that are not forwarders, where + # nothing else reads the slot. + index = kind_positions.get(_called_name(node.func)) + if ( + index is not None + and not any(kw.arg == "kind" for kw in node.keywords) + and not _is_journal_write(node, rel) + ): + literal = _positional_kind_literal(node, index) + if literal is not None: + findings.append( + ("journalkindliteral", rel, node.lineno, line_at(node.lineno), literal) + ) + elif ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and (rel, node.name) in JOURNAL_DYNAMIC_KIND_ALLOW + ): + default = _kind_param_default(node) + if default is not None: + findings.append( + ("journalkindliteral", rel, node.lineno, line_at(node.lineno), default) + ) + + # Every refusal-helper DEFINITION (`_refuse_*` / `_reject_*`) and every + # #414-family isolation-refusal CALL — the two surfaces `REFUSAL_HELPER_DEFS` + # and `ISOLATION_CONFLICT_CALLERS` enumerate. The def side needs no alias + # resolution (a definition IS its name); the call side resolves both guarded + # names through `_call_aliases`, exactly like the re-arm detector above. + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith( + ("_refuse_", "_reject_") + ): + findings.append(("refusaldef", rel, node.lineno, line_at(node.lineno), node.name)) + if isinstance(node, ast.Call) and _names_isolation_refusal( + node.func, isolation_aliases, isolation_wrapper_aliases + ): + findings.append( + ( + "isolationcall", + rel, + node.lineno, + line_at(node.lineno), + enclosing_names.get(id(node)), + ) + ) + return findings @@ -2312,6 +2847,85 @@ def test_run_state_writer_and_transaction_inventory_is_complete(): assert _production_call_sites("state_lock") == RUN_STATE_TRANSACTIONS +def test_refusal_helper_inventory_is_complete(): + """Every `_refuse_*`/`_reject_*` helper definition in the tree has a declared + row, in both directions. Review iteration 6 (and four bot rounds after it) kept + finding one shape by hand — a refusal landed with no test row, caught only by a + later review pass — and the journal-FIELD inventory beside this one is the gate + that caught `reaches_redrive`; this is the same gate for the refusal surface. A + row here is the PR-time decision, not the test itself: the helper's refusal + behavior still needs its own test landed with the row. + + Anti-vacuity is structural, the exact-inventory property the rearm Counter + relies on: the declared side is non-empty, so a scan that stops finding + definitions reddens the staleness direction instead of passing green. + + Graded as a Counter WITH multiplicity, not a set of names — + `ISOLATION_CONFLICT_CALLERS`' rationale, applied to definitions: a SECOND def + of a declared name in the same file (a platform-conditional twin, say) is a new + refusal body the emit reports twice, and a set comparison absorbed it silently + (measured). Every declared row's count is 1 today, which `Counter` over the set + encodes. + + Ablation: delete the `refusaldef` emit and this reddens with all nine declared + rows stale; add `def _refuse_nothing()` to `runs.py` and this reddens naming + it; add a platform-conditional TWIN def of `_refuse_live_session` to `runs.py` + and the count comparison reddens with the set of names unchanged.""" + findings = _of("refusaldef") + scanned = Counter((rel, name) for _, rel, _, _, name in findings) + declared = Counter(REFUSAL_HELPER_DEFS) + changed = {key for key in set(scanned) | set(declared) if scanned[key] != declared[key]} + detail = [ + f" {rel}:{ln}: {name} — {txt.strip()}" + for _, rel, ln, txt, name in findings + if (rel, name) in changed + ] + assert scanned == declared, ( + "the `_refuse_*`/`_reject_*` helper definitions moved. A NEW helper — a " + "second same-named def in one file included — lands WITH its (file, name) " + "row and the test asserting what it refuses in the same PR; a helper no " + "module defines any more loses its row, which otherwise stands as a " + "pre-approval for the next helper that reuses the name:\n" + f" scanned: {sorted(scanned.elements())}\n" + f" declared: {sorted(declared.elements())}\n" + "\n".join(detail) + ) + + +def test_isolation_conflict_refusal_sites_are_enumerated(): + """The #414 refusal is reached from exactly the declared call sites, counted + WITH multiplicity — both the `bmadconfig.worktree_isolation_conflict` predicate + and its CLI wrapper `_reject_isolation_conflict`, so a new surface reaching the + pair through either spelling reddens this row. That is the `96aa09a9` shape: + `cmd_resolve`'s pre-session refusal landed as a wrapper call with no test row, + and nothing structural named the omission until a review pass did. + + Multiplicity is load-bearing on today's tree — `cmd_resolve` legitimately calls + the wrapper twice, so a set of keys would absorb a third call there silently + (`test_isolation_callsite_count_does_not_hide_a_second_call_in_one_function` + pins the counting itself). + + Ablation: delete the `isolationcall` emit and this reddens (eleven declared, + zero scanned); add a third `_reject_isolation_conflict` call inside + `cmd_resolve` and the count comparison reddens naming the site.""" + findings = _of("isolationcall") + sites = _isolation_callsite_counts(findings) + declared = Counter(ISOLATION_CONFLICT_CALLERS) + changed = {key for key in set(sites) | set(declared) if sites[key] != declared[key]} + detail = [ + f" {rel}:{ln}: in {fn or ''} — {txt.strip()}" + for _, rel, ln, txt, fn in findings + if (rel, fn) in changed + ] + assert sites == declared, ( + "the #414-family refusal call sites moved (worktree_isolation_conflict / " + "_reject_isolation_conflict). A new surface refusing the pair lands WITH " + "its own refusal test in the same PR; a removed one deletes its row — " + "update ISOLATION_CONFLICT_CALLERS only alongside that decision:\n" + f" scanned: {sorted(sites.elements())}\n" + f" declared: {sorted(declared.elements())}\n" + "\n".join(detail) + ) + + def _journal_field_offenders(findings) -> list[tuple[str, int, str, str]]: """The routing invariant as a filter, in the two directions a finding can fail: a field name that neither ``diagnostics`` nor the benign inventory accounts for, @@ -2435,6 +3049,101 @@ def test_journal_kinds_are_literal_or_the_position_is_declared(): ) +def test_journal_kind_inventory_is_complete(): + """Every literal journal kind a producer writes is a declared `JOURNAL_KINDS` + row, in both directions — the enumerate-vs-declare gate for the kind axis. The + ~196 literal kinds had no inventory at all: a new record kind could land, with + or without a test row, and only a later review pass would ask what covers it. + Now the question is asked by CI, at PR time, on the diff that introduces the + kind. + + Both directions matter. A NEW kind fails the undeclared arm naming its file, + line and kind; a RENAME fails both arms at once — the new spelling undeclared, + the old row stale — so the old row cannot survive as a pre-approval for the + next kind that reuses it. The staleness arm's bound is stated on + `JOURNAL_KINDS`: it sees the union of producers, so one writer of a SHARED + kind can drop it without reddening anything while another still writes it. + + A declared dynamic-kind position writes a parameter, so its kinds are read + where a literal reaches it — a caller's `kind="..."` keyword, or the parameter + default — by the emit's second arm (`JOURNAL_KINDS`' header); only the f-string + family is absent, by `JOURNAL_KINDS`' stated bound, and the sibling literalness + test above governs whether a POSITION may be dynamic at all. Consumer-side kind + parity — readers matching kinds by literal — stays DW-82's, out of scope here. + + Anti-vacuity is structural: the declared set is non-empty, so deleting the + `journalkindliteral` emit reddens the staleness arm with the entire inventory + rather than passing green. + + Both arms are graded from ONE scan in ONE assertion + (`_journal_kind_inventory_drift`): as two sequential asserts a rename reported + only the undeclared spelling, and the stale row surfaced a run later, after the + new row had landed (review pass 2). + + Ablation: delete the `journalkindliteral` emit and this reddens with all 204 + rows stale; duplicate engine.py's epic-boundary write under the kind + `"guard-ablation-probe"` and ONLY this test reddens, naming the kind and site; + add `self._skip_review_and_commit(task, kind="guard-ablation-probe")` to + engine.py and ONLY this test reddens, naming the call; rename engine.py's + `epic-boundary` write and the ONE failure names both the new spelling and the + stale row.""" + undeclared, stale = _journal_kind_inventory_drift(_of("journalkindliteral")) + assert (undeclared, stale) == ([], set()), ( + "the literal journal kinds and JOURNAL_KINDS disagree. A kind a producer " + "writes but no row declares — add its row IN THE SAME PR as what covers its " + "record: a diagnostics routing row if any field carries an identifier, a " + "path or free text, and the test asserting the record at the layer that " + "reads it; or drop the write. A row no producer writes any more — delete it " + "and retire its routing/test rows deliberately, because a stale row " + "pre-approves the next record that reuses the name:\n" + + "\n".join( + f" undeclared {rel}:{ln}: {kind!r} — {txt.strip()}" + for rel, ln, txt, kind in undeclared + ) + + ("\n" if undeclared and stale else "") + + "\n".join(f" stale row: {kind!r}" for kind in sorted(stale)) + ) + + +def _journal_kind_inventory_drift( + findings, +) -> tuple[list[tuple[str, int, str, str]], set[str]]: + """Both arms of the kind inventory from one set of `journalkindliteral` + findings: the literal kinds written but undeclared (with their sites), and the + declared rows nothing writes any more. Returned together so the inventory test + can grade them in one assertion — a rename is one defect with two faces.""" + scanned = {kind for _, _, _, _, kind in findings} + undeclared = [ + (rel, ln, txt, kind) for _, rel, ln, txt, kind in findings if kind not in JOURNAL_KINDS + ] + return undeclared, JOURNAL_KINDS - scanned + + +def test_journal_kind_inventory_drift_reports_a_rename_on_both_arms(): + """A rename is one undeclared spelling AND one stale row, from the same findings. + + Ablation: make `_journal_kind_inventory_drift` return the stale arm only when + the undeclared arm is empty (the sequential-assert shape) and this reddens.""" + synthetic = [ + ("journalkindliteral", "engine.py", 1, f'journal.append("{kind}")', kind) + for kind in sorted(JOURNAL_KINDS) + if kind != "epic-boundary" + ] + [ + ( + "journalkindliteral", + "engine.py", + 7728, + 'self.journal.append("epic-boundary-renamed", epic=e)', + "epic-boundary-renamed", + ) + ] + undeclared, stale = _journal_kind_inventory_drift(synthetic) + assert [(rel, ln, kind) for rel, ln, _, kind in undeclared] == [ + ("engine.py", 7728, "epic-boundary-renamed") + ] + assert stale == {"epic-boundary"} + + def test_journal_field_guard_actually_saw_the_producers(): """The sibling assertion is an ABSENCE, so it is green both when every field is accounted for and when the scan stopped finding journal writes at all. This is @@ -3883,15 +4592,196 @@ def test_rearm_call_detector_stays_silent_on_non_calls(label, source): assert not found, f"the {label!r} shape produced a `rearmcall` finding:\n{source}" +# The refusal-helper detector's matrix: `(label, source, expected def names)`. The +# surface is DEFINITIONS — a new `_refuse_*`/`_reject_*` helper is a new refusal +# behavior that must land with an inventory row and its own test — so calls, +# lookalike prefixes and prose must all stay silent or the inventory fills with +# noise it cannot force a decision about. +REFUSAL_DEF_PROBES = [ + ( + "plain-def", + "def _refuse_live_session(project, run_id, verb):\n return None\n", + {"_refuse_live_session"}, + ), + ( + "reject-spelling", + "def _reject_bad_run_id(run_id):\n return None\n", + {"_reject_bad_run_id"}, + ), + ( + "async-def", + "async def _refuse_slow_probe(target):\n return None\n", + {"_refuse_slow_probe"}, + ), + # A method is a definition too — `engine.Engine._refuse_gated_story` is one of + # the nine rows the real tree declares. + ( + "method-def", + "class Engine:\n def _refuse_gated_story(self, story_key):\n return None\n", + {"_refuse_gated_story"}, + ), +] +REFUSAL_DEF_NON_PROBES = [ + # A CALL is not a definition: call sites belong to each helper's own tests, and + # flagging them would report every use as a new refusal behavior. + ("call-not-a-def", 'def f():\n _refuse_live_session(project, run_id, "stop")\n'), + # The prefix is `_refuse_`/`_reject_` WITH the trailing underscore: a name that + # merely starts `_refus` is not claiming to be a refusal helper. + ("similar-prefix", "def _refusal_note(story_key):\n return None\n"), + # …and a public spelling makes no `_refuse_*` claim either. + ("public-spelling", "def refuse_everything():\n return None\n"), + # Prose naming a helper is a Constant, not a def. + ("prose", 'def f():\n """Calls _refuse_live_session first."""\n return 1\n'), +] + + +@pytest.mark.parametrize( + ("label", "source", "expected"), REFUSAL_DEF_PROBES, ids=[p[0] for p in REFUSAL_DEF_PROBES] +) +def test_refusal_def_detector_flags_every_definition_shape(label, source, expected): + """Each definition shape is found and reported by name. `runs.py` is passed + because nothing in this detector is file-scoped — the enumeration lives in the + tree-wide inventory test, not here. + + Ablation: delete the `refusaldef` emit and every row here reddens.""" + found = {f[4] for f in _scan_source(source, "runs.py") if f[0] == "refusaldef"} + assert found == expected, f"the {label!r} shape resolved to {sorted(found)}:\n{source}" + + +@pytest.mark.parametrize( + ("label", "source"), REFUSAL_DEF_NON_PROBES, ids=[p[0] for p in REFUSAL_DEF_NON_PROBES] +) +def test_refusal_def_detector_stays_silent_on_lookalikes(label, source): + """A call, a lookalike prefix, a public spelling and prose are not refusal-helper + definitions. The inventory is an equality assertion, so a false positive fails as + loudly as a miss. + + Ablation: widen the emit's prefix match to `_refus` and the similar-prefix row + reddens.""" + found = [f for f in _scan_source(source, "runs.py") if f[0] == "refusaldef"] + assert not found, f"the {label!r} shape produced a `refusaldef` finding:\n{source}" + + +# The #414-family call detector's matrix: `(label, source, expected enclosing +# function)`. Both spellings of the refusal are probed — the `bmadconfig` predicate +# and the rc-returning CLI wrapper — because a new surface can reach the pair +# through either, and the `96aa09a9` site (cmd_resolve's pre-session arm) arrived +# through the wrapper. +ISOLATION_CALL_PROBES = [ + ( + "qualified-predicate-call", + "def cmd_validate(args):\n" + " conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation)\n", + "cmd_validate", + ), + ( + "bare-wrapper-call", + "def cmd_run(args):\n" + " if (rc := _reject_isolation_conflict(paths, pol)) is not None:\n" + " return rc\n", + "cmd_run", + ), + # A rename-on-import and an assignment alias are just as callable — the + # `_call_aliases` shapes, one per guarded name. + ( + "renamed-predicate-import", + "from .bmadconfig import worktree_isolation_conflict as conflict_for\n" + "def f(args):\n" + " conflict_for(paths, isolation)\n", + "f", + ), + ( + "assigned-wrapper-alias", + "check = _reject_isolation_conflict\ndef f(args):\n check(paths, pol)\n", + "f", + ), +] +ISOLATION_CALL_NON_PROBES = [ + # The definitions are not calls. The wrapper's own predicate call is a real + # finding on today's tree — `("cli.py", "_reject_isolation_conflict")` is a row + # of the declared Counter — so the bodies here are stubs on purpose. + ( + "predicate-definition", + "def worktree_isolation_conflict(paths, isolation):\n return None\n", + ), + ("wrapper-definition", "def _reject_isolation_conflict(paths, pol):\n return None\n"), + # A different function whose name merely embeds the guarded one. + ("similar-name", "def f():\n worktree_isolation_conflicts(paths)\n"), + ("reference-not-a-call", "def f():\n handler = bmadconfig.worktree_isolation_conflict\n"), + ( + "prose", + 'def f():\n """bmadconfig.worktree_isolation_conflict(paths, mode) decides."""\n' + " return 1\n", + ), +] + + +@pytest.mark.parametrize( + ("label", "source", "fn"), ISOLATION_CALL_PROBES, ids=[p[0] for p in ISOLATION_CALL_PROBES] +) +def test_isolation_call_detector_reports_the_site(label, source, fn): + """Each call shape is found and attributed to its enclosing function — the key + the declared Counter is built on. `cli.py` is passed because nothing in this + detector is file-scoped. + + Ablation: delete the `isolationcall` emit and every row here reddens.""" + found = [f for f in _scan_source(source, "cli.py") if f[0] == "isolationcall"] + assert len(found) == 1, f"the {label!r} shape produced {len(found)} findings:\n{source}" + assert found[0][4] == fn, f"the {label!r} shape attributed to {found[0][4]!r}" + + +@pytest.mark.parametrize( + ("label", "source"), ISOLATION_CALL_NON_PROBES, ids=[p[0] for p in ISOLATION_CALL_NON_PROBES] +) +def test_isolation_call_detector_stays_silent_on_non_calls(label, source): + """Definitions, a similarly-named neighbour, a bare reference and prose are not + call sites. The tree-wide assertion is a Counter equality, so a false positive + fails as loudly as a miss. + + Ablation: relax `_names_guarded_verify_call`'s name equality to a substring + match and the similar-name row reddens.""" + found = [f for f in _scan_source(source, "cli.py") if f[0] == "isolationcall"] + assert not found, f"the {label!r} shape produced an `isolationcall` finding:\n{source}" + + +def _isolation_callsite_counts(findings) -> Counter: + """Call-site multiplicity, not just distinct enclosing functions — the + `_rearm_callsite_counts` idiom, and load-bearing on the real tree: + `cli.cmd_resolve` legitimately calls the wrapper twice.""" + return Counter((rel, fn) for _, rel, _, _, fn in findings) + + +def test_isolation_callsite_count_does_not_hide_a_second_call_in_one_function(): + """Ablation: collapse `_isolation_callsite_counts` to a set of keys and this + reddens — `cmd_resolve` would then absorb a third call silently.""" + source = ( + "def cmd_resolve(args):\n" + " if (rc := _reject_isolation_conflict(paths, pol)) is not None:\n" + " return rc\n" + " if (rc := _reject_isolation_conflict(paths, pol)) is not None:\n" + " return rc\n" + ) + found = [f for f in _scan_source(source, "cli.py") if f[0] == "isolationcall"] + assert _isolation_callsite_counts(found) == Counter({("cli.py", "cmd_resolve"): 2}) + + # The journal detector's probe matrix, as `(label, source, expected)` where # `expected` is the exact set of field names the scan must extract — `None` standing # for an unresolvable splat. Asserting the SET rather than "something was found" is # what makes a partial splat resolution fail here instead of quietly under-reporting. JOURNAL_FIELD_PROBES = [ - # The three receiver spellings in the tree. + # The four receiver spellings in the tree. ("self-journal", 'self.journal.append("k", story_key=s, patch=p)\n', {"story_key", "patch"}), ("bare-journal", 'journal.append("k", branch=b)\n', {"branch"}), ("private-journal", "self._journal.append(kind, plugin=name)\n", {"plugin"}), + # The constructor-inline spelling `Journal(run_dir).append(...)` — three live + # sites in runs.py use it, and the receiver is an ast.Call, so the named-handle + # match alone left them (and their kinds and fields) entirely unscanned. + ( + "constructor-inline-receiver", + 'def f(run_dir):\n Journal(run_dir).append("k", pid=1)\n', + {"pid"}, + ), # A splat resolved through the literal stores that build it, in both store # shapes and across the conditional-dict form `engine._run_inner` uses. ( @@ -4016,6 +4906,10 @@ def test_journal_forwarder_calls_enter_the_inventory(label, rel, source, expecte # the most common in the language, so anchoring on the receiver is load-bearing. ("list-append", "results.append(SessionResult(status=s, stop_seen=True))\n"), ("attribute-list-append", "self.entries.append(dict(kind=k, story_key=s))\n"), + # A constructor that merely ends in a `.append` is not a journal write unless + # the constructed thing IS a Journal — the constructor arm is name-anchored + # exactly like the handle arm. + ("constructor-lookalike", 'NotAJournal(run_dir).append("k", pid=1)\n'), # A journal write with no fields at all produces nothing to route. ("kind-only", 'self.journal.append("run-start")\n'), # Prose naming the call is a Constant, not a Call. @@ -4173,6 +5067,225 @@ def test_journal_kind_probes_flag_a_non_literal_kind(): assert not [f for f in _scan_source(source, "sweep.py") if f[0] == "journalkind"], source +def test_journal_kind_literal_probes_extract_the_kind(): + """The kind inventory's detector half: a journal write whose kind IS a string + literal emits that kind — including a kind-only write (`run-complete` and three + siblings), which the FIELD detector never reports because there is no keyword + to carry it, and a declared forwarder's call site, whose kind would otherwise + stop at `plugins/bus.py::_log`'s wall. + + Ablation: delete the `journalkindliteral` emit and every row here reddens.""" + for source, rel, kind in ( + ( + 'def f(self):\n self.journal.append("run-start", story_key=s)\n', + "sweep.py", + "run-start", + ), + # The kind-only shape: no keywords, so no `journalfield` finding exists to + # derive the kind from — this emit is the only reader. + ('def f(self):\n journal.append("run-complete")\n', "engine.py", "run-complete"), + ( + 'def f(self):\n self._journal.append("plugin-loaded", plugin=name)\n', + "plugins/registry.py", + "plugin-loaded", + ), + ('def f(self):\n self._log("plugin-hook", rc=rc)\n', "plugins/bus.py", "plugin-hook"), + # The kinds a declared dynamic-kind POSITION receives from outside it: the + # literal `kind=` a caller hands it, and the position's own parameter + # default — keyword-only (`engine._skip_review_and_commit`) or + # positional-or-keyword (`sweep._close_bundle_ledger_when_spec_status`). + # The write inside spells a parameter, so nothing else reads these. + ( + 'def f(self):\n self._skip_review_and_commit(task, kind="review-skipped-awaiting-operator")\n', + "engine.py", + "review-skipped-awaiting-operator", + ), + ( + 'def _skip_review_and_commit(self, task, *, kind="review-skipped"):\n' + " self.journal.append(kind, story_key=s)\n", + "engine.py", + "review-skipped", + ), + ( + "def _close_bundle_ledger_when_spec_status(self, task, spec_file, status, " + 'kind="sweep-bundle-closed"):\n return None\n', + "sweep.py", + "sweep-bundle-closed", + ), + ): + found = [f[4] for f in _scan_source(source, rel) if f[0] == "journalkindliteral"] + assert found == [kind], f"extracted {found} from:\n{source}" + + +def test_journal_kind_literal_probes_stay_silent_on_lookalikes(): + """The complement: a non-literal kind (the literalness test's territory), an + `.append` on a non-journal receiver, a forwarder NAME outside its declared + file, and prose are all silent — the inventory must not fill itself with + strings that never reach `Journal.append`. + + Ablation: drop `_is_journal_write`'s receiver anchor (accept any `.append`) and + the list-append row reddens.""" + for source, rel in ( + ("def f(self):\n self.journal.append(kind, story_key=s)\n", "sweep.py"), + ( + 'def f(self):\n self.journal.append(f"{family}-pruned", count=n)\n', + "recovery_flow.py", + ), + ('def f(self):\n results.append("done")\n', "sweep.py"), + ('def f(self):\n self._log("plugin-hook", rc=rc)\n', "stories_engine.py"), + ( + 'def f():\n """journal.append("prose-kind") is described here."""\n return 1\n', + "sweep.py", + ), + # The forwarder-kind arm is keyed `(file, name)` like the position it + # serves: the same call in a file that declares no such position, a + # `kind=` keyword on an undeclared callee, and a non-string default on its + # def are all silent. + ( + 'def f(self):\n self._skip_review_and_commit(task, kind="review-skipped")\n', + "sweep.py", + ), + ('def f(self):\n self.emit(kind="review-skipped")\n', "engine.py"), + ("def _skip_review_and_commit(self, task, *, kind=None):\n return None\n", "engine.py"), + ): + assert not [f for f in _scan_source(source, rel) if f[0] == "journalkindliteral"], source + + # The one shape here that is NOT silent, and used to be. A non-literal `kind=` AT + # a declared position is unresolvable rather than absent: `JOURNAL_DYNAMIC_KIND_ALLOW` + # waives the literalness test for the write inside, so the inventory is the only + # arm left that can fail loud on it. + # + # Ablation: return None instead of the sentinel for a non-literal `kind=` and this + # reddens. + unresolvable = [ + f[4] + for f in _scan_source( + "def f(self):\n self._skip_review_and_commit(task, kind=chosen)\n", "engine.py" + ) + if f[0] == "journalkindliteral" + ] + assert unresolvable == [UNRESOLVED_DYNAMIC_KIND], unresolvable + + +# The declared position whose `kind` is positional-or-keyword, with NO default, so the +# definition arm contributes nothing and each row's only finding is the one the +# positional arm read. `self` is dropped from the parameter list because a bound call +# never fills its slot — the offset this arm has to get right. +_POSITIONAL_KIND_DEF = ( + "def _close_bundle_ledger_when_spec_status(self, task, spec_file, status, kind):\n" + " self.journal.append(kind, story_key=s)\n" + "\n" +) + + +def test_journal_kind_literal_reads_a_positional_dynamic_kind(): + """A literal handed a declared dynamic-kind position POSITIONALLY is inventoried, + not just a `kind=` keyword. + + `sweep._close_bundle_ledger_when_spec_status(task, spec, status, "new-kind")` is + legal Python — `kind` is positional-or-keyword — and reached the journal with no + `JOURNAL_KINDS` row anyone had to decide on: the arm read `node.keywords` only, so + the kind was never graded and the inventory reported itself complete. The three + ways a literal reaches such a position (keyword, POSITIONAL, parameter default) now + all feed the same emit. + + The unresolvable row is the fail-loud half: a `*args` splat covering the slot yields + a kind no row can declare, so the inventory reddens naming the site instead of + sharing its silence with "this call passed no literal". + + Ablation: restore the `for kw in node.keywords` loop as the arm's only reader and + the first row reddens; delete the `ast.Starred` branch and the second does.""" + for source, rel, kinds in ( + ( + _POSITIONAL_KIND_DEF + "def caller(self):\n" + ' self._close_bundle_ledger_when_spec_status(task, spec, status, "sweep-bundle-closed")\n', + "sweep.py", + ["sweep-bundle-closed"], + ), + ( + _POSITIONAL_KIND_DEF + "def caller(self):\n" + ' self._close_bundle_ledger_when_spec_status(*rest, "sweep-bundle-closed")\n', + "sweep.py", + [UNRESOLVED_DYNAMIC_KIND], + ), + ): + found = [f[4] for f in _scan_source(source, rel) if f[0] == "journalkindliteral"] + assert found == kinds, f"extracted {found} from:\n{source}" + + +def test_journal_kind_literal_positional_arm_stays_silent_on_lookalikes(): + """The complement, in the four directions the positional arm must not invent a kind: + a KEYWORD-ONLY `kind` (no positional slot exists, so a string in that argument + position is some other parameter's), an EMPTY slot (the parameter default, which the + definition arm reports instead — `sweep.py`'s own + `_close_bundle_ledger_when_spec_status(task, str(spec_file), success_status)` call + relies on it, so flagging an omitted `kind` would redden the clean tree), the same + call in a file declaring no such position, and a declared FORWARDER whose positional + kind the journal-write emit already reports — double-reporting one site would make a + `found == [kind]` probe redden for a reason that is not a defect. + + A slot OCCUPIED by a non-literal is the direction that is deliberately not here: it + is asserted RED below, because the literalness test that would otherwise catch it is + waived at a declared position. + + Ablation: drop the `_is_journal_write` guard and the forwarder row reddens with two + findings; key `kind_positions` by name alone and the wrong-file row reddens; fold the + empty slot into `UNRESOLVED_DYNAMIC_KIND` and the omitted-`kind` row reddens.""" + for source, rel in ( + ( + "def _skip_review_and_commit(self, task, *, kind):\n" + " self.journal.append(kind, story_key=s)\n" + "\n" + "def caller(self):\n" + ' self._skip_review_and_commit(task, "review-skipped")\n', + "engine.py", + ), + ( + _POSITIONAL_KIND_DEF + "def caller(self):\n" + " self._close_bundle_ledger_when_spec_status(task, spec, status)\n", + "sweep.py", + ), + ( + _POSITIONAL_KIND_DEF + "def caller(self):\n" + ' self._close_bundle_ledger_when_spec_status(task, spec, status, "sweep-bundle-closed")\n', + "stories_engine.py", + ), + ): + assert not [f for f in _scan_source(source, rel) if f[0] == "journalkindliteral"], source + + # The slot this arm used to share with "no literal passed": OCCUPIED by a + # non-literal. `JOURNAL_DYNAMIC_KIND_ALLOW` waives the literalness test for the + # write inside the position, so the inventory is the only arm that can fail loud — + # and it stayed green while a variable carrying an undeclared kind reached the + # journal. + # + # Ablation: return None instead of the sentinel for a non-literal slot and this + # reddens. + unresolvable = [ + f[4] + for f in _scan_source( + _POSITIONAL_KIND_DEF + "def caller(self):\n" + " self._close_bundle_ledger_when_spec_status(task, spec, status, chosen)\n", + "sweep.py", + ) + if f[0] == "journalkindliteral" + ] + assert unresolvable == [UNRESOLVED_DYNAMIC_KIND], unresolvable + + # The forwarder: exactly one finding, from the journal-write emit, not two. + forwarder = ( + "def _log(self, kind, **fields):\n" + " self._journal.append(kind, **fields)\n" + "\n" + "def caller(self):\n" + ' self._log("plugin-hook", rc=rc)\n' + ) + found = [ + f[4] for f in _scan_source(forwarder, "plugins/bus.py") if f[0] == "journalkindliteral" + ] + assert found == ["plugin-hook"], found + + def test_journal_routing_tables_are_read_from_diagnostics(): """`JOURNAL_ROUTED_FIELDS` and `JOURNAL_KIND_ROUTED_FIELDS` are built from the live `diagnostics` tables, not copied, so the guard cannot drift from the module diff --git a/tests/test_resolve.py b/tests/test_resolve.py index dbe56916..0378a2e2 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -1476,6 +1476,30 @@ def test_rearm_journals_a_skip_when_the_recorded_spec_is_not_readable(tmp_path): assert load_state(run_dir).tasks["6-4-cli-list-command"].phase == Phase.PENDING +@pytest.mark.parametrize("isolated", [True, False]) +def test_rearm_records_the_redrive_mode_on_a_skipped_flip(project, isolated): + """The mode rides the record because the renderer cannot re-derive it. + + Same argument as `refused` one field above: the operator surfaces read this journal + OUT OF PROCESS, with neither the task nor the live policy to measure. The sibling + `rearm-spec-write-unreachable` already writes exactly this field for exactly this + reason; before it, the flip-skipped renderer inferred the mount from + `reaches_redrive` and told an isolated run it mounts no worktree. + + Both legs use the same unreachable-spec shape, so the mode is the only thing that + varies — the record must follow `isolated_redrive`, not the run's shape. + + Ablation: drop the `redrive=` kwarg from the producer's `journal.append` and both + legs redden on `KeyError`; hard-code either literal and one leg reddens. + """ + run_dir, _, _ = _escalated_run(project.project, spec_file="wt/_bmad-output/specs/gone.md") + + runs.rearm_escalation(run_dir, isolated_redrive=isolated, resolution_recorded=True) + + (flip,) = [e for e in _kinds(run_dir) if e["kind"] == "rearm-spec-flip-skipped"] + assert flip["redrive"] == ("isolated" if isolated else "in-place") + + @pytest.mark.parametrize("repo", [True, False]) def test_rearm_records_an_unreachable_spec_even_when_the_advance_failed(tmp_path, repo): """The two #640 degrades COMPOSE; they do not substitute for each other. @@ -4945,7 +4969,14 @@ def test_rearm_writes_the_project_rooted_spec_when_no_worktree_was_recorded(tmp_ @pytest.mark.parametrize( ("field", "value"), - [("files", 3), ("files", None), ("files", [1, 2]), ("commits", 3), ("commits", None)], + [ + ("files", 3), + ("files", None), + ("files", [1, 2]), + ("files", "new.txt"), + ("commits", 3), + ("commits", None), + ], ) def test_rearm_event_notice_survives_a_journal_shape_json_admits(field, value): """A malformed journal line must not raise out of either surface's `finally`. @@ -4974,6 +5005,32 @@ def test_rearm_event_notice_survives_a_journal_shape_json_admits(field, value): assert isinstance(message, str) +def test_rearm_event_notice_does_not_spell_a_bare_string_field_letter_by_letter(): + """The one shape `_journal_sequence`'s guard exists for, and the one its sibling + parametrization cannot grade. + + That row asserts only that no exception escapes, which a widened guard satisfies + too. `_journal_sequence`'s docstring gives the actual reason it refuses to iterate + a `str`: `", ".join("abc")` renders `"a, b, c"`, so a bare string would reach the + operator spelled out one character at a time. Nothing pinned that until here. + + Scoped honestly: no first-party producer can emit this. Both writers of these + fields pass lists (`verify.patch_new_files`, `verify.commits_above`), so the guard + is defensive against a hand-edited or third-party journal line — the same threat + model the sibling row's docstring invokes, `Journal.entries()` doing `json.loads` + with no shape filter. + + Ablation: widen the guard to `isinstance(value, (list, tuple, str))` and this + reddens on `n, e, w`; every row of the sibling parametrization stays green. + """ + notice = runs.rearm_event_notice({"kind": "stale-restore-excluded", "files": "new.txt"}) + + assert notice is not None + _severity, message, _next_step = notice + assert "new.txt" in message + assert "n, e, w" not in message + + def test_rearm_event_notice_splits_the_flip_skip_on_the_refusal(): """One kind, two outcomes — and the operator-facing halves must not be swapped. @@ -5006,6 +5063,101 @@ def test_rearm_event_notice_splits_the_flip_skip_on_the_refusal(): assert step == "" +def test_rearm_event_notice_does_not_promise_a_worktree_to_a_run_without_one(): + """`refused=False` is reached for TWO disjoint reasons, and the record's own + discriminator is what tells them apart out of process. + + `refused = spec_path.is_file() and write_reaches_the_redrive`. The sibling row above + feeds only the SECOND failure — a worktree-local copy the re-drive discards — and + pins "COMMITTED spec" as correct for it. On the first failure the re-drive reads that + same path, so telling the operator the failed flip is harmless is wrong at exactly + the moment it is not. The producer's own row + `test_rearm_journals_a_skip_when_the_recorded_spec_is_not_readable` builds that + state, with an empty `worktree_path` and a missing spec. + + The IN-PLACE leg is the one graded here, and it says so on the record: "mounts no + worktree" is a claim about the mode, not about reachability, and + `test_rearm_event_notice_takes_the_mount_claim_from_the_record` grades the two legs + where inferring it from `reaches_redrive` asserted the opposite of the truth. + + A record written before `reaches_redrive` existed keeps the wording it was written + under — asserted, because the alternative is a reader silently re-classifying old + journals it cannot re-derive the answer for. + + Ablation: delete the `if entry.get("reaches_redrive")` branch and the first leg + reddens on the worktree sentence; return the new branch unconditionally and the + absent-field leg reddens instead. + """ + entry = { + "kind": "rearm-spec-flip-skipped", + "spec_file": "specs/s1.md", + "status": "ready-for-dev", + "refused": False, + } + + _, unreadable, unreadable_step = runs.rearm_event_notice( + {**entry, "reaches_redrive": True, "redrive": "in-place"} + ) + assert "COMMITTED spec" not in unreadable + assert "mounts no worktree" in unreadable + assert unreadable_step # this leg HAS a remedy: the recorded path is wrong + + _, discarded, _ = runs.rearm_event_notice({**entry, "reaches_redrive": False}) + assert "COMMITTED spec" in discarded + + # a pre-`reaches_redrive` record is not re-classified + _, legacy, _ = runs.rearm_event_notice(entry) + assert "COMMITTED spec" in legacy + + +def test_rearm_event_notice_takes_the_mount_claim_from_the_record(): + """`reaches_redrive` does NOT imply "no worktree", and inferring it asserted the + opposite of the truth on the isolated leg. + + `spec_reaches_the_redrive`'s isolated arm answers True through + `_spec_is_shared_with_the_redrive` — an artifact dir configured outside the project + tree, reachable precisely BECAUSE every checkout sees that one file, with a worktree + very much mounted. The renderer runs out of process and cannot re-derive the mode, so + the producer records it, exactly as the sibling `rearm-spec-write-unreachable` does. + + Only the mount clause is at stake: "the re-drive reads that same path" is what + `reaches_redrive` alone proves, and it is asserted on every leg here. + + An ABSENT `redrive` drops the clause rather than defaulting. The sibling's + "absent means isolated" is sound only because its in-place arm is newer than the + field; this kind was journalled from BOTH modes before the field existed, so absent + is genuinely unknown and a guess would be the same defect in the other direction. + + Ablation: hard-code `mode = "in-place"` and the isolated and absent legs redden; + default the lookup to `"isolated"` (the sibling's rule) and the absent leg reddens + alone. + """ + entry = { + "kind": "rearm-spec-flip-skipped", + "spec_file": "specs/s1.md", + "status": "ready-for-dev", + "refused": False, + "reaches_redrive": True, + } + + _, isolated, isolated_step = runs.rearm_event_notice({**entry, "redrive": "isolated"}) + assert "mounts no worktree" not in isolated + assert "outside the worktree it mounts" in isolated + assert "reads that same path" in isolated + assert isolated_step # the remedy is the same one: the recorded path is wrong + + _, in_place, _ = runs.rearm_event_notice({**entry, "redrive": "in-place"}) + assert "mounts no worktree" in in_place + assert "reads that same path" in in_place + + # pre-`redrive` record: no mode claim at all, and no guess in either direction + _, legacy, legacy_step = runs.rearm_event_notice(entry) + assert "mounts no worktree" not in legacy + assert "outside the worktree it mounts" not in legacy + assert "reads that same path" in legacy + assert legacy_step + + def test_rearm_event_notice_splits_the_abort_three_ways_on_the_rollback(): """One kind, THREE renderings, and the split is by what the surface may CLAIM about the file — not by how the re-arm failed. @@ -5133,7 +5285,7 @@ def test_rearm_holds_the_resume_only_on_the_record_that_proves_a_wedge(): established that the committed spec does not carry the status the re-drive routes on, so resuming on it is futile rather than risky: step-01 halts blocked on `unrecognized status in existing story file` and the escalation is spent. Its - next_step already read "commit the corrected spec before resuming" while both + next_step already read "commit the corrected spec ... before resuming" while both default surfaces resumed in the same breath. The advisory kinds must NOT hold. `stale-restore-commits` is the record @@ -5167,6 +5319,210 @@ def test_rearm_holds_the_resume_only_on_the_record_that_proves_a_wedge(): assert runs.rearm_holds_the_resume(None) is False +def test_rearm_holds_the_resume_on_the_flip_no_repair_here_can_reach(): + """The third qualifying record, and the reason this is keyed on FLAGS not the kind. + + `rearm-spec-flip-skipped` covers three outcomes under one kind. On the + `reaches_redrive and not refused` leg the producer has already proven futility the + same way the two kinds above do: `refused = spec_path.is_file() and + write_reaches_the_redrive`, so reaching-and-not-refused means the flip addressed the + copy the re-drive reads AND that path is not a readable file here — the re-drive + reads the same path and finds no spec there to route on. That arm's next_step says + "restore the recorded spec path ... BEFORE RESUMING", which was a lie on the two + surfaces that re-arm and resume in one gesture. + + The other two arms must not hold, and keying on the bare kind would have taken them + with it. `refused` raises `RearmError` from the producer, so there is no resume to + hold. The remaining arm carries no next_step at all — its imperative belongs to + `rearm-spec-write-unreachable`, which holds the resume itself. + + Ablation: restore the bare two-kind tuple and the first assertion reddens; drop the + `not entry.get("refused")` conjunct and the refused leg reddens; drop the + `reaches_redrive` conjunct and the discarded-copy leg reddens. + """ + entry = {"kind": "rearm-spec-flip-skipped", "spec_file": "specs/s1.md"} + + assert runs.rearm_holds_the_resume({**entry, "reaches_redrive": True, "refused": False}) is True + # the abort: no resume happens at all, so there is nothing to hold + assert runs.rearm_holds_the_resume({**entry, "reaches_redrive": True, "refused": True}) is False + # the worktree-local copy the re-drive discards: no next_step, no hold + assert ( + runs.rearm_holds_the_resume({**entry, "reaches_redrive": False, "refused": False}) is False + ) + # a pre-`reaches_redrive` record proves nothing and must not be re-classified + assert runs.rearm_holds_the_resume(entry) is False + + +@pytest.mark.parametrize( + ("entry", "expected"), + [ + ( + { + "kind": "rearm-spec-write-unreachable", + "spec_file": "wt/specs/s1.md", + "status": "ready-for-dev", + "redrive": "in-place", + }, + "Correct the spec in the main checkout with `status: ready-for-dev` " "before resuming", + ), + ( + { + "kind": "rearm-spec-write-unreachable", + "spec_file": "wt/specs/s1.md", + "status": "in-review", + "target_branch": "main", + "redrive": "isolated", + }, + "Commit the corrected spec on `main` with `status: in-review` before resuming", + ), + ( + { + "kind": "rearm-spec-flip-skipped", + "spec_file": "/srv/artifacts/specs/s1.md", + "status": "ready-for-dev", + "refused": False, + "reaches_redrive": True, + "redrive": "isolated", + }, + "Restore the recorded spec path with `status: ready-for-dev` before resuming", + ), + ( + { + "kind": "rearm-upstream-write-unreachable", + "stories_root": "/proj/docs/stories", + "target_branch": "main", + "status": "ready-for-dev", + }, + "Commit the corrected SPEC.md / stories.yaml on `main` with " + "`status: ready-for-dev` before resuming", + ), + ], + ids=[ + "write-unreachable-in-place", + "write-unreachable-isolated", + "flip-skipped-holding", + "upstream-write-unreachable", + ], +) +def test_holding_remedies_name_the_status_the_redrive_routes_on(entry, expected): + """Every remedy that HOLDS the resume must name the status it has to leave behind. + + The hold buys the operator one gesture before the re-drive reads the tree, and + routing is decided by the spec's frontmatter status alone: a spec with none HALTs + the re-driven session on `unrecognized status in existing story file`, and one + still carrying the escalated attempt's terminal status routes to "ingest as + context, do not resume". So an operator who obeyed a remedy naming only the FILE + and the TREE — corrected it in the main checkout, committed it on the pinned + branch, put it back at the recorded path — could still spend the escalation on a + session that cannot route, having done exactly what they were told. The status is + what turns each remedy from necessary into sufficient. + + All FOUR holding arms are graded together, because the gap was identical on each + and fixing the cited arm alone would have left it on the rest. The fourth, + `rearm-upstream-write-unreachable`, renders the clause on the same idiom as its + siblings so the remedies stay one uniform contract; it is graded here on a + constructed entry that CARRIES a status, which is what pins the interpolation + itself. On the leg its producer actually emits the clause is empty — that record + fires only on the sentinel path, where `_clear_sentinel` deletes the spec and the + re-dispatch re-plans from PENDING — and the empty rendering is pinned by the + sibling test below, so both halves of that arm's behaviour are held. + + The value is read off the record, never recomputed: the producer writes the + `target_status` it tried to flip to, which is `in-review` after a restore and + `ready-for-dev` otherwise, and both are pinned here so a hardcoded literal cannot + pass. + + Ablation: drop the `{to}` / `_redrive_status_clause(entry)` interpolation from any + one arm and that row reddens on the remedy it renders. + """ + assert runs.rearm_holds_the_resume(entry) is True # all three HOLD; that is the point + _severity, _message, next_step = runs.rearm_event_notice(entry) + assert next_step == expected + + +@pytest.mark.parametrize( + ("entry", "expected"), + [ + ( + { + "kind": "rearm-spec-write-unreachable", + "spec_file": "wt/specs/s1.md", + "target_branch": "main", + "redrive": "isolated", + }, + "Commit the corrected spec on `main` before resuming", + ), + ( + { + "kind": "rearm-upstream-write-unreachable", + "stories_root": "/proj/docs/stories", + "target_branch": "main", + }, + "Commit the corrected SPEC.md / stories.yaml on `main` before resuming", + ), + ], + ids=["write-unreachable-legacy", "upstream-write-unreachable-as-produced"], +) +def test_holding_remedy_names_no_status_for_a_record_that_carries_none(entry, expected): + """A record with no status must drop the clause, not guess at one. + + Two shapes reach this. A `rearm-spec-write-unreachable` predating the field: the + renderer runs out of process from a journal line alone and a journal is read back + by later versions, so the migration shape is reachable on a plain upgrade. And + EVERY `rearm-upstream-write-unreachable` its producer emits today — that append + carries no `status`, because the sentinel leg it fires on has none to carry, so + the arm renders exactly the remedy it rendered before the clause was added. This + row is what proves the uniformity change is inert on the real record rather than + quietly putting a placeholder status in front of an operator. + + Same principle the `target_branch` clause already follows: a remedy that names no + value beats one that names a guess, and `status: ?` on an operator's terminal is + worse than silence. + + Ablation: default `_redrive_status_clause`'s read to the display placeholder `"?"` + and both rows redden on a remedy telling the operator to commit `status: ?`. + """ + _severity, _message, next_step = runs.rearm_event_notice(entry) + + assert next_step == expected + assert "status" not in next_step.lower() + + +def test_flip_skipped_holding_message_reports_a_missing_spec_not_a_stale_status(): + """The holding arm ENTAILS the file is absent, so the message may not claim a status. + + The producer writes `refused = spec_path.is_file() and write_reaches_the_redrive` + and this arm is `reaches_redrive and not refused`, which forces `is_file()` False: + there is nothing at that path to carry a status. Saying the re-drive "will see the + escalated attempt's status" described the ONE thing this arm proves cannot happen, + and it contradicted the remedy beside it — an operator told the file is there with + the wrong status has no reason to restore it. + + Matched case-insensitively on the negative half: the claim being ablated differs + from a benign mention only by its leading capital, and an assertion that a + capitalization slipped past would pass for the wrong reason. + + Ablation: restore the "will see the escalated attempt's status" tail and both + halves redden. + """ + entry = { + "kind": "rearm-spec-flip-skipped", + "spec_file": "/srv/artifacts/specs/s1.md", + "status": "ready-for-dev", + "refused": False, + "reaches_redrive": True, + "redrive": "isolated", + } + + _severity, message, next_step = runs.rearm_event_notice(entry) + + assert "finds no spec there to route on" in message + assert "escalated attempt's status" not in message.lower() + # the remedy has to stay obeyable with the file MISSING: restore it, do not commit it + assert next_step.lower().startswith("restore the recorded spec path") + assert "commit" not in next_step.lower() + + def test_rearm_event_notice_ignores_a_non_mapping_entry(): """A bare scalar on its own journal line is not an entry. diff --git a/tests/test_runs.py b/tests/test_runs.py index 1b397132..1df70732 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -3016,11 +3016,20 @@ def test_restamp_code_root_aims_the_mirror_the_rearm_reads(tmp_path, recorded): MISSING value, not a divergent one: it migrates silently, and calling it a move would fire the warning once on every pre-upgrade run. + The journal line is graded on the same three rows, because it is the DURABLE half + and the return value is not: the caller prints that string to stderr or a TUI toast + and it is gone. Worse, this re-stamp is what makes `cli._resume_paused_run`'s own + `code_root_changed` record read `false` later in the same gesture — both sides read + `bmadconfig.load_paths` on one project, so once the mirror is aimed the compare + NECESSARILY agrees. Without this line the one gesture where the root actually moved + is the one that leaves no trace, while plain `resume` still writes one. + Ablation: drop the `if not moved: return None` arm and `legacy` reddens on the message; return the message without the `save_state` and `moved` reddens on the - persisted root while the other two rows still pass. + persisted root while the other two rows still pass; delete the `journal.append` and + `moved` reddens on the record alone, with every message assertion still green. """ - from bmad_loop.journal import STATE_FILE + from bmad_loop.journal import STATE_FILE, Journal run = escalated_run(tmp_path, "r1", story_key="s1") now = tmp_path / "code" @@ -3049,6 +3058,169 @@ def test_restamp_code_root_aims_the_mirror_the_rearm_reads(tmp_path, recorded): else: assert message is None + # ...and the move is RECORDED, on exactly the row that moved + records = [ + e for e in Journal(run.run_dir).entries() if e["kind"] == "rearm-code-root-restamped" + ] + assert len(records) == (1 if recorded == "moved" else 0) + if records: + # `code_root_changed` is resume's own field name, so a reader correlates the two + # surfaces without knowing which one wrote the line + assert records[0]["code_root_changed"] is True + assert records[0]["repo"] == str(now) + + +def test_restamp_code_root_keeps_the_move_retryable_when_the_record_fails(tmp_path, monkeypatch): + """The move and an intent marker land in one atomic state write; the record is + appended after it and the marker cleared only once the append returned. So an + append that fails leaves the root MOVED and the marker SET, and the retry — which + would otherwise exit at "already agrees" with the record never written and the + later `run-resume` line reporting `code_root_changed=False` — re-enters, writes + the record the move still owes, and clears the marker. + + Ablations: drop the `or state.code_root_restamp_pending` half of the early + return and the retry reddens on `None`; clear the marker before the append and + it reddens the same way; never set it and the first assertion reddens.""" + from bmad_loop.journal import Journal + + run = escalated_run(tmp_path, "r1", story_key="s1") + now = tmp_path / "code" + now.mkdir() + run.state.repo_root = str(tmp_path / "was") + save_state(run.run_dir, run.state) + real_append = Journal.append + failures = iter([OSError(30, "Read-only file system")]) + + def append_once_failing(self, kind, **fields): + fault = next(failures, None) + if fault is not None: + raise fault + real_append(self, kind, **fields) + + monkeypatch.setattr(Journal, "append", append_once_failing) + + with pytest.raises(OSError): + runs.restamp_code_root(run.run_dir, now) + persisted = load_state(run.run_dir) + assert persisted.code_root == now # the move is durable... + assert persisted.code_root_restamp_pending is True # ...and the record still owed + + message = runs.restamp_code_root(run.run_dir, now) # the retry + + assert message is not None + persisted = load_state(run.run_dir) + assert persisted.code_root == now + assert persisted.code_root_restamp_pending is False + records = [ + e for e in Journal(run.run_dir).entries() if e["kind"] == "rearm-code-root-restamped" + ] + assert [r["repo"] for r in records] == [str(now)] # exactly once, on the retry + + +def test_restamp_code_root_records_no_move_the_state_write_did_not_make(tmp_path, monkeypatch): + """The other half of the ordering. A record written AHEAD of `save_state` asserted + a completed move that a failed save then never made — the journal said the root + changed while state.json still named the old tree, permanently, since the retry + would write a second such record. With the record after the persisted move, a + failed save leaves nothing behind: no move, no marker, no record, and the retry + redoes the whole thing exactly once. + + Ablation: move the `Journal(...).append` above the first `save_state` and this + reddens on the record count after the failed call.""" + from bmad_loop.journal import Journal + + run = escalated_run(tmp_path, "r1", story_key="s1") + was = tmp_path / "was" + now = tmp_path / "code" + now.mkdir() + run.state.repo_root = str(was) + save_state(run.run_dir, run.state) + real_save = runs.save_state + failures = iter([OSError(28, "No space left on device")]) + + def save_once_failing(target, state): + fault = next(failures, None) + if fault is not None: + raise fault + real_save(target, state) + + monkeypatch.setattr(runs, "save_state", save_once_failing) + + with pytest.raises(OSError): + runs.restamp_code_root(run.run_dir, now) + persisted = load_state(run.run_dir) + assert persisted.repo_root == str(was) # the move did NOT commit... + assert persisted.code_root_restamp_pending is False + records = lambda: [ # noqa: E731 + e for e in Journal(run.run_dir).entries() if e["kind"] == "rearm-code-root-restamped" + ] + assert records() == [] # ...so nothing may claim it did + + message = runs.restamp_code_root(run.run_dir, now) # the retry + + assert message is not None + persisted = load_state(run.run_dir) + assert persisted.code_root == now and persisted.code_root_restamp_pending is False + assert [r["repo"] for r in records()] == [str(now)] + + +@pytest.mark.parametrize("retry_root", ["was", "third"]) +def test_restamp_code_root_discharges_the_owed_record_before_moving_again( + tmp_path, monkeypatch, retry_root +): + """The owed record names the root the MARKER still describes, not the retry's. + + `code_root_restamp_pending` is a bare bool: the only surviving description of the + root an unlanded record was owed for is `state.repo_root` itself. An operator whose + record-append failed, and who then re-points the root AGAIN before retrying — + restoring the original, or moving to a third tree — would otherwise have the owed + root overwritten and its record lost forever, leaving the move with no durable + trace on any surface. Both existing failure-path tests retry with the SAME root, + where the owed root and the retry's root coincide and the loss cannot show. + + Ablation: drop the `if state.code_root_restamp_pending:` discharge append ahead of + `state.repo_root = new` and this reddens on the record list — only the retry's own + root is recorded and the owed one is gone.""" + from bmad_loop.journal import Journal + + run = escalated_run(tmp_path, "r1", story_key="s1") + was = tmp_path / "was" + owed = tmp_path / "owed" + owed.mkdir() + again = was if retry_root == "was" else tmp_path / "third" + run.state.repo_root = str(was) + save_state(run.run_dir, run.state) + real_append = Journal.append + failures = iter([OSError(30, "Read-only file system")]) + + def append_once_failing(self, kind, **fields): + fault = next(failures, None) + if fault is not None: + raise fault + real_append(self, kind, **fields) + + monkeypatch.setattr(Journal, "append", append_once_failing) + + with pytest.raises(OSError): + runs.restamp_code_root(run.run_dir, owed) + persisted = load_state(run.run_dir) + assert persisted.code_root == owed # the move is durable, its record owed + assert persisted.code_root_restamp_pending is True + + message = runs.restamp_code_root(run.run_dir, again) # the root moves AGAIN + + assert message is not None + persisted = load_state(run.run_dir) + assert persisted.code_root == again + assert persisted.code_root_restamp_pending is False + records = [ + e for e in Journal(run.run_dir).entries() if e["kind"] == "rearm-code-root-restamped" + ] + # The owed root is discharged FIRST, under its own name, and the second move + # gets its own row — one record per move, neither of them lost. + assert [r["repo"] for r in records] == [str(owed), str(again)] + assert all(r["code_root_changed"] is True for r in records) + def test_restamp_code_root_reloads_after_a_rival_writer(tmp_path, monkeypatch): """Ablation: move restamp_code_root's load above state_lock and the rival's diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 6b4d157c..7b8dfbdc 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -98,8 +98,23 @@ def _rearm_outcome(key: str, *entries: dict) -> runs_mod.RearmOutcome: for entry in entries if (notice := runs_mod.rearm_event_notice(entry)) is not None ) + # Mirrors `runs._RearmJournal.append`, first-wins included: the surface under test + # renders the held record's own `next_step`, so a helper that dropped it would let + # the hold toast pass on a step no record produced. + held = next( + ( + notice + for entry in entries + if runs_mod.rearm_holds_the_resume(entry) + and (notice := runs_mod.rearm_event_notice(entry)) is not None + ), + None, + ) return runs_mod.RearmOutcome( - key, notices, any(runs_mod.rearm_holds_the_resume(entry) for entry in entries) + key, + notices, + any(runs_mod.rearm_holds_the_resume(entry) for entry in entries), + held[2] if held is not None else "", ) @@ -5867,13 +5882,15 @@ async def test_escalation_rearm_holds_the_resume_it_folds_in(project, monkeypatc `rearm-spec-write-unreachable` fires only once the re-arm has proven the committed spec does not carry the status the re-drive routes on — and this path drops the - table's `next_step` precisely because it resumes in the same gesture. That silenced - the one record whose remedy MUST land first in BOTH halves: the imperative was - dropped as moot, and the resume it was warning against happened anyway, mounting a - fresh worktree onto the still-terminal committed spec. - - The re-arm itself is kept — the story is armed and persisted — and the toast names - what the operator can finish from this screen: commit, then resume. The + table's `next_step` on every ADVISORY toast precisely because it resumes in the same + gesture. That silenced the one record whose remedy MUST land first in BOTH halves: + the imperative was dropped as moot, and the resume it was warning against happened + anyway, mounting a fresh worktree onto the still-terminal committed spec. + + The re-arm itself is kept — the story is armed and persisted — and the hold toast + carries the held record's OWN `next_step` (`RearmOutcome.hold_next_step`), which on + this record is the commit. Since the hold is what stops the fold-in, "before + resuming" is finally true on this surface when it renders here. The `rearm-baseline-restamp-skipped` control keeps this a narrowing rather than "warnings stop resumes": it is a warning on the same walk, and the resume still fires. @@ -5930,12 +5947,97 @@ def fake_rearm(rd, sk, *, isolated_redrive=False, resolution_recorded=False, pro assert calls == [] # the resume this gesture folds in did NOT fire assert any("re-armed 1" in n for n in notes) # ...while the re-arm itself stands - assert any("commit the corrected spec, then resume this run" in n for n in notes) + assert any( + "not resuming in this gesture. Commit the corrected spec with " + "`status: ready-for-dev` before resuming — the run stays paused and resumable " + "from this screen." in n + for n in notes + ) # the record that proved it still renders, and its warning sibling did not hold assert any("land in a tree it discards" in n for n in notes) assert any("is not a readable file from here" in n for n in notes) +@pytest.mark.parametrize("redrive", ["in-place", "isolated"]) +async def test_escalation_rearm_hold_names_the_holding_record_s_own_remedy( + project, monkeypatch, redrive +): + """The hold toast must carry the HELD record's remedy, not one hardcoded literal. + + Four records hold this surface's fold-in resume and their remedies differ. The + newest — `rearm-spec-flip-skipped` on its `reaches_redrive and not refused` arm — + is journalled with `refused = spec_path.is_file() and write_reaches_the_redrive`, + so the holding arm ENTAILS `spec_path.is_file()` is False: there is no corrected + spec at that path to commit, and on the isolated arm the path can be a shared + artifact directory outside the project that is not a Git repository at all. The + hardcoded "commit the corrected spec" was therefore not merely unhelpful there, it + was impossible. Both re-drive modes reach this arm, so both are asserted. + + The negative half is the point and is matched case-insensitively: the fallback + literal only differs from the ablated one by its leading capital, and a negative + assertion that a capitalization slipped past would pass for the wrong reason. + + Ablation: revert `_do_rearm`'s hold branch to the hardcoded literal and BOTH legs + redden — the positive on the missing path remedy, the negative on the commit + imperative that cannot be obeyed. + """ + from bmad_loop import resolve, runs + from bmad_loop.journal import Journal + + calls: list[str] = [] + notes: list[str] = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "resume_detached", lambda proj, rid: calls.append(rid)) + monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") + + def fake_rearm(rd, sk, *, isolated_redrive=False, resolution_recorded=False, project_root=None): + Journal(rd).append( + "rearm-spec-flip-skipped", + story_key=sk, + spec_file="/srv/artifacts/specs/s1.md", + status="ready-for-dev", + refused=False, + reaches_redrive=True, + redrive=redrive, + ) + return _journal_rearm_outcome(rd, sk) + + monkeypatch.setattr(runs, "rearm_escalation", fake_rearm) + orig_notify = BmadLoopApp.notify + monkeypatch.setattr( + BmadLoopApp, + "notify", + lambda self, msg, **kw: notes.append(str(msg)) or orig_notify(self, msg, **kw), + ) + run_dir, _spec = _stories_paused_run( + project.project, + stage="escalation", + spec_status="blocked", + spec_checkpoint=False, + blocked_result="Blocked: needs a human decision on the auth scheme.", + ) + marker = resolve.resolution_path(run_dir, "1") + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text("{}", encoding="utf-8") + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await _open_review(app, pilot, EscalationModal) + await pilot.click(await ready(pilot, "#act-rearm")) + await until(pilot, lambda: any("not resuming" in n for n in notes)) + + assert calls == [] # this arm holds, so the folded-in resume did NOT fire + assert any("re-armed 1" in n for n in notes) # ...while the re-arm itself stands + held = [n for n in notes if "not resuming" in n] + assert len(held) == 1 + assert ( + "not resuming in this gesture. Restore the recorded spec path with " + "`status: ready-for-dev` before resuming — the run stays paused and resumable " + "from this screen." == held[0] + ) + # the remedy the record CANNOT have: the holding arm proves the path is not a file + assert "commit the corrected spec" not in held[0].lower() + + async def test_escalation_rearm_holds_without_a_renderable_notice(project, monkeypatch): """The authoritative hold is independent of whether there is a toast to render.""" from bmad_loop import resolve, runs