diff --git a/AGENTS.md b/AGENTS.md index 105b04ee9..d0f148bf5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -184,7 +184,7 @@ Safest local verification sequence after non-trivial changes: **Repo Config Trust Boundary (security)** - The daemon runs `commands.*` from `.no-mistakes.yaml` verbatim via `sh -c`, and `agent` selects which process launches with the maintainer's credentials. The code-executing selection fields (`commands.{test,lint,format}` and `agent`) are therefore loaded from the trusted default branch at a **pinned SHA** resolved by a fresh fetch, never from the pushed SHA. The run aborts when the trusted commit or its present config cannot be read and parsed; a readable tree with no config is valid. See `internal/daemon/manager.go` `startRun`, `loadTrustedRepoConfig`, and `assertGateTrustedConfigReadable`. -- `document.instructions` (the repo's documentation placement policy), `review.path_instructions` (path-scoped review guidance appended to the review prompt), `disable_project_settings` (the gate-agent project-instruction opt-out), `no_ci` (positive declaration that the repository intentionally has no CI), and `ci.rerun_transient` (how many times a transiently failed check may be re-run) are also trusted-only, regardless of `allow_repo_commands`: a pushed branch must not weaken any of those boundaries, self-declare no-CI to bypass checks, or steer its own review; enabling the commands opt-in must not drop the maintainer's own trusted values; and every re-run `ci.rerun_transient` authorizes bills another provider-side workflow run to the repository, so a contributor must not be able to raise it (the operator's own global `ci.rerun_transient` is a separate, non-contributor surface that the trusted repo value still overrides). When the opt-out is enabled, only adapters with verified effective suppression may launch. Other non-executing fields (`ignore_patterns`, `auto_fix`, `commit`, `intent`, `test`) are still read from the pushed branch. +- `document.instructions` (the repo's documentation placement policy), `review.path_instructions` (path-scoped review guidance appended to the review prompt), `disable_project_settings` (the gate-agent project-instruction opt-out), `no_ci` (positive declaration that the repository intentionally has no CI), `ci.rerun_transient` (how many times a transiently failed check may be re-run), and `pre_push_check` (the veto run before a push moves an already-existing remote branch, see the Rebase Base & Force-Push Safety section below) are also trusted-only, regardless of `allow_repo_commands`: a pushed branch must not weaken any of those boundaries, self-declare no-CI to bypass checks, steer its own review, or inject or delete its own pre-push guard; enabling the commands opt-in must not drop the maintainer's own trusted values; and every re-run `ci.rerun_transient` authorizes bills another provider-side workflow run to the repository, so a contributor must not be able to raise it (the operator's own global `ci.rerun_transient` is a separate, non-contributor surface that the trusted repo value still overrides). When the opt-out is enabled, only adapters with verified effective suppression may launch. Other non-executing fields (`ignore_patterns`, `auto_fix`, `commit`, `intent`, `test`) are still read from the pushed branch. - Selecting which trusted config applies to a run must never depend on a pushed-branch field. `review.path_instructions` is matched against the COMPLETE changed-file set, never the `ignore_patterns`-filtered subset, because filtering there lets a contributor suppress a maintainer's rule from their own review by ignoring its glob. `reviewablePaths` (`internal/pipeline/steps/common_diff.go`) answers only "does this run have anything to work on". - `pr.base_branch` (the PR, rebase, and CI-merge-conflict-auto-fix integration branch, falling back to `Repo.DefaultBranch` when unset) is trusted-default-branch-only, but unlike the fields in the bullet above it is the deliberate exception that also honors the `allow_repo_commands: true` opt-in, since it controls where an already-maintainer-authorized PR lands rather than what executes. Once a PR already exists, its actual forge base branch (read live via `scm.PRBaseBranchReader`) is authoritative for CI merge-conflict repair and base-branch tip monitoring over a since-changed `pr.base_branch`, and PR lookup matches the existing PR by branch alone, never filtered by base, so a later config change updates that PR instead of opening a duplicate against the new base. Full semantics are owned by `docs/src/content/docs/reference/repo-config.md` (`pr.base_branch`). Regressions: `TestEffectiveRepoConfig_PRBaseBranchTrustedOnly`, `TestEffectiveRepoConfig_PRBaseBranchOptInUsesPushedValue`, `TestEffectiveRepoConfig_PRBaseBranchOptInWithNoTrustedCopyUsesPushedValue`, `TestLoadRepoConfig_PRBaseBranchRejectsInvalidBranchName`, `TestLoadRepoConfig_PRBaseBranchEmptyIsValid`, `TestPRStep_UsesConfiguredBaseBranch`, `TestRebaseStep_UsesConfiguredPRBaseBranch`, `TestCIStep_AutoFixUsesExistingPRBaseAfterConfigChanges`, `TestPRStep_ExistingPRAgainstDifferentBaseIsUpdatedNotDuplicated`. - `allow_repo_commands` is per-repo, read only from the trusted default-branch copy, and defaults `false`; a contributor cannot self-enable it from a pushed branch. The e2e harness models a trusted single-developer environment and commits `allow_repo_commands: true` via `SetupOpts.AllowRepoCommands`; security tests pass `false`. @@ -331,7 +331,8 @@ The rationale lives in the `resolveRun` doc comment and the status-rendering com - Rebase bases come from the freshly fetched authoritative remote refs, never local or stale state; and a branch built on unpushed local-default-branch commits parks with `NeedsApproval` + `AutoFixable=false` instead of silently widening the PR (`detectBundledLocalDefaultCommits`, #283). - Every force-push routes through `resolveForcePushDecision`, which re-reads the live remote head and allows the push only for a new branch, an already-equal remote, an unchanged `lastSeenSHA`, or remote commits already incorporated by patch-id (excluding `^baseSHA` history the run knowingly rewrites). Anything else refuses, and a failed ls-remote/fetch fails closed; never degrade to a bare `--force`/`--force-with-lease` without an explicit anchor. - `lastSeenSHA` must stay the head the run last **observed** (from run/prior-run push provenance or the remote-tracking ref), never the live remote tip: the rebase step refreshes `origin/` only on a normal push, NOT on a force push. CI repairs commit locally and restart validation at Review; the later Push step owns their remote update and force-push safety. Anchoring a lease to a SHA read immediately before pushing is the original #281 bug (it always passes and protects nothing); always-fetching the branch on force push recreates it. Never reintroduce either. -- Regressions: `TestPushStep_RefusesToClobberAdvancedUpstreamBranch` (#305), `TestForcePushRun_RefusesToClobberOutOfBandBranchCommit`, `TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommits` (#283), `TestResolveForcePushDecision_*`, `TestExecutor_CIRestartRevalidatesBeforePush`, `TestPushStep_AllowsForcePushAfterMidRunRebaseOverPriorPushedGeneration` (#837), `TestPushStep_AllowsForcePushOnRerunOverPriorRunPushedGeneration` (#837). +- `pre_push_check` (`internal/pipeline/steps/prepush.go`) is the repository's veto on a push that would move an ALREADY-EXISTING remote branch, i.e. the only push that can land under an open PR an external merge process owns. It never runs for a new-branch creation, an already-equal remote, or an unset field; a non-zero exit refuses before any object moves. Its command and the PR identity it receives are resolved best-effort, but the guard itself is trusted-default-branch-only regardless of `allow_repo_commands` (`EffectiveRepoConfig`): a pushed branch must be able neither to inject it nor to delete it. Unlike `commands.{test,lint,format}`, the command runs with `StepContext.AppRoot` (the app root, `paths.Root()`) as its cwd, never `WorkDir`: those fields legitimately validate the pushed content, but this field is a security veto, so a repository-relative script must not resolve against contributor-pushed content the same trusted command string could otherwise be pointed at forever. `prePushBaseBranch` seeds `BaseBranch` from `effectivePRBaseBranch`, then `livePRBaseBranch` overrides it with the open PR's live forge base when a `scm.PRBaseBranchReader` is available, matching the CI step's precedence for an existing PR over a since-changed `pr.base_branch`. +- Regressions: `TestPushStep_RefusesToClobberAdvancedUpstreamBranch` (#305), `TestForcePushRun_RefusesToClobberOutOfBandBranchCommit`, `TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommits` (#283), `TestResolveForcePushDecision_*`, `TestExecutor_CIRestartRevalidatesBeforePush`, `TestPushStep_AllowsForcePushAfterMidRunRebaseOverPriorPushedGeneration` (#837), `TestPushStep_AllowsForcePushOnRerunOverPriorRunPushedGeneration` (#837), `TestPushStep_PrePushCheck*`, `TestRunConfiguredPrePushCheck_SkipMatrix`, `TestRepoConfig_PrePushCheck`, `TestRunConfiguredPrePushCheck_RunsOutsideWorktree`, `TestRunConfiguredPrePushCheck_UsesLivePRBaseBranch`. **macOS Release Signing (permanent identity)** diff --git a/docs/src/content/docs/reference/environment.md b/docs/src/content/docs/reference/environment.md index 2963e0075..1aaf3de60 100644 --- a/docs/src/content/docs/reference/environment.md +++ b/docs/src/content/docs/reference/environment.md @@ -278,6 +278,10 @@ Disable telemetry collection. When set to a disabling value, telemetry stays off even if a runtime or embedded website ID is available. +## Variables no-mistakes sets for your commands + +The `NO_MISTAKES_*` variables above are read by no-mistakes. One repository setting goes the other way and hands variables **to** a command you configure: [`pre_push_check`](/no-mistakes/reference/repo-config/#pre_push_check) receives the pull request and commit identity of the push it is being asked to approve. That contract is documented with the field. + ## Environment the daemon sees When the daemon runs through a managed service (launchd, systemd user service, Task Scheduler), the macOS and Linux service definitions include a default `PATH` with common user and system binary directories. They also bake in any proxy variables (`HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`, `ALL_PROXY`) that were set when you installed or refreshed the service, so the daemon and the agents it spawns can reach the network through your proxy even when the login-shell probe is unavailable. Once baked in, the values are preserved across later service refreshes and restarts even when the proxy variables are not exported in that shell, so a routine `daemon restart` or a binary upgrade will not strip them; export the variables again only when you need to change or remove them. Both the upper- and lower-case spellings are forwarded exactly as you set them, because tooling is inconsistent about which it reads (curl, for example, honors only the lower-case `http_proxy` for plain-HTTP requests). Because a proxy URL can embed credentials (for example `http://user:pass@host`), the generated service file is restricted to owner-only `0600` permissions whenever proxy values are forwarded into it. When no proxy variables are set, the generated definition is unchanged and keeps the conventional `0644` mode. Windows Task Scheduler inherits your logon environment and needs no forwarding. At daemon startup, the daemon resolves environment from your login shell on macOS and Linux, preserves your shell `PATH` order, and appends any missing well-known directories such as `~/.local/bin`, `~/go/bin`, `~/.cargo/bin`, `~/bin`, `/opt/homebrew/bin`, `/usr/local/bin`, `/usr/bin`, and `/bin`. If login-shell resolution fails or returns no entries, the daemon logs a warning and uses an augmented process-environment fallback that may omit version-manager directories such as nvm, fnm, or volta. On Windows it reuses the current process environment. diff --git a/docs/src/content/docs/reference/pipeline-steps.md b/docs/src/content/docs/reference/pipeline-steps.md index e4864bdbc..5f26df273 100644 --- a/docs/src/content/docs/reference/pipeline-steps.md +++ b/docs/src/content/docs/reference/pipeline-steps.md @@ -193,6 +193,7 @@ Pushes the validated branch to the configured push target. - Immediately before remote mutation, reloads the durable review-approved commit and refuses to push when that binding is missing, malformed, or unreachable - Requires the commit proposed for push to equal or descend from the review-approved commit, allowing commits made by later pipeline steps without authorizing unrelated history - Re-reads the push target via `git ls-remote` before pushing +- For a push that would move an **already-existing** remote branch, runs the repository's [`pre_push_check`](/no-mistakes/reference/repo-config/#pre_push_check) when one is configured, and refuses the push on a non-zero exit. Creating a branch for the first time, and a remote already at the pushed head, are not gated - For existing branches, refuses to force-push when the live remote carries commits the pipeline has not incorporated by patch-id - Fails closed when the remote safety check cannot verify whether the push would discard existing remote work - Uses `--force-with-lease=:` with an explicit SHA anchor for allowed existing-branch rewrites diff --git a/docs/src/content/docs/reference/repo-config.md b/docs/src/content/docs/reference/repo-config.md index 4ffdf56fe..312fbbe69 100644 --- a/docs/src/content/docs/reference/repo-config.md +++ b/docs/src/content/docs/reference/repo-config.md @@ -8,7 +8,8 @@ Per-repo configuration lives in `.no-mistakes.yaml` at the root of your reposito :::caution[Security: gate-control fields are read from the default branch] `commands.*` execute arbitrary shell on the daemon host via `sh -c` / `cmd.exe /c`, and `agent` selects which process launches there (including ordered fallback lists, ACP aliases such as `cursor`, and `acp:` targets) with the maintainer's credentials. To prevent a supply-chain attack where a contributor lands a hostile value on a gated branch, the daemon always reads **`commands` and `agent` from your default branch** (e.g. `origin/main`), never from the pushed SHA, and reads them at the exact commit a fresh fetch resolved (so a stale `origin/` ref cannot serve a value the live default branch removed). -The daemon also reads `document.instructions`, `review.path_instructions`, `disable_project_settings`, `no_ci`, `ci.rerun_transient`, and `test.evidence.branch` only from that trusted copy. +The daemon also reads `document.instructions`, `review.path_instructions`, `disable_project_settings`, `no_ci`, `ci.rerun_transient`, `pre_push_check`, and `test.evidence.branch` only from that trusted copy. +`pre_push_check` is trusted-only for two independent reasons: it runs shell on the daemon host, and it is the guard standing in front of the very push a contributor's branch is asking for, so a pushed branch must be able neither to inject it nor to delete it. `pr.base_branch` is trusted-default-branch-only as well, but unlike those fields it follows the same `allow_repo_commands: true` opt-in exception as `commands`/`agent` (see [`pr.base_branch`](#prbase_branch) below). If the default branch cannot be fetched and resolved to a readable commit, or its present `.no-mistakes.yaml` cannot be read and parsed, the run aborts before launching an agent. A readable default-branch tree with no `.no-mistakes.yaml` is valid and uses defaults. @@ -49,6 +50,12 @@ review: instructions: | Prose changes only. Do not request test coverage. +# Optional veto run before the push step moves a branch that already exists on +# the remote. Runs outside the pushed worktree, so this must be an absolute +# path or a PATH-resolved binary, never a repository-relative script. +# Read only from the trusted default branch. Unset = no check. +pre_push_check: "sh /etc/no-mistakes/merge-queue-hold.sh" + # For orchestration repos whose project instructions would misidentify gate agents. # Read only from the trusted default branch. Defaults to false. disable_project_settings: true @@ -175,6 +182,101 @@ If checks still appear on a declared no-CI repository, their actual states are p This field is honored **only from the trusted default-branch copy** of `.no-mistakes.yaml`, regardless of `allow_repo_commands`. A feature branch cannot self-declare `no_ci: true` to bypass checks, and cannot clear a trusted declaration either. +### pre_push_check + +A shell command run immediately before the [Push](/no-mistakes/reference/pipeline-steps/#push) step moves a branch that **already exists** on the push remote. A non-zero exit refuses the push. + +| | | +| --- | --- | +| Type | `string` (shell command) | +| Default | unset (no check runs, and push behavior is exactly as it was before this field existed) | + +#### The problem it solves + +A gate round can produce a new commit - a review fix, a lint fix, a rebase - for a branch that already has an open pull request. If an **external merge process** owns that pull request at that moment (a merge queue, a batching merge bot that builds several approved PRs together on a scratch branch, a release train), pushing a new commit changes the PR's head SHA and invalidates whatever that process has in flight. Where PRs are batched, one new commit can throw away the CI cycle for every other pull request in the same batch, not just the one that moved. + +no-mistakes cannot know which repositories have such a process, what it is, or how it signals "I am holding this PR right now" - so it does not guess. `pre_push_check` is the seam where a repository answers that question for itself. + +#### What the check receives + +Unlike `commands.{test,lint,format}`, the command does **not** run with the pushed branch's worktree as its working directory - it runs from the no-mistakes app root instead, in the same environment configured commands get, plus the variables below. This is deliberate: `pre_push_check` is a security veto the repository relies on to protect a pull request an external process already owns, so its own behavior must stay trusted-only too. Running it inside the pushed worktree would let a contributor shadow a repo-relative script (like the example below) with their own branch content and defeat the guard using the daemon's credentials. Point `pre_push_check` at an **absolute path** to a script that lives outside any repository checkout, or at a binary on `PATH`; a repository-relative path will not resolve. + +| Variable | Value | +| --- | --- | +| `NO_MISTAKES_PR_URL` | URL of the open pull request on this branch, or empty when none was resolved | +| `NO_MISTAKES_PR_NUMBER` | Number of that pull request, or empty | +| `NO_MISTAKES_REF` | Full ref being pushed, e.g. `refs/heads/feature` | +| `NO_MISTAKES_BRANCH` | Short branch name, e.g. `feature` | +| `NO_MISTAKES_BASE_BRANCH` | Branch this change targets | +| `NO_MISTAKES_HEAD_SHA` | Commit about to be published | +| `NO_MISTAKES_REMOTE_SHA` | Commit the remote branch (and therefore the open pull request) points at right now | + +The pull request identity comes from the run's own recorded PR when it has one, and otherwise from a forge lookup by branch, so it is available on the **first** push of a run against an already-open pull request - the exact push this guard exists for. That lookup is best effort: an unreachable, unauthenticated, or unsupported forge leaves `NO_MISTAKES_PR_URL` and `NO_MISTAKES_PR_NUMBER` empty rather than silently disabling a guard the repository asked for. A check that needs the identity can look it up itself from `NO_MISTAKES_BRANCH`. + +#### When it runs + +It runs for a push that would **change the head of a branch that already exists on the remote**. That is the only push that can land underneath an open pull request. + +It does **not** run when: + +- `pre_push_check` is unset - no subprocess, no forge lookup, nothing changes +- The push creates the branch on the remote for the first time. There is nothing on the remote yet, so there is no pull request and no external process to disturb; opening a brand-new pull request is never gated by this hook +- The remote branch already points at the commit being pushed, so nothing moves + +#### Outcome + +| Exit code | Result | +| --- | --- | +| `0` | The push proceeds exactly as it would without the field | +| non-zero | The push is refused. **No object is moved and the remote branch is untouched.** The step fails with a message naming the pull request, the commit range that was going to move, the exit code, and the check's own output | + +A refusal is a hold, not a corruption: re-run the gate once the pull request is no longer held, or clear the hold and push manually if the update is intended. + +If the command cannot be launched at all (missing interpreter, unreadable script), the step fails with that error rather than pushing. + +#### Example + +A merge-queue-aware check. Adapt the hold signals to whatever your queue actually sets - a label, a check name, a lock branch, an API field: + +```sh +#!/bin/sh +# /etc/no-mistakes/merge-queue-hold.sh - deployed to the daemon host outside +# any repository checkout, so a pushed branch cannot rewrite it. +# Refuse to move a pull request head that an external merge process owns. +set -eu + +# No open pull request on this branch: nothing external can own it. +[ -n "${NO_MISTAKES_PR_NUMBER:-}" ] || exit 0 + +# 1. A label the queue applies while it holds a pull request. +hold_labels='queued|merging|in-merge-queue' +labels=$(gh pr view "$NO_MISTAKES_PR_NUMBER" --json labels --jq '.labels[].name' 2>/dev/null || true) +if printf '%s\n' "$labels" | grep -Eqi "^($hold_labels)$"; then + echo "PR #$NO_MISTAKES_PR_NUMBER is held by the merge queue." + echo "Not moving its head from $NO_MISTAKES_REMOTE_SHA to $NO_MISTAKES_HEAD_SHA." + exit 1 +fi + +# 2. Some queues signal with a pending check on the pull request head instead. +if gh pr checks "$NO_MISTAKES_PR_NUMBER" --json name,state \ + --jq '.[] | select(.name | test("merge.?queue"; "i")) | select(.state == "PENDING") | .name' \ + 2>/dev/null | grep -q .; then + echo "A merge-queue build is in flight on PR #$NO_MISTAKES_PR_NUMBER; not moving its head." + exit 1 +fi + +exit 0 +``` + +```yaml +# .no-mistakes.yaml, on your default branch +pre_push_check: "sh /etc/no-mistakes/merge-queue-hold.sh" +``` + +The same shape works for any forge: swap `gh` for the CLI or API your host provides, or query your merge bot directly. + +This field is honored **only from the trusted default-branch copy** of `.no-mistakes.yaml`, regardless of `allow_repo_commands`, for two independent reasons. It runs arbitrary shell on the daemon host like `commands.*` do, and it is the guard standing in front of the very push a contributor's branch is asking for - so a pushed branch must be able neither to inject a command nor to delete the guard that would hold it back. + ### pr.base_branch Select the branch that newly created pull requests target. diff --git a/internal/config/config.go b/internal/config/config.go index cdce06ab2..16231eff5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -212,6 +212,20 @@ type RepoConfig struct { // the pushed SHA), so a contributor cannot self-enable. Default false: // the pushed branch controls nothing that executes. AllowRepoCommands bool `yaml:"allow_repo_commands"` + // PrePushCheck is an optional shell command run immediately before the push + // step updates a branch that ALREADY exists on the push remote - the case + // where an external merge process (a merge queue, a batching merge bot, a + // release train) may already own the open pull request on that branch. A + // non-zero exit refuses the push instead of moving the PR head underneath + // that process. Empty (the default) disables the check entirely and leaves + // push behavior unchanged. + // + // It executes arbitrary shell on the daemon host exactly like commands.*, + // and it is a SAFETY boundary, so it is honored ONLY from the trusted + // default-branch copy of .no-mistakes.yaml (see EffectiveRepoConfig) and + // ignores allow_repo_commands: a pushed branch must be able neither to + // inject a command nor to delete the maintainer's guard on its own push. + PrePushCheck string `yaml:"pre_push_check"` // PR carries pull-request routing settings. BaseBranch controls where a PR // lands, so EffectiveRepoConfig treats it as trusted-only unless the // repository explicitly opts into pushed settings. @@ -398,6 +412,7 @@ func (c *RepoConfig) UnmarshalYAML(value *yaml.Node) error { Commands Commands `yaml:"commands"` IgnorePatterns []string `yaml:"ignore_patterns"` AllowRepoCommands bool `yaml:"allow_repo_commands"` + PrePushCheck string `yaml:"pre_push_check"` AutoFix AutoFixRaw `yaml:"auto_fix"` CI CIRaw `yaml:"ci"` Commit CommitRaw `yaml:"commit"` @@ -418,6 +433,7 @@ func (c *RepoConfig) UnmarshalYAML(value *yaml.Node) error { c.Commands = raw.Commands c.IgnorePatterns = raw.IgnorePatterns c.AllowRepoCommands = raw.AllowRepoCommands + c.PrePushCheck = raw.PrePushCheck c.AutoFix = raw.AutoFix c.CI = raw.CI c.Commit = raw.Commit @@ -515,6 +531,10 @@ type Config struct { // project-level settings/instructions suppressed; the daemon fails the run // closed if the resolved harness has no verified suppression knob. DisableProjectSettings bool + // PrePushCheck is the resolved, trusted-only pre-push safety command (see + // the RepoConfig field). Empty means no check is configured and the push + // step behaves exactly as it did before the field existed. + PrePushCheck string // NoCI is the resolved, trusted-only declaration that this repository // intentionally has no CI (see the RepoConfig field). When true and the // forge reports zero checks, the CI monitor treats that as all-checks-passed. @@ -2126,6 +2146,12 @@ func EffectiveRepoConfig(pushed, trusted *RepoConfig, allowRepoCommands bool) *R // billed to the repository. It is trusted-only for that reason, so a // pushed branch cannot raise its own rerun budget to the cap. effective.CI = trusted.CI + // pre_push_check both executes shell on the daemon host and guards the + // push that the pushed branch is itself asking for. It is trusted-only + // regardless of allow_repo_commands for both halves of that: a + // contributor must be able neither to inject the command nor to delete + // the maintainer's guard from the very branch it is meant to hold back. + effective.PrePushCheck = trusted.PrePushCheck // test.evidence.branch names the git ref evidence commits are pushed // to with the maintainer's credentials. It is trusted-only so a pushed // branch cannot aim them at another branch of the repository; the rest @@ -2145,6 +2171,7 @@ func EffectiveRepoConfig(pushed, trusted *RepoConfig, allowRepoCommands bool) *R effective.DisableProjectSettings = false effective.NoCI = false effective.CI = CIRaw{} + effective.PrePushCheck = "" effective.Test.Evidence.Branch = nil if !allowRepoCommands { effective.PR = PRRaw{} @@ -2513,6 +2540,7 @@ func Merge(global *GlobalConfig, repo *RepoConfig) *Config { // repo is the EffectiveRepoConfig result, so this value is already // trusted-only (EffectiveRepoConfig sourced it from the trusted copy). DisableProjectSettings: repo.DisableProjectSettings, + PrePushCheck: strings.TrimSpace(repo.PrePushCheck), NoCI: repo.NoCI, } diff --git a/internal/config/config_repo_test.go b/internal/config/config_repo_test.go index ebd62144e..121dbf85f 100644 --- a/internal/config/config_repo_test.go +++ b/internal/config/config_repo_test.go @@ -592,3 +592,64 @@ func TestLoadRepo_LegacyAutoFixBabysit(t *testing.T) { t.Fatalf("ci auto-fix = %d, want 0", *cfg.AutoFix.CI) } } + +// TestRepoConfig_PrePushCheck covers the pre-push safety hook end to end at the +// configuration layer: it parses from .no-mistakes.yaml, it resolves onto the +// merged config, and it is honored ONLY from the trusted default-branch copy. +// The trust rule has two independent reasons, and both have to hold: the value +// is shell that runs on the daemon host, and it is the guard standing in front +// of the very push the pushed branch is asking for. +func TestRepoConfig_PrePushCheck(t *testing.T) { + trustedCommand := "scripts/merge-queue-hold.sh" + pushedCommand := "true" + + parsed, err := LoadRepoFromBytes([]byte("pre_push_check: \"" + trustedCommand + "\"\n")) + if err != nil { + t.Fatal(err) + } + if parsed.PrePushCheck != trustedCommand { + t.Fatalf("parsed pre_push_check = %q, want %q", parsed.PrePushCheck, trustedCommand) + } + + pushed := &RepoConfig{PrePushCheck: pushedCommand} + trusted := &RepoConfig{PrePushCheck: trustedCommand} + + effective := EffectiveRepoConfig(pushed, trusted, false) + if effective.PrePushCheck != trustedCommand { + t.Fatalf("pre_push_check = %q, want %q from the trusted copy", effective.PrePushCheck, trustedCommand) + } + if got := Merge(DefaultGlobalConfig(), effective).PrePushCheck; got != trustedCommand { + t.Fatalf("resolved pre_push_check = %q, want %q", got, trustedCommand) + } + + // allow_repo_commands opts in to pushed commands and agent selection. It + // must not also let a branch replace the guard on its own push. + optedIn := EffectiveRepoConfig(pushed, trusted, true) + if optedIn.PrePushCheck != trustedCommand { + t.Fatalf("pre_push_check with allow_repo_commands = %q, want %q", optedIn.PrePushCheck, trustedCommand) + } + + // A branch that removes the field must not disarm the maintainer's guard. + silent := EffectiveRepoConfig(&RepoConfig{}, trusted, true) + if silent.PrePushCheck != trustedCommand { + t.Fatalf("pre_push_check for a branch with no value = %q, want %q", silent.PrePushCheck, trustedCommand) + } + + // No trusted copy means no check at all, never the pushed one. + withoutTrusted := EffectiveRepoConfig(pushed, nil, true) + if withoutTrusted.PrePushCheck != "" { + t.Fatalf("pre_push_check without a trusted copy = %q, want unset", withoutTrusted.PrePushCheck) + } +} + +// TestMerge_PrePushCheckDefaultsToUnset pins the opt-in guarantee at the +// configuration layer: a repository that never mentions the field resolves to +// an empty command, which the push step treats as "behave exactly as before". +func TestMerge_PrePushCheckDefaultsToUnset(t *testing.T) { + if got := Merge(DefaultGlobalConfig(), &RepoConfig{}).PrePushCheck; got != "" { + t.Fatalf("default pre_push_check = %q, want unset", got) + } + if got := Merge(DefaultGlobalConfig(), &RepoConfig{PrePushCheck: " \n\t "}).PrePushCheck; got != "" { + t.Fatalf("whitespace-only pre_push_check = %q, want unset", got) + } +} diff --git a/internal/pipeline/executor.go b/internal/pipeline/executor.go index f1bad9c79..674c54466 100644 --- a/internal/pipeline/executor.go +++ b/internal/pipeline/executor.go @@ -131,6 +131,16 @@ func (e *Executor) runEvidenceDir(runID string) string { return e.paths.RunEvidenceDir(configured, runID) } +// appRoot returns the no-mistakes app root, a stable directory outside any +// repository checkout, for steps that need to run a trusted subprocess +// without a repo-relative path resolving against contributor-pushed content. +func (e *Executor) appRoot() string { + if e.paths == nil { + return "" + } + return e.paths.Root() +} + // SetGateReconcileTimings overrides the interval between approval-gate // reconciliation checks and the deadline for each check. It is primarily used // by deterministic tests and specialized embeddings; non-positive values keep @@ -826,6 +836,7 @@ func (e *Executor) executeStep(ctx context.Context, step Step, sr *db.StepResult Sessions: e.sessions, Shared: e.shared, EvidenceDir: e.runEvidenceDir(run.ID), + AppRoot: e.appRoot(), Fixing: state.fixing, PreviousFindings: state.previousFindings, Log: writeLog, diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 73449c543..a5b324a88 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -39,7 +39,14 @@ type StepContext struct { // prompt and the PR step's publisher - names the same directory. Empty only // in embeddings that never gather evidence. EvidenceDir string - Env []string // extra environment variables for subprocesses (used in tests) + // AppRoot is the no-mistakes app root (NM_HOME), a stable directory that + // always exists and never contains contributor-pushed content. Steps that + // must run a trusted subprocess without exposing it to a repo-relative + // path a pushed branch could shadow (e.g. pre_push_check) use this as the + // subprocess working directory instead of WorkDir. Empty only in + // embeddings that never construct a *paths.Paths. + AppRoot string + Env []string // extra environment variables for subprocesses (used in tests) // UserIntent is a short, possibly-empty summary of what the change author // was trying to accomplish. It's surfaced in step prompts so agents have // context beyond the diff. Its authority depends on IntentSource: an diff --git a/internal/pipeline/steps/common_exec.go b/internal/pipeline/steps/common_exec.go index aae8c7004..7545fa6de 100644 --- a/internal/pipeline/steps/common_exec.go +++ b/internal/pipeline/steps/common_exec.go @@ -49,12 +49,22 @@ func mergeEnv(extra []string) []string { if len(extra) == 0 { return nil } - merged := make([]string, 0, len(os.Environ())+len(extra)) + return overrideEnv(os.Environ(), extra) +} + +// overrideEnv returns base with every entry of extra applied: an entry whose +// key is already present replaces it in place (so PATH stays where the caller +// put it), and the rest are appended in order. +func overrideEnv(base, extra []string) []string { + if len(extra) == 0 { + return base + } + merged := make([]string, 0, len(base)+len(extra)) overrides := make(map[string]string, len(extra)) for _, entry := range extra { overrides[envKey(entry)] = entry } - for _, entry := range os.Environ() { + for _, entry := range base { key := envKey(entry) if override, ok := overrides[key]; ok { merged = append(merged, override) diff --git a/internal/pipeline/steps/helpers_test.go b/internal/pipeline/steps/helpers_test.go index fc81d3756..915e3fce4 100644 --- a/internal/pipeline/steps/helpers_test.go +++ b/internal/pipeline/steps/helpers_test.go @@ -171,13 +171,16 @@ func newTestContext(t *testing.T, ag agent.Agent, workDir, baseSHA, headSHA stri // a per-test directory so a step under test can never write evidence // into a shared location the next test would then observe. EvidenceDir: filepath.Join(t.TempDir(), "evidence", "run-1"), - WorkDir: workDir, - Agent: ag, - Config: &config.Config{Agent: types.AgentClaude, Commands: cmds}, - DB: database, - Log: func(s string) {}, - LogChunk: func(s string) {}, - LogFile: func(s string) {}, + // A per-test stand-in for NM_HOME: a directory that exists and is never + // the repo worktree, mirroring the production app root. + AppRoot: t.TempDir(), + WorkDir: workDir, + Agent: ag, + Config: &config.Config{Agent: types.AgentClaude, Commands: cmds}, + DB: database, + Log: func(s string) {}, + LogChunk: func(s string) {}, + LogFile: func(s string) {}, } } diff --git a/internal/pipeline/steps/prepush.go b/internal/pipeline/steps/prepush.go new file mode 100644 index 000000000..c8f0ff234 --- /dev/null +++ b/internal/pipeline/steps/prepush.go @@ -0,0 +1,308 @@ +package steps + +import ( + "fmt" + "os" + "strings" + + "github.com/kunchenguid/no-mistakes/internal/pipeline" + "github.com/kunchenguid/no-mistakes/internal/safepath" + "github.com/kunchenguid/no-mistakes/internal/safeurl" + "github.com/kunchenguid/no-mistakes/internal/scm" +) + +// prePushCheckOutputMaxBytes bounds how much of a blocking check's output is +// quoted back in the refusal error. The refusal travels through step errors, +// the TUI, and the run record, so the quoted tail stays small; the complete +// output is always written to the step log first. +const prePushCheckOutputMaxBytes = 4 * 1024 + +// prePushTarget describes the remote branch a push is about to move, plus the +// open pull request that branch is known to belong to. It is the decision +// context handed to a repository's pre_push_check command. +type prePushTarget struct { + Ref string + Branch string + BaseBranch string + // HeadSHA is the commit the pipeline is about to publish. + HeadSHA string + // RemoteSHA is the commit the remote branch (and therefore the open PR) + // currently points at. This is the value an external merge process has + // already read, approved, or started building. + RemoteSHA string + // PRURL and PRNumber identify the open pull request on this branch when + // no-mistakes could resolve one. Both are empty when the forge could not be + // consulted; the check command can still look the PR up itself from the + // branch name and the worktree it runs in. + PRURL string + PRNumber string +} + +func (t prePushTarget) env() []string { + return []string{ + "NO_MISTAKES_REF=" + t.Ref, + "NO_MISTAKES_BRANCH=" + t.Branch, + "NO_MISTAKES_BASE_BRANCH=" + t.BaseBranch, + "NO_MISTAKES_HEAD_SHA=" + t.HeadSHA, + "NO_MISTAKES_REMOTE_SHA=" + t.RemoteSHA, + "NO_MISTAKES_PR_URL=" + t.PRURL, + "NO_MISTAKES_PR_NUMBER=" + t.PRNumber, + } +} + +// describe names the thing being pushed under, preferring the pull request +// identity when one was resolved so the refusal reads the way the operator +// thinks about it. +func (t prePushTarget) describe() string { + switch { + case t.PRNumber != "" && t.PRURL != "": + return fmt.Sprintf("pull request #%s (%s)", t.PRNumber, t.PRURL) + case t.PRURL != "": + return fmt.Sprintf("pull request %s", t.PRURL) + case t.PRNumber != "": + return fmt.Sprintf("pull request #%s", t.PRNumber) + default: + return fmt.Sprintf("existing remote branch %s", t.Branch) + } +} + +// prePushCheckBlockedError reports that the repository's configured +// pre_push_check refused this push. It is deliberately a distinct type: the +// push did not fail, it was declined by a policy the repository asked for, and +// the message has to say so plainly enough that the operator knows the fix is +// to wait or to talk to the external process rather than to retry. +type prePushCheckBlockedError struct { + command string + target prePushTarget + exitCode int + output string +} + +func (e *prePushCheckBlockedError) Error() string { + var b strings.Builder + fmt.Fprintf(&b, + "refusing to push: this repository's pre_push_check declined to move %s from %s to %s (%q exited with code %d). "+ + "The branch was left untouched. This guard exists because an external merge process - a merge queue, a batching merge bot, a release train - "+ + "may already own the open pull request, and pushing a new head underneath it invalidates whatever that process has in flight. "+ + "Re-run the gate once the pull request is no longer held, or clear the hold and push manually if this update is intended.", + e.target.describe(), shortObjectID(e.target.RemoteSHA), shortObjectID(e.target.HeadSHA), e.command, e.exitCode, + ) + if output := strings.TrimSpace(e.output); output != "" { + b.WriteString("\npre_push_check output: ") + b.WriteString(output) + } + return b.String() +} + +// runConfiguredPrePushCheck runs the repository's pre_push_check before the +// push step moves a branch that already exists on the push remote, and turns a +// non-zero exit into a refusal. +// +// The check is scoped to a push that changes the head of an ALREADY-EXISTING +// remote branch, because that is the only push that can land underneath an +// open pull request. Creating the branch for the first time (decision.newBranch) +// cannot: there is nothing on the remote yet, so there is no pull request and +// no external process to disturb. A push whose head the remote already carries +// (decision.upToDate) moves nothing at all. Both are skipped, so opening a +// brand-new PR is never gated by this hook. +// +// An unset pre_push_check is a complete no-op: no forge lookup, no subprocess, +// no behavior change. +// +// The command runs with sctx.AppRoot (the no-mistakes app root) as its +// working directory, never sctx.WorkDir. pre_push_check is trusted-only +// precisely because it is a security veto a repository relies on to protect +// an externally owned pull request, unlike commands.{test,lint,format}, whose +// entire job is to validate the pushed content itself. Running it inside the +// pushed worktree would let a contributor shadow a repo-relative script +// (e.g. the documented `pre_push_check: "scripts/merge-queue-hold.sh"`) with +// their own branch content and defeat the guard with the daemon's own +// credentials. A repository-relative script therefore will not resolve; the +// check must name an absolute path or a PATH-resolved binary. +func runConfiguredPrePushCheck(sctx *pipeline.StepContext, decision forcePushDecision, target prePushTarget) error { + if sctx.Config == nil { + return nil + } + command := strings.TrimSpace(sctx.Config.PrePushCheck) + if command == "" { + return nil + } + if decision.newBranch { + sctx.Log("skipping pre_push_check: creating a new remote branch, so no existing pull request can be pushed under") + return nil + } + if decision.upToDate { + sctx.Log("skipping pre_push_check: the remote branch already points at this head, so nothing moves") + return nil + } + + target.RemoteSHA = decision.remoteSHA + var freshBaseBranch string + target.PRURL, target.PRNumber, freshBaseBranch = resolvePrePushPRIdentity(sctx, target.Branch) + if freshBaseBranch != "" { + // A freshly listed PR/MR already carries the forge's live target + // branch on every supported provider, not just GitHub - prefer it + // over the configured fallback before trying the GitHub-only + // dedicated lookup below. + target.BaseBranch = freshBaseBranch + } + if actual := livePRBaseBranch(sctx, target.PRURL, target.PRNumber); actual != "" { + // A dedicated by-identity lookup is the most current read (it also + // covers the case where the PR identity came from the run's durable + // record instead of a fresh listing above), so it wins when the + // provider supports it. + target.BaseBranch = actual + } + + checkDir := strings.TrimSpace(sctx.AppRoot) + if checkDir == "" { + return fmt.Errorf("pre_push_check %q: no app root available to run the check outside the pushed worktree", command) + } + + sctx.Log(fmt.Sprintf("running pre_push_check before updating %s: %s", target.describe(), command)) + output, exitCode, err := runShellCommandWithProcessEnv(sctx.Ctx, checkDir, prePushCheckEnv(sctx, target), command) + if err != nil { + return fmt.Errorf("run pre_push_check %q: %w", command, err) + } + if strings.TrimSpace(output) != "" { + // The complete output belongs in the step log; only a clamped, redacted + // copy travels with the refusal. + sctx.LogFile(output) + } + if exitCode != 0 { + return &prePushCheckBlockedError{ + command: command, + target: target, + exitCode: exitCode, + output: redactPrePushOutput(output), + } + } + sctx.Log("pre_push_check passed") + return nil +} + +// prePushCheckEnv layers the decision context on top of the environment the +// step would otherwise run a configured command with, so the check keeps the +// daemon's PATH, credentials, and any step-scoped overrides. +func prePushCheckEnv(sctx *pipeline.StepContext, target prePushTarget) []string { + base := stepEnvironment(sctx) + if base == nil { + base = os.Environ() + } + return overrideEnv(base, target.env()) +} + +// redactPrePushOutput clamps the check's output for embedding in the refusal +// and scrubs the two things the rest of this package already refuses to +// publish: credential-bearing URLs and the operator's home directory. +func redactPrePushOutput(output string) string { + output = strings.TrimSpace(output) + if len(output) > prePushCheckOutputMaxBytes { + output = truncateTextAtLineBoundary(output, prePushCheckOutputMaxBytes, "[pre_push_check output truncated; the complete output is in the push step log]") + } + return safepath.RedactText(safeurl.RedactText(output)) +} + +// resolvePrePushPRIdentity finds the open pull request the branch about to be +// pushed belongs to. It prefers the run's own durably recorded PR URL and +// otherwise asks the forge, which is what makes the identity available on the +// FIRST push of a run against an already-open pull request - the exact push +// this hook exists to guard. When the forge has to be asked, baseBranch is +// also returned from that same lookup: every supported provider's PR listing +// already carries its live target branch, so this is a live read, not the +// configured fallback, on any provider - not just the one with a dedicated +// scm.PRBaseBranchReader (see livePRBaseBranch). baseBranch is empty when the +// identity instead came from the run's durable record, since that path does +// not re-list the PR. +// +// Every failure is best-effort and non-fatal: the check still runs with empty +// PR fields. A forge that is unreachable, unauthenticated, or unsupported must +// not be able to silently disable a guard the repository asked for, and a +// command that needs the PR identity can look it up itself. +func resolvePrePushPRIdentity(sctx *pipeline.StepContext, branch string) (prURL, prNumber, baseBranch string) { + if sctx.Run != nil && sctx.Run.PRURL != nil { + if recorded := strings.TrimSpace(*sctx.Run.PRURL); recorded != "" { + return recorded, prNumberFromURL(recorded), "" + } + } + host, skipReason := buildHost(sctx, resolvedProvider(sctx)) + if host == nil { + sctx.LogFile(fmt.Sprintf("pre_push_check: pull request identity unavailable: %s", skipReason)) + return "", "", "" + } + if err := host.Available(sctx.Ctx); err != nil { + sctx.LogFile(fmt.Sprintf("pre_push_check: pull request identity unavailable: %v", err)) + return "", "", "" + } + pr, err := host.FindPR(sctx.Ctx, branch, "") + if err != nil { + sctx.LogFile(fmt.Sprintf("pre_push_check: pull request lookup for %s failed: %v", branch, err)) + return "", "", "" + } + if pr == nil { + return "", "", "" + } + number := strings.TrimSpace(pr.Number) + url := strings.TrimSpace(pr.URL) + if number == "" && url != "" { + number = prNumberFromURL(url) + } + return url, number, strings.TrimSpace(pr.BaseBranch) +} + +// prePushBaseBranch reports the configured integration base branch as a +// fallback for when no open pull request (or no live-readable base) is known +// yet. Once a pull request exists, runConfiguredPrePushCheck overrides this +// with its actual live base branch via livePRBaseBranch, matching the +// precedence the CI step already applies: a since-changed pr.base_branch must +// not misdescribe an existing pull request's real target. +func prePushBaseBranch(sctx *pipeline.StepContext) string { + return effectivePRBaseBranch(sctx) +} + +// livePRBaseBranch asks the forge for the open pull request's actual base +// branch, when one is known. An already-open pull request's live target is +// authoritative over a since-changed pr.base_branch for the same reason the +// CI step prefers it (see effectivePRBaseBranch callers in ci.go): the check +// exists to describe the branch's real target to the guard, not a +// hypothetical one a later config edit selected. Best effort: an unsupported +// provider, an unreachable forge, or a lookup failure leaves the caller's +// existing value untouched rather than blocking the check. +func livePRBaseBranch(sctx *pipeline.StepContext, prURL, prNumber string) string { + if prURL == "" && prNumber == "" { + return "" + } + host, skipReason := buildHost(sctx, resolvedProvider(sctx)) + if host == nil { + sctx.LogFile(fmt.Sprintf("pre_push_check: live base branch unavailable: %s", skipReason)) + return "" + } + reader, ok := host.(scm.PRBaseBranchReader) + if !ok { + return "" + } + actual, err := reader.GetPRBaseBranch(sctx.Ctx, &scm.PR{Number: prNumber, URL: prURL}) + if err != nil { + sctx.LogFile(fmt.Sprintf("pre_push_check: live base branch lookup failed: %v", err)) + return "" + } + return strings.TrimSpace(actual) +} + +// prNumberFromURL extracts the trailing numeric segment every supported +// forge's PR/MR URL ends with, and returns "" for anything else rather than +// guessing. +func prNumberFromURL(prURL string) string { + trimmed := strings.TrimRight(strings.TrimSpace(prURL), "/") + idx := strings.LastIndex(trimmed, "/") + if idx < 0 || idx+1 >= len(trimmed) { + return "" + } + candidate := trimmed[idx+1:] + for _, r := range candidate { + if r < '0' || r > '9' { + return "" + } + } + return candidate +} diff --git a/internal/pipeline/steps/prepush_test.go b/internal/pipeline/steps/prepush_test.go new file mode 100644 index 000000000..7bc4b5677 --- /dev/null +++ b/internal/pipeline/steps/prepush_test.go @@ -0,0 +1,457 @@ +package steps + +import ( + "os" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" + + "github.com/kunchenguid/no-mistakes/internal/config" + "github.com/kunchenguid/no-mistakes/internal/pipeline" +) + +// prePushProbe is a scripted pre_push_check. It records the decision context it +// was handed plus the remote head as it stood WHILE the check ran, then exits +// with the configured code. +type prePushProbe struct { + command string + recordPath string + remotePath string +} + +// ran reports whether the check executed at all. +func (p prePushProbe) ran() bool { + _, err := os.Stat(p.recordPath) + return err == nil +} + +func (p prePushProbe) mustRun(t *testing.T) map[string]string { + t.Helper() + data, err := os.ReadFile(p.recordPath) + if err != nil { + t.Fatalf("pre_push_check did not run: %v", err) + } + fields := map[string]string{} + for _, line := range strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") { + key, value, ok := strings.Cut(strings.TrimSpace(line), "=") + if !ok { + continue + } + fields[key] = strings.TrimSpace(value) + } + return fields +} + +// remoteHeadDuringCheck is the SHA the remote branch pointed at when the check +// ran. It is what proves the hook is a PRE-push hook: on the passing path the +// push succeeds afterwards, so a value equal to the pre-push remote head is the +// only evidence that ordering was respected. +func (p prePushProbe) remoteHeadDuringCheck(t *testing.T) string { + t.Helper() + data, err := os.ReadFile(p.remotePath) + if err != nil { + t.Fatalf("pre_push_check did not observe the remote: %v", err) + } + fields := strings.Fields(string(data)) + if len(fields) == 0 { + return "" + } + return fields[0] +} + +// newPrePushProbe writes a platform-appropriate check script and returns the +// command string a repository would put in pre_push_check. +func newPrePushProbe(t *testing.T, upstream, ref string, exitCode int) prePushProbe { + t.Helper() + dir := t.TempDir() + probe := prePushProbe{ + recordPath: filepath.Join(dir, "context"), + remotePath: filepath.Join(dir, "remote"), + } + + if runtime.GOOS == "windows" { + script := filepath.Join(dir, "pre-push-check.cmd") + body := "@echo off\r\n" + + "(\r\n" + + "echo pr_url=%NO_MISTAKES_PR_URL%\r\n" + + "echo pr_number=%NO_MISTAKES_PR_NUMBER%\r\n" + + "echo ref=%NO_MISTAKES_REF%\r\n" + + "echo branch=%NO_MISTAKES_BRANCH%\r\n" + + "echo base_branch=%NO_MISTAKES_BASE_BRANCH%\r\n" + + "echo head_sha=%NO_MISTAKES_HEAD_SHA%\r\n" + + "echo remote_sha=%NO_MISTAKES_REMOTE_SHA%\r\n" + + ") > \"" + probe.recordPath + "\"\r\n" + + "git ls-remote \"" + upstream + "\" " + ref + " > \"" + probe.remotePath + "\"\r\n" + + "echo the pull request is held by an external merge process\r\n" + + "exit /b " + strconv.Itoa(exitCode) + "\r\n" + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + probe.command = `"` + script + `"` + return probe + } + + script := filepath.Join(dir, "pre-push-check.sh") + body := "#!/bin/sh\n" + + "{\n" + + "printf 'pr_url=%s\\n' \"$NO_MISTAKES_PR_URL\"\n" + + "printf 'pr_number=%s\\n' \"$NO_MISTAKES_PR_NUMBER\"\n" + + "printf 'ref=%s\\n' \"$NO_MISTAKES_REF\"\n" + + "printf 'branch=%s\\n' \"$NO_MISTAKES_BRANCH\"\n" + + "printf 'base_branch=%s\\n' \"$NO_MISTAKES_BASE_BRANCH\"\n" + + "printf 'head_sha=%s\\n' \"$NO_MISTAKES_HEAD_SHA\"\n" + + "printf 'remote_sha=%s\\n' \"$NO_MISTAKES_REMOTE_SHA\"\n" + + "} > '" + probe.recordPath + "'\n" + + "git ls-remote '" + upstream + "' " + ref + " > '" + probe.remotePath + "'\n" + + "printf 'the pull request is held by an external merge process\\n'\n" + + "exit " + strconv.Itoa(exitCode) + "\n" + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + probe.command = "'" + script + "'" + return probe +} + +// newPrePushFixture builds a worktree whose feature branch is already published +// on a bare upstream and then advances the local head, so the push step is in +// the exact position this hook guards: about to move an existing remote branch. +func newPrePushFixture(t *testing.T) (sctx *pipeline.StepContext, upstream, publishedHead, newHead string) { + t.Helper() + upstream = t.TempDir() + gitCmd(t, upstream, "init", "--bare") + + dir, baseSHA, submittedHead := setupGitRepo(t) + gitCmd(t, dir, "remote", "add", "origin", upstream) + gitCmd(t, dir, "push", "origin", "main") + gitCmd(t, dir, "push", "origin", "feature") + publishedHead = submittedHead + + if err := os.WriteFile(filepath.Join(dir, "fix.txt"), []byte("pipeline fix\n"), 0o644); err != nil { + t.Fatal(err) + } + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-m", "no-mistakes: apply agent fixes") + newHead = gitCmd(t, dir, "rev-parse", "HEAD") + + sctx = newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, newHead, config.Commands{}) + sctx.Repo.UpstreamURL = upstream + sctx.Run.Branch = "refs/heads/feature" + recordReviewApproval(t, sctx, newHead) + return sctx, upstream, publishedHead, newHead +} + +// TestPushStep_PrePushCheckBlocksUpdateToAlreadyOpenPullRequest is the +// regression test for the gap this hook closes. A gate round produced a fix +// commit for a branch whose open pull request an external merge process already +// owns; pushing it changes the PR head and invalidates whatever that process +// has in flight. With pre_push_check configured, the veto must land BEFORE any +// object moves and say plainly why. +func TestPushStep_PrePushCheckBlocksUpdateToAlreadyOpenPullRequest(t *testing.T) { + t.Parallel() + sctx, upstream, publishedHead, newHead := newPrePushFixture(t) + probe := newPrePushProbe(t, upstream, "refs/heads/feature", 3) + sctx.Config.PrePushCheck = probe.command + prURL := "https://github.com/test/repo/pull/7" + sctx.Run.PRURL = &prURL + + _, err := (&PushStep{}).Execute(sctx) + if err == nil { + t.Fatal("expected the configured pre_push_check to refuse the push") + } + for _, want := range []string{"pre_push_check", "pull request #7", "exited with code 3", "external merge process"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal must explain itself; missing %q in:\n%s", want, err) + } + } + if !strings.Contains(err.Error(), "held by an external merge process") { + t.Fatalf("refusal must quote the check's own output:\n%s", err) + } + + if remote := gitCmd(t, upstream, "rev-parse", "refs/heads/feature"); remote != publishedHead { + t.Fatalf("remote moved from %s to %s despite the refusal", publishedHead, remote) + } + if fileAtRef(t, upstream, "refs/heads/feature", "fix.txt") { + t.Fatal("the blocked commit reached the remote branch") + } + + dbRun, err := sctx.DB.GetRun(sctx.Run.ID) + if err != nil { + t.Fatal(err) + } + if dbRun.LastPushedSHA != nil && *dbRun.LastPushedSHA == newHead { + t.Fatal("a refused push must not be recorded as delivered") + } + if dbRun.PushActive { + t.Fatal("push-active marker remained set after the refusal") + } +} + +// TestPushStep_PrePushCheckPassesAndRunsBeforeThePush pins the other half: +// exit 0 pushes exactly as before, and the check saw the pre-push remote head, +// which is what makes it a pre-push hook rather than a post-mortem. +func TestPushStep_PrePushCheckPassesAndRunsBeforeThePush(t *testing.T) { + t.Parallel() + sctx, upstream, publishedHead, newHead := newPrePushFixture(t) + probe := newPrePushProbe(t, upstream, "refs/heads/feature", 0) + sctx.Config.PrePushCheck = probe.command + prURL := "https://github.com/test/repo/pull/7" + sctx.Run.PRURL = &prURL + + if _, err := (&PushStep{}).Execute(sctx); err != nil { + t.Fatalf("a passing pre_push_check must not change push behavior: %v", err) + } + + if observed := probe.remoteHeadDuringCheck(t); observed != publishedHead { + t.Fatalf("check observed remote head %s, want the pre-push head %s", observed, publishedHead) + } + if remote := gitCmd(t, upstream, "rev-parse", "refs/heads/feature"); remote != newHead { + t.Fatalf("remote head = %s, want the pushed head %s", remote, newHead) + } + + fields := probe.mustRun(t) + want := map[string]string{ + "pr_url": prURL, + "pr_number": "7", + "ref": "refs/heads/feature", + "branch": "feature", + "base_branch": "main", + "head_sha": newHead, + "remote_sha": publishedHead, + } + for key, expected := range want { + if fields[key] != expected { + t.Errorf("pre_push_check %s = %q, want %q", key, fields[key], expected) + } + } +} + +// TestPushStep_PrePushCheckSkipsFirstPublicationOfABranch pins the scope: the +// hook guards pushes that land under something already open. Publishing a +// branch for the first time - the push that a brand-new pull request is created +// from - has nothing to land under and must not be gated. +func TestPushStep_PrePushCheckSkipsFirstPublicationOfABranch(t *testing.T) { + t.Parallel() + upstream := t.TempDir() + gitCmd(t, upstream, "init", "--bare") + + dir, baseSHA, headSHA := setupGitRepo(t) + gitCmd(t, dir, "remote", "add", "origin", upstream) + gitCmd(t, dir, "push", "origin", "main") + + sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, headSHA, config.Commands{}) + sctx.Repo.UpstreamURL = upstream + sctx.Run.Branch = "refs/heads/feature" + recordReviewApproval(t, sctx, headSHA) + + probe := newPrePushProbe(t, upstream, "refs/heads/feature", 1) + sctx.Config.PrePushCheck = probe.command + + if _, err := (&PushStep{}).Execute(sctx); err != nil { + t.Fatalf("creating a new remote branch must not be gated: %v", err) + } + if probe.ran() { + t.Fatal("pre_push_check ran while publishing a branch for the first time") + } + if remote := gitCmd(t, upstream, "rev-parse", "refs/heads/feature"); remote != headSHA { + t.Fatalf("remote head = %s, want %s", remote, headSHA) + } +} + +// TestPushStep_PrePushCheckUnsetLeavesExistingBehaviorIntact is the +// opt-in guarantee: a repository that never heard of this field pushes exactly +// as it did before, with no subprocess and no forge lookup. +func TestPushStep_PrePushCheckUnsetLeavesExistingBehaviorIntact(t *testing.T) { + t.Parallel() + sctx, upstream, _, newHead := newPrePushFixture(t) + if sctx.Config.PrePushCheck != "" { + t.Fatal("pre_push_check must default to unset") + } + + if _, err := (&PushStep{}).Execute(sctx); err != nil { + t.Fatal(err) + } + if remote := gitCmd(t, upstream, "rev-parse", "refs/heads/feature"); remote != newHead { + t.Fatalf("remote head = %s, want %s", remote, newHead) + } +} + +// TestRunConfiguredPrePushCheck_SkipMatrix pins the three cases that must never +// launch the command, straight against the decision the push step hands it. +func TestRunConfiguredPrePushCheck_SkipMatrix(t *testing.T) { + t.Parallel() + tests := []struct { + name string + configure bool + decision forcePushDecision + }{ + {name: "unset", configure: false, decision: forcePushDecision{remoteSHA: strings.Repeat("a", 40)}}, + {name: "new branch", configure: true, decision: forcePushDecision{newBranch: true}}, + {name: "remote already at head", configure: true, decision: forcePushDecision{remoteSHA: strings.Repeat("a", 40), upToDate: true}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, headSHA, config.Commands{}) + probe := newPrePushProbe(t, dir, "refs/heads/feature", 1) + if tt.configure { + sctx.Config.PrePushCheck = probe.command + } + + if err := runConfiguredPrePushCheck(sctx, tt.decision, prePushTarget{ + Ref: "refs/heads/feature", + Branch: "feature", + HeadSHA: headSHA, + }); err != nil { + t.Fatalf("skipped case must not fail: %v", err) + } + if probe.ran() { + t.Fatal("pre_push_check ran for a push it does not guard") + } + }) + } +} + +// writeRelativePrePushScript writes a trivial pre_push_check script at relPath +// under root and returns the command a repository could configure to invoke +// it BY THAT RELATIVE PATH, i.e. relying on the process working directory to +// resolve it - exactly what a contributor's own pushed branch can supply at a +// path the trusted config names (the documented example is +// "scripts/merge-queue-hold.sh"). +func writeRelativePrePushScript(t *testing.T, root, relPath string, exitCode int) string { + t.Helper() + full := filepath.Join(root, relPath) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if runtime.GOOS == "windows" { + full += ".cmd" + relPath += ".cmd" + body := "@echo off\r\nexit /b " + strconv.Itoa(exitCode) + "\r\n" + if err := os.WriteFile(full, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + return relPath + } + body := "#!/bin/sh\nexit " + strconv.Itoa(exitCode) + "\n" + if err := os.WriteFile(full, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + return "sh " + relPath +} + +// TestRunConfiguredPrePushCheck_RunsOutsideWorktree is the regression test for +// the trust-boundary bypass a repository-relative pre_push_check script would +// otherwise open: pre_push_check is trusted-only precisely because it is a +// security veto, but running it inside the pushed worktree would let a +// contributor shadow a repo-relative script - such as the documented +// "scripts/merge-queue-hold.sh" example - with their own branch content and +// make the trusted command always pass using the daemon's own credentials. +// The script here lives ONLY in the pushed worktree (sctx.WorkDir), so the +// check must fail to resolve it rather than silently executing it. +func TestRunConfiguredPrePushCheck_RunsOutsideWorktree(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, headSHA, config.Commands{}) + + // A contributor-controlled script at a repository-relative path that + // always exits 0 (i.e. it would silently defeat the guard if it ran). + sctx.Config.PrePushCheck = writeRelativePrePushScript(t, dir, "scripts/merge-queue-hold.sh", 0) + + err := runConfiguredPrePushCheck(sctx, forcePushDecision{remoteSHA: strings.Repeat("a", 40)}, prePushTarget{ + Ref: "refs/heads/feature", + Branch: "feature", + HeadSHA: headSHA, + }) + if err == nil { + t.Fatal("a repository-relative pre_push_check script resolved against the pushed worktree; a contributor's own branch could shadow the trusted check") + } +} + +// TestRunConfiguredPrePushCheck_UsesLivePRBaseBranch pins the same precedence +// the CI step already applies (AGENTS.md, pr.base_branch): once a pull +// request exists, its actual forge base branch is authoritative over a +// since-changed pr.base_branch, because the check exists to describe the real +// target of the pull request the guard protects, not a hypothetical one a +// later config edit selected. +func TestRunConfiguredPrePushCheck_UsesLivePRBaseBranch(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, headSHA, config.Commands{}) + prURL := "https://github.com/test/repo/pull/42" + sctx.Run.PRURL = &prURL + sctx.Config.PR.BaseBranch = "main" + env, _ := fakeGHWithBase(t, prURL, "develop") + sctx.Env = env + + probe := newPrePushProbe(t, dir, "refs/heads/feature", 0) + sctx.Config.PrePushCheck = probe.command + + if err := runConfiguredPrePushCheck(sctx, forcePushDecision{remoteSHA: strings.Repeat("a", 40)}, prePushTarget{ + Ref: "refs/heads/feature", + Branch: "feature", + BaseBranch: prePushBaseBranch(sctx), + HeadSHA: headSHA, + }); err != nil { + t.Fatal(err) + } + + fields := probe.mustRun(t) + if fields["base_branch"] != "develop" { + t.Fatalf("pre_push_check base_branch = %q, want the pull request's live base %q, not the stale configured %q", fields["base_branch"], "develop", "main") + } +} + +// TestRunConfiguredPrePushCheck_UsesFreshlyListedBaseBranchOnNonGitHubForge +// pins the same live-over-configured precedence as +// TestRunConfiguredPrePushCheck_UsesLivePRBaseBranch, but for a provider that +// has no dedicated scm.PRBaseBranchReader (GitLab). The base branch still +// must not come from the stale configured value: FindPR's own listing already +// carries the merge request's real target branch, and that data must not be +// discarded just because no GitHub-only follow-up lookup exists for it. +func TestRunConfiguredPrePushCheck_UsesFreshlyListedBaseBranchOnNonGitHubForge(t *testing.T) { + t.Parallel() + dir, baseSHA, headSHA := setupGitRepo(t) + sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, headSHA, config.Commands{}) + sctx.Repo.UpstreamURL = "https://gitlab.com/test/repo.git" + sctx.Config.PR.BaseBranch = "main" + mrJSON := `{"iid":42,"web_url":"https://gitlab.com/test/repo/-/merge_requests/42","state":"opened","target_branch":"develop"}` + env, _ := fakeGlab(t, mrJSON) + sctx.Env = env + + probe := newPrePushProbe(t, dir, "refs/heads/feature", 0) + sctx.Config.PrePushCheck = probe.command + + if err := runConfiguredPrePushCheck(sctx, forcePushDecision{remoteSHA: strings.Repeat("a", 40)}, prePushTarget{ + Ref: "refs/heads/feature", + Branch: "feature", + BaseBranch: prePushBaseBranch(sctx), + HeadSHA: headSHA, + }); err != nil { + t.Fatal(err) + } + + fields := probe.mustRun(t) + if fields["base_branch"] != "develop" { + t.Fatalf("pre_push_check base_branch = %q, want the merge request's live target %q, not the stale configured %q", fields["base_branch"], "develop", "main") + } +} + +func TestPRNumberFromURL(t *testing.T) { + t.Parallel() + tests := map[string]string{ + "https://github.com/owner/name/pull/1234": "1234", + "https://gitlab.com/group/name/-/merge_requests/42/": "42", + "https://example.com/owner/name/pulls/9": "9", + "https://github.com/owner/name/pull/": "", + "https://github.com/owner/name": "", + "": "", + } + for input, want := range tests { + if got := prNumberFromURL(input); got != want { + t.Errorf("prNumberFromURL(%q) = %q, want %q", input, got, want) + } + } +} diff --git a/internal/pipeline/steps/push.go b/internal/pipeline/steps/push.go index 8b9d08c10..09b9b0244 100644 --- a/internal/pipeline/steps/push.go +++ b/internal/pipeline/steps/push.go @@ -94,6 +94,20 @@ func (s *PushStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, e if err != nil { return nil, fmt.Errorf("push to %s: %w", pushTarget, err) } + + // A push that moves an already-existing remote branch can land underneath + // an open pull request an external merge process already owns, so give the + // repository its configured veto before any object moves. Unset (the + // default) is a no-op; creating the branch for the first time is skipped. + if err := runConfiguredPrePushCheck(sctx, decision, prePushTarget{ + Ref: ref, + Branch: branch, + BaseBranch: prePushBaseBranch(sctx), + HeadSHA: headBeingPushed, + }); err != nil { + return nil, err + } + switch { case decision.newBranch: // New branch: regular push (no force needed). diff --git a/internal/pipeline/steps/steps_test.go b/internal/pipeline/steps/steps_test.go index 08bd3463f..abe0036ed 100644 --- a/internal/pipeline/steps/steps_test.go +++ b/internal/pipeline/steps/steps_test.go @@ -142,6 +142,13 @@ func fakeGHHandler(args []string) { os.Exit(0) } if len(args) >= 2 && args[0] == "pr" && args[1] == "view" { + if jsonField, ok := fakeCLIFlagValue(args, "--json"); ok && jsonField == "baseRefName" { + if prBase == "" { + os.Exit(1) + } + fmt.Println(prBase) + os.Exit(0) + } if prURL != "" { fmt.Println(prURL) os.Exit(0) diff --git a/internal/scm/forgejo/forgejo.go b/internal/scm/forgejo/forgejo.go index b73f0cd4a..dfa49acb0 100644 --- a/internal/scm/forgejo/forgejo.go +++ b/internal/scm/forgejo/forgejo.go @@ -587,7 +587,7 @@ func (h *Host) normalizePull(pull pullRequest) (*scm.PR, error) { if pull.State != "open" && pull.State != "closed" { return nil, fmt.Errorf("forgejo-axi returned unknown PR state %q", pull.State) } - return &scm.PR{Number: strconv.Itoa(pull.Number), URL: pull.URL, HeadSHA: pull.HeadSHA}, nil + return &scm.PR{Number: strconv.Itoa(pull.Number), URL: pull.URL, HeadSHA: pull.HeadSHA, BaseBranch: pull.Base}, nil } func (h *Host) normalizeChecks(result checksResult) ([]scm.Check, error) { diff --git a/internal/scm/forgejo/forgejo_test.go b/internal/scm/forgejo/forgejo_test.go index c3bc85984..83f1e5cec 100644 --- a/internal/scm/forgejo/forgejo_test.go +++ b/internal/scm/forgejo/forgejo_test.go @@ -293,7 +293,7 @@ func TestPRLifecycleCommandsAndIdempotentCreate(t *testing.T) { host := newTestHost(recorder) found, err := host.FindPR(context.Background(), "feature/forgejo", "main") - if err != nil || found == nil || found.Number != "42" || found.URL != testPRURL || found.HeadSHA != testHeadSHA { + if err != nil || found == nil || found.Number != "42" || found.URL != testPRURL || found.HeadSHA != testHeadSHA || found.BaseBranch != "main" { t.Fatalf("FindPR() = (%+v, %v)", found, err) } created, err := host.CreatePR(context.Background(), "feature/forgejo", "main", scm.PRContent{Title: "Title", Body: "line one\nline two"}) diff --git a/internal/scm/gitea/gitea.go b/internal/scm/gitea/gitea.go index 2137ee045..787e84bd0 100644 --- a/internal/scm/gitea/gitea.go +++ b/internal/scm/gitea/gitea.go @@ -141,7 +141,7 @@ func (h *Host) FindPR(ctx context.Context, branch, base string) (*scm.PR, error) if base != "" && item.Base != base { continue } - return &scm.PR{Number: item.Index, URL: item.URL}, nil + return &scm.PR{Number: item.Index, URL: item.URL, BaseBranch: strings.TrimSpace(item.Base)}, nil } return nil, nil } diff --git a/internal/scm/gitea/gitea_test.go b/internal/scm/gitea/gitea_test.go index d9342a20e..3ff64e57d 100644 --- a/internal/scm/gitea/gitea_test.go +++ b/internal/scm/gitea/gitea_test.go @@ -153,19 +153,21 @@ func TestFindPRMatchesByHeadBranch(t *testing.T) { host := New(giteaTestCmdFactory(map[string]giteaTestResponse{ "tea pulls list --repo owner/repo --login work --fields index,title,state,url,head,base --output json": { stdout: `[{"index":"3","title":"other","state":"open","url":"https://gitea.example.com/owner/repo/pulls/3","head":"other-branch","base":"main"},` + - `{"index":"7","title":"mine","state":"open","url":"https://gitea.example.com/owner/repo/pulls/7","head":"feature/x","base":"main"}]`, + `{"index":"7","title":"mine","state":"open","url":"https://gitea.example.com/owner/repo/pulls/7","head":"feature/x","base":"develop"}]`, }, }), nil, "gitea.example.com", "work", "owner/repo") - pr, err := host.FindPR(context.Background(), "feature/x", "main") + // No base filter, so the returned BaseBranch can only have come from the + // listing's own "base" field, not from an input echoed back. + pr, err := host.FindPR(context.Background(), "feature/x", "") if err != nil { t.Fatalf("FindPR() error = %v", err) } if pr == nil { t.Fatal("FindPR() = nil, want PR") } - if pr.Number != "7" || pr.URL != "https://gitea.example.com/owner/repo/pulls/7" { - t.Fatalf("FindPR() = %+v, want PR #7", pr) + if pr.Number != "7" || pr.URL != "https://gitea.example.com/owner/repo/pulls/7" || pr.BaseBranch != "develop" { + t.Fatalf("FindPR() = %+v, want PR #7 targeting develop", pr) } }