Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/issue-1509-review-coverage-checklist-skip.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: patch
type: Fixed
---

- **Refuse a `skipped-intentional` review-coverage checklist claim the diff does not authorize.** `workpad.py`'s `--record-review-coverage` now recomputes the reviewed diff from git alone — the reviewed head recorded on the coverage record's as-of anchor measured against the pull request's own base (falling back to `origin/HEAD`) — and refuses a `skipped-intentional` claim whose diff exceeds the profile row that authorizes the skip (changed lines below 100, changed files at most 3, config-only extensions, and, only in this engine's own repository, no engine-source path). An unresolvable recomputation records the axis `unestablished` rather than refusing, a confirmed one writes today's record unchanged and reports the measured values, and a recorded override channel downgrades to a non-clean bare `skipped` that still forces a disposition. `phase-1-checklist.md` now names the `checklist_skipped = "failure"` literal at the generation-failure point. (#1966)
2 changes: 2 additions & 0 deletions docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,8 @@ Caches the diff to a run-scoped path. **Phase 0.5** classifies the diff with fiv

A `small_diff AND config_only` change skips the checklist phases (intentional). An `engine_self_modifying` change forces the **full** checklist — it is a checklist-only override and forces no Phase 3 agent on; the Phase 3 roster is decided by Phase 3.1's applicability gates on every profile. `detect_all_audit` is additive (it never suppresses the other flags' profile): it forces a **Phase 3.1.5 completeness-critic pass** that independently re-enumerates the audit's target population by a signal *other than the audit's own pattern* and records any uncovered member as a finding — the engine's guard against a vacuous or self-certified "detect-all" audit.

*The `checklist_skipped` field.* The outcome the flags above drive is recorded in the review-coverage record's `checklist_skipped` field, whose three legal values are `"intentional"` (the profile row authorized the skip on a `small_diff AND config_only` diff), `"failure"` (checklist generation failed), and `null` (the checklist ran). The coverage-record `checklist`-axis spellings `skipped-intentional` / `skipped-failure` and the field values `"intentional"` / `"failure"` are the same axis in two spellings. Because a clean `skipped-intentional` claim asserts an entitlement to that profile row, `scripts/workpad.py` recomputes the diff (changed lines, file count, extensions, and — only in this repository — the engine-source path set) from git before accepting it: a resolved recomputation that disproves the row refuses the write with the failed condition named, an unresolvable one records the axis `unestablished`, and an override channel downgrades the claim to bare `skipped` (non-clean, disposition-required) — issue #1509.

