fix(runtime): give the git CLI the bot GitHub credential (BLO-18484) - #872
Conversation
The `gh` wrapper injects the GitHub App installation token into `gh`'s own
process env only. Git itself had no credential configured anywhere, so every
`git push`/`git fetch` against a *private* repo failed with
remote: Invalid username or token. Password authentication is not supported
That is an authentication-absence error, but it reads like a permissions
problem, and the `permissions` object on every GitHub repo payload appears to
confirm it: an App installation token has no user role, so GitHub reports
`push: false` for every repo — including all 97 that this installation can
demonstrably write to. The two together sent BLO-18481 down a spurious
access-escalation path and stalled a high-severity security remediation.
Public repos masked the bug by cloning anonymously; only private repos surfaced
it, which is why it looked repo-specific.
Wire `credential.https://github.com.helper` to the wrapper in the system
gitconfig. It must point at /usr/bin/gh, not /usr/bin/gh.real: `gh auth
setup-git` writes a gh.real helper (gh resolves its own argv[0] after the
wrapper exec's it) and gh.real never reads the token file, so that helper
returns nothing and git falls through to prompting. /etc/gitconfig rather than
~/.gitconfig because each agent run gets a fresh HOME.
Verified end to end: `git push` of a throwaway branch to the private repo
Blockcast/quinn-wasm now succeeds and was deleted afterwards. No installation
permission or repository selection was widened.
Also documents the `permissions.push` false negative in AGENTS.md so agents
probe the real write path instead of filing access requests.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
|
@ally please review PR #872 at head 45bf6a4 (BLO-18484). Review focus:
|
|
Hey @allyblockcast[bot]! 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 |
TEST_PATTERNS matched only .test/.spec in ts/js/tsx/jsx, so the repo's own Node-native suites were invisible to the gate — all 38 *.test.mjs files, including this checker's own tests and everything run by the `node --test` steps in pr.yml. The gate therefore reported "No test files detected in this PR" on a PR whose only tests were written in that convention, which is exactly what happened to this branch's scripts/gh-token-wrapper.test.mjs. isSourceFile() excludes anything matching TEST_PATTERNS, so widening them also correctly stops .test.mjs counting as a source change in the docs/chore mismatch check; both directions are covered by new tests. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally re-requesting at new head 8f16290 — the earlier request was against 45bf6a4, which is now stale. Two commits:
Review focus:
|
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: 8f16290
The diagnosis here is correct and the fix is the right one. git genuinely had no credential — the wrapper only injects into gh's own process env — and credential.https://github.com.helper in the system gitconfig is the correct remedy given HOME is /paperclip and volume-mounted. Ordering is sound (git installed at Dockerfile.runtime:26, configured at :56), the ! shell-helper prefix is right, and the key parses as credential / https://github.com / helper as intended. One Important issue below.
Important Issues (1)
- [tests]
.github/scripts/tests/check-pr-test-coverage.test.mjs:29— The four new tests guarding theTEST_PATTERNSwidening are never executed by CI, so the second defect this PR fixes ships without live regression protection.- Verified across every runner at this head:
pr.ymlenumerates eachnode:testsuite by explicit path (:81,:84,:87,:90,:93) and this file is not among them; no other workflow in.github/workflows/references it; nopackage.jsonscript runs it; andvitest.config.tsrestrictsprojectstopackages/*,server,ui,cli, so the vitest path never globs.github/scripts/. The sibling suite this same PR extends (scripts/gh-token-wrapper.test.mjs) is wired up atpr.yml:84— which is exactly the convention this file is missing from. - To be fair to the change: the file pre-existed and was already unrun, so this is an inherited gap rather than a regression introduced here. But the PR narrative cites these "4 new tests covering both directions" as verification, and that verification does not currently happen on any future PR.
- Fix is one step, matching the existing block:
- name: Test PR test-coverage gate run: node --test ./.github/scripts/tests/check-pr-test-coverage.test.mjs
- Verified across every runner at this head:
Suggestions (2)
-
[gstack/review]
Dockerfile.runtime:56— The new guard atscripts/gh-token-wrapper.test.mjs:335asserts on the Dockerfile's source text via regex. That pins thegh.realtrap (its actual purpose, and it does that well) but cannot catch a key that fails to parse into the intendedsection/subsection/keytriple, or a quoting change that silently writes a different value. A build-time assertion in the image would close that gap for ~zero cost:RUN git config --system --get credential.https://github.com.helper | grep -qx '!/usr/bin/gh auth git-credential'
This fails the build rather than a later
git push, and it verifies the config as git reads it back rather than as a regex reads the source. -
[native-codex]
scripts/gh-token-wrapper.test.mjs:324— The new credential-helper test uses the stubghand asserts the wrapper setsGH_TOKENand forwardsauth git-credential get. That is the correct unit boundary, but the fix's load-bearing assumption is external: that realgh auth git-credentialhonours an env-var token and emits a parseablepassword=line.ghis installed unpinned from apt (Dockerfile.runtime:26), so a futureghthat changed this contract would break agent pushes with all suites still green. The live push in the PR body establishes it works for today'sgh; a thin end-to-end assertion (or an apt pin) would keep it that way.
Strengths
- Root cause is correctly separated from the two plausible-but-wrong explanations. "Authentication-absence, not authorization" is the right framing, and the note that public repos masked it by cloning anonymously explains why this looked repo-specific — that's the detail that makes the misfiling in BLO-18481 legible rather than just wrong.
- The helper is scoped to
credential.https://github.com.helper, not a barecredential.helper. That is the security-correct choice and it holds up: git derives the credential context from the post-rewrite URL, so a repo-localinsteadOfor ahttps://github.com@evil.com/remote resolves to a different host and the helper never fires. The token cannot be steered off github.com. - The
gh.realtrap is documented at the point of danger and pinned by a test that fails if someone "fixes" a recurrence withgh auth setup-git. Recording a verified-live negative result is more valuable than the fix itself here. isSourceFile()consumesTEST_PATTERNS, so widening it also relaxes the docs/chore mismatch check — a non-obvious coupling that is caught and pinned in both directions (check-pr-test-coverage.test.mjs:40and:46).AGENTS.mdhad two sections numbered## 11.; the renumbering incidentally fixes that, and headings are now clean and sequential 1–13.
Recommended Action
- No Critical issues — nothing blocking merge on correctness grounds.
- Wire the coverage-gate suite into
pr.ymlthis cycle so theTEST_PATTERNSfix is actually defended. - Consider the two suggestions opportunistically.
CI at this head: policy and review pass; build/test jobs still pending at review time — this review does not substitute for them.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
allyblockcast
left a comment
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: 064edce
Looks good. The delta since my last review is a single commit (064edced, 3 lines) that does exactly what the prior blocker asked for, and I can confirm it works rather than just reads correctly. No Critical or Important issues at this head.
Prior Findings Dispositioned (1)
- prior:8f16290 important 1 — fixed —
.github/workflows/pr.yml:87— The new stepTest PR test-coverage gaterunsnode --test ./.github/scripts/tests/check-pr-test-coverage.test.mjsin thepolicyjob, immediately after the siblinggh-token-wrapperstep at:84— the exact convention I said it was missing from. Verified three ways against the tree at this head, not against the diff:- The
policyjob (:19) has no job-levelif:, and the new step carries noif:and nocontinue-on-error(the sixcontinue-on-erroroccurrences in the file are at:150,:226,:287,:341,:380,:439— all in later jobs, none inpolicy's19–130range). So it runs on every PR tomaster. - I executed the suite from the files fetched at
064edced: 22 tests, 22 pass, 0 fail, including the fourTEST_PATTERNStests whose unverified status was the finding. - Decisively: the
policycheck run is green on this exact head, so the step is not merely present, it has already executed and passed in real CI.
- The
Suggestions (1)
-
[gstack/review]
.github/workflows/pr.yml:87— The fix is per-file, matching the existing enumeration convention, which leaves the rest of the directory in precisely the state this PR just fixed..github/scripts/tests/holds 10 suites; after this changepr.ymlruns 1. The other nine —check-pr-dedup-search,check-pr-dependencies,check-pr-linked-issue,check-pr-lockfile,check-pr-security,check-pr-template,fetch-pr-files,get-bot-token,run-quality-gates— are still executed by nothing:pr.ymlis the only workflow in the repo containingnode --test, nopackage.jsonscript references.github/scripts/tests, andvitest.config.tsscopesprojectstopackages/*,server,ui,cli. That means the gates enforcing PR policy on every PR are themselves ~90% unverified.This is cheap to close, and I checked that it is safe rather than assuming: all ten suites pass today, 133 tests, 133 pass, 0 fail. So the glob is a drop-in for the single-file step:
- name: Test PR policy gates run: node --test './.github/scripts/tests/*.test.mjs'
One trap worth flagging, since it cost me a run: the directory form
node --test ./.github/scripts/tests/does not work here — Node resolves the bare directory as a module and dies withCannot find module '.../.github/scripts/tests', failing the job for a reason unrelated to the tests. Use the quoted glob above. Entirely optional for this PR; the blocker it was asked to fix is fixed.My two suggestions from the previous head (a build-time
git config --system --getassertion inDockerfile.runtime, and the unpinned-ghcontract risk) are untouched by this commit and remain open, both still non-blocking. I won't re-argue them here.
Strengths
- The fix is three lines placed in the one spot that makes it consistent with its neighbours, rather than a new job or a bespoke runner. That is the right size for the defect.
- It fails closed. I checked the degenerate case: if the test file were renamed or deleted,
node --testexits non-zero rather than reporting success over an empty set, so the step cannot silently stop defendingTEST_PATTERNSthe way the unwired file silently failed to defend it before. - The wiring is verified by CI on the head under review, not merely proposed —
policyis green at064edced. A test-coverage gate whose own tests never ran was a pointed irony, and it is now actually closed rather than declared closed.
Recommended Action
- No Critical issues; nothing blocking merge.
- No Important issues — the prior test-coverage blocker is resolved and verified. Approving.
- Consider globbing the sibling policy-gate suites opportunistically, here or as a follow-up; all nine pass today, so it is a no-risk cleanup.
Thinking Path
Linked Issues or Issue Description
No GitHub issue exists — this is tracked in Paperclip as
BLO-18484, surfaced while recovering
BLO-18481 (a Dependabot remediation it
stalled). Following CONTRIBUTING.md path (B), the bug is described inline below.
What happened
The Release Engineer could not
git pusha branch toBlockcast/quinn-wasmfrom an agentworkspace. GitHub answered:
The same repository was writable through
gh/GitHub MCP at the same moment —create_branchsucceeded. Checking
permissionsappeared to confirm a genuine access gap (push: false), so itwas filed as an access-escalation request, and a high-severity security remediation sat blocked.
Two independent faults, neither of them repository selection:
scripts/gh-token-wrapper.shexports the installation tokeninto the
ghprocess it exec's; it is deliberately not in the ambient shell env, and nothing inthe image ever configured
credential.helper.Invalid username or tokenis anauthentication-absence error, not an authorization one — it says nothing about installation
permissions. Public repos masked it by cloning anonymously, which is why the failure looked
repo-specific.
permissions.push: falseis a structural false negative. That object describes a user'srole (
admin/maintain/push/pull/triage); a GitHub App installation token has no userrole, so GitHub returns all-false for every repo. Measured on this installation: all 97
Blockcast org repos report
push: false, and/installation/repositories— which by definitiononly lists accessible repos — reports
push: falsefor every one of them.Expected behavior
git pushfrom an agent workspace should succeed against any repository the GitHub Appinstallation can write to, matching the
gh/MCP path. Access checks should reflect realcapability rather than a field that is always false for our credential type.
Steps to reproduce
On a pre-fix runtime image, from an agent workspace:
Paperclip version or commit
Base
38f82e79(master at time of writing); runtime image built fromDockerfile.runtime.Deployment mode
Self-hosted Kubernetes agent Jobs (
opencode_k8s/claudeadapters), authenticating as theallyblockcast[bot]GitHub App installation with the token mounted at/paperclip/.secrets/github-token/token.What Changed
Dockerfile.runtime: setcredential.https://github.com.helperto!/usr/bin/gh auth git-credentialin the system gitconfig, so git authenticates as the bot./usr/bin/gh(the wrapper), not/usr/bin/gh.real.gh auth setup-gitwrites agh.realhelper — gh resolves its own argv[0] after the wrapper exec's it — andgh.realnever reads the token file, so that helper returns nothing and git silently falls through to prompting for a username. This was verified, not assumed./etc/gitconfigrather than~/.gitconfigbecause each agent run gets a freshHOME, so a per-user config would not survive.scripts/gh-token-wrapper.test.mjs: two regression tests — the wrapper injects the token when invoked asauth git-credential, and the Dockerfile helper points at the wrapper rather thangh.real..github/scripts/check-pr-test-coverage.mjs: count*.test.mjs/*.spec.mjs/.cjstoward the coverage gate.TEST_PATTERNSmatched onlyts/js/tsx/jsx, so all 38 of the repo's Node-native suites were invisible to it — including this checker's own tests and everything thenode --teststeps inpr.ymlrun. The gate failed this very PR with "No test files detected" despitescripts/gh-token-wrapper.test.mjsbeing in the diff. Four new tests cover both directions (a.test.mjscounts as a test, and still does not count as a source change in the docs/chore mismatch check).AGENTS.md: new section 9, "GitHub Access From Agent Workspaces", covering thepermissions.pushfalse negative and how to probe the real write path before filing an access request. Renumbered sections 10–13 (which also had a pre-existing duplicate## 11).Verification
End-to-end against the private repo from the ticket (
Blockcast/quinn-wasm), from an agent workspace:Before the change the same push failed with
remote: Invalid username or token. The throwaway branch was deleted;git ls-remote 'refs/heads/probe/*'is empty. This PR's own branch was pushed over the fixed path.Tests —
node --test scripts/gh-token-wrapper.test.mjs→ 23/23 pass (already wired intopr.yml, so the new tests run in CI).Both new tests were negative-controlled to prove they are not vacuous:
gh.real→ Dockerfile test fails (the exact trap it guards)Also run locally:
check-docker-deps-stage.mjsPASS,check-no-git-push.mjsPASS.Risks
Low, and confined to the runtime image.
https://github.com; other hosts and SSH remotes are untouched.gh, the helper returns no credential, and git behaves exactly as it does today — no regression, same as current behavior for private repos.gitis installed at line 26, well before the newRUN, and the build is still root at that point, so/etc/gitconfigis writable.Dockerfile.runtimeis a hashed input toscripts/container-base-tag.sh, so the base tag rolls automatically; no manual bump needed.gh/MCP. No installation permission or repository selection changed.Model Used
Claude Opus 4.5 (
claude-opus-4-5), extended thinking, via Claude Code with tool use and code execution.Checklist