Skip to content

fix(cicd): exclude spec- and docs-only PRs from the Release QA report - #37484

Open
nollymar wants to merge 1 commit into
mainfrom
claude/release-qa-report-filtering-304795
Open

fix(cicd): exclude spec- and docs-only PRs from the Release QA report#37484
nollymar wants to merge 1 commit into
mainfrom
claude/release-qa-report-filtering-304795

Conversation

@nollymar

@nollymar nollymar commented Sep 9, 2026

Copy link
Copy Markdown
Member

Closes #37486

Proposed Changes

The release QA report (.github/scripts/release-qa-status) flags every merged PR whose linked issue lacks a QA : Passed / QA : Not Needed / QA : Failed label. Since Spec-Kit landed, every feature ships two PRs and PR 1 carries spec.md alone under specs/<issue>-<slug>/. Those PRs contain nothing runnable, so QA has nothing to exercise and they never earn a QA label — yet the report counted them as un-QA'd, inflating the :rotating_light: Slack warning and @-mentioning their authors on every release. Pure documentation PRs had the same problem.

The report had no idea what files a PR touched: classifyExclusion only ever read authorType, author, labels and title. So:

  • github.ts — new fetchChangedFiles: batched GraphQL (20 PRs per query) rather than one REST listFiles per PR, so the cost is roughly one extra request per twenty on top of the 3+ REST calls per PR the tool already makes. Unlike its sibling fetchClosingIssueRefs it swallows GraphQL errors instead of re-throwing — there, an empty result silently demotes PRs to unlinked and floods Slack, so failing loudly is right; here an unknown file list just means no path-based exclusion, i.e. exactly the previous behaviour, and killing the whole QA section over it would be the worse trade.
  • exclusions.ts — new isDocumentationPath() plus a 4th rule in classifyExclusion, applied after the title heuristics so a bot-authored spec PR still reads bot-author.
  • types.ts — two new ExclusionReason values (spec-only, docs-only) and changedFiles?: string[], where undefined means unknown and is deliberately distinct from [].
  • format.ts — Excluded-section blurbs name the new reasons, and that section now renders on a clean release (see Additional Info).

