fix(auto-rebase): source predicate from reusable's own commit, not @v1 (restores review-ready gate)#528
Conversation
The reusable defaulted `tooling_ref` to `v1` and checked out petry-projects/.github@v1 to source lib/eligibility.sh. But eligibility.sh landed in #468 (after the v1 tag), so every run failed with: .auto-rebase-tooling/.github/scripts/auto-rebase/lib/eligibility.sh: No such file or directory ##[error]Process completed with exit code 1. This broke auto-rebase in petry-projects/.github itself (local `./` ref → default tooling_ref=v1) and would break every consumer the moment the restriction reached them. The review-ready gate (#465) was therefore live nowhere. Fix: default tooling_ref to empty and resolve the checkout to `github.job_workflow_sha` — the commit SHA of this reusable file for the run. The predicate is now always sourced from the exact same commit as the workflow that uses it, eliminating version skew. `tooling_ref` remains an explicit override for end-to-end branch testing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LUUSUQHqwLWZ583SAs41K
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe reusable auto-rebase workflow now defaults ChangesAuto-rebase workflow fallback and lint update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Code Review
This pull request updates the documentation in .github/scripts/auto-rebase/README.md to clarify that the tooling_ref input now defaults to empty, resolving to the workflow's own commit rather than v1. The review feedback suggests a minor phrasing improvement to change 'behind PRs' to 'out-of-date PRs' for better readability.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 02b28b504622f048dcce7a45f0d61ee5a2205bc4
Review mode: triage-approved (single reviewer)
Summary
Fixes the auto-rebase reusable workflow so it sources lib/eligibility.sh from the reusable's own commit instead of the mutable @v1 tag (which predates eligibility.sh and made every run fail). Changes the tooling_ref default from 'v1' to '' and resolves the checkout ref to inputs.tooling_ref || github.job_workflow_sha. Scope is small (3 files, +20/-8): the reusable workflow, its README, and a supporting ci.yml actionlint ignore for the job_workflow_sha context property. The fix is security-positive — it pins tooling to an immutable commit SHA rather than a movable tag, and the checkout keeps persist-credentials: false.
Linked issue analysis
No issues are formally linked (closingIssuesReferences is empty). The PR body and commit reference #465 (review-ready gate) and #468 (eligibility.sh introduction) as context for the regression being fixed; these are explanatory, not closing references. Nothing to verify as 'closed' here.
Findings
- Core change (auto-rebase-reusable.yml): defaulting tooling_ref to '' and falling back to github.job_workflow_sha is the correct, idiomatic way to bind a reusable workflow's helper-script checkout to its own pinned commit. Eliminates the version skew that caused the failures. No injection or credential-exposure concerns (job_workflow_sha is GitHub-provided; persist-credentials: false retained).
- ci.yml: the added actionlint ignore ('"job_workflow_sha" is not defined') is a legitimate, scoped supporting change — actionlint 1.7.7's schema omits job_workflow_sha. The Lint check passes with it, confirming correctness. Not mentioned in the PR body's Fix section but clearly in-scope.
- README.md: docs updated to match the new default; also incorporates gemini-code-assist's wording suggestion ('behind PRs' -> 'out-of-date PRs').
- No secrets, no auth/crypto changes, no DB/migration changes.
- Secret scanning MCP tool (run_secret_scanning) was not available in this environment; relied on the passing gitleaks CI check instead. No fabricated result.
- Note (per PR author, out of scope): feature-ideation-reusable.yml reportedly carries the same tooling_ref: v1 default pattern — flagged there for a separate fix; not addressed here, which is acceptable.
CI status
All substantive checks green: Lint, ShellCheck, Lint and bats, CodeQL (Analyze actions), SonarCloud, AgentShield, Agent Security Scan, Secret scan (gitleaks), Dependency audit, and pr-auto-review all SUCCESS. CodeRabbit status SUCCESS and CodeRabbit review APPROVED at head SHA. Dependency-audit language scanners SKIPPED (no matching ecosystems). The only non-success entries are dev-lead dispatch/ci-relay marked CANCELLED — these belong to the agentic review pipeline itself (superseded runs) and are not real failures. reviewDecision is REVIEW_REQUIRED pending the org-leads human gate, which is expected.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #528 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
…ive (#736) (#953) * docs(initiatives): record review-ready gate now deployed & verified live The §2 ≥50% verdict was computed from the eligible-PR multiplier (a predicate snapshot). At authoring time the restriction was not actually filtering in production: the central reusable defaulted tooling_ref to v1 (predates eligibility.sh from #468), so auto-rebase failed to source the predicate and ran the original unrestricted fan-out everywhere. Fixed in petry-projects/.github#528 (job_workflow_sha) on 2026-06-24, then the auto-rebase/stable channel was promoted org-wide. Gate now verified live (.github-private PR#744 went from auto-updated to skipped). Adds a dated §2 update note + Status-line tag. The free mitigation behind the "defer Merge Queue" decision is now genuinely in effect, so the §4 deferral holds on stronger footing. Docs-only; no decision change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LUUSUQHqwLWZ583SAs41K * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
* feat(personas): add the persona @-mention router Serves the addressing contract that .github#752 defined. ONE router for every persona — personas do NOT each ship a mention workflow (§4.1); a stub per agent is exactly the drift the manifest exists to prevent, and it is how the fleet got two divergent registries the last time. There is no persona index, by design Because address.handle's slug MUST equal `id`, and `id` MUST equal the persona's directory name (both enforced by validate-personas.py), '@petry-projects/qa-lead' resolves to personas/qa-lead/persona.yml by convention. The manifest is the index-of-record (§1.1) — nothing derives, caches or duplicates it. This also avoids inverting the established derivation direction: canary-rings.json (.github) -> release/registry.yml (.github-private), whereas a persona index would have had to flow .github-private -> .github. A 404 is a meaningful answer ("not a persona"), which is also how a real, non-persona team like @petry-projects/org-leads falls through harmlessly. What decides what The manifest, not this router: whether the mention surface is enabled, in what mode, behind which trust floor, under which opt_out_label. The router restates none of it. A persona may TIGHTEN the job-level trust default but never loosen it — an undeclared floor denies, so a persona that forgot to declare trust is not more permissive than one that did. Recursion (the reason the core is a tested library, not inline shell) Comments posted via a PAT re-trigger workflows, unlike GITHUB_TOKEN. .github-private#860 burned 1,481 identical acks in 4.5h from a SINGLE self-loop. With N mutually addressable personas the cycles are combinatorial, not self-loops: qa-lead answering a thread that mentions dev-lead is enough. So the guard is two independent axes — bot actor AND agent marker — enforced both in the job-level `if` (so secrets never reach a bad run) and in pm_should_route (so it is testable). The marker match is a PREFIX: #860's first fix matched one exact ack string and still self-looped through a different agent comment. Contents - scripts/lib/persona-mention.sh — the pure, network-free decision core. Fetching is the workflow's job, so the suite pins routing behaviour with no API access at all. - tests/persona_mention.bats — 31 tests, recursion guards most heavily. - .github/workflows/persona-mention-tests.yml — the runner. A bats suite with no CI runner is unenforced (the #613/#624 gap canary-rollout-tests.yml exists to close); added WITH the suite rather than after it. - .github/workflows/persona-mention-reusable.yml — the router. Sources the lib at `github.job_workflow_sha` so the logic always matches the pinned workflow version (the #465/#528 tooling_ref lesson); every event field arrives via env, never inline `${{ }}` in a run: body. - standards/workflows/persona-mention.yml — the single caller stub, pinned to persona-mention/v1-stable (major-scoped per #657, matching live stubs). - standards/canary-rings.json — registers the reusable so a release can be cut. Deliberately NOT here - ring-pins.sh RING_REUSABLES is untouched: adding persona-mention would make compliance-audit demand the stub in repos that do not have it and make the deploy sweep report drift — manufacturing failures for a reusable with no cut release. That belongs to rollout. - No release cut, no repo enrolled, and no persona runtime exists yet, so NOTHING routes on merge. The router dispatches `persona-mention` to .github-private, where nothing listens yet — a safe no-op. Verification (CI's exact invocations, run locally) - bats tests/persona_mention.bats: 31/31. - shellcheck --severity=warning -x scripts/**/*.sh: clean. - actionlint -ignore SC2129 -ignore '"job_workflow_sha" is not defined' .github/workflows/*.yml: clean. (That ignore is pre-existing and correct — job_workflow_sha is a real context property actionlint 1.7.7's schema omits; the merged auto-rebase-reusable.yml trips the same false positive.) - markdownlint-cli2: clean. - canary-rings.json: spliced as TEXT, not re-serialized — the file's encoding is genuinely mixed (em-dashes escaped, arrows half-literal) and its agent order is insertion order, not alphabetical, so a json round-trip rewrote ~140 untouched lines. Result is 63 insertions / 0 deletions, and the entry is structurally identical to pr-review-mention's but for host/reusable/run_workflow. One bug the tests caught: pm_manifest_query piped python into jq, so the pipeline reported JQ's status and a parse failure returned empty output with exit 0 — indistinguishable from "the manifest says no". Now captured and checked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reviews): address review comments [skip ci-relay] * fix(personas): close the fail-open paths and enforce the write gate Addresses the review findings on this PR (#755) and the bot review. 1. Fail-open trio — errors were read as negative answers The systemic flaw #755 names as its headline finding. Three instances here: - Manifest fetch: `curl -f` collapses 404 and 5xx into one exit code, so a raw.githubusercontent hiccup silently disabled EVERY persona fleet-wide, with no error anywhere and indistinguishable from nobody being addressed. Now switched on the status code: 200 routes, 404 is a real answer ("not a persona" — a plain team like @petry-projects/org-leads, or a typo), anything else is a hard failure. - Labels: `|| echo ""` turned an API error into "no labels", which would bypass an opt-out AND arm an ungated write. Now fails closed. Discussions genuinely have no labels surface, so an empty set is the TRUE answer there — that difference is load-bearing (see 2). - pm_manifest_query already fixed in 518ff2b (pipeline reported jq's status). 2. gate_label is now ENFORCED (security) The schema makes gate_label required for write surfaces, but only enforces that it is DECLARED — declaring a lock is not locking the door. The router read `mode`, passed it in the payload, and dispatched. '@petry-projects/dev-lead do X' would have been an ungated write the moment the first write persona onboarded. Now: write-mode requires the gate label to be APPLIED; a write persona with no gate_label is a hard error; and write-mode on a surface where labels are unavailable (discussions) is refused, because the gate cannot be verified. gate_label is read by its own function, not a 4th space-separated field on pm_mention_decision: an empty optional field silently shifts the next one into its place ("true write qa-lead" parses the gate AS the opt_out_label). A security gate is the last place to accept that. Test 42 pins it. 3. contents: read + PyYAML (Copilot) The job checks out the tooling repo but never granted contents: read. PyYAML is NOT documented as preinstalled on the runner images, and this reusable runs on the CALLER's runner, so it cannot be assumed — our own tests install it explicitly, which proved the gap. Added a cheap no-op-when-present step. Also persist-credentials: false on the tooling checkout. 4. Mention precision (#755 finding 9) The router fired where GitHub itself renders NO mention: fenced code, inline code. Pasting a usage example or documenting the handle summoned the persona. Blockquotes are a DELIBERATE divergence — GitHub does notify on a quoted mention, but its one-click "Quote reply" copies a whole prior comment prefixed '>', and re-running an agent over quoted text is not what the quoter meant. Neither recursion axis catches it (a human quoting a human is not a bot and carries no marker). To address a persona, mention it outside the quote. Verification (CI's exact invocations) - bats tests/persona_mention.bats: 42/42 (31 + 11 new covering fences, tilde fences, inline code, blockquotes, the two must-still-fire cases, and the gate_label read-shift trap). - shellcheck --severity=warning -x scripts/**/*.sh: clean. - actionlint (CI's ignores): clean. NOT fixed here, deliberately - aliases (#755 finding 1): the merged schema promises address.aliases[] but index-free routing cannot honour it. Dropping it needs an ORDERED cross-repo pair — .github-private must remove `aliases: []` from qa-lead's manifest first, or dropping it from the schema (additionalProperties: false) breaks validate-personas on main fleet-wide. Tracked in #755; it is a contract change and deserves its own PR. - review_requested routing (#755 finding 10) and the 👀 ack (finding 11) are scoped out to keep this PR about correctness and security. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(personas): drop the dead review_requested trigger from the stub The stub subscribed to pull_request/review_requested, but the router has no path for it — the job's `if` never admits the event, so every reviewer assignment fleet-wide would start a run that immediately skips. §4 does say "Reviewer-assignment counts here too", and it is a real gap (#755 finding 10). But the trigger lands WITH the routing, not before it: a trigger firing into a router that ignores it is dead weight that only burns runs and makes the stub lie about what it supports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(personas): hash-lock the PyYAML install The dependency-hardening gate caught the PyYAML fix from 1953ccd: not ok 1 every 'pip install' in a workflow is hash- and binary-locked It was right to. `pip install --quiet pyyaml` is an unpinned, unverified supply-chain install — and this reusable runs on EVERY repo in the fleet that adopts the stub, so it is the worst possible place to resolve a package at runtime from whatever PyPI serves that day. Replaced with the org's established pattern (ci.yml, dependency-audit-reusable): pip install --require-hashes --only-binary :all: -r <name>-requirements.txt Adds scripts/persona-mention-requirements.txt — PyYAML 6.0.2 pinned with all 53 wheel hashes, generated by pip-compile --generate-hashes, with the regeneration command in its header like the sibling requirements files. Satisfies SonarCloud githubactions S8541/S8543/S8544/S8545 as the other files document. The `python3 -c 'import yaml' || ...` fast path is kept, so the install is a no-op on runners that already have it. Verification - bats test/workflows/dependency-hardening.bats: 3/3 (was 1 failing) — including "each hash-locked requirements file referenced by a workflow exists". - bats tests/persona_mention.bats: 42/42. - shellcheck + actionlint (CI's invocations): clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reviews): address review comments [skip ci-relay] * fix(personas): do NOT authenticate the manifest fetch — it adds a silent-death path Reverts the Authorization header added in 9333997, which was made in response to CodeRabbit's 🔴 Critical "the repo is private, so raw.githubusercontent will 404 and every persona gets skipped". That premise is false, and acting on it introduces the exact bug this PR exists to fix. petry-projects/.github-private is PUBLIC despite its name (private: false — every one of the org's 11 repos is public). Measured against the real manifest URL: unauthenticated -> 200 valid token -> 200 expired token -> 404 empty token -> 404 raw.githubusercontent answers a bad or MISSING token with 404, not 401. Our router — correctly — treats 404 as a real answer: "not a persona". So with an Authorization header, the moment GH_PAT_WORKFLOWS expires, or a repo adopts the stub before the secret exists, or a fork PR runs without secrets, EVERY persona 404s and is silently skipped as "not a persona". Fleet-wide, silent, and indistinguishable from nobody being addressed. Unauthenticated cannot fail that way for a public repo. The header buys nothing (200 either way) and adds a silent-death path — the precise fail-open class this PR closes. Reverted, with the measurements recorded inline so the next reader (human or agent) does not re-add it on the same false premise. The stub's comment re-indent from 9333997 is kept; it is cosmetic and fine. Verification - bats tests/persona_mention.bats: 42/42. - actionlint (CI's ignores): clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>



Problem
The auto-rebase review-ready gate (#465 / #468) is live nowhere — and #468 introduced a regression in
petry-projects/.githubitself.The reusable defaulted
tooling_reftov1and checked outpetry-projects/.github@v1to sourcelib/eligibility.sh. Buteligibility.shonly landed in #468, after thev1tag (d3d768d, 2026-05-13). So every run fails:Evidence:
petry-projects/.github's own auto-rebase is failing 5/5 recent runs (e.g. run 28103191068) while it has 8 open non-Dependabot PRs to act on. The 7 consumer repos still pin the pre-#468 reusable (v1/v2/@auto-rebase/stable/SHA), so they run the original unrestricted fan-out (verified:.github-privatePR#744 — no approval, noauto-rebase:readylabel — still auto-updated on the 3 most recent runs).Fix
tooling_refto empty and resolve the tooling checkout to${{ inputs.tooling_ref || github.job_workflow_sha }}— the commit SHA of this reusable file for the run.lib/eligibility.sh) is now always sourced from the exact same commit as the workflow version the caller pinned → no skew, ever.tooling_refstays as an explicit override for end-to-end branch testing.Why
job_workflow_shaA separately-pinned
tooling_refcan drift from the reusable version (too old → missing file; too new → predicate/workflow mismatch).github.job_workflow_shabinds tooling to the reusable's own commit, so a consumer pinning@auto-rebase/stablegets the predicate from whatever commitstablepoints at — automatically consistent.Rollout (after this merges)
petry-projects/.github's own auto-rebase immediately on merge.auto-rebase/stablechannel tag to this commit → deploys the review-ready gate to the 5 on-channel repos (TalkTerm, broodly, google-app-scripts, bmad-bgreat-suite, markets).ContentTwin@sha,.github-private@v2) to@auto-rebase/stable.Note (out of scope)
feature-ideation-reusable.ymlcarries the sametooling_ref: v1default pattern — likely the same latent bug. Flagged for a separate fix.🤖 Generated with Claude Code
Summary by CodeRabbit