Skip to content

feat: route pipeline PR creation through configured credentials - #1858

Open
quinnbot-ai wants to merge 3 commits into
kunchenguid:mainfrom
quinnbot-ai:fm/nm-pr-wrapper-interception
Open

feat: route pipeline PR creation through configured credentials#1858
quinnbot-ai wants to merge 3 commits into
kunchenguid:mainfrom
quinnbot-ai:fm/nm-pr-wrapper-interception

Conversation

@quinnbot-ai

Copy link
Copy Markdown

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 create and pr edit, the two mutations a fine-grained token is forbidden from, and delegates every other invocation, including the pipeline's own pr list and pr view reads, 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

  • Add fm-gh.sh to inject a locally configured PR-capable credential while preserving transparent pass-through behavior when unconfigured.
  • Add an opt-in, PATH-wide gh shim and safe installer that route only pr create and pr edit, delegate other commands, verify precedence, and protect foreign symlinks.
  • Document the no-mistakes limitation and proposed upstream credential seam, with regression and maintainer-verification coverage for the chosen routing mechanism.

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
$ bin/fm-gh-shim-install.sh --install --dir <fixture-shim>
installed: <fixture-shim>/gh -> <worktree>/bin/fm-gh-shim.sh
delegates to: <fixture-real-gh>

$ bin/fm-gh-shim-install.sh --check --dir <fixture-shim>
shim: installed at <fixture-shim>/gh
real gh: <fixture-real-gh>
first gh on evaluated PATH: <fixture-shim>/gh
precedence: <fixture-shim> wins

$ gh pr create --head feature --base main --repo example/project --title Routed --body-file -
https://github.com/example/project/pull/42

Delegation record from the real gh boundary:
delegated argv: gh pr create --head feature --base main --repo example/project --title Routed --body-file -
delegated GH_TOKEN: <none>
delegated GITHUB_TOKEN: <configured-pr-capable-token>

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 before ln -sf. The current guard permits any existing symlink, so installation can silently replace a package-manager or user-owned gh link and cannot restore it on uninstall. Allow only an absent target or a link already owned by this installer.
  • ⚠️ bin/fm-gh.sh:18 - Use GH_TOKEN in the supported credential prefix or explicitly clear it. gh prioritizes GH_TOKEN over GITHUB_TOKEN, so a daemon carrying a forbidden GH_TOKEN will ignore the documented injected GITHUB_TOKEN and still fail at pr create. Add a regression with a bad ambient GH_TOKEN.
  • ⚠️ bin/fm-gh.sh:41 - Trim whitespace before classifying comments. A line such as # explanation is 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.sh
  • bash /var/folders/pj/3vh4dgp11qs_j186lws96lmw0000gn/T/no-mistakes-evidence/01KZCZSKGXZP5CNWSWX2CBZGVN/pr-routing-evidence.sh
  • git status --short confirmed the worktree remained clean after testing
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

QuinnBot 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant