diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 822229b35..c54bf8963 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -28,6 +28,7 @@ Fixes # - [ ] This PR targets `dev` - [ ] My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in. - [ ] I actually ran the app (`docker compose up` or `uvicorn app:app`) and verified the change works end-to-end. Type-checks and unit tests are not enough. +- [ ] I did not run the app/runtime validation and stated that gap in **How to Test**. Leave this unchecked when the app-run box above is checked. ## How to Test diff --git a/.github/scripts/check-pr-description.js b/.github/scripts/check-pr-description.js index f5dabea5d..2e60be574 100644 --- a/.github/scripts/check-pr-description.js +++ b/.github/scripts/check-pr-description.js @@ -21,11 +21,11 @@ module.exports = async ({ github, context, core }) => { return strip(m?.[0].replace(new RegExp(`#+\\s+${heading}`, 'i'), '') ?? ''); } - const problems = []; + const descriptionProblems = []; // 1. Summary must be filled in. if (section('Summary').length < 20) { - problems.push('**Summary** is empty or too short — describe what changed and why.'); + descriptionProblems.push('**Summary** is empty or too short — describe what changed and why.'); } // 2. Linked Issue must reference a real issue. Accept a bare #NNN, a closing @@ -34,18 +34,18 @@ module.exports = async ({ github, context, core }) => { const linkedSection = section('Linked Issue'); const hasIssueRef = /#\d+\b/.test(linkedSection) || /\/issues\/\d+/.test(linkedSection); if (!linkedSection || !hasIssueRef) { - problems.push('**Linked Issue** — add a reference like `Fixes #NNN`, a bare `#NNN`, or a link to the issue.'); + descriptionProblems.push('**Linked Issue** — add a reference like `Fixes #NNN`, a bare `#NNN`, or a link to the issue.'); } // 3. At least one Type of Change box must be checked. const typeBlock = body.match(/##\s+Type of Change[\s\S]*?(?=\n##\s|$)/i)?.[0] ?? ''; if (!/- \[x\]/i.test(typeBlock)) { - problems.push('**Type of Change** — check at least one box.'); + descriptionProblems.push('**Type of Change** — check at least one box.'); } // 4. Duplicate-search checklist item must be checked. if (!/- \[x\] I searched/i.test(body)) { - problems.push('**Checklist** — check the duplicate-search box to confirm you searched existing issues and PRs.'); + descriptionProblems.push('**Checklist** — check the duplicate-search box to confirm you searched existing issues and PRs.'); } // 5. How to Test must contain enough real detail for a reviewer to act on. @@ -53,7 +53,79 @@ module.exports = async ({ github, context, core }) => { // code block — so we only require non-trivial content, not a specific shape. const howTo = section('How to Test'); if (howTo.length < 30) { - problems.push('**How to Test** — explain how a reviewer can verify this change. Numbered steps, the commands you ran, or a short code block all work — give a sentence or two of real detail (not just "tested locally").'); + descriptionProblems.push('**How to Test** — explain how a reviewer can verify this change. Numbered steps, the commands you ran, or a short code block all work — give a sentence or two of real detail (not just "tested locally").'); + } + + // Classify paths from GitHub's API. This workflow runs in the privileged base + // context, so it must never check out or execute code from the PR branch. + const changedFiles = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: prNum, per_page: 100, + }); + const changedPaths = changedFiles.map(file => file.filename); + + function isUiSensitivePath(filename) { + const path = filename.toLowerCase(); + return path.startsWith('static/') + || path.startsWith('templates/') + || /\.(?:html?|css|svg)$/.test(path); + } + + function isDocsOnlyPath(filename) { + const path = filename.toLowerCase(); + return /\.(?:md|mdx|rst|adoc|txt)$/.test(path) + || (path.startsWith('docs/') && !isUiSensitivePath(path)); + } + + function isRuntimeSensitivePath(filename) { + const path = filename.toLowerCase(); + if (isUiSensitivePath(path)) return false; + if (path.startsWith('tests/') || path.startsWith('.github/')) return false; + return /^(?:app\.py|routes\/|services\/|src\/|core\/|mcp_servers\/|scripts\/|docker\/)/.test(path) + || /^(?:dockerfile|docker-compose.*\.ya?ml|requirements(?:-optional)?\.txt|pyproject\.toml|setup\.py)$/.test(path) + || /\.(?:py|sh|ps1|bat)$/.test(path); + } + + let classification = 'tooling'; + if (changedPaths.some(isUiSensitivePath)) { + classification = 'UI-sensitive'; + } else if (changedPaths.some(isRuntimeSensitivePath)) { + classification = 'backend/runtime'; + } else if (changedPaths.length > 0 && changedPaths.every(isDocsOnlyPath)) { + classification = 'docs-only'; + } + + const appRan = /- \[x\]\s+I actually ran the app\b/i.test(body); + const appNotRun = /- \[x\]\s+I did not run the app\/runtime validation\b/i.test(body); + const screenshotChecked = /- \[x\]\s+\*\*Screenshot or short clip\*\*/i.test(body); + const screenshotSection = section('Screenshots / clips'); + const hasVisualEvidence = /!\[[^\]]*\]\([^)]+\)|<(?:img|video|source)\b[^>]*(?:src|href)=|https?:\/\/[^\s)]+/i.test(screenshotSection); + const evidenceGaps = []; + let needsRuntimeValidation = false; + let needsVisualEvidence = false; + + if (classification === 'backend/runtime' || classification === 'UI-sensitive') { + if (appRan && appNotRun) { + needsRuntimeValidation = true; + evidenceGaps.push('The app-run and explicit not-run boxes are both checked. Select the one state that is true.'); + } else if (!appRan) { + needsRuntimeValidation = true; + if (appNotRun) { + evidenceGaps.push('The author explicitly reports that app/runtime validation was not performed.'); + } else { + evidenceGaps.push('App/runtime validation is not author-attested. Check the run box only after running it, or check the explicit not-run box and describe the gap.'); + } + } + } + + if (classification === 'UI-sensitive') { + if (!screenshotChecked) { + needsVisualEvidence = true; + evidenceGaps.push('The screenshot/clip checkbox is not checked for this UI-sensitive change.'); + } + if (!hasVisualEvidence) { + needsVisualEvidence = true; + evidenceGaps.push('The Screenshots / clips section does not contain an actual attachment or link.'); + } } // ── Comment ────────────────────────────────────────────────────────────── @@ -62,22 +134,43 @@ module.exports = async ({ github, context, core }) => { }); const existing = comments.find(c => (c.body ?? '').includes(MARKER)); - if (problems.length === 0) { + if (descriptionProblems.length === 0 && evidenceGaps.length === 0) { if (existing) { await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id }); } } else { - const commentBody = [ - MARKER, - '⚠️ **PR description — action needed**', - '', - 'The following required sections are missing or incomplete. Please update the PR description to address them:', - '', - problems.map(p => `- ${p}`).join('\n'), + const commentLines = [MARKER]; + if (descriptionProblems.length > 0) { + commentLines.push( + '⚠️ **PR description — action needed**', + '', + 'The following required sections are missing or incomplete. Please update the PR description to address them:', + '', + descriptionProblems.map(problem => `- ${problem}`).join('\n'), + ); + } else { + commentLines.push( + '⚠️ **PR description is complete; validation evidence is still outstanding**', + '', + `Changed-file classification: **${classification}**.`, + ); + } + if (evidenceGaps.length > 0) { + commentLines.push( + '', + '**Author-reported runtime / visual state**', + '', + evidenceGaps.map(gap => `- ${gap}`).join('\n'), + '', + 'Checkboxes are author attestations. GitHub Actions results remain the execution evidence for CI; this check does not prove that a local command ran.', + ); + } + commentLines.push( '', '---', - '_This comment is deleted automatically once all sections are complete._', - ].join('\n'); + '_This comment updates automatically when the description or changed files change._', + ); + const commentBody = commentLines.join('\n'); if (existing) { await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body: commentBody }); @@ -97,34 +190,47 @@ module.exports = async ({ github, context, core }) => { return true; } catch (e) { if (e.status === 404) return false; + if (e.status === 403) { + core.warning(`Could not inspect label "${name}" — token lacks label read access; skipping.`); + return false; + } throw e; } } - async function swapLabel(num, add, remove) { - if (await labelExists(add)) { + async function setLabel(name, wanted) { + if (wanted && await labelExists(name)) { try { - await github.rest.issues.addLabels({ owner, repo, issue_number: num, labels: [add] }); + await github.rest.issues.addLabels({ owner, repo, issue_number: prNum, labels: [name] }); } catch (e) { // Fail soft on a token that can't write labels so a label permission // problem never masks the actual description verdict. - if (e.status !== 403) throw e; - core.warning(`Could not add "${add}" — token lacks label write here; skipping.`); + if (e.status !== 403 && e.status !== 404) throw e; + core.warning(`Could not add "${name}" — label is unavailable or the token lacks label write access; skipping.`); } + } else if (wanted) { + core.warning(`Label "${name}" does not exist in the repo — skipping. Create it once to enable labelling.`); } else { - core.warning(`Label "${add}" does not exist in the repo — skipping. Create it once to enable labelling.`); - } - try { - await github.rest.issues.removeLabel({ owner, repo, issue_number: num, name: remove }); - } catch (e) { - if (e.status !== 404 && e.status !== 410 && e.status !== 403) throw e; + try { + await github.rest.issues.removeLabel({ owner, repo, issue_number: prNum, name }); + } catch (e) { + if (e.status !== 404 && e.status !== 410 && e.status !== 403) throw e; + } } } - if (problems.length === 0) { - await swapLabel(prNum, 'ready for review', 'needs work'); - } else { - await swapLabel(prNum, 'needs work', 'ready for review'); - core.setFailed(`PR description has ${problems.length} issue(s) — see bot comment for details.`); + const descriptionComplete = descriptionProblems.length === 0; + const evidenceComplete = evidenceGaps.length === 0; + const isDraft = Boolean(context.payload.pull_request.draft); + await setLabel( + 'ready for review', + descriptionComplete && evidenceComplete && !isDraft, + ); + await setLabel('needs work', !descriptionComplete); + await setLabel('needs runtime validation', needsRuntimeValidation); + await setLabel('needs visual evidence', needsVisualEvidence); + + if (!descriptionComplete) { + core.setFailed(`PR description has ${descriptionProblems.length} issue(s) — see bot comment for details.`); } }; diff --git a/.github/workflows/pr-description-check.yml b/.github/workflows/pr-description-check.yml index 53f0b5f50..32f78bede 100644 --- a/.github/workflows/pr-description-check.yml +++ b/.github/workflows/pr-description-check.yml @@ -5,7 +5,11 @@ on: # works on fork PRs. Safe here: the checkout pins to the base branch (no fork # code runs) and the scripts only read context.payload and call the GitHub API. pull_request_target: # zizmor: ignore[dangerous-triggers] - types: [opened, edited, synchronize, reopened, ready_for_review] + types: [opened, edited, synchronize, reopened, ready_for_review, converted_to_draft] + +concurrency: + group: pr-description-${{ github.event.pull_request.number }} + cancel-in-progress: true # Default-deny at the workflow level; each job opts into only the scopes it needs. # Note: modifying a PR's labels/comments needs pull-requests:write even though the @@ -59,12 +63,14 @@ jobs: check-mergeable: name: Flag unmergeable PRs + needs: check-description runs-on: ubuntu-latest permissions: pull-requests: write issues: write - # Skip bots: they open PRs programmatically and have their own process. - if: github.event.pull_request.user.type != 'Bot' + # Run after description validation failures, but never from an obsolete + # workflow run canceled by a newer PR event. + if: ${{ !cancelled() && github.event.pull_request.user.type != 'Bot' }} steps: - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: diff --git a/tests/test_pr_description_check.py b/tests/test_pr_description_check.py new file mode 100644 index 000000000..b8413b4d6 --- /dev/null +++ b/tests/test_pr_description_check.py @@ -0,0 +1,313 @@ +"""Regression coverage for PR description and validation-state checks.""" + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + + +_REPO = Path(__file__).resolve().parent.parent +_CHECKER = _REPO / ".github" / "scripts" / "check-pr-description.js" +_WORKFLOW = _REPO / ".github" / "workflows" / "pr-description-check.yml" +pytestmark = pytest.mark.skipif(not shutil.which("node"), reason="node not on PATH") + + +def _body(*, app_ran=False, app_not_run=False, screenshot=False, media=""): + return f"""## Summary + +This focused change has enough concrete summary detail for the checker. + +## Linked Issue + +Fixes #5934 + +## Type of Change + +- [x] CI / tooling / configuration + +## Checklist + +- [x] I searched open issues and open PRs. +- [{'x' if app_ran else ' '}] I actually ran the app and verified the change works end-to-end. +- [{'x' if app_not_run else ' '}] I did not run the app/runtime validation and stated that gap in How to Test. + +## How to Test + +Run the focused checker regression tests and inspect their exact assertions. + +## Visual / UI changes + +- [{'x' if screenshot else ' '}] **Screenshot or short clip** of the running change. + +### Screenshots / clips + +{media} +""" + + +def _run_checker(files, body, *, missing_labels=(), draft=False): + harness = r""" +const checkPrDescription = require(process.argv[1]); +const input = JSON.parse(process.argv[2]); +const calls = []; +const listFiles = async () => {}; +const listComments = async () => {}; + +const github = { + paginate: async (method) => { + if (method === listFiles) return input.files.map(filename => ({ filename })); + if (method === listComments) return []; + throw new Error('unexpected paginated method'); + }, + rest: { + pulls: { listFiles }, + issues: { + listComments, + getLabel: async ({ name }) => { + if (input.missingLabels.includes(name)) { + const error = new Error(`missing label: ${name}`); + error.status = 404; + throw error; + } + return { data: { name } }; + }, + addLabels: async (params) => calls.push({ method: 'addLabels', params }), + removeLabel: async (params) => calls.push({ method: 'removeLabel', params }), + createComment: async (params) => calls.push({ method: 'createComment', params }), + updateComment: async (params) => calls.push({ method: 'updateComment', params }), + deleteComment: async (params) => calls.push({ method: 'deleteComment', params }), + }, + }, +}; +const context = { + payload: { + pull_request: { + number: 42, + body: input.body, + draft: input.draft, + }, + }, + repo: { owner: 'odysseus-dev', repo: 'odysseus' }, +}; +const core = { + warning: (message) => calls.push({ method: 'warning', message }), + setFailed: (message) => calls.push({ method: 'setFailed', message }), +}; + +checkPrDescription({ github, context, core }) + .then(() => process.stdout.write(JSON.stringify(calls))) + .catch((error) => { + console.error(error); + process.exitCode = 1; + }); +""" + payload = json.dumps( + { + "files": files, + "body": body, + "missingLabels": list(missing_labels), + "draft": draft, + } + ) + proc = subprocess.run( + ["node", "-e", harness, str(_CHECKER), payload], + capture_output=True, + text=True, + cwd=str(_REPO), + timeout=30, + ) + assert proc.returncode == 0, proc.stderr + return json.loads(proc.stdout) + + +def _added_labels(calls): + return { + call["params"]["labels"][0] + for call in calls + if call["method"] == "addLabels" + } + + +def _comment(calls): + comments = [call for call in calls if call["method"] == "createComment"] + return comments[0]["params"]["body"] if comments else "" + + +@pytest.mark.parametrize( + ("files", "body"), + [ + (["README.md"], _body()), + (["routes/example.py"], _body(app_ran=True)), + ( + ["static/js/example.js"], + _body( + app_ran=True, + screenshot=True, + media="https://github.com/user-attachments/assets/example", + ), + ), + ], +) +def test_complete_expected_state_is_ready(files, body): + calls = _run_checker(files, body) + + assert _added_labels(calls) == {"ready for review"} + assert not _comment(calls) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_ui_checkbox_without_media_still_needs_visual_evidence(): + calls = _run_checker( + ["static/js/example.js"], + _body(app_ran=True, screenshot=True), + ) + + assert _added_labels(calls) == {"needs visual evidence"} + assert "does not contain an actual attachment or link" in _comment(calls) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_explicit_not_run_is_honest_but_not_ready(): + calls = _run_checker( + ["services/example.py"], + _body(app_not_run=True), + ) + + assert _added_labels(calls) == {"needs runtime validation"} + assert "explicitly reports that app/runtime validation was not performed" in _comment(calls) + assert "does not prove that a local command ran" in _comment(calls) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_missing_validation_label_fails_soft(): + calls = _run_checker( + ["services/example.py"], + _body(app_not_run=True), + missing_labels=("needs runtime validation",), + ) + + assert "needs runtime validation" not in _added_labels(calls) + assert any( + call["method"] == "warning" + and 'Label "needs runtime validation" does not exist' in call["message"] + for call in calls + ) + assert not any(call["method"] == "setFailed" for call in calls) + + +@pytest.mark.parametrize( + "filename", + ("requirements.txt", "requirements-optional.txt"), +) +def test_requirement_manifests_require_runtime_validation(filename): + calls = _run_checker( + [filename], + _body(app_not_run=True), + ) + + assert _added_labels(calls) == {"needs runtime validation"} + assert not any( + call["method"] == "addLabels" + and call["params"]["labels"] == ["ready for review"] + for call in calls + ) + assert any( + call["method"] == "removeLabel" + and call["params"]["name"] == "ready for review" + for call in calls + ) + assert "Changed-file classification: **backend/runtime**." in _comment(calls) + + +def test_old_template_runtime_pr_is_not_ready_without_attestation(): + calls = _run_checker( + ["routes/example.py"], + _body(), + ) + + assert _added_labels(calls) == {"needs runtime validation"} + assert "App/runtime validation is not author-attested" in _comment(calls) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_conflicting_runtime_attestations_are_not_ready(): + calls = _run_checker( + ["services/example.py"], + _body(app_ran=True, app_not_run=True), + ) + + assert _added_labels(calls) == {"needs runtime validation"} + assert "both checked" in _comment(calls) + assert not any( + call["method"] == "addLabels" + and call["params"]["labels"] == ["ready for review"] + for call in calls + ) + + +def test_structurally_invalid_description_still_fails_hard_gate(): + calls = _run_checker( + ["README.md"], + "## Summary\nshort\n", + ) + + assert any(call["method"] == "setFailed" for call in calls) + assert "PR description" in _comment(calls) + assert "needs work" in _added_labels(calls) + assert "ready for review" not in _added_labels(calls) + + +def test_github_workflow_only_change_does_not_require_app_runtime(): + calls = _run_checker( + [".github/workflows/example.yml"], + _body(), + ) + + assert _added_labels(calls) == {"ready for review"} + assert not _comment(calls) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_draft_pr_never_receives_ready_for_review(): + calls = _run_checker( + ["README.md"], + _body(), + draft=True, + ) + + assert "ready for review" not in _added_labels(calls) + assert any( + call["method"] == "removeLabel" + and call["params"]["name"] == "ready for review" + for call in calls + ) + assert not any(call["method"] == "setFailed" for call in calls) + + +def test_workflow_serializes_readiness_before_mergeability(): + workflow = _WORKFLOW.read_text() + + assert ( + "types: [opened, edited, synchronize, reopened, ready_for_review, " + "converted_to_draft]" + ) in workflow + assert "group: pr-description-${{ github.event.pull_request.number }}" in workflow + assert "cancel-in-progress: true" in workflow + + mergeable = workflow.split(" check-mergeable:", 1)[1] + assert "needs: check-description" in mergeable + assert ( + "if: ${{ !cancelled() && github.event.pull_request.user.type != 'Bot' }}" + in mergeable + ) + + +def test_privileged_pr_workflow_executes_only_base_code(): + workflow = _WORKFLOW.read_text() + + assert "pull_request_target:" in workflow + assert "ref: ${{ github.base_ref }}" in workflow + assert "persist-credentials: false" in workflow + assert "github.event.pull_request.head" not in workflow