[codex] Fix Ally user-token selection through gh wrapper - #718
Merged
Conversation
kkroo
marked this pull request as ready for review
July 18, 2026 07:48
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
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 Jul 30, 2026
13 tasks
kkroo
pushed a commit
that referenced
this pull request
Jul 30, 2026
… base (BLO-18997) Addresses both Important findings from Ally's review of c69bbfe. 1. `shipped-catalog.test.ts` still pinned the contract this PR inverts. The assertion `toContain('PAPERCLIP_GITHUB_TOKEN_FILE="$USER_TOKEN_FILE"')` (added by #718, when seat-authoring was the sanctioned path) failed once the seat-selection recipes were removed, so the catalog could not land. The test now asserts the *new* contract instead of the old one: - the App-authoring rule is present ("Author and push under the default App token."), - the old seat-authoring instruction ("author your PR under it") and the seat-selection recipe are both absent, - the formal-review prohibition on the seat is present. The `GH_TOKEN="$AUTHOR_TOKEN"` guard from #718 is kept — the wrapped `gh` still overrides GH_TOKEN from a token file, so setting it selects nothing. 2. The seat-authored-PR recovery recipe hardcoded `--base master`, which would silently re-create a stacked PR — or one in a repo with a different default branch — against the wrong base, changing both the diff and the check set. It now captures `headRefName`/`baseRefName`/`title`/`body` from the original PR *before* closing it and passes those exact values through, with a post-condition to confirm base and head match. Manifest regenerated for the new SKILL.md: 10643 bytes, sha256 477cf058…, contentHash sha256:6cbe8cec…. Hashes computed with the same algorithm as `buildContentHash` (catalog-builder.ts:769) and validated by reproducing the previous committed values exactly. `npx vitest run src/shipped-catalog.test.ts` → 11/11 pass. Co-Authored-By: Claude <noreply@anthropic.com>
13 tasks
kkroo
added a commit
that referenced
this pull request
Jul 31, 2026
… (BLO-18997) (#832) * docs(skills): forbid formal PR reviews under the user-seat merge token (BLO-18925) The github-pr-workflow bundled skill reaches every engineering agent (recommendedForRoles: [engineer]). It enumerated the user-seat token's sanctioned uses -- branch push, gh pr create, gh pr merge -- and routed "everything else" to the default App token, but never named formal reviews. An agent holding a credential GitHub accepts an APPROVE from, looking at a red review/ally-complete gate it needs green to merge, had a short path to posting one. Prohibition by omission is not a control. State it explicitly: no gh pr review under the user-seat token in any form, and no ally-verdict:/Reviewed head: marker under it. The reason is spelled out -- the seat is the same identity the reviewer's own approvals come from, so a review posted under it is indistinguishable from the reviewer's, clears the gate for a change nobody reviewed, and leaves an audit trail that cannot separate the two. The sanctioned move on a red gate is to get a review, not to post one. Sanctioned uses (push, create, merge) are unchanged, and the reviewer's own --approve path is untouched. Also regenerates generated/catalog.json, which pins per-file sha256 and sizeBytes. Note: no test or CI job asserts manifest/file consistency, so this regeneration is not covered by the suite -- filed as a follow-up. * docs(skills): state the reviewer identity model once (BLO-18925) Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955). * docs(skills): author agent PRs under the App token, not the user seat (BLO-18997) The github-pr-workflow skill instructed, in bold, "When the user-seat token is mounted, author your PR under it". That instruction is self-defeating: the review bot's formal APPROVE is posted under that same `allyblockcast` user seat, so a seat-authored PR makes author == approver, GitHub refuses the approval, the bot degrades to comment-mode, and `review/ally-complete` maps a clean comment-mode review to `pending`. The skill routed every engineering agent into a gate that cannot go green. Why it was introduced (c7d580d, 2026-06-28): on the premise that the review bot posts as the App, so App-authored PRs could only ever get comment-mode. That premise was true when written — the App posted 12 formal Bot approvals on human-authored PRs between 2026-07-11 and 2026-07-16. Ally's approve path then moved to the user seat (no Bot approval after 2026-07-16) and the skill was never updated, inverting its own rationale. Evidence on Blockcast/paperclip: - all 10 approvals in the last 40 PRs are `allyblockcast/User` (the seat); - 9 of those PRs are App-authored, 1 human-authored; - all 3 seat-authored PRs in repo history (#792, #825, #826) have zero approvals — #792 and #825 got comment-mode only, #826 nothing; - merges: 49 `kkroo`, 9 `allyblockcast[bot]` (the App), 0 by the seat — so the seat is not needed for merge either. Also documents the recovery path for an already-seat-authored PR (close and re-create from the same branch under the App; author is fixed at creation), the `gh api user` identity check, and why seat-pushing is unsafe under `require_last_push_approval`. Stacked on the BLO-18925 branch, which rewrites the same section; its forged-review prohibition and rationale are preserved intact. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): repin the catalog test to App-authoring; don't hardcode base (BLO-18997) Addresses both Important findings from Ally's review of c69bbfe. 1. `shipped-catalog.test.ts` still pinned the contract this PR inverts. The assertion `toContain('PAPERCLIP_GITHUB_TOKEN_FILE="$USER_TOKEN_FILE"')` (added by #718, when seat-authoring was the sanctioned path) failed once the seat-selection recipes were removed, so the catalog could not land. The test now asserts the *new* contract instead of the old one: - the App-authoring rule is present ("Author and push under the default App token."), - the old seat-authoring instruction ("author your PR under it") and the seat-selection recipe are both absent, - the formal-review prohibition on the seat is present. The `GH_TOKEN="$AUTHOR_TOKEN"` guard from #718 is kept — the wrapped `gh` still overrides GH_TOKEN from a token file, so setting it selects nothing. 2. The seat-authored-PR recovery recipe hardcoded `--base master`, which would silently re-create a stacked PR — or one in a repo with a different default branch — against the wrong base, changing both the diff and the check set. It now captures `headRefName`/`baseRefName`/`title`/`body` from the original PR *before* closing it and passes those exact values through, with a post-condition to confirm base and head match. Manifest regenerated for the new SKILL.md: 10643 bytes, sha256 477cf058…, contentHash sha256:6cbe8cec…. Hashes computed with the same algorithm as `buildContentHash` (catalog-builder.ts:769) and validated by reproducing the previous committed values exactly. `npx vitest run src/shipped-catalog.test.ts` → 11/11 pass. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): make seat-PR recovery SHA-safe; guard credentials structurally (BLO-18997) Addresses both important findings from Ally's review of 3c16d95. Recovery recipe (SKILL.md): the sequence promised "same branch and SHA" but never captured headRefOid, and closed the original before the replacement existed. Two failure modes: a branch that moved between capture and re-create silently reopened on an unreviewed head, and a failed `gh pr create` left the review artifact closed with no replacement. Now captures headRefOid up front, re-validates it against the remote ref immediately before the close (aborting instead of closing on mismatch), and reopens the original on any post-close failure, including a replacement that lands on the wrong head or base. Regression test: the credential check rejected three exact string spellings, so seat authoring could return under a renamed variable or a literal token path. Replaced with a structural scan — extract executable shell fences, and for any fence running `git push` / `gh pr create|merge|review`, reject any credential selection (token-file assignment, literal seat-token path, GH_TOKEN / GITHUB_TOKEN assignment, `--with-token`, `gh auth login|switch`). Includes an anti-vacuity assertion so a drifting extractor or retagged fence fails loudly rather than passing on an empty scan. A third test pins the recovery invariants. Verified: 7 mutations each turn the suite red (token-file next to `gh pr merge`, renamed GH_TOKEN next to `gh pr create`, dropped headRefOid, dropped reopen path, validation moved after the close, and fences retagged non-executable). Recovery fence parses under `bash -n`. Manifest regenerated (SKILL.md now 12510 bytes, sha256 160083f0…). * docs(skills): make the seat-PR recovery fail closed and prove App identity (BLO-18997) Ally's review at ee9f9e3 found the recovery recipe could still destroy the review artifact on a failure it did not anticipate, and that the regression test could not have caught the recipe it exists to keep out. - Fail closed before the destructive close. Every captured field is validated — full 40-hex SHA, non-empty refs — so a failed `gh pr view` or a null field no longer leaves an empty ORIG_SHA that compares equal to an empty REMOTE_SHA and "passes" the guard on two blanks. `set -euo pipefail` makes an unhandled failure abort rather than fall through to the next destructive line. - Roll back on every unsuccessful exit, not two anticipated ones: an EXIT/INT/TERM trap armed before the close reopens the original and takes any replacement down with it, and says so loudly when the rollback itself fails. - Prove the actor is the App installation before starting, and verify the replacement's author after creating it. Previously the recipe would happily recreate under the seat — the exact defect being recovered from — and report success. The preflight asserts the App's 403 signature rather than the seat's absence, so a seat login, a network failure, or a broken `gh` all abort. (`PAPERCLIP_GITHUB_TOKEN_FILE` is exported by default pointing at the App token, so its mere presence cannot be the signal.) - Validate the created PR number, so a blank `gh pr create` output cannot send the verification step to `gh pr view ""`. - Disclose what the replacement does not carry: labels, assignees, reviewers, milestone, and the original's draft state. Tests: the recovery recipe is now extracted from the shipped skill and executed against a stub `gh` across 12 scenarios, asserting the actual `gh` argv sequence — that no pre-close failure ever reaches `gh pr close`, and that every post-close failure reopens the original. That replaces presence-only regex assertions. Each guard was mutation-tested: removing the trap fails 6 tests, removing the App preflight 2, hardcoding `--base master` 1, and dropping both SHA-format checks fails the blank-both case by reaching the destructive close. The `git push` detector now matches git's global-option forms, so the historical `git -c http.https://github.com/.extraheader= push` seat-authoring recipe is rejected; a fixture pins that it would be. Catalog regenerated; SKILL.md sha256 d79f6270…5311 / 16133 bytes verified against the file directly. * fix(skills): make seat PR recovery signal safe Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: CTO <cto@blockcast.network> Co-authored-by: Blockcast CTO <cto@blockcast.net> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Omar Ramadan <omar@blockcast.net> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: allyblockcast[bot] <allyblockcast[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
1 participant
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.
Summary
allyblockcastuser-seat credential throughPAPERCLIP_GITHUB_TOKEN_FILEfor push, PR creation, and merge commandsGH_TOKENcannot switch identities inside the agent imageGH_TOKEN="$AUTHOR_TOKEN"guidanceRoot cause
The agent image's
/usr/bin/gh-token-wrapper.shdeliberately reloadsGH_TOKENfromPAPERCLIP_GITHUB_TOKEN_FILEon every invocation. The workflow skill attempted to overrideGH_TOKENdirectly, so the wrapper silently replaced the user PAT with the default App installation token. Self-authored PR approvals therefore still ran asallyblockcast[bot]and GitHub rejected them.The managed Ally instructions were corrected separately at runtime so future review jobs use the mounted user PAT immediately.
Validation
pnpm --filter @paperclipai/skills-catalog test(15 passed)pnpm --filter @paperclipai/skills-catalog validatepnpm --filter @paperclipai/skills-catalog typecheckgit diff --checkPAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token gh api userresolves toallyblockcastuser ID296676656