diff --git a/.changeset/issue-1509-review-coverage-checklist-skip.md b/.changeset/issue-1509-review-coverage-checklist-skip.md new file mode 100644 index 0000000000..c278b48afa --- /dev/null +++ b/.changeset/issue-1509-review-coverage-checklist-skip.md @@ -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) diff --git a/docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md b/docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md index ec62e7ce9b..a9bc57c191 100644 --- a/docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md +++ b/docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md @@ -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 (`verdictrulefilelinedetail`) 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 :` — 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 — `` — 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 )` — 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. diff --git a/lib/test/test_python_scripts.py b/lib/test/test_python_scripts.py index 002f22e013..616b491587 100755 --- a/lib/test/test_python_scripts.py +++ b/lib/test/test_python_scripts.py @@ -251,6 +251,9 @@ def make_args(**overrides): # issue #1510 adds the optional as-of anchor head, read via getattr. record_review_coverage=None, review_coverage_disposition=[], record_review_coverage_head=None, + # issue #1509 review-coverage diff recomputation — read via getattr on the + # write path; a base ref for the range and an explicit override channel. + record_review_coverage_base=None, record_review_coverage_override=None, # issue #1462 prompt-extension row reconciliation — read on every call. reconcile_extension_rows=False, # issue #1876 mid-phase resume-point record — read on every call. @@ -12585,6 +12588,362 @@ def _arity_msg(**kw): # Restore the module-load bypass so any later Complete tests are not gated on the record. workpad._review_coverage_verdict = lambda prog_content: None +# ── issue #1509: a `skipped-intentional` checklist claim is refused when the diff +# ── it was recorded over does not satisfy the profile row that authorizes the skip. +# The recomputation reads the reviewed head (the record's as-of anchor) against the +# PR base, measuring the diff from git alone rather than trusting any value the run +# supplied. Fixtures are real temp git repos with real commits (git is not mocked). +def _rc_git(args, cwd): + return _subprocess.run( + ['git', '-c', 'user.email=t@e', '-c', 'user.name=t', *args], + cwd=str(cwd), capture_output=True, text=True) + + +def _rc_diff_repo(name, before, after, *, base='main', plugin=False): + """Build a temp git repo: commit `before` on `base`, then `after` on a feature + branch. Each of `before`/`after` maps relative path -> file text. Returns + (repo_path, head_sha). `plugin=True` seeds `.claude-plugin/plugin.json` naming + this engine so the engine-source arm fires (repository identity, not dir names).""" + d = Path(tempfile.mkdtemp(prefix='rc1509-' + name + '-')) + rc = _rc_git(['init', '-q', '-b', base, '.'], d) + if rc.returncode != 0: + raise AssertionError( + '#1509 harness: git init failed (rc=%d): %s' % (rc.returncode, rc.stderr)) + if plugin: + (d / '.claude-plugin').mkdir(parents=True, exist_ok=True) + (d / '.claude-plugin' / 'plugin.json').write_text('{"name": "prflow"}\n') + _rc_git(['add', '.claude-plugin/plugin.json'], d) + for rel, text in before.items(): + p = d / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + _rc_git(['add', '--', rel], d) + _rc_git(['commit', '-q', '-m', 'base'], d) + _rc_git(['checkout', '-q', '-b', 'feat'], d) + for rel, text in after.items(): + p = d / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + _rc_git(['add', '--', rel], d) + _rc_git(['commit', '-q', '-m', 'feat'], d) + head = _rc_git(['rev-parse', 'HEAD'], d).stdout.strip() + return d, head + + +def _rc_intentional_write(repo, head, *, base='main', override=None): + """Drive a --record-review-coverage write with checklist=skipped-intentional over + `repo`, returning the _UpdateError message or None when the write applied.""" + ov = {} if override is None else {'record_review_coverage_override': override} + try: + apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', + 'skipped-intentional'], + record_review_coverage_head=head, + record_review_coverage_base=base, + repo_root=str(repo), + **ov), []) + except workpad._UpdateError as e: + return str(e) + return None + + +# Reproduction (RED before the fix): the #1504 shape — more changed files than the +# file ceiling allows, all config-only extensions, under the line ceiling — must be +# REFUSED, because the profile row is disproved. Today's code accepts it. +_rc_repro_6f, _rc_repro_6f_head = _rc_diff_repo( + '6file', + {f'doc{i}.md': 'x\n' for i in range(6)}, + {f'doc{i}.md': 'x\ny\n' for i in range(6)}) +_rc_repro_6f_msg = _rc_intentional_write(_rc_repro_6f, _rc_repro_6f_head) +assert_eq("#1509 repro: skipped-intentional over a 6-file diff is refused (was accepted)", + True, _rc_repro_6f_msg is not None and "No PATCH was made" in _rc_repro_6f_msg) +assert_eq("#1509 repro: the 6-file refusal names the file-count condition and measured value", + True, (_rc_repro_6f_msg or "").find("6") >= 0 + and "file" in (_rc_repro_6f_msg or "")) + +# Reproduction (RED before the fix): the #1503 shape — within the file ceiling but +# over the line ceiling on a config-only diff — must be REFUSED. +_rc_repro_153, _rc_repro_153_head = _rc_diff_repo( + '153line', + {'a.md': '', 'b.md': '', 'c.md': ''}, + {'a.md': '\n'.join(f'l{i}' for i in range(60)) + '\n', + 'b.md': '\n'.join(f'l{i}' for i in range(60)) + '\n', + 'c.md': '\n'.join(f'l{i}' for i in range(33)) + '\n'}) +_rc_repro_153_msg = _rc_intentional_write(_rc_repro_153, _rc_repro_153_head) +assert_eq("#1509 repro: skipped-intentional over a 153-line diff is refused (was accepted)", + True, _rc_repro_153_msg is not None + and "No PATCH was made" in _rc_repro_153_msg + and "line" in _rc_repro_153_msg) + + +def _rc_checklist_axis(body): + """The stored checklist axis value in the review-coverage record on `body`.""" + payloads = workpad._review_coverage_payloads(body) + rec = workpad._parse_review_coverage_payload(payloads[0]) if payloads else None + return (rec or {}).get('checklist') + + +# AC1: the numstat counting rule — sum added+deleted across rows, one file per row, a +# binary `-` row contributes 0 lines but 1 file; a truncated/non-numeric row is malformed. +assert_eq("#1509 AC1: numstat sums added+deleted across rows, one file per row", + (2, 6), workpad._parse_numstat_counts("1\t2\ta.py\n3\t0\tb.py\n")) +assert_eq("#1509 AC1: a binary row (dash columns) counts 1 file and 0 lines", + (1, 0), workpad._parse_numstat_counts("-\t-\tbin.png\n")) +assert_eq("#1509 AC1: an empty diff is 0 files and 0 lines", + (0, 0), workpad._parse_numstat_counts("")) +assert_eq("#1509 AC1: a path containing a space is one file (path is the 3rd field)", + (1, 3), workpad._parse_numstat_counts("1\t2\tfoo bar.py\n")) +assert_raises("#1509 AC1: a truncated mid-row numstat line is malformed (ValueError)", + ValueError, lambda: workpad._parse_numstat_counts("1\t2\ta.py\n3\t")) +assert_raises("#1509 AC1: a non-integer count column is malformed (ValueError)", + ValueError, lambda: workpad._parse_numstat_counts("x\t2\ta.py\n")) + +# AC6/AC4: a confirming small config-only diff writes the skipped-intentional record +# unchanged and reports the measured values on its success (stderr) output. +_rc_ok_repo, _rc_ok_head = _rc_diff_repo( + '2file-ok', {'a.md': 'x\n', 'b.md': 'y\n'}, + {'a.md': 'x\nz\n', 'b.md': 'y\nw\n'}) +_rc_ok_buf = io.StringIO() +with contextlib.redirect_stderr(_rc_ok_buf): + _rc_ok_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_head=_rc_ok_head, + record_review_coverage_base='main', repo_root=str(_rc_ok_repo)), []) +assert_eq("#1509 AC6: a confirming diff keeps checklist=skipped-intentional (record unchanged)", + "skipped-intentional", _rc_checklist_axis(_rc_ok_body)) +assert_eq("#1509 AC6: the visible row still reads checklist=skipped-intentional", + True, "checklist=skipped-intentional" in _rc_ok_body) +assert_eq("#1509 AC4: a confirmed write reports the measured file and line counts on success output", + True, "changed file(s)" in _rc_ok_buf.getvalue() + and "changed line(s)" in _rc_ok_buf.getvalue()) + +# A path containing a double-quote is parsed cleanly (name-only -z, no quoting), so a +# small config-only diff that includes one is still confirmed rather than mismeasured. +_rc_q_repo, _rc_q_head = _rc_diff_repo( + 'quote', {'a.md': 'x\n'}, {'a.md': 'x\ny\n', 'q"x.md': 'hi\n'}) +assert_eq("#1509 AC1: a path containing a quote does not break the recomputation (confirmed)", + None, _rc_intentional_write(_rc_q_repo, _rc_q_head)) + +# AC5: an unresolvable recomputation downgrades the checklist axis to `unestablished` +# and records the reason — never a refusal. Three unresolvable shapes. +_rc_noh_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_base='main', repo_root=str(_rc_ok_repo)), []) +assert_eq("#1509 AC5: an unestablished reviewed head downgrades checklist to unestablished (no refusal)", + "unestablished", _rc_checklist_axis(_rc_noh_body)) +assert_eq("#1509 AC5: ...and the downgrade reason is recorded alongside the record", + True, "could not be recomputed" in _rc_noh_body) +# git cannot run at a non-existent repo root (OSError) → unresolvable, not a refusal. +_rc_oserr_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_head=_rc_ok_head, record_review_coverage_base='main', + repo_root='/no/such/repo/path/1509'), []) +assert_eq("#1509 AC5: a failing git invocation (bad repo root) downgrades to unestablished", + "unestablished", _rc_checklist_axis(_rc_oserr_body)) +# A base ref that does not exist → merge-base fails → unresolvable, not a refusal. +_rc_nobase_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_head=_rc_ok_head, + record_review_coverage_base='no-such-base', repo_root=str(_rc_ok_repo)), []) +assert_eq("#1509 AC5: an unreadable/absent base ref downgrades to unestablished (no refusal)", + "unestablished", _rc_checklist_axis(_rc_nobase_body)) +# A depth-limited clone whose merge base is unreachable → unestablished, not refused. +_rc_shallow = Path(tempfile.mkdtemp(prefix='rc1509-shallow-')) +_rc_clone = _rc_git(['clone', '-q', '--depth', '1', 'file://' + str(_rc_ok_repo), + str(_rc_shallow / 'c')], _rc_shallow) +_rc_shallow_repo = _rc_shallow / 'c' +_rc_shallow_head = _rc_git(['rev-parse', 'HEAD'], _rc_shallow_repo).stdout.strip() +_rc_shallow_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_head=_rc_shallow_head, + record_review_coverage_base='no-such-base', repo_root=str(_rc_shallow_repo)), []) +assert_eq("#1509 AC5: a depth-limited checkout with no resolvable base is unestablished, not refused", + "unestablished", _rc_checklist_axis(_rc_shallow_body)) + +# AC13: the override channel records a non-clean bare `skipped` and names the override — +# never a clean value. Confirmed even over a diff the recomputation would otherwise refuse. +_rc_ovr_body = apply_mut(_CP_BODY, make_args( + record_review_coverage=['full', 'attempted', 'complete', 'skipped-intentional'], + record_review_coverage_head=_rc_repro_6f_head, record_review_coverage_base='main', + repo_root=str(_rc_repro_6f), + record_review_coverage_override='the diff is genuinely config-only; auto-measure misfired'), []) +assert_eq("#1509 AC13: the override records bare `skipped` (a non-clean value), not skipped-intentional", + "skipped", _rc_checklist_axis(_rc_ovr_body)) +assert_eq("#1509 AC13: bare `skipped` is not in the checklist clean set (forces a disposition)", + False, "skipped" in workpad._REVIEW_COVERAGE_CLEAN['checklist']) +assert_eq("#1509 AC13: the record names that the override was used", + True, "overridden" in _rc_ovr_body) + +# AC3 (engine-source arm) + repository identity: a `lib/**` change in THIS repository is +# refused (engine source forces the checklist on), but the SAME change on any other +# repository is confirmed — the engine arm is gated on repository identity, not dir names. +_rc_eng_repo, _rc_eng_head = _rc_diff_repo( + 'engine', {'lib/foo.md': 'x\n'}, {'lib/foo.md': 'x\ny\n'}, plugin=True) +_rc_eng_msg = _rc_intentional_write(_rc_eng_repo, _rc_eng_head) +assert_eq("#1509 AC3: a lib/** change in this engine's own repo is refused (engine-source arm)", + True, _rc_eng_msg is not None + and "engine's own source set" in _rc_eng_msg) +_rc_noneng_repo, _rc_noneng_head = _rc_diff_repo( + 'nonengine', {'lib/foo.md': 'x\n'}, {'lib/foo.md': 'x\ny\n'}, plugin=False) +assert_eq("#1509 AC3: the same lib/** change on another repository is confirmed (engine arm excluded)", + None, _rc_intentional_write(_rc_noneng_repo, _rc_noneng_head)) +assert_eq("#1509 AC3: repository identity is decided by plugin.json name, not directory names", + (True, False), + (workpad._is_engine_own_repo(str(_rc_eng_repo)), + workpad._is_engine_own_repo(str(_rc_noneng_repo)))) +# A non-config extension (.py) in the diff is refused, naming the offending path. +_rc_py_repo, _rc_py_head = _rc_diff_repo( + 'pyext', {'a.md': 'x\n'}, {'a.md': 'x\ny\n', 'b.py': 'z\n'}) +_rc_py_msg = _rc_intentional_write(_rc_py_repo, _rc_py_head) +assert_eq("#1509 AC3: a non-config extension in the diff is refused, naming the offending path", + True, _rc_py_msg is not None and "b.py" in _rc_py_msg + and "non-config-only extension" in _rc_py_msg) + + +def _rc_lines(n): + return '\n'.join(f'l{i}' for i in range(n)) + '\n' + + +# AC3 ceiling boundaries ( str | None: return None +# issue #1509: the diff-profile row that authorizes a `skipped-intentional` checklist +# skip. These constants MIRROR skills/review/phases/phase-0-setup.md §0.5 (`small_diff`, +# `config_only`, `engine_self_modifying`); the divergence test in +# lib/test/test_python_scripts.py reads that file's arms and goes RED if they drift from +# these. The four prose copies of the engine-source path set stay unrefactored — this is +# the recomputation's comparand, not a new single source for them. +_REVIEW_COVERAGE_SMALL_DIFF_LINE_CEILING = 100 # total changed lines strictly below this +_REVIEW_COVERAGE_SMALL_DIFF_FILE_CEILING = 3 # changed-file count at most this +_REVIEW_COVERAGE_CONFIG_ONLY_EXTS = frozenset( + {'.yml', '.yaml', '.json', '.md', '.toml', '.ini', '.lock', '.txt'}) +# engine_self_modifying arm 1 — DevFlow's own source dirs (this repository's own tree). +_REVIEW_COVERAGE_ENGINE_SOURCE_PREFIXES = ('skills/', 'agents/', 'lib/') +# arm 2 — a prompt extension under the DevFlow state directory (any depth), `.md` only. +_REVIEW_COVERAGE_ENGINE_STATE_DIRS = ('.prflow', '.devflow') +# arm 3 — the root agent-instruction file (any depth), by basename. +_REVIEW_COVERAGE_ENGINE_ROOT_AGENT_FILE = 'CLAUDE.md' + + +def _parse_numstat_counts(numstat: str): + """(file_count, changed_line_total) from `git diff --numstat` output (issue #1509). + + Sums added+deleted across all rows and counts one file per row: a binary row (`-` in + both count columns) contributes 0 lines but 1 file. A row lacking the three + tab-separated fields (a truncated line) or whose count column is neither an integer + nor `-` is malformed — raise ValueError so the caller routes to an unresolvable + measurement rather than trusting a wrong count.""" + files = 0 + lines = 0 + for row in numstat.split('\n'): + if not row: + continue + parts = row.split('\t') + if len(parts) < 3: + raise ValueError(f'malformed --numstat row {row!r}') + for col in (parts[0], parts[1]): + if col != '-': + lines += int(col) # ValueError on a non-integer column → unresolvable + files += 1 + return files, lines + + +def _recompute_diff_facts(anchor_head, base_ref, repo_root): + """Recompute the reviewed diff's size and paths from git alone (issue #1509). + + Measures the reviewed head (`anchor_head`, the record's as-of anchor) against the PR + base (`base_ref`, else the `origin/HEAD` symbolic ref) — the SAME range the Phase 0.5 + classification measured, never the working tree at write time. Returns + {'resolved': bool, 'reason': str, 'lines': int, 'files': int, 'paths': [str]}. + + resolved is False — never a refusal; the caller records the checklist axis + `unestablished` — when the reviewed head is unestablished/absent, no base ref can be + read, no merge base exists (unrelated histories on a depth-limited checkout), or any + git invocation fails (non-zero exit or OSError) or emits a malformed row. Mirrors + `_repo_root`'s habit of catching both CalledProcessError and OSError.""" + def _unresolved(reason): + return {'resolved': False, 'reason': reason, + 'lines': 0, 'files': 0, 'paths': []} + + if not anchor_head or anchor_head == _REVIEW_COVERAGE_ANCHOR_UNESTABLISHED: + return _unresolved( + 'the reviewed head is unestablished, so the diff it was recorded over ' + 'cannot be measured') + + def _git(argv): + return subprocess.run( + ['git', *argv], cwd=repo_root, check=True, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding='utf-8').stdout + + try: + # `_git` runs check=True, so an unreadable base (origin/HEAD unset) or an + # unresolvable merge base (unrelated histories on a depth-limited checkout) + # raises here and is caught below as unresolved — no separate empty-value guard + # is reachable after it. + base = base_ref or _git( + ['symbolic-ref', '--short', 'refs/remotes/origin/HEAD']).strip() + merge_base = _git(['merge-base', anchor_head, base]).strip() + files, lines = _parse_numstat_counts( + _git(['diff', '--numstat', merge_base, anchor_head])) + paths = [p for p in _git( + ['diff', '--name-only', '-z', merge_base, anchor_head]).split('\0') if p] + except (subprocess.CalledProcessError, OSError, ValueError) as e: + return _unresolved(f'the diff measurement did not resolve ({e})') + return {'resolved': True, 'reason': '', + 'lines': lines, 'files': files, 'paths': paths} + + +def _is_engine_own_repo(repo_root) -> bool: + """Whether `repo_root` is THIS engine's own repository (issue #1509), decided by + repository identity rather than directory names: its `.claude-plugin/plugin.json` + names this plugin. A consumer's checkout — whose own `lib/` is unrelated product + code — returns False, so the engine-source refusal arm never fires undiagnosably + on it, while the classifier's own use of the arms is unchanged.""" + if not repo_root: + return False + try: + with open(os.path.join(repo_root, '.claude-plugin', 'plugin.json'), + encoding='utf-8') as f: + manifest = json.load(f) + except (OSError, ValueError): + return False + # `prflow` is the frozen canonical plugin name (CLAUDE.md rename Tier 1, single- + # sourced in lib/rename-map.json and the manifest `name`): do not rename it here in + # isolation, or this identity check silently stops recognizing the engine's own repo. + return isinstance(manifest, dict) and manifest.get('name') == 'prflow' + + +def _review_coverage_engine_source_paths(paths): + """The subset of `paths` in the engine's own source set — the arms of + phase-0-setup.md's `engine_self_modifying` (issue #1509).""" + hits = [] + for p in paths: + base = p.rsplit('/', 1)[-1] + first = p.split('/', 1)[0] + if (p.startswith(_REVIEW_COVERAGE_ENGINE_SOURCE_PREFIXES) + or (first in _REVIEW_COVERAGE_ENGINE_STATE_DIRS + and base.endswith('.md')) + or base == _REVIEW_COVERAGE_ENGINE_ROOT_AGENT_FILE): + hits.append(p) + return hits + + +def _review_coverage_profile_disproof(facts, repo_root) -> str | None: + """Why the recomputed diff does NOT satisfy the profile row that authorizes a + `skipped-intentional` skip, naming each failed condition and its measured value — + or None when the profile row is confirmed (issue #1509). Assumes facts['resolved']. + The engine-source arm applies only in this engine's own repository (AC): on any + other repository it is excluded from the refusal predicate.""" + reasons = [] + if facts['lines'] >= _REVIEW_COVERAGE_SMALL_DIFF_LINE_CEILING: + reasons.append( + f"the changed-line total {facts['lines']} is not below the ceiling of " + f"{_REVIEW_COVERAGE_SMALL_DIFF_LINE_CEILING}") + if facts['files'] > _REVIEW_COVERAGE_SMALL_DIFF_FILE_CEILING: + reasons.append( + f"the changed-file count {facts['files']} exceeds the ceiling of " + f"{_REVIEW_COVERAGE_SMALL_DIFF_FILE_CEILING}") + bad_ext = [p for p in facts['paths'] + if os.path.splitext(p)[1] not in _REVIEW_COVERAGE_CONFIG_ONLY_EXTS] + if bad_ext: + reasons.append( + 'these changed paths have a non-config-only extension: ' + + ', '.join(sorted(bad_ext))) + if _is_engine_own_repo(repo_root): + engine = _review_coverage_engine_source_paths(facts['paths']) + if engine: + reasons.append( + "these changed paths are in the engine's own source set (the " + 'checklist is forced on for them): ' + ', '.join(sorted(engine))) + return '; '.join(reasons) if reasons else None + + def _review_coverage_dispositions(progress_content: str) -> dict: """The recorded dispositions as `{gap: reason}`, read from the `## Progress` content. Each is one row carrying a `review-coverage-disposition:` marker @@ -5587,6 +5738,7 @@ def _apply_mutations(body: str, args, failed_ticks) -> str: # "flag absent" rather than raising AttributeError. review_coverage = getattr(args, 'record_review_coverage', None) review_coverage_payload = None + review_coverage_auto_notes: list[str] = [] if review_coverage: # Arity is guaranteed by argparse's nargs=4 from the CLI, but a programmatic # caller (the suite builds `args` directly) can pass a short list, which `zip` @@ -5621,6 +5773,69 @@ def _apply_mutations(body: str, args, failed_ticks) -> str: f"--record-review-coverage-head: {_anchor_head!r} is not a " "lowercase-hex head SHA (or 'unestablished'). No PATCH was made." ) + # issue #1509: a `skipped-intentional` checklist claim is accepted only when the + # diff it was recorded over satisfies the profile row that authorizes the skip + # (skills/review/phases/phase-0-setup.md §0.5). Recompute the diff from git over + # the reviewed head (the anchor above) against the PR base: a resolved-and- + # disproved row is a hard refusal (AC3); an unresolvable measurement downgrades + # the axis to `unestablished` — never a refusal (AC5); an explicit --override + # downgrades to bare `skipped` (non-clean, forces a disposition — AC13); a + # confirmed row keeps the value and writes exactly today's record (AC6). + # Copy here — after _require_arity above proved it is a real sequence — so the + # downgrade never mutates the caller's list and a non-sequence still hits the + # arity refusal rather than this list() (issue #1544). + review_coverage = list(review_coverage) + _checklist_idx = _REVIEW_COVERAGE_AXES.index('checklist') + _rc_override = getattr(args, 'record_review_coverage_override', None) + if review_coverage[_checklist_idx] == 'skipped-intentional': + if _rc_override: + review_coverage[_checklist_idx] = 'skipped' + review_coverage_auto_notes.append( + 'review-coverage recomputation overridden — the ' + 'skipped-intentional checklist claim is recorded as bare `skipped` ' + '(non-clean; a --review-coverage-disposition is required): ' + + _rc_override) + else: + _rc_repo_root = getattr(args, 'repo_root', None) or _repo_root() + _rc_facts = _recompute_diff_facts( + _anchor_head, getattr(args, 'record_review_coverage_base', None), + _rc_repo_root) + if not _rc_facts['resolved']: + review_coverage[_checklist_idx] = 'unestablished' + review_coverage_auto_notes.append( + 'review-coverage checklist recorded `unestablished` — the ' + 'skipped-intentional diff could not be recomputed: ' + + _rc_facts['reason']) + else: + _rc_disproof = _review_coverage_profile_disproof( + _rc_facts, _rc_repo_root) + if _rc_disproof: + raise _UpdateError( + "--record-review-coverage: a `skipped-intentional` " + "checklist claim is not authorized by the diff (measured " + f"{_rc_facts['files']} file(s), {_rc_facts['lines']} " + f"line(s)): {_rc_disproof}. No PATCH was made." + ) + # AC4: a confirmed write reports the measured values on success. + # The engine-source clause is honest per §2.3.6: it names the arm + # as verified only in this engine's own repo, where the arm was + # actually evaluated; on any other repo the arm is excluded from the + # predicate, so the breadcrumb says so rather than asserting a check + # that did not run. + _rc_engine_note = ( + 'and non-engine-source: verified' + if _is_engine_own_repo(_rc_repo_root) + else '(engine-source arm not evaluated: not this engine\'s ' + 'repository)') + sys.stderr.write( + 'workpad.py: review-coverage skipped-intentional confirmed — ' + f"{_rc_facts['files']} changed file(s), {_rc_facts['lines']} " + f'changed line(s), path-set config-only {_rc_engine_note}\n') + elif _rc_override: + sys.stderr.write( + 'workpad.py: --record-review-coverage-override is ignored — it applies ' + 'only to a skipped-intentional checklist, not ' + f"{review_coverage[_checklist_idx]!r}\n") _anchor_asof = _utc_now_compact() review_coverage_payload = ':'.join( list(review_coverage) + [_anchor_head, _anchor_asof]) @@ -5956,7 +6171,9 @@ def _apply_mutations(body: str, args, failed_ticks) -> str: if getattr(args, 'note_file', None): _notes_in.append(_note_file_payload(args)) _notes = [n for n in _notes_in if n not in _checkpoint_covered_texts] - progress_notes = _notes + scope_decision_notes + deferred_filed_notes + [ + # issue #1509: a review-coverage downgrade/override records its reason as an ordinary + # ## Progress note alongside the (mutated) record row. + progress_notes = _notes + review_coverage_auto_notes + scope_decision_notes + deferred_filed_notes + [ f'{text} {_checkpoint_marker(key)}' for key, text in checkpoint_inserts ] # Completion-evidence marker (issue #1087): validated above; a later validated key @@ -6682,6 +6899,25 @@ def main(): 'predates a later standalone review from a current one. A ' 'lowercase-hex SHA; omit it to record the head as ' '"unestablished". Only meaningful with --record-review-coverage.') + u.add_argument('--record-review-coverage-base', default=None, metavar='REF', + help='The pull request base branch the review-coverage ' + 'recomputation measures the reviewed head against (issue ' + '#1509). A "skipped-intentional" checklist claim is refused ' + 'when the diff between the base and the reviewed head does not ' + 'satisfy the profile row that authorizes the skip (changed ' + 'lines < 100, changed files <= 3, config-only extensions, and ' + "in this repository no engine-source path). Falls back to the " + 'origin/HEAD symbolic ref when omitted; when the diff cannot be ' + 'recomputed the checklist axis is recorded "unestablished" ' + 'rather than refused. Only meaningful with ' + '--record-review-coverage.') + u.add_argument('--record-review-coverage-override', default=None, metavar='REASON', + help='Override the issue-#1509 recomputation for a ' + '"skipped-intentional" checklist claim: record it as bare ' + '"skipped" instead (non-clean — it then forces a ' + '--review-coverage-disposition exactly as bare skipped does) ' + 'and note that the override was used. REASON states why. Never ' + 'yields a clean record.') u.add_argument('--review-coverage-disposition', nargs=2, action='append', default=[], metavar=('GAP', 'REASON'), help='Carry a recorded review-coverage gap forward under a stated ' diff --git a/skills/review/phases/phase-1-checklist.md b/skills/review/phases/phase-1-checklist.md index a6f46362a9..afc054ebaf 100644 --- a/skills/review/phases/phase-1-checklist.md +++ b/skills/review/phases/phase-1-checklist.md @@ -127,7 +127,7 @@ Return an empty JSON array `[]` if a second pass surfaces nothing new. Extract the JSON array from the agent's response (look for the ```json code fence). -If the agent fails or returns malformed JSON, retry once. If it fails again, log: "Verification checklist generation failed. Proceeding with existing agents only." Set a `checklist_skipped` flag and skip to Phase 3. +If the agent fails or returns malformed JSON, retry once. If it fails again, log: "Verification checklist generation failed. Proceeding with existing agents only." Set `checklist_skipped = "failure"` and skip to Phase 3. Store the parsed checklist items for Phase 1.5 (if batched) or Phase 2 (if single-batch).