From 7305999649a7375b8b938297a0db7b84fd861614 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 30 Aug 2026 09:45:38 -0400 Subject: [PATCH 1/3] docs: correct rule 10's host count and generalise the deep-review skip Two inaccuracies on main, both found by exercising the jobs #578 flipped rather than by reading the files. **Rule 10 said "the same eight hosts".** It is nine. Copilot caught during review that the eight-host Claude-session list omits `nodejs.org`, which setup-node falls back to when the toolcache misses and the `actions/node-versions` lookup fails. The workflows were fixed; that sentence was not. It now names the ninth host and says why measurement could not have found it: none of the three jobs the list was copied from runs setup-node, and across six audit runs the toolcache hit every time, so the fallback left no endpoint in any log. Review caught it, not measurement. That is worth stating next to a rule whose whole subject is measuring allowlists. **The deep-review skip condition was too narrow.** CLAUDE.md said a PR that MODIFIES claude-review.yml is not deep-reviewed. The real condition is a PR whose copy of that file differs from the default branch's, because the workflow runs from the PR head. A branch that merely predates a change to the file is equally stale and equally silent -- green job, no review, no signal. That is not hypothetical: #578's flip changed claude-review.yml, so every PR opened before it now inherits a skipped review. #605 reproduced it exactly (its own branch copy still read `egress-policy: audit`, session skipped), and merging main into the branch fixed it. Also records the blob host rotation reaching a seventh distinct name (sa17, from the `claude` verification run), which strengthens the existing "cannot be pinned even in principle" argument. --- .github/CLAUDE.md | 11 +++++++++-- CLAUDE.md | 14 +++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index c2e2b2d7a..4ab4ac367 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -482,7 +482,13 @@ jobs (`ci.yml`, `deploy.yml`, `test-deploy.yml`, `visual-regression.yml`, `story `claude-pr-loop` (`fix` and `verify`). Nineteen jobs; the command below is the check. - **Audit, deliberately, pending a measured allowlist** — none, as of #578. That issue closed the group by measuring all six of its members instead of guessing for them, and all six landed on - the same eight hosts the Claude-session jobs above already used. Keep the group here rather + the same nine hosts: the eight the Claude-session jobs above already used, plus `nodejs.org`. + That ninth is the interesting one, and it is why "measured" is not the same as "complete". None + of `ai-scan`/`scan`, `ai-triage`/`triage` or `claude-repro`/`author` runs `actions/setup-node`, + so their eight-host list never needed it; all six of #578's jobs do run it, and setup-node falls + back to `nodejs.org/dist` when the toolcache misses and the `actions/node-versions` lookup fails. + Six audit runs could not surface that, because the toolcache hit every time and an unexercised + path leaves no endpoint in any log. Review caught it, not measurement. Keep the group here rather than deleting it: audit remains the correct starting point for an **existing** live job whose egress has never been observed, and the recipe under this rule is what keeps it a short state rather than an open-ended one. What it is not is a resting place, which is the whole reason @@ -564,7 +570,8 @@ Two things about reading its output, both learned assembling the #578 lists: `productionresultssa.blob.core.windows.net`, `run-actions--azure-*.actions.githubusercontent.com` and `hosted-compute-*.githubapp.com` are the runner talking to its own control plane. The blob host cannot be pinned even in - principle — its name rotates per run (sa3, sa6, sa7, sa9, sa11 and sa13 across six runs). Run + principle — its name rotates per run (sa3, sa6, sa7, sa9, sa11, sa13 and sa17 so far, and the + list keeps growing every time anyone looks). Run 33221210633 is the evidence that omitting them is right: `auto-close-duplicates` at `block` with the five-host list observed only `api.github.com` and `github.com`, and completed all fifteen of its API calls under the firewall. diff --git a/CLAUDE.md b/CLAUDE.md index 8d72415fa..4f45db778 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,9 +145,17 @@ green and every AI review thread is resolved. lands as a PR review from `claude` marked ``; it reviewed the code checked out when its workflow started, which a racing push may have superseded — so look for that review comment (not the current head's checks) and verify its findings - against current code. **Today a PR that modifies `claude-review.yml` is not deep-reviewed:** - the run logs `Skipping action due to workflow validation` and posts nothing while the job - still goes green — so check for the review comment, never the job's conclusion. That is a + against current code. **Today a PR whose copy of `claude-review.yml` differs from the default + branch's is not deep-reviewed:** the run logs `Skipping action due to workflow validation` and + posts nothing while the job still goes green — so check for the review comment, never the job's + conclusion. Read that condition as written, because the narrower version ("a PR that _modifies_ + `claude-review.yml`") is what this line said until it bit. The workflow runs from the **PR + head's** copy, so a branch that merely PREDATES a change to that file is equally stale and + equally silent. #578's flip changed the file, and every PR opened before it inherited a + skipped review; #605 reproduced it (`"egress_policy":"audit"` from the branch's own old copy, + session skipped) and merging `main` into the branch fixed it. Expect this after any edit to + `claude-review.yml`, and bring a branch up to date before trusting a green deep-review job on + it. That is a consequence of configuration rather than a property of the action: the validation lives on the OIDC to app-token exchange, and `setupGitHubToken()` returns before reaching it whenever a `github_token` input is supplied — the same early return `ai-triage.yml` already documents From 23062c3a9cd4634dc59e4de81e4e9c0587c29f5e Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 30 Aug 2026 10:36:21 -0400 Subject: [PATCH 2/3] docs: tighten both corrections after review Copilot caught two accuracy defects in a PR whose entire subject is accuracy, which is a fair thing to have happen. **Branch age is not the criterion.** The example said a branch that "predates" a change to claude-review.yml is stale, which contradicts the precise condition stated two sentences earlier. Age is neither necessary nor sufficient: an old branch that has merged or rebased the current version is fine, and a branch opened minutes ago off a stale base is not. What matters is only whether the head's retained copy still matches the default branch. **Measured set is not the allowlist.** The inventory said all six jobs "landed on the same nine hosts", which conflates what the runs produced with what the list contains -- and contradicts the next two sentences, which say nodejs.org never appeared in any run. The runs surfaced six application hosts, a strict subset of the eight already in use; nodejs.org is on the list despite never being observed, and that gap is the whole reason the paragraph exists. --- .github/CLAUDE.md | 8 +++++--- CLAUDE.md | 14 ++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index 4ab4ac367..93dc0b7d0 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -481,9 +481,11 @@ jobs (`ci.yml`, `deploy.yml`, `test-deploy.yml`, `visual-regression.yml`, `story `claude-implement` (`implement`), `bestaxbot-reply` (`respond`), `claude-review` (`review`), `claude-pr-loop` (`fix` and `verify`). Nineteen jobs; the command below is the check. - **Audit, deliberately, pending a measured allowlist** — none, as of #578. That issue closed the - group by measuring all six of its members instead of guessing for them, and all six landed on - the same nine hosts: the eight the Claude-session jobs above already used, plus `nodejs.org`. - That ninth is the interesting one, and it is why "measured" is not the same as "complete". None + group by measuring all six of its members instead of guessing for them. All six now **use** the + same nine-host allowlist, and the gap between that and what the measurement produced is the part + worth keeping: the runs surfaced six application hosts, a strict subset of the eight the + Claude-session jobs above already used, and `nodejs.org` appeared in no run at all. It is on the + list anyway, and that ninth host is why "measured" is not the same as "complete". None of `ai-scan`/`scan`, `ai-triage`/`triage` or `claude-repro`/`author` runs `actions/setup-node`, so their eight-host list never needed it; all six of #578's jobs do run it, and setup-node falls back to `nodejs.org/dist` when the toolcache misses and the `actions/node-versions` lookup fails. diff --git a/CLAUDE.md b/CLAUDE.md index 4f45db778..b3e26ffc5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -150,12 +150,14 @@ green and every AI review thread is resolved. posts nothing while the job still goes green — so check for the review comment, never the job's conclusion. Read that condition as written, because the narrower version ("a PR that _modifies_ `claude-review.yml`") is what this line said until it bit. The workflow runs from the **PR - head's** copy, so a branch that merely PREDATES a change to that file is equally stale and - equally silent. #578's flip changed the file, and every PR opened before it inherited a - skipped review; #605 reproduced it (`"egress_policy":"audit"` from the branch's own old copy, - session skipped) and merging `main` into the branch fixed it. Expect this after any edit to - `claude-review.yml`, and bring a branch up to date before trusting a green deep-review job on - it. That is a + head's** copy, so what matters is only whether that copy still matches the default branch — + never the branch's age, and never whether the PR touched the file. An old branch that has since + merged or rebased the current version is fine; a branch opened five minutes ago off a stale + base is not. #578's flip changed the file, so every PR still carrying the pre-flip copy + inherited a skipped review; #605 reproduced it (`"egress_policy":"audit"` read from the branch's + own retained copy, session skipped) and merging `main` in fixed it. Expect this after any edit + to `claude-review.yml`, and confirm the head's copy matches before trusting a green + deep-review job. That is a consequence of configuration rather than a property of the action: the validation lives on the OIDC to app-token exchange, and `setupGitHubToken()` returns before reaching it whenever a `github_token` input is supplied — the same early return `ai-triage.yml` already documents From 9cadfb4c125218bc87a97266598114c6fef1a958 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Sun, 30 Aug 2026 10:44:33 -0400 Subject: [PATCH 3/3] docs: fix the same narrow skip explanation inside claude-review.yml Copilot's point, and a fair one: this PR corrected the misconception in CLAUDE.md while leaving it intact at claude-review.yml:534, which is exactly where someone editing the workflow would read it. The comment said the action skips "when the PR modifies this workflow file". The real condition is whether the PR head's copy differs from the default branch's, whether or not the PR touched it. Now says so, and names the incident: #578's flip changed this workflow, and every PR still carrying the pre-flip copy started getting silently skipped reviews without having touched the file. Comment-only change. No logic, triggers, permissions or action SHAs, and the harden-runner step still reads block with nine hosts and its assertion immediately after. --- .github/workflows/claude-review.yml | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index ab6be4732..291503118 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -531,9 +531,16 @@ jobs: # usage-limit failure on the very first call). The PR then silently # gets no review while the run shows green, and the AI loop's # deep-review gate waits on a review that never comes. Fail loudly. - # An empty/missing execution file is tolerated: the action skips - # without a session when the PR modifies this workflow file (its - # OIDC exchange requires the file to match the default branch). + # An empty/missing execution file is tolerated: the action skips without + # a session whenever the PR HEAD's copy of this file differs from the + # default branch's (its OIDC exchange requires them to match). + # + # Read that as written. It is NOT "when the PR modifies this file" — that + # narrower version is what CLAUDE.md said until #578's own flip changed + # this workflow and every PR still carrying the pre-flip copy started + # getting silently skipped reviews. The branch never touched the file; + # it just had not caught up. Age is not the criterion either: a branch + # that has since merged or rebased the current version is fine. - name: Fail on silent session error if: steps.claude.outcome == 'success' env: