Repository navigation
docs: correct rule 10's host count and generalise the deep-review skip condition - #608
Conversation
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.
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR updates harden-runner host measurements and Azure blob-host examples. It also clarifies deep-review skip conditions, stale workflow copies, related incidents, and branch update guidance. ChangesHarden-runner inventory
Deep-review workflow guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This is a documentation-only correction with no workflow behavior change, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description provides a detailed summary, affected documentation scope, related issue references, rationale, and verification evidence. It does not use all template headings or checklist items, but the substantive information is mostly complete. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Two passages still conflate branch age or measured endpoints with the actual conditions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates automation documentation based on observed workflow behavior.
Changes:
- Clarifies stale workflow copies can silently skip deep reviews.
- Corrects the egress allowlist to nine hosts.
- Records another rotating Actions blob hostname.
File summaries
| File | Description |
|---|---|
CLAUDE.md |
Documents the generalized deep-review skip condition. |
.github/CLAUDE.md |
Updates allowlist and rotating-host guidance. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Preview DeploymentPreview URL: https://655a83d1.bestax.pages.dev |
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.
Preview DeploymentPreview URL: https://b7be1e1c.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The workflow’s inline documentation still retains the obsolete modifies-only explanation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
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.
Preview DeploymentPreview URL: https://63b24b4b.bestax.pages.dev |
|
🎉 This PR is included in version 4.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Two inaccuracies now on
main, both found by exercising the jobs #578 flipped rather than byreading the files. Docs only, no workflow behaviour changes.
1. Rule 10 said "the same eight hosts". It is nine.
Copilot caught during #602's review that the eight-host Claude-session list omits
nodejs.org,which
actions/setup-nodefalls back to when the toolcache misses and theactions/node-versionslookup fails. The workflows were fixed; that sentence in rule 10 was not, so
maincurrentlyclaims eight while all six jobs carry nine.
The corrected text also says why measurement could not have found it, which matters more than
the number: none of
ai-scan/scan,ai-triage/triageorclaude-repro/authorrunssetup-node, so their list never needed the host. Across six audit runs the toolcache hit everytime, so the fallback never executed and left no endpoint in any log. Review caught it, measurement
could not have. That is worth stating plainly next to a rule whose entire subject is measuring
allowlists.
Caught by the
respondverification session, which quoted the stale line back while confirming itsown job's state.
2. The deep-review skip condition was too narrow, and it is biting right now
CLAUDE.mdsaid a PR that modifiesclaude-review.ymlis not deep-reviewed. The realcondition 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.
Not hypothetical. #578's flip changed
claude-review.yml, so every PR opened before e5ed56b nowinherits a skipped deep review. Reproduced on #605:
"egress_policy":"audit"(the branch's own stale copy), session skipped, job greenmaininto the branch, re-ran: 33289641518 —block, assertion green, session ranAt the time of writing #601, #584, #551, #469 and #461 are all in that state.
3. Blob host rotation, seventh name
The
claudeverification run producedproductionresultssa17, joining sa3/6/7/9/11/13. Recorded,since it strengthens the existing "cannot be pinned even in principle" argument. Phrased as a
growing list rather than a fixed enumeration, because it will keep growing.
Verification evidence this came out of
Three of the six #578 jobs are now confirmed under enforcement, each read from a real run rather
than from YAML — assertion green,
"egress_policy":"block", zeroReverted changes/timed outin the agent log, and the session completing real work:
claude-review/reviewbestaxbot-reply/respondclaude/claudeTracked in #607, along with a gotcha worth keeping: grep the
Post Harden runnersection forReverted changes, not the whole run log — the assertion step's own error string contains thatphrase, so an unscoped grep always self-matches. Same shape as the
grep egress-policy: blocktrap the rule already documents.
Refs #578
Summary by CodeRabbit