Repository navigation
fix(auto-rebase): source predicate from reusable's own commit, not @v1 (restores review-ready gate) - #528
Merged
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Author
Dev-Lead — review-changes (applied)Changes committed and pushed. |
don-petry
enabled auto-merge (squash)
June 24, 2026 22:40
don-petry
disabled auto-merge
June 24, 2026 22:40
Contributor
Author
Dev-Lead — review-changes (applied)Changes committed and pushed. |
don-petry
enabled auto-merge (squash)
June 24, 2026 22:41
don-petry
disabled auto-merge
June 24, 2026 22:42
don-petry
enabled auto-merge (squash)
June 24, 2026 22:44
don-petry
disabled auto-merge
June 24, 2026 22:44
Contributor
Author
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
don-petry
enabled auto-merge (squash)
June 24, 2026 22:45
donpetry-bot
approved these changes
Jun 24, 2026
donpetry-bot
left a comment
donpetry-bot
left a comment
Contributor
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.
don-petry
disabled auto-merge
June 24, 2026 23:20
|
Contributor
Author
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #528 |
Contributor
Author
|
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. |
don-petry
added a commit
to combsmith/.github-private
that referenced
this pull request
Jun 27, 2026
…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 combsmith/.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>
don-petry
added a commit
that referenced
this pull request
Jul 17, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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