Skip to content

The review engine's checklist skip is accepted without checking the profile row that authorizes it - #1966

Merged
The01Geek merged 9 commits into
mainfrom
issue-1509-the-review-engines-checklist-skip-is-accepted
Aug 25, 2026
Merged

The review engine's checklist skip is accepted without checking the profile row that authorizes it#1966
The01Geek merged 9 commits into
mainfrom
issue-1509-the-review-engines-checklist-skip-is-accepted

Conversation

@prflow-implementer

@prflow-implementer prflow-implementer Bot commented Aug 25, 2026

Copy link
Copy Markdown

Summary

  • A skipped-intentional review-coverage checklist claim was accepted without ever checking the profile row that authorizes it, so four merged engine-changing PRs recorded a clean coverage record while the checklist never ran. scripts/workpad.py now recomputes the reviewed diff from git and refuses the claim when the authorizing row is disproved.
  • The refusal is reserved for a measurement that resolves and disproves the row; an unresolvable recomputation degrades the axis to unestablished rather than blocking finalization, and a recorded override channel downgrades to a non-clean bare skipped.

Changes

workpad.py — review-coverage recompute gate: --record-review-coverage recomputes the reviewed diff from git alone (git diff --numstat for counts, --name-only -z for paths) over the reviewed head recorded on the coverage record's as-of anchor, measured against the PR's own base branch (--record-review-coverage-base, falling back to the origin/HEAD symbolic ref). A skipped-intentional claim is accepted only when the recomputation confirms the profile row (skills/review/phases/phase-0-setup.md §0.5): changed lines below 100, changed files at most 3, config-only extensions, and — only in this engine's own repository, detected by .claude-plugin/plugin.json's name — no engine-source path. A resolved-and-disproved row refuses the write non-zero, naming the failed condition and measured value; an unresolvable measurement records the axis unestablished; a confirmed row writes today's record byte-for-byte and reports the measured values on success.

workpad.py — override channel: a new --record-review-coverage-override REASON downgrades a skipped-intentional claim to bare skipped — a non-clean value that forces a --review-coverage-disposition exactly as bare skipped does today — and records that the override was used. No override path yields a clean record.

skills/review/phases/phase-1-checklist.md: the generation-failure step now names the literal checklist_skipped = "failure", matching the exact string phase-4-verdict.md and lib/efficiency-trace.jq compare against.

skills/implement/phases/phase-3-review.md: the /simplify gate tick now also records a simplify outcome: note in the same call.

docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md: names the checklist_skipped field and its three legal values ("intentional", "failure", null) alongside the five Phase 0.5 flags, and states that the coverage-record checklist-axis spellings and the field values are the same axis in two spellings.

Resolves

Resolves #1509

