Skip to content

feat(gh): route pipeline PR creation through a configurable credential - #97

Merged
quinnbot-ai merged 4 commits into
mainfrom
fm/nm-pr-wrapper-interception-fork
Aug 7, 2026
Merged

quinnbot-ai merged 4 commits into
mainfrom
fm/nm-pr-wrapper-interception-fork

Conversation

@quinnbot-ai

Copy link
Copy Markdown
Owner

Intent

Give firstmate a supported way to route the no-mistakes pipeline's PR-creation step through a credential authorized for createPullRequest, instead of the pipeline using whatever gh resolves from the daemon's environment.

A GitHub fine-grained personal access token is forbidden from the GraphQL createPullRequest mutation. Such a token passes gh auth status and satisfies every REST call the earlier steps make, then fails only at gh pr create with "Resource not accessible by personal access token" - at the end of an otherwise green run.

Investigation: no supported seam exists

Read from the installed no-mistakes v1.41.2 binary and the upstream source at release 1.46.0, commit 20892e6.

  • internal/scm/github executes every forge call by the bare name gh, 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. agent_path_override covers agent binaries only; commands.* covers lint, test, and format.
  • Bitbucket Cloud, by contrast, reads NO_MISTAKES_BITBUCKET_EMAIL, NO_MISTAKES_BITBUCKET_API_TOKEN, and NO_MISTAKES_BITBUCKET_API_BASE_URL. GitHub has no equivalent.
  • stepCmd already honors a step-scoped PATH and credential environment whenever StepContext.Env is populated, but pipeline.go declares that field as test-only and the production executor never sets it. The mechanism exists; a supported way to configure it does not.

The installed tool is not patched. Tracking it clean was the governing constraint.

What this adds

  • bin/fm-gh.sh runs one command with the credential prefix from a gitignored config/gh-credential. With no such file it execs the command unchanged, so an unconfigured home behaves exactly as before and no machine-specific vault path enters tracked material.
  • bin/fm-gh-shim.sh is installed as a symlink named gh. It routes only pr create and pr edit - the two mutations the forbidden token class cannot perform - and delegates everything else, 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.
  • bin/fm-gh-shim-install.sh installs, removes, and verifies PATH precedence. It refuses to overwrite a gh it does not own and refuses to remove a symlink it did not create.

Nothing installs automatically.

A limit stated rather than papered over

Interception cannot be scoped to a task worktree. The PR step runs inside the no-mistakes daemon, whose environment is resolved once from a login-shell probe at startup, so nothing inside a task worktree is on the PATH that resolves gh. Any shim is PATH-wide by construction. docs/no-mistakes-pr-credential.md documents that plainly, which is why installation is explicitly opt-in.

docs/proposals/nm-pr-step-interception.md specifies the upstream configuration seam that would retire the shim, precise enough to become an upstream PR.

Verification

tests/fm-gh-shim.test.sh (11 assertions) drives the shim and wrapper against a fake gh that records its argv and the credential it received. It covers the exact argument vector the PR step builds, credential precedence, pass-through, recursion, installer ownership and precedence, and bash 3.2.

Recorded evidence in docs/verification/gh-pr-credential.md: the ambient token returns FORBIDDEN on a createPullRequest probe, while the wrapped credential passes authorization and fails only on nonexistent refs (UNPROCESSABLE). Nothing was created by the probe.

The mechanism was also exercised end to end: this change's own upstream pull request was opened by the pipeline's PR step while the shim held precedence.

Relationship to upstream

kunchenguid/firstmate#1858 carries this same change upstream and remains open as the pure-upstream proposal, together with the config-seam document. That PR's CI is held at action_required pending maintainer approval, which is why this fork-side pull request exists: it lets the fleet land the fix without waiting on upstream.

Two behavioral notes for review:

  • gh gives GH_TOKEN precedence over GITHUB_TOKEN, so the wrapper clears both before running a configured prefix; otherwise an ambient GH_TOKEN would silently defeat the injected credential. The unconfigured pass-through path is left untouched.
  • One commit adapts the suite's fixture-root handling to this branch's base, where a cleanup trap can remove the temporary directory before it is used.

QuinnBot added 4 commits August 6, 2026 19:43
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.
The gh shim suite resolves its fixture root with `cd` so its installer cases can
compare against the installer's own normalized output. A cleanup trap registered
inside the `fm_test_tmproot` command substitution can remove that directory
before it is ever used, which made the normalizing `cd` fail and collapse every
fixture path to the filesystem root.

Create the directory before normalizing so the suite holds regardless of when the
cleanup trap runs.
@quinnbot-ai
quinnbot-ai merged commit ae95570 into main Aug 7, 2026
23 of 24 checks passed
quinnbot-ai added a commit that referenced this pull request Aug 10, 2026
#97)

* feat(gh): route pipeline PR creation through a configurable credential

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.

* no-mistakes(review): Harden PR credential routing and shim ownership

* no-mistakes(document): Correct PR credential routing documentation

* test(gh): create the fixture root before normalizing it

The gh shim suite resolves its fixture root with `cd` so its installer cases can
compare against the installer's own normalized output. A cleanup trap registered
inside the `fm_test_tmproot` command substitution can remove that directory
before it is ever used, which made the normalizing `cd` fail and collapse every
fixture path to the filesystem root.

Create the directory before normalizing so the suite holds regardless of when the
cleanup trap runs.

---------

Co-authored-by: QuinnBot <quinnbot@proton.me>
quinnbot-ai added a commit that referenced this pull request Aug 10, 2026
#97)

* feat(gh): route pipeline PR creation through a configurable credential

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.

* no-mistakes(review): Harden PR credential routing and shim ownership

* no-mistakes(document): Correct PR credential routing documentation

* test(gh): create the fixture root before normalizing it

The gh shim suite resolves its fixture root with `cd` so its installer cases can
compare against the installer's own normalized output. A cleanup trap registered
inside the `fm_test_tmproot` command substitution can remove that directory
before it is ever used, which made the normalizing `cd` fail and collapse every
fixture path to the filesystem root.

Create the directory before normalizing so the suite holds regardless of when the
cleanup trap runs.

---------

Co-authored-by: QuinnBot <quinnbot@proton.me>
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