**Phase 0.6 — deterministic stale counted-prose lint (runs immediately after 0.5).** A deterministic pre-pass, `scripts/stale-prose-lint.py` (python3-stdlib), that flags **diff-added prose whose counted claims a later commit outgrows or falsifies** — the authoring-speed front line in front of the LLM self-contradicting-diff carve-out. It reads the unified diff the engine already cached in 0.2 on stdin and resolves each claim's referent against the current head via an explicit `--rev HEAD` (never `git merge-base`, so it is shallow-clone safe), emitting TSV rows (`verdict<TAB>rule<TAB>file<TAB>line<TAB>detail`) and exiting `0` (no STALE), `1` (at least one STALE), or `2` (internal error). Four rule classes, defined authoritatively in the helper's own header (the engine does **not** paraphrase them): **R1** a range header its block outgrew ("Cases A–B"), **R2** a legend sum contradicting an `Expected total = N`, **R3/R3b** an exact `count-locked` header (exact/two-item counts), and **R4** a deny-absolute about a shell operator token the same file also asserts permitted. Each `STALE` row enters **Phase 4.2 verdict computation** as an engine finding at the config-gated severity, carrying its TSV row verbatim as evidence — no new verdict rule and no new accounting rule. `UNRESOLVABLE` rows are informational and **never gate**; `VERIFIED` rows need no action. The lint is **move-aware** (issue #629): a diff-added prose line that is a byte-identical relocation — an extraction refactor moving prose it did not author — is never graded `STALE` but is *demoted* to an `UNRESOLVABLE` row naming the relocation, under multiplicity and referent rules the helper's header defines authoritatively (again, not paraphrased here) along with the design record bounding the exemption. The lint also honors an **illustrative-example opt-out** (issue #635): a prose/comment line carrying the plain, language-agnostic marker `stale-prose-lint: example` is skipped for **all** rule recognition (R1–R4 and both non-gating recognition tiers) — so a design-record example, a fixture-comment idiom, or engine docs *describing* a claim shape are no longer graded as a real claim of that shape — and instead of falling silent the helper emits one non-gating `UNRESOLVABLE` "EX" audit row, exactly as with the move-aware demotion; the marker match, its trailing negative-lookahead (so `examples`/`example-driven` do not match), and the disclosed non-goals are defined authoritatively in the helper's own module-header design record. Because the engine is shared, standalone `/prflow:review`, `/prflow:review-and-fix`, and the shadow pass all inherit the phase identically. A second **recognition-only** tier (issue #818) sits alongside the `count-locked` one and the `EX` audit row: it recognizes a **coverage universal** — a *coverage-scope* token from a closed set (deliberately wider than the universal quantifiers: `only`/`complete`/`entire`/`whole` are scope claims and are recognized too) adjacent to a coverage-referent noun from a closed set ("every call site", "all four arms") — and emits a non-gating `CU` row, plus an `RT` row for the declared `stale-prose-lint: rule-text` opt-out, whose visibility property mirrors `EX`'s; both closed sets are specified authoritatively in the helper's header, not here. Like the `count-locked` tier it resolves no referent, never emits `STALE`, and leaves the four gating classes' own verdicts untouched — the one control-flow change is that R4's short-circuit now fires only when R4 actually emitted a row, so a deny-absolute carrying no backticked operator token no longer silently swallows either non-gating tier (it can add only non-gating rows, never change the exit code). The two non-gating tiers overlap by design and neither terminates the other, so a line matching both emits both rows. Phase 0.6 emits these rows like any other, but does not **act** on them — their consumer is the implement engine's Phase 2 §2.3.4b sweep, which runs the helper in its `--worktree` post-image mode — the on-disk file rather than `git show <rev>:<path>` — so an **uncommitted** tree, including a brand-new `.changeset/*.md`, resolves to its post-change content and the claim is grounded before commit rather than at review time. Detection ships in the engine; the pin-or-don't-write policy for a `count-locked` row lives in this repo's `.prflow/prompt-extensions/review-and-fix.md`.

*Cross-run adjudication carry-forward (issue #466).* A STALE row is re-derived from scratch every run, so a false positive the lint keeps raising (a `count-locked` claim that is actually accurate against HEAD — the lint miscounted) would re-gate on every run with no channel to make a prior triage stick. The engine carries a false-positive adjudication forward across runs. **Producer (Phase 4.1.7, PR mode):** when a run's Phase 4 triage resolves a STALE row's claim against HEAD and finds it accurate, it renders the finding Informational with the **concrete referent evidence** (the true count and what the lint miscounted) and stamps one hidden payload line — `<!-- prflow:lint-fp-adjudicated <base64 of the row's TSV> -->` — inside a sentinel-delimited adjudications section of the run's `prflow:review-progress` comment (written only by the Phase 4 finalize write). base64 keeps a `--` in the detail excerpt from terminating the HTML comment; a row with no recorded referent evidence is not stamped, and **an `R4` (modality-conflict) row is never stamped** — see the rule-scope carve-out below. **Consumer (Phase 0.6, PR mode):** before STALE rows are finalized as findings, the deterministic, network-free helper `scripts/match-lint-adjudications.py` (the sibling of `scripts/match-deferrals.py`) joins the current rows against the payloads found in this PR's own prior **trusted** progress comments and returns a demotion map; a row whose decoded `(rule, path, detail)` is **byte-for-byte identical** to an adjudicated payload (the TSV line number is excluded from the key, so a renumbered paragraph still matches, while any change to the detail text re-examines the prose fresh) is demoted to Informational — annotated `previously adjudicated false positive (run <run-key>)` — and excluded from verdict computation at every configured `stale_prose.severity`. **Rule scope — `R4` is never carried forward.** The match key is safe only because the count/range rules (`R1`/`R2`/`R3`) embed the *observed referent* in their detail, so moving the counted code changes the detail and a stale adjudication stops matching. `R4`'s detail is a pure function of the *claim* line and says nothing about the permitting line, so an old `R4` adjudication would keep matching after a later commit added a **genuine** contradicting permit — demoting exactly the self-contradicting-diff finding the lint exists to catch. `R4` rows are therefore never stamped and never demoted; the exclusion is counted (`rows_rule_excluded`) and surfaced, never silent. **Trust** requires the run-keyed `prflow:review-progress` marker on the comment together with a `Bot`-type author (or a login in `.prflow.allowed_bots`, read via `config-get.sh`); payloads are honored **only** inside the sentinels (a payload quoted anywhere else — e.g. in a rendered evidence line — is data, never an instruction), ambiguity (two current STALE rows sharing a payload key) never demotes, and a malformed payload is skipped with a breadcrumb. The join is **PR-scoped by construction** (it reads only this PR's own comments) and **degrades loudly** — an absent, refused, or erroring helper leaves every STALE row at its configured severity and records a degraded-check note, never a silent skip. The blast radius is bounded to the severity gate: a demoted row never drives the threshold-independent self-contradicting-diff carve-out.
Expand Down
Loading
Loading