Test Plan

  • lib/test/test_python_scripts.py #1509 cases pass: the #1504 (6-file) and #1503 (153-line) reproductions are refused (RED before the fix, GREEN after).
  • AC5 downgrade cases — unestablished reviewed head, failing git invocation, absent base ref, depth-limited clone — record unestablished and never refuse.
  • AC3 repository-identity cases — a lib/** / CLAUDE.md / .prflow/**.md change is refused in this repo but confirmed on another repository (engine arm excluded).
  • AC13 override cases — the override records a non-clean bare skipped, and is a no-op (announced on stderr) on a non-skipped-intentional checklist.
  • AC12 divergence test turns RED if the module's profile-row arms drift from phase-0-setup.md §0.5.
  • AC1 numstat-parsing matrix — binary rows, empty diff, spaced/quoted paths, truncated and non-numeric rows.

Visual Changes

N/A

Breaking Changes

None. Existing workpads written before this change are not re-validated retroactively; the axis vocabulary and its clean set are unchanged (only which records are accepted changes). No workflow file, config-schema key, or installer payload changes, and no new tool grant is required.

Prompt-surface size

Derived at cc42fdf6d718015b43f3e5b371c8109d7812df02 against merge-base 5fc1f6591550c79ce108e44394bbeacf564130fe (origin/main). Covered: tracked *.md under skills/, agents/, .prflow/prompt-extensions/.

Path Before After Δ bytes Δ %
skills/review/phases/phase-1-checklist.md 11,761 11,766 +5 +0.0%
Whole covered surface 2,138,929 2,138,934 +5 +0.0%

Generated via /prflow:implement (v2.34.25, claude-opus-4-8, low)

@prflow-implementer
prflow-implementer Bot marked this pull request as ready for review August 25, 2026 20:50
@prflow-implementer

Copy link
Copy Markdown
Author

/prflow:review

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow Review — PR #1966

Status: ❌ Review failed

Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32898515882

@prflow-implementer

Copy link
Copy Markdown
Author

DevFlow review stall backstop — this cloud review ended with no verdict for cc42fdf6d718015b43f3e5b371c8109d7812df02. Auto-resume attempt 1:

/devflow:review

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow review: no verdict-post receipt was found for this run.

  • Actions run id: 32898515882
  • Pull-request head SHA this step resolved: cc42fdf6d718015b43f3e5b371c8109d7812df02

No run-scoped verdict-post receipt was found for this run: either Phase 4.4's
verdict emitter did not run, or it ran and could not write its receipt (look for a
could not write the verdict-post receipt breadcrumb in the job log).

No review authored by this run's reviewer identity is recorded in the reviews
API for the head above, so this run left the reviews API and reviewDecision untouched.
It recorded no verdict anywhere.

Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker,
and the verdict-derivation consumers do not read it as a verdict.

This comment is a record of that gap. It is not a verdict, and it neither approves nor
rejects this pull request.

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow Review — PR #1966

Status: ❌ Review failed

Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32898679634

@prflow-implementer

Copy link
Copy Markdown
Author

DevFlow review stall backstop — this cloud review ended with no verdict for cc42fdf6d718015b43f3e5b371c8109d7812df02. Auto-resume attempt 2:

/devflow:review

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow review: no verdict-post receipt was found for this run.

  • Actions run id: 32898679634
  • Pull-request head SHA this step resolved: cc42fdf6d718015b43f3e5b371c8109d7812df02

No run-scoped verdict-post receipt was found for this run: either Phase 4.4's
verdict emitter did not run, or it ran and could not write its receipt (look for a
could not write the verdict-post receipt breadcrumb in the job log).

No review authored by this run's reviewer identity is recorded in the reviews
API for the head above, so this run left the reviews API and reviewDecision untouched.
It recorded no verdict anywhere.

Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker,
and the verdict-derivation consumers do not read it as a verdict.

This comment is a record of that gap. It is not a verdict, and it neither approves nor
rejects this pull request.

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow Review — PR #1966

Status: ❌ Review failed

Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32898845908

@github-actions

Copy link
Copy Markdown
Contributor

PRFlow review: no verdict-post receipt was found for this run.

  • Actions run id: 32898845908
  • Pull-request head SHA this step resolved: cc42fdf6d718015b43f3e5b371c8109d7812df02

No run-scoped verdict-post receipt was found for this run: either Phase 4.4's
verdict emitter did not run, or it ran and could not write its receipt (look for a
could not write the verdict-post receipt breadcrumb in the job log).

No review authored by this run's reviewer identity is recorded in the reviews
API for the head above, so this run left the reviews API and reviewDecision untouched.
It recorded no verdict anywhere.

Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker,
and the verdict-derivation consumers do not read it as a verdict.

This comment is a record of that gap. It is not a verdict, and it neither approves nor
rejects this pull request.

@prflow-implementer

Copy link
Copy Markdown
Author

/prflow:review

@prflow-reviewer

prflow-reviewer Bot commented Aug 25, 2026

Copy link
Copy Markdown

PRFlow Review — PR #1966

Status: ✅ APPROVE with notes
Diff profile: engine_self_modifying (touches lib/**, skills/**) → full checklist
Run: View run
Reviewed HEAD: a7ce589
Last updated: 2026-08-25

Blueprint

  • Classify diff (Phase 0.5)
  • Generate verification checklist (Phase 1) — 20 items
  • Verify checklist (Phase 2) — 20 passed, 0 failed, 0 inconclusive
  • Review agents (Phase 3)
  • Aggregate & verdict (Phase 4)
  • Run complete — everything this run owed

Verdict: APPROVE with notes

All 13 acceptance criteria are satisfied and all 20 verification-checklist items PASS. The one substantive finding is a factual inaccuracy in a diff-added internal-doc paragraph (docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md); it is behavior-inert prose (Phase 4.1.5 cap), so it is capped to Suggestion and does not block merge — but it should be fixed. The remaining items are Suggestion-level test-gaps and one narrow fail-open.

Issue Compliance

Reviewed against issue #1509: The review engine's checklist skip is accepted without checking the profile row that authorizes it — criteria from the /prflow:implement workpad comment; scope unchanged. Requirement-based checklist items are included in the verification results below. Criteria came from the /prflow:implement workpad comment — this run's authoritative set, possibly narrowed from the issue. No divergence between the workpad's criteria and the issue body's.

Repo-specific gates:

  • Prompt-surface edit routing evidence gate (diff touches skills/review/phases/phase-1-checklist.md): discharged — the linked issue The review engine's checklist skip is accepted without checking the profile row that authorizes it #1509 workpad carries a Writing-skills evidence: marker with all four dispositions: skill-loaded=yes, guidance-applied=yes, pressure-scenario=no, micro-tests=no (each with a stated reason; a recorded no fully discharges).
  • Verification-evidence advisory: satisfied — the workpad carries a Verification evidence: marker (cloud implement tier whole-suite pass).

Verification Checklist Results

20 passed, 0 failed, 0 inconclusive — 0 via lite probe, 20 via agent.

✅ Passed items (20 of 20) — click to expand
  • VC-1: recomputes files/lines/paths from git alone (numstat + name-only), no caller values trusted [scripts/workpad.py]
  • VC-2: range = reviewed head (as-of anchor) vs PR base; origin/HEAD only when base unreadable [scripts/workpad.py]
  • VC-3: skipped-intentional accepted only when resolved & lines<100 & files<=3 & config-only exts & (this repo) no engine-source; else non-zero naming failed condition+value [scripts/workpad.py]
  • VC-4: confirmed write reports measured file/line/path-set verdict on success output [scripts/workpad.py]
  • VC-5: unresolvable recomputation → checklist axis 'unestablished' + reason, never refusal [scripts/workpad.py]
  • VC-6: confirmed row leaves value unchanged; payload composition identical (byte-for-byte) [scripts/workpad.py]
  • VC-7: bare 'skipped' bypasses recomputation; not in clean set (still a gap) [scripts/workpad.py]
  • VC-8: only 'skipped-intentional' triggers new logic; other axis values unchanged [scripts/workpad.py]
  • VC-9: axis tuple + clean tuple verbatim; _validate_review_coverage_axis_specs runs at import [scripts/workpad.py]
  • VC-10: DEVFLOW_SYSTEM_OVERVIEW.md names checklist_skipped field + 3 values + two-spellings statement [docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md]
  • VC-11: phase-1-checklist.md sets checklist_skipped = "failure" matching phase-4-verdict.md + efficiency-trace.jq [skills/review/phases/phase-1-checklist.md]
  • VC-12: AC12 divergence test parses phase-0-setup.md arms vs module constants (non-vacuous) [lib/test/test_python_scripts.py]
  • VC-13: override channel records non-clean bare 'skipped', forces disposition, names override; no-op on other axes [scripts/workpad.py]
  • VC-14: git invocations under check=True; CalledProcessError/OSError/ValueError → unresolved (no crash) [scripts/workpad.py]
  • VC-15: numstat parsing — sum add+del, 1 file/row, binary '-'→0 lines/1 file, malformed→ValueError [scripts/workpad.py]
  • VC-16: _is_engine_own_repo keys on plugin.json name=='prflow' (frozen literal); OSError/ValueError→False [scripts/workpad.py]
  • VC-17: engine-source arms match phase-0-setup.md §0.5 (prefix / state-dir+.md / CLAUDE.md basename) [scripts/workpad.py]
  • VC-18: make_args new fields match argparse dests read via getattr [lib/test/test_python_scripts.py]
  • VC-19: repo_root = getattr(args,'repo_root',None) or _repo_root() used as cwd [scripts/workpad.py]
  • VC-20: anchor compared to _REVIEW_COVERAGE_ANCHOR_UNESTABLISHED sentinel [scripts/workpad.py]

Code Review Findings

🟡 Suggestion / Minor

  1. docs/internal/DEVFLOW_SYSTEM_OVERVIEW.md:491 (diff-added paragraph) — the "two spellings" statement is inaccurate against HEAD: (a) skipped-failure is not a coverage-record checklist-axis value — the axis vocabulary is ('complete','not-applicable','skipped-intentional','skipped','unestablished') (pinned verbatim by AC9); skipped-failure is an efficiency-trace.jq verification_posture value (a different artifact). (b) checklist_skipped is the review-engine runtime flag (intentional/failure/null), not a field of the review-coverage record. The true, narrower correspondence is checklist_skipped = "intentional" ↔ coverage-axis skipped-intentional; the "failure"skipped-failure half has no coverage-axis counterpart. (raised by 2/5 agents) [documented_falsehood; capped to Suggestion by Phase 4.1.5 behavior-inert prose cap — limb one: no tool reads DEVFLOW_SYSTEM_OVERVIEW.md prose to decide program behavior (only the repo's own doc-presence pins reference it, which assert the prose itself); limb two: docs/internal/ is pruned by vendor-slice.sh:106 and shipped to no consumer. Both limbs hold → inert. Recommend fixing the prose regardless, as the AC's requested statement is itself imprecise.]
  2. scripts/workpad.py (_is_engine_own_repo, ~497–514) — the engine-source arm fails open on a corrupt/unreadable .claude-plugin/plugin.json in the engine's own repo: (OSError, ValueError)False silently drops the engine-source refusal arm, so a small config-only engine-source diff would be confirmed. Narrow (needs an unreadable manifest) but a fail-open on the one condition the arm enforces; consider distinguishing "manifest absent" (legitimately False) from "manifest present but unreadable" (route the whole recomputation to unestablished). (raised by 1/5 agents) {kind: logic_error}
  3. Test-gaps (pr-test-analyzer): the origin/HEAD base fallback is exercised only on its failure arm (never a success-arm run with base_ref=None); a confirmed skipped-intentional write inside the engine's own repo (the empty-engine-hits + "verified" breadcrumb arm) is untested; mixed binary numstat rows (-\t5), extensionless/dotfile paths, whole-record (not just axis) byte-identity, and the end-to-end disposition-forcing consequence of override→bare-skipped are unasserted. {kind: test_gap} — advisory over-grade shape 3 (single-source pr-test-analyzer); non-gating at the critical threshold.
  4. Minor: _recompute_diff_facts derives files from --numstat but paths from --name-only -z (two git calls); deriving files from len(paths) would remove a latent rename-detection inconsistency. Both fail closed, so not a fail-open. Style: the long progress_notes concatenation line could wrap.

Verdict accounting:

  • Verdict threshold: critical (default). Only findings at/above Critical drive REJECT; none present.
  • over-grade annotation: finding 1 capped by the behavior-inert prose cap; finding 3 flagged suspected over-grade (shape 3), advisory-only.
  • truthfulness sweep: no finding promoted (finding 1's prose is cap-covered and excluded from carve-out promotion).
  • intra-diff contradiction scan: no contradiction found.
  • Phase 4.0 deferrals: no Scope-Acknowledged Findings block in the PR body (block_present false).

Findings (live)

All five Phase-3 agents returned (code-reviewer, silent-failure-hunter, comment-analyzer, pr-test-analyzer, requesting-code-review). The requesting-code-review final pass returned APPROVE. Note: its EXTENSION-STATUS: loaded-empty resolved-root=unset for the requesting-code-review extension (present-empty) — observability-only, non-gating.

@prflow-reviewer prflow-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE with notes — full report in PR comment

The complete review report (checklist results, findings, details) is in the
PRFlow Review progress comment on this PR.

@The01Geek
The01Geek merged commit 6108aa9 into main Aug 25, 2026
9 checks passed
@The01Geek
The01Geek deleted the issue-1509-the-review-engines-checklist-skip-is-accepted branch August 25, 2026 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The review engine's checklist skip is accepted without checking the profile row that authorizes it

1 participant