docs: clarify Ally review gate routing - #1325
Merged
Merged
Conversation
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
This was referenced Aug 20, 2026
13 tasks
kkroo
force-pushed
the
codex/clarify-ally-review-gate
branch
from
September 2, 2026 00:30
3195be3 to
422964a
Compare
kkroo
marked this pull request as ready for review
September 2, 2026 02:25
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 422964a
Critical Issues (0)
Important Issues (1)
- [native-codex]
docs/runbooks/onprem-k8s-agent-pr-review-gate.md:32-40— The revised runbook incorrectly saysreview/ally-completeaccepts only an exact-head App approval and that an App-authored PR must be reopened under an independent author. The current Paperclip gate implementation explicitly supports Ally's App-authoredCOMMENTEDformal reviews and comment-shaped reviews, and the App is intentionally the author-only identity in this workflow; the separate onprem ruleset is the Team approval requirement. Following this text would cause operators to replace valid App-authored PRs and seek an approval the App cannot provide.- Describe the App artifact as the gate's accepted Ally review evidence (including
COMMENTEDself-review where applicable), and describe the Team approval as the independent merge requirement. Verify the runbook against the live gate before publishing.
- Describe the App artifact as the gate's accepted Ally review evidence (including
Suggestions (0)
Strengths
- The change correctly distinguishes the App identity from the User seat and keeps the Team approval as a separate requirement.
- The exact-head language is explicit and useful for operators.
Recommended Action
- Address Important issues before merge.
- Consider Suggestions opportunistically.
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
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.
Thinking Path
The on-prem review-gate runbook told operators to collect two approvals but blurred which identity satisfies which control.
review/ally-completeand the singletonBlockcast/onprem-k8s-ally-reviewerteam rule are separate gates satisfied by separate identities, and the old wording could be read as either one standing in for the other. A reader who took the User-seat approval as satisfying the App gate would end up one independent exact-head attestation short, and discover it only at merge time.The second gap was the self-review case. The old text said a required reviewer "cannot supply its own required review" without stating plainly that a Paperclip agent normally authors as
allyblockcast[bot], so in practice most agent PRs need an independent-author replacement before the App can approve.What Changed
Wording only, in
docs/runbooks/onprem-k8s-agent-pr-review-gate.md. Names the App approval as the only evidencereview/ally-completeaccepts, states the App-authored replacement path explicitly rather than by implication, and folds the two "do not treat X as Y" cautions into one sentence covering both substitutions.Verification
Rebased onto current master and confirmed it still applies cleanly: zero commits behind,
git merge-tree --write-treereturns 0 (clean), one commit ahead.Scope-checked against the repo's own gate definitions rather than assumed:
docs:is inSKIP_PREFIXES/SKIP_ISSUE_PREFIXES/SKIP_TEST_PREFIXES, so the dedup, linked-issue and test-coverage gates do not apply here;check-pr-template.mjshas no skip list, so this description was written against itsREQUIRED_SECTIONSand validated by running that checker directly.Risks
Documentation only — no code path, no runtime behaviour, nothing to roll back beyond the text.
The real risk is the text being wrong rather than the change being unsafe. I inherited this PR after a rebase and did not re-derive its claims from the current implementation; I confirmed the diff still applies and reads coherently, not that
review/ally-completestill accepts exactly what the runbook says it accepts. Anyone relying on this operationally should confirm the gate's behaviour againstserver/src/before following it, and a reviewer who knows that gate should treat the two-identity claim as the thing to check.Model Used
claude-opus-5[1m]