PR body gate (2) make-pr preflight runs the PR-description validator - #841
Merged
mergify[bot] merged 15 commits intoSep 24, 2026
Conversation
description_check only read the description's prose for claims about the repo's past, so preflight printed "ok preflight passed" for a body the required PR Body check then rejected after publication (#780-#789, #793, #795). describe() now also runs engine/skills/draft-pr/scripts/validate-pr-body.mjs on the same file and prints its errors under the gate line. Exit 1 from the validator fails preflight. No node, a missing validator, a timeout, or any other exit code prints "description unchecked: ..." and fails too -- a check that could not run is not a pass. The #795 description lands as a fixture: it exits 1 with Test Plan and Revert Plan outside <details> and code names in the Summary, and the new tests show preflight failing on it and still passing a valid body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ew claim: make-pr preflight runs the PR-description validator on --body-file and fails when it fails, so it no longer prints 'preflight passed' for a description the required PR Body check rejects. Review lane: policy Safety invariant: The change can only add a failure or an UNCHECKED notice; a PR description that passes engine/skills/draft-pr/scripts/validate-pr-body.mjs today is never newly blocked. Effectiveness measurement: A preflight test feeds the original #795 description (Test Plan and Revert Plan not inside <details>, code names in Summary), which the validator rejects with exit 1, and asserts preflight exits non-zero and prints the validator's errors; a passing description still gives 'ok preflight passed'; a validator that cannot run gives an unchecked failure, not a pass. Slice rationale: One script, one claim: preflight's description step also runs the schema validator. Architectural effect: preflight's describe() step covers the same rules as the required PR Body check. Goal: Stop preflight from approving a PR description the required check will reject. Motivation: A reflect pass found PR descriptions on catstack PRs #780-#789, #793 and #795 failing the required PR Body check after publication, and the user had to ask for a manual fix of every PR. In catstack the guard stayed silent: fed `mergify stack push` and `gh pr create --body-file <failing body>` it exited 0 with no output, while the same stack push in the Invoker checkout fired. Read at origin/main: engine/skills/make-pr/scripts/preflight.py describe() only runs description_check, then main() prints 'ok preflight passed'; the session that opened #795 got that line and then ran gh pr create. Alternative considerations: Leaving the validator to CI was set aside because the failure then shows up only after publication. Reimplementing the rules in Python was set aside because it would drift from the validator. Implementation details: In engine/skills/make-pr/scripts/preflight.py describe(), after description_check, run `node engine/skills/draft-pr/scripts/validate-pr-body.mjs --body-file <file>` from the repo root with a timeout. Print its output lines indented like the other gates. Exit 1 from the validator fails preflight; a missing node binary, a timeout, or any other exit code prints 'description unchecked: <reason>' and fails preflight. Add the tests to engine/skills/make-pr/tests/test_preflight.py with the #795 body as a fixture file under engine/skills/make-pr/tests/fixtures/. Non-goals: No change to the validator, the hook, or the make-pr SKILL.md rules. Do not edit engine/skills/draft-pr/scripts/validate-pr-body.mjs, scripts/pr/validate-pr-body-local.mjs, or any other file open PR #742 changes; call the validator as it is. If a change would overlap an open PR, stop and report instead. Layer: domain Feature state: active Files: - engine/skills/make-pr/scripts/preflight.py - engine/skills/make-pr/tests/test_preflight.py - engine/skills/make-pr/tests/fixtures/pr795-failing-body.md Change types: - engine/skills/make-pr/scripts/preflight.py: modify - engine/skills/make-pr/tests/test_preflight.py: modify - engine/skills/make-pr/tests/fixtures/pr795-failing-body.md: create Acceptance criteria: - `python3 -m unittest discover -s engine/skills/make-pr/tests -v` exits 0. - `python3 scripts/check_skill_test_coverage.py --base origin/main --head HEAD` exits 0. - `python3 scripts/check_skill_file_refs.py` exits 0. - `python3 scripts/check_no_new_comments.py` exits 0. Exit code: 0
…w claim: `python3 -m unittest discover -s engine/skills/make-pr/tests -v` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 -m unittest discover -s engine/skills/make-pr/tests -v`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 0
…w claim: `python3 scripts/check_no_new_comments.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_no_new_comments.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 1
…w claim: `python3 scripts/check_skill_test_coverage.py --base origin/main --head HEAD` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_skill_test_coverage.py --base origin/main --head HEAD`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 0
…w claim: `python3 scripts/check_skill_file_refs.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_skill_file_refs.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 0
…w claim: `python3 scripts/check_no_new_comments.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_no_new_comments.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 1
…w claim: `python3 scripts/check_no_new_comments.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_no_new_comments.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Solution: Review claim: `python3 scripts/check_no_new_comments.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_no_new_comments.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0.
…w claim: `python3 scripts/check_no_new_comments.py` passes on the finished branch. Review lane: proof Safety invariant: Verification is read-only and alters no repository file. Effectiveness measurement: The command's exit code is the direct measurement. Slice rationale: One check per proof task. Architectural effect: None; verification only. Goal: Prove the slice. Motivation: Running the check is the proof. Alternative considerations: The full suite was not required because the slice touches one component with its own tests. Implementation details: Run `python3 scripts/check_no_new_comments.py`. Non-goals: No mutations. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 0
…lidator-2/g0.t0.a-a35a7d83c-ef8fb868
…lidator-3/g0.t0.a-a689c3f08-de1115a4
…lidator-4/g0.t1.a-a137e395d-3ab0404e
…o ephemeral inter-task handoff files remain in the worktree before the merge gate. Review lane: cleanup Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files. Effectiveness measurement: The script exits non-zero if any handoff artifact remains. Slice rationale: Required terminal scrub for every implementation workflow. Architectural effect: None; hygiene only. Goal: Leave the branch free of handoff artifacts. Motivation: Handoff files must not reach the PR. Alternative considerations: Manual cleanup was set aside as non-deterministic. Implementation details: Run scripts/scrub-handoff-artifacts.sh. Non-goals: No product edits. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0. Exit code: 0
…c4e653a5-a4933bc3 — Review claim: No ephemeral inter-task handoff files remain in the worktree before the merge gate. Review lane: cleanup Safety invariant: The scrub script only checks for known handoff artifact names and never touches source, tests, or other repository files. Effectiveness measurement: The script exits non-zero if any handoff artifact remains. Slice rationale: Required terminal scrub for every implementation workflow. Architectural effect: None; hygiene only. Goal: Leave the branch free of handoff artifacts. Motivation: Handoff files must not reach the PR. Alternative considerations: Manual cleanup was set aside as non-deterministic. Implementation details: Run scripts/scrub-handoff-artifacts.sh. Non-goals: No product edits. Layer: app_regression Feature state: active Layer exception: allowed. Verification and the terminal scrub run after the docs commit so they check the final branch the PR will carry. Acceptance criteria: - The command exits 0.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a9c913. Configure here.
Contributor
|
Queued — the merge queue status continues in this comment ↓. |
Owner
Author
|
Mergify repair stopped: unresolved bot review thread PRRT_kwDOT3uYWs6lQXLR. The retry cap was reached for current head 9a9c913. |
The required PR Body check passes --changed-files-file, which is how the validator rejects changed file and folder names in the Summary and a Review Unit that does not match the diff. Preflight passed only --body-file, so those descriptions passed locally and failed after publish. describe() now takes the changed paths preflight already computed and writes them to a temp file for the validator. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Change-Id: Id36a64869b7cfb2b76b912fd3cf81a88a39dd344
Owner
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
This pull request spent 28 minutes 47 seconds in the queue, including 27 minutes 45 seconds running CI. Required conditions to merge
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Before a PR goes up, a local check reads its description and says pass or fail. People trust that pass and publish.
The problem: that check only looked for claims about history. It never ran the format rules the required GitHub check runs.
So a dozen descriptions got a local pass, then failed the required check after publishing. Each one needed a manual fix.
The fix: the local check now also runs the same format checker. It fails when that checker fails, or when the checker cannot run at all.
Review Claim
The local pre-publish check now fails any PR description that the required description check on GitHub would reject, and it fails when that rule check cannot run.
Review Lane
behavior
Review Unit
engine-runtime
Safety Invariant
The change can only add a failure or an unchecked notice. A description that passes
engine/skills/draft-pr/scripts/validate-pr-body.mjstoday is never newly blocked:validate_body()returns the validator's own exit code when it is 0 or 1, anddescribe()returnsstatus or schema_status, so a validator exit 0 adds no failure.Slice Rationale
One script, one claim: the description step in
engine/skills/make-pr/scripts/preflight.pyalso runs the schema validator. The one-linerealpathchange inscripts/ci/check_no_new_comments.pyrides along.scripts/check_no_new_comments.pyis a symlink toci/check_no_new_comments.py; run through it, theorigin/mainversion resolved the wrong repo root and exited 1 withModuleNotFoundError: No module named 'detect'. Withrealpathit exits 0.Non-goals
engine/skills/draft-pr/scripts/validate-pr-body.mjs),scripts/pr/validate-pr-body-local.mjs, the hook, or the make-prSKILL.mdrules.Test Plan
Test Plan
python3 -m unittest discover -s engine/skills/make-pr/tests -v— includes the newtest_preflight.pycases: the cat-mode: fleet upkeep runs from one script #795 description fixture (tests/fixtures/pr795-failing-body.md, plan sections not collapsed, code names in Summary) makes preflight exit non-zero and print the validator's errors; a passing description still printsok preflight passed; missingnode, a timeout, or an unexpected exit code printsdescription unchecked: <reason>and fails.python3 scripts/check_skill_test_coverage.py --base origin/main --head HEADpython3 scripts/check_skill_file_refs.pypython3 scripts/check_no_new_comments.pybash scripts/scrub-handoff-artifacts.shRevert Plan
Revert Plan
git revert <merge-sha>🤖 Generated with Claude Code
Note
Low Risk
Developer-only preflight and CI helper path resolution; may block publish when Node or schema validation is unavailable, with no change to production runtime behavior.
Overview
make-pr preflight now runs the same Node PR-body schema validator (
validate-pr-body.mjs) that the required GitHub check uses, in addition to the existing history-claimsdescription_check.describe()fails if either step fails, and if the validator cannot run (no Node, missing script, timeout, or unexpected exit), preflight treats that as unchecked and fails rather than passing.New
validate_body()shells out to the validator with a 120s timeout and surfaces its stdout/stderr in preflight output. Tests add a #795-style failing body fixture,TestDescriptionSchemaValidator(validator errors, full preflight failure, pass path, and unchecked edge cases), and stubvalidate_bodyin the history-check unit test so responsibilities stay split.scripts/ci/check_no_new_comments.pyresolves its directory withos.path.realpathinstead ofabspath, so invoking the script via thescripts/check_no_new_comments.pysymlink resolves the correct repo root and can importdetect.Reviewed by Cursor Bugbot for commit 9a9c913. Bugbot is set up for automated code reviews on this repo. Configure here.