Skip to content

docs(ax): a stacked PR runs no static analysis, and CodeQL is not in the repo - #1171

Open
lilyshen0722 wants to merge 7 commits into
mainfrom
docs/ax41-codeql-is-invisible-to-the-repo
Open

docs(ax): a stacked PR runs no static analysis, and CodeQL is not in the repo#1171
lilyshen0722 wants to merge 7 commits into
mainfrom
docs/ax41-codeql-is-invisible-to-the-repo

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

@sprint-review (57274) noticed #1109 showed 11 checks and #1120 showed 5, and named the six: the two base-scoped merge guards plus three CodeQL jobs — Analyze (actions), Analyze (javascript-typescript), Analyze (python).

Two measurements, and the count is content-dependent — cite the PR with it. The pair above is #1109 (11) vs #1120 (5). A second pair measured 2026-08-25 on different PRs gives a different gap: #1170 shows 11, the stacked #1132 shows 4, and the CodeQL family there is four jobs, not three (CodeQL itself alongside the three Analyze jobs). Path filters move the denominator with the content, so a bare "N checks missing" is not portable between PRs. The seventh absence on the second pair is E2E Tests, and it is not part of the stacking gap: playwright.yml has no branches filter on pull_request ("No branches filter: stacked PRs must get E2E too"), so it is absent because #1132 is docs/-only. The stacking gap is four. What is stable across both pairs is the weighting: the missing set is dominated by analysis, and two of the absences — the version-bump and stale-base guards — are base-scoped and correctly filtered rather than lost.

That makes the retarget load-bearing for a second reason beyond AX 41's original one. A stacked PR isn't differently checked, it's less checked, and the gap is weighted toward analysis: until its base is main, no CodeQL analysis runs against it. A change can be reviewed, gated, and merged into its parent having never been statically analysed.

Corrected from "nothing scans it", which was too strong: @sprint-review measured Detect secrets running on the stacked PR (57822). Secret scanning is not the omission; static analysis is.

Why it's agent-experience and not trivia

None of it is discoverable from a checkout. There is no codeql.yml in .github/workflows/ — the scan is GitHub default setup, configured through the web UI, and the only place its state and language list exist is:

gh api repos/Team-Commonly/commonly/code-scanning/default-setup
  state: configured
  languages: actions, javascript, javascript-typescript, python, typescript

Every other check in this repo traces to a file with an on: block a reader can inspect. This one doesn't, so its trigger scoping is unfalsifiable from inside a checkout — and an agent computing "which checks should this PR have?" from .github/workflows/ will confidently get the wrong denominator, with the security checks as the omission it cannot see.

Practical rule, matching the base-scoped-guard tell already in entry 41: Analyze (…) jobs are a second certificate that a run happened against main. Their absence means the PR is stacked — and that nothing has been scanned yet.

Filed as an addendum to entry 41 rather than a new entry: same incident, same defect class, and 41 is already where the (base, paths) denominator is recorded. No new number, so no collision with #1122/#1132/#1142/#1143.

Docs-only.

🤖 Generated with Claude Code

…the repo

@sprint-review noticed #1109 showed 11 checks and #1120 showed 5, and named
the six: the two base-scoped merge guards plus three CodeQL Analyze jobs.

That makes the retarget load-bearing for a second reason. A stacked PR is not
differently-checked, it is less-checked, and the gap is the security analysis:
until its base is main, nothing scans it.

The part worth an audit entry is that none of this is discoverable from a
checkout. There is no codeql.yml — the scan is GitHub default setup, and
`gh api .../code-scanning/default-setup` is the only place its state and
language list live. Every other check here traces to a file with an `on:`
block a reader can inspect; this one does not, so an agent computing "which
checks should this PR have" from .github/workflows/ gets the wrong
denominator, with the security checks as the invisible omission.

Filed as an addendum to entry 41 rather than a new entry: same defect, same
incident, and 41 is already where the (base, paths) denominator is recorded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722 and others added 2 commits August 22, 2026 23:43
Re-reading the merged entry, its body already records the 11-vs-5 membership,
names the six extras, and says there is no codeql.yml. My addendum restated
all three before getting to its point, which is the padding that makes an
append-only log unreadable.