What counts as documentation: specs/**, docs/**, and loose *.md / *.mdx.

What stays in QA scope — agent tooling is checked first: .claude/, .agents/, .cursor/, .specify/, and any CLAUDE.md / AGENTS.md at any depth. In this repo that markdown is the deliverable — #37309 (feat(skills): add dot-pr-spec-summary) shipped a whole feature without touching a non-markdown file. Without the exception, .claude/skills/x/SKILL.md would match the markdown pattern and drop a real feature out of the report.

Fail safe throughout: a PR is excluded only when its file list was fetched in full and every path is documentation. An absent or truncated list keeps the PR in QA scope — over-reporting beats silently hiding a code change. GitHub caps first at 100 and this isn't paginated past that, which is a judgement call rather than an oversight: the list exists only to answer "is every file documentation?", and a 100+ file PR never is.

Blast radius is CI tooling only — a standalone TypeScript CLI run by .github/workflows/cicd_6-release.yml. No new files, no changes to the workflow, and nothing under dotCMS/ or core-web/.

Checklist

  • Tests
  • Translations — n/a, no user-facing strings
  • Security Implications Contemplated — no new secrets or permissions; the new GraphQL query is read-only and PR numbers are re-validated with Number.isInteger(n) && n > 0 before alias interpolation, matching the existing guard in fetchClosingIssueRefs

Additional Info

Verified against a real releasev26.09.03-01...v26.09.09-01, 38 PRs, run before and after the change:

bucket before after
failed 0 0
missing 25 17
unlinked 0 0
external 0 0
passed 13 12
excluded 0 9

All 9 newly-excluded PRs were re-checked independently via gh, not through the code under test: every one is genuinely spec/docs-only (#37103, #37188, #37189, #37190, #37392, #37404, #37430, #37434, #37437). The reverse check found no false negatives — no docs-only PR was left in the flagged buckets. The truncation guard also fired for real on #37423 (>100 files), which correctly stayed in QA scope.

78 tests pass (cd .github/scripts/release-qa-status && npm ci && npm test); npx tsc --noEmit is clean.

Two things a reviewer should weigh:

  1. I removed an early return in renderText / renderMarkdown. Both bailed on flagged === 0 before the Excluded section. After this change the common case is exactly that — nothing flagged because the gaps were spec PRs — so the drop in counts would go unexplained. The Excluded table now renders on clean releases too, keeping the skip list auditable. renderSlack is untouched and still returns the empty string when nothing needs review, so this adds no Slack noise. This is a small behaviour change beyond the strict minimum; easy to revert if you'd rather keep the early return.
  2. docs(sdk-cli): add spec for dotcms agent setup #37392 moved passedexcluded. A spec PR whose linked issue already carried QA : Passed. Correct under the new rules, but it does mean passed no longer counts spec PRs that happened to be labelled.

Unrelated observation, not fixed here: npm start -- --format json > file writes npm's own banner ahead of the JSON, so a redirect isn't valid JSON. Pre-existing and harmless in the workflow (markdown into $GITHUB_STEP_SUMMARY), but worth knowing if anyone scripts against the JSON output.

Screenshots

n/a — CLI output only. Text format on the verification run:

Summary:
  failed:   0
  missing:  16
  unlinked: 0
  external: 0
  passed:   13
  excluded: 9

Excluded (9)
  Bot / dependency-bump / version-bump / release-machinery / spec-only / docs-only PRs
  (skipped before QA check).
  - #37103 docs(opensearch): add the ES → OpenSearch migration runbook … [docs-only]
  - #37404 Spec: UVE contentlet permission gating — @rjvelazco [spec-only]
  - #37434 docs(block-editor): revise the #37340 spec to an editor-only fix [spec-only]
  …

🤖 Generated with Claude Code

Since Spec-Kit landed, every feature ships two PRs and PR 1 carries spec.md
alone under specs/<issue>-<slug>/. Those PRs contain nothing runnable, so QA
has nothing to exercise and they never earn a QA label — yet the release QA
report counted them as un-QA'd, inflating the Slack warning and @-mentioning
their authors on every release. Pure documentation PRs had the same problem.

The report had no idea what files a PR touched: classifyExclusion only ever
read authorType, author, labels and title. So:

- github.ts: new fetchChangedFiles, batched GraphQL (20 PRs per query) rather
  than one REST listFiles per PR. Unlike fetchClosingIssueRefs it swallows
  errors instead of re-throwing — an unknown file list just means no
  path-based exclusion, i.e. the previous behaviour, so failing the whole QA
  section over it would be the worse trade.
- exclusions.ts: isDocumentationPath + a 4th rule, applied after the title
  heuristics so a bot-authored spec PR still reads bot-author. Documentation
  is specs/**, docs/** and loose *.md / *.mdx. Agent tooling is checked first
  and stays in QA scope (.claude/, .agents/, .cursor/, .specify/, any
  CLAUDE.md / AGENTS.md) — in this repo that markdown *is* the deliverable,
  and without the exception .claude/skills/x/SKILL.md would match the
  markdown pattern and drop a real feature out of the report.
- types.ts: spec-only and docs-only reasons; changedFiles?: string[] where
  undefined means unknown, deliberately distinct from [].

Fail safe throughout: a PR is excluded only on a file list fetched in full
whose every path is documentation. An absent or truncated list keeps the PR
in QA scope — over-reporting beats silently hiding a code change.

Excluded PRs also now render on a clean release. Both renderers previously
returned before that section when nothing was flagged, and after this change
the common case is exactly that — nothing flagged *because* the gaps were
spec PRs — which would leave the drop in counts unexplained. Slack is
untouched and still silent when nothing needs review.

Verified against v26.09.03-01...v26.09.09-01 (38 PRs): missing 25 -> 17,
excluded 0 -> 9. All 9 re-checked independently via gh — every one genuinely
spec/docs-only, and no docs-only PR was left behind. The truncation guard
fired for real on #37423 (>100 files), which correctly stayed in QA scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nollymar's task in 1m 25s —— View job


Code Review — Release QA report filtering

  • Read the full diff (8 files)
  • Reviewed exclusions.ts, github.ts, types.ts, format.ts + tests
  • Checked error paths, fail-safe logic, and injection surface

This is a well-designed, well-documented change. The undefined vs [] distinction for "unknown file list", the fail-safe-toward-QA-scope stance, the agent-tooling-before-docs ordering, and the swallow-vs-rethrow contrast against fetchClosingIssueRefs are all correct and clearly justified in the code comments. Test coverage is genuinely good — isDocumentationPath edge cases, the .claude//CLAUDE.md deliverable exception, mixed spec+impl, unknown list, and reason precedence are all covered.

New Issues

  • 🟡 Medium: .github/scripts/release-qa-status/src/github.ts:428 (the try/catch in fetchChangedFiles) — Assumption: Octokit's graphql() throws a GraphqlResponseError on a partial response (some aliases resolve, others error), rather than returning the partial data. If so, a single problematic PR in a 20-PR batch discards the file lists for the other 19, marking all 20 undefined. What to verify: confirm this is acceptable — the consequence is purely over-reporting (those 19 stay in QA scope, exactly today's behavior), so it's benign and consistent with the stated fail-safe intent, but it does mean one bad PR reduces path-based exclusion across its whole batch. Non-blocking. If you wanted to tighten it, GraphqlResponseError exposes .data, so the catch could still parse whatever resolved.

Notes (non-blocking)

  • isDocumentationPath matches .claude/, docs/, specs/ only as root prefixes (startsWith). A nested some/module/docs/x.md or sub/.claude/y.md would fall through to the *.md pattern (→ documentation) or to implementation respectively. This is fine for the current repo layout — just noting the prefixes are root-anchored by design, not depth-agnostic like the CLAUDE.md/AGENTS.md basename check.
  • The changedFiles: [] case is correctly kept in QA scope by the files.length > 0 guard in classifyExclusion and is explicitly tested — good, since GitHub would never return [] for a real merged PR, so an empty list genuinely signals "don't know".

No security concerns: PR numbers are re-validated with Number.isInteger(n) && n > 0 before alias interpolation (matching the existing fetchClosingIssueRefs guard), and the query is read-only.

Verdict: No blocking issues. The one Medium is an informational observation about batch-error granularity, not a defect — the outcome is safe in every case.
· claude/release-qa-report-filtering-304795

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Exclude spec-only and documentation-only PRs from the Release QA report

1 participant