feat: route pipeline PR creation through configured credentials - #1858
Open
quinnbot-ai wants to merge 3 commits into
Open
feat: route pipeline PR creation through configured credentials#1858quinnbot-ai wants to merge 3 commits into
quinnbot-ai wants to merge 3 commits into
Conversation
added 3 commits
August 6, 2026 19:10
The no-mistakes pipeline opens pull requests by shelling out to `gh`, and a
GitHub fine-grained personal access token is forbidden from the
createPullRequest mutation, so an ambient token of that class breaks the PR
step at the end of an otherwise green run while every earlier step succeeds.
Investigation of the installed v1.41.2 binary and upstream source at release
1.46.0 found no supported configuration seam: the GitHub provider executes
`gh` by bare name with no binary-path or credential knob, while Bitbucket
Cloud reads its credentials from named environment variables. The step
execution layer already honors a step-scoped PATH and credential environment
through StepContext.Env, but that field is declared test-only and the
production executor never populates it.
Add the firstmate-side mitigation instead of patching the tool:
- fm-gh.sh runs one command with the credential prefix from
config/gh-credential, and execs unchanged when unconfigured.
- fm-gh-shim.sh routes only `pr create` and `pr edit` through that wrapper
and delegates everything else to the real gh.
- fm-gh-shim-install.sh installs, removes, and verifies PATH precedence, and
is never run automatically.
Interception must sit on the daemon's PATH because the PR step runs in the
daemon, so it cannot be scoped to a task worktree; that limit is documented
rather than papered over. docs/proposals/nm-pr-step-interception.md specifies
the upstream config seam that would retire the shim.
The wrapper expands its prefix as ${PREFIX[@]+"${PREFIX[@]}"} because bash
3.2, the system bash macOS ships, rejects an empty array expansion under
`set -u` and would otherwise break every unconfigured home.
4 tasks
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.
Intent
Give firstmate a supported way to route the no-mistakes pipeline's PR-creation step through bin/fm-gh.sh, which holds the vault credential authorized for createPullRequest, instead of no-mistakes using its own gh path. Background: on 2026-08-06 no-mistakes autonomously created a clip-engine PR via its own gh invocation; repo, identity, and head were all correct so nothing needed undoing, but the pipeline's PR step cannot currently be intercepted once review has responded, and the configured default GitHub token lacks createPullRequest, so pipeline PR creation fails on most repos while bin/fm-gh.sh succeeds.
The core constraint is PURE-KUN POSTURE: do NOT locally patch the installed no-mistakes tool, because we track Kun Chen's tools clean. First investigate what no-mistakes ALREADY supports by reading its installed docs, help, and config surface for any supported hook, wrapper, gh-binary override, PATH-based interception, or config knob that lets the PR step run through a caller-supplied command, and prefer any supported configuration over new code. If a supported seam exists, implement the firstmate-side wiring in this repo with tests. If NO supported seam exists, implement the best non-invasive firstmate-side mitigation available without forking no-mistakes (for example a PATH shim exposed only inside task worktrees IF that is robust and honest), AND write a concrete upstream proposal document at docs/proposals/nm-pr-step-interception.md describing the config seam we want upstream in kunchenguid's no-mistakes, precise enough to turn into an upstream PR.
Investigation outcome that shaped this diff: no supported seam exists. Confirmed against the installed v1.41.2 binary and the upstream source at release 1.46.0, commit 20892e6. internal/scm/github executes gh by the bare name with no binary-path or credential parameter; neither the global config.yaml nor the per-repo .no-mistakes.yaml exposes a GitHub credential, a gh path, or a PR-step command override; Bitbucket Cloud by contrast reads NO_MISTAKES_BITBUCKET_EMAIL, NO_MISTAKES_BITBUCKET_API_TOKEN, and NO_MISTAKES_BITBUCKET_API_BASE_URL, an asymmetry the upstream proposal targets; and stepCmd in internal/pipeline/steps/common_exec.go already honors a step-scoped PATH and credential environment whenever StepContext.Env is populated, but internal/pipeline/pipeline.go declares that field as test-only and the production executor never sets it, so the mechanism exists while a supported way to configure it does not.
A deliberate correction to the original task framing, made after reading the source: a worktree-scoped PATH shim is NOT possible. The PR step runs inside the no-mistakes daemon, whose environment is resolved once from a login-shell probe at startup, so nothing placed inside a task worktree is on the PATH that resolves gh. Any shim is therefore PATH-wide by construction. That limit is documented plainly rather than papered over, and the shim is consequently explicitly opt-in and never installed automatically.
Deliberate decisions a reviewer reading only the diff would not know. The credential itself is kept out of tracked material and read from a gitignored config/gh-credential command prefix, so this shared template repo carries no machine-specific vault path and a home with no such file is a transparent pass-through that behaves exactly as before. The shim routes only
pr createandpr edit, the two mutations a fine-grained token is forbidden from, and delegates every other invocation, including the pipeline's ownpr listandpr viewreads, straight to the real gh so the privileged credential is never spent on reads. Installation is a symlink rather than a copy because the shim locates fm-gh.sh relative to its own real path. No spawn or brief plumbing was added, and none is needed, because interception happens where the PR step actually runs, in the daemon, so one installation covers every pipeline run. The installer refuses to overwrite a non-symlink gh or remove a symlink it does not own, and reports PATH precedence because a shim the daemon never resolves is indistinguishable from no shim at all.The wrapper expands its credential prefix as ${PREFIX[@]+"${PREFIX[@]}"} rather than "${PREFIX[@]}" because under set -u, bash 3.2, the system bash macOS still ships, rejects an empty array's expansion as an unbound variable. That was a real bug found and fixed during this work: it broke the unconfigured pass-through path that every home without config/gh-credential takes, and it is pinned by a self-skipping legacy-shell regression.
This task touches firstmate's shared tracked material, so the firstmate-coding-guidelines contract applies: one full sentence per line in tracked Markdown, plain dash and never an em dash, shellcheck-clean bin scripts through bin/fm-lint.sh, colocated tests named .test.sh under tests/, every new prose surface classified in docs/documentation-audiences.json, and active empirical evidence recorded as a maintainer-verification record under docs/verification/.
Acceptance criteria: the chosen mechanism is demonstrated working, by test or recorded evidence, for at least one simulated PR-step invocation routing through bin/fm-gh.sh; no local modification of the installed no-mistakes tool; and documentation states exactly which mechanism was chosen and why, and what remains for upstream.
What Changed
fm-gh.shto inject a locally configured PR-capable credential while preserving transparent pass-through behavior when unconfigured.ghshim and safe installer that route onlypr createandpr edit, delegate other commands, verify precedence, and protect foreign symlinks.Risk Assessment
✅ Low: Captain, the three prior defects are closed without introducing a new reachable failure path, and the change remains an explicit, well-documented containment with the durable fix deferred upstream.
Testing
Reviewed the target implementation and documentation, ran the focused behavioral suite, captured an end-to-end simulated PR-creation transcript, and confirmed testing left the target commit's worktree clean; all exercised behavior satisfied the intent.
Evidence: Simulated PR-step routing transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
bin/fm-gh-shim-install.sh:108- Refuse foreign symlinks beforeln -sf. The current guard permits any existing symlink, so installation can silently replace a package-manager or user-ownedghlink and cannot restore it on uninstall. Allow only an absent target or a link already owned by this installer.bin/fm-gh.sh:18- UseGH_TOKENin the supported credential prefix or explicitly clear it.ghprioritizesGH_TOKENoverGITHUB_TOKEN, so a daemon carrying a forbiddenGH_TOKENwill ignore the documented injectedGITHUB_TOKENand still fail atpr create. Add a regression with a bad ambientGH_TOKEN.bin/fm-gh.sh:41- Trim whitespace before classifying comments. A line such as# explanationis currently returned as the prefix and makes the wrapper attempt to execute#, despite the documented first-non-comment-line contract.🔧 Fix: Harden PR credential routing and shim ownership
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-gh-shim.test.shbash /var/folders/pj/3vh4dgp11qs_j186lws96lmw0000gn/T/no-mistakes-evidence/01KZCZSKGXZP5CNWSWX2CBZGVN/pr-routing-evidence.shgit status --shortconfirmed the worktree remained clean after testing✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.