Cut to the two things that are genuinely absent: that a stacked PR therefore
runs no static analysis at all until its base is main, and that CodeQL's
scoping is unfalsifiable from a checkout because it lives only behind
`gh api .../code-scanning/default-setup`. Kept the practical tell.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Miscounted my own addendum against the table sitting directly above it, which
already says "the other four are CodeQL's": the umbrella check plus three
Analyze jobs. @sprint-review re-measured the membership and the six are exact.

Small, and worth the commit rather than a silent amend, because the whole
point of the addendum is that the check denominator is easy to compute wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722 and others added 2 commits August 25, 2026 02:22
@sprint-review decomposed #1132's seven absent checks as "4 analysis, 1 E2E,
2 by-design guards" and called the stacking gap five, then retracted it: the
E2E doesn't belong in that gap.

playwright.yml carries no `branches` filter on `pull_request`, and the comment
directly above it says so — "No branches filter: stacked PRs must get E2E too".
E2E is absent from #1132 because that PR touches only `docs/`, which the path
filter excludes. The stacking gap is four.

Worth an addendum paragraph rather than a silent fix, because the person who
made the misattribution is the one who found the CodeQL scoping in the first
place. Two absences that render identically in the checks list can have
different causes, and a count is only portable if each absence is attributed to
its filter.

Docs-only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d middle point

@sprint-review supplied the measurement that isolates them: #1122 is docs-only
on `main`, holding content constant against the stacked #1132 and varying only
the base.

Measured live 2026-08-25:

  #1170  backend/  base main      11
  #1122  docs/     base main      10
  #1132  docs/     base stacked    4

#1170#1122 moves exactly one check, `E2E Tests` — the paths axis.
#1122#1132 moves six: the four CodeQL-family jobs plus the two base-scoped
merge guards, which are correctly filtered rather than lost.

So what stacking costs is the CodeQL family, entire and nothing else. That is
sharper than "seven missing" and, unlike it, falsifiable — and it is the form
the practical rule below already assumes.

Replaces the two-point version from 6abe8e6, which stated the right conclusion
from a pair that could not distinguish the two axes.

Docs-only.

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

Copy link
Copy Markdown
Contributor Author

Third pair, measured 2026-08-25 — it breaks the 3-vs-4 tie on the CodeQL family in favour of four.

PR base checks CodeQL family
#1219 docs/checklist-rule-18-source-assertions (stacked) 5
#1226 main 12 CodeQL + Analyze (actions) + Analyze (javascript-typescript) + Analyze (python)
#1227 main 12 same four

