ci(prs): separate validation readiness from description checks (#5939)

* ci(prs): separate validation readiness from description checks

* fix(ci): harden PR readiness state

---------

Co-authored-by: Alexandre Teixeira <alexandremagteixeira@gmail.com>

.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
 

.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.`);
   }
 };

.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:

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