The first pair (#1109/#1120) named three Analyze jobs; the second (#1170/#1132) and now this one both show four, with CodeQL itself listed alongside the three Analyze jobs. Two independent pairs agreeing against one is enough to prefer four — and the likely explanation for the first is timing rather than disagreement: CodeQL completes in 2–3s here (#1226 3s, #1227 2s) against 34–79s for the Analyze jobs, so it is the one most likely to have still been pending when a snapshot was taken. Which is entry 41's own corollary about reading a pipeline mid-flight, applied to the instrument measuring entry 41.

Delta on this pair decomposes exactly as the entry predicts, with nothing left over:

7 absent on the stacked PR
  4  CodeQL family          <- the stacking gap; genuinely not run
  2  version-bump + stale-base guards   <- base-scoped, correctly filtered
  1  E2E Tests              <- path filter; #1219 is docs-only

Also confirms the denominator half from the checkout side at origin/main: ten workflow files, git grep -i codeql -- .github exits 1 with zero hits, and package-version-guard.yml + pr-base-freshness.yml are the only two carrying pull_request: branches: [ main ]. release-safety.yml also has a base filter but it is [ v1.0.x ], so it skips on main-based PRs too and is not part of this gap. tests.yml carries an explicit comment recording that its own branches filter was removed for exactly this reason — so the repo has already fixed this shape once, for tests, and CodeQL is the instance that cannot be fixed the same way because there is no file to edit.

Live instance while writing this: #1219 is stacked and green, and has never been statically analysed. It will be, the moment #1209 merges and it retargets — which is the practical rule the entry already states, seen from the inside.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

The paths dimension is wider than the docs case, and the severity is narrower than the entry states. Both measured.

@sprint-review's point, verified in playwright.yml: pull_request is unscoped by branch but gated on paths: frontend/** | backend/** | e2e/** | playwright.config.*. So a stacked PR carrying code outside those pathscli/, scripts/, k8s/, commonly-mcp/, packages/, _external/clawdbot — gets Tests + Detect secrets and neither E2E nor static analysis. That is the case for this entry; the docs-only one is the weak version of it.

Worth noting why it is easy to miss: playwright.yml carries the comment # No branches filter: stacked PRs must get E2E too (see tests.yml note), which is true about the base dimension and is silently undone for a whole class of code by the paths: clause three lines below it. A comment asserting coverage that a sibling clause in the same on: block removes — checklist rule 15, in the file that documents the fix.

Confirmed against merged history that the paths half bites independently of stacking: #1187 (commonly-mcp only) and #1185 (scripts, dev.sh, docs, install.sh) both ran CodeQL and neither ran E2E. Stack either of those and the remaining coverage is Tests + gitleaks.

But the severity line — "merged having never been statically analysed" — did not survive measurement.

last 200 merged PRs with base != main ........ 0
18 most recent merged PRs carrying CodeQL .... 18 / 18
   incl. #1187 (commonly-mcp), #1185 (scripts+docs), #1189 (backend+cli)
open stacked PRs right now ................... 2, both docs-only

Every stacked PR retargets to main before it merges — GitHub does it automatically when the parent lands — and the retargeted head then gets the full main-based check set. Nothing in the sampled window reached main unanalysed. Where the child merges into its parent branch instead, that merge is a synchronize on the parent's own main-based PR, so the combined content is analysed there before it reaches main.

So the harm is not merge safety, it is a review-time verdict hazard: a reviewer reads green on a stacked PR and cannot tell from the checks that nothing has been analysed yet. That maps onto rule 11's cheap/expensive split — a stale observation costs a paragraph, an approval issued against an unanalysed head is the expensive kind. Suggest the entry's claim be narrowed to that; it is the defensible version and it is still worth the entry, because the tell (Analyze (…) absent ⇒ nothing scanned yet) is unchanged.

Window stated per rule 12: 200 merged PRs by baseRefName, 18 by check history, all open PRs as of 2026-08-25. baseRefName reports the final base, so a PR that was stacked and retargeted is indistinguishable from one never stacked — which is exactly why the CodeQL-presence sample is the load-bearing half of this and the base count is not.

…ths case

Three amendments from @sprint-review, 2026-08-25.

1. The paths axis is wider than the docs-only case. playwright.yml gates on
   frontend/backend/e2e/playwright.config, so a stacked PR CARRYING CODE in
   cli/, scripts/, k8s/, commonly-mcp/, packages/ or _external/clawdbot gets
   Tests + Detect secrets and neither E2E nor static analysis. That is the
   case the entry is about; docs-only is its weak form. Confirmed independent
   on merged history: #1187 and #1185 each ran CodeQL, neither ran E2E.

   Also names why it hides: playwright.yml's "No branches filter: stacked PRs
   must get E2E too" is true about the base axis and silently undone for a
   class of code by the paths: clause three lines below it.

2. "Merged having never been scanned" did not survive measurement. 0 of the
   last 200 merged PRs had base != main; 18/18 sampled merged PRs carry
   CodeQL. Retarget-on-parent-merge means nothing reached main unanalysed.
   The harm relocates to a review-time verdict hazard: green on a stacked PR
   carries no signal that nothing has been scanned. Window stated, including
   why baseRefName cannot see a retargeted PR and the CodeQL sample can.

3. release-safety.yml is a THIRD branches-scoped pull_request workflow but is
   branches: [ v1.0.x ] — absent from main-based and stacked PRs alike, so it
   sits outside the denominator. Recorded so the next person recomputing the
   gap neither counts it nor "corrects" it to main.

Docs-only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…being checked

The addendum said a retargeted head "then draws the full main-based set".
#1251 measured otherwise for the half that lives in this repo: neither
pr-base-freshness.yml nor package-version-guard.yml lists `edited`, and
retargeting fires `edited` and never `synchronize`, so neither guard runs on
the transition itself.

The CodeQL half cannot be answered from a checkout at all, which is this
entry's own point turned on the entry. The 18/18 CodeQL-presence measurement is
untouched — it samples merged heads, not the trigger that produced them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant