mirror of
https://github.com/kunchenguid/no-mistakes.git
synced 2026-09-19 07:53:54 +08:00
fix(pipeline): safely publish CI repairs with proven continuity (#887)
* feat(ci): make post-repair revalidation opt-in A CI repair used to restart the whole pipeline at Review, so every repaired check cost another full Review, Test, Document, Lint, Push, and PR pass over the change. That is the most expensive single behavior the pipeline has, and it was unconditional. ci.revalidate_repairs selects between the two deliveries. It defaults to false: the repair is published immediately through publishRunHead - the same guarded path the Push step uses, so review-approved-head continuity, the force-with-lease anchor, remote verification, the push binding, and the gate-mirror update all still apply - and the CI monitor keeps watching the same run. Set true to keep the repair local, revoke the run's review approval, and restart validation at Review so no CI repair is published without having been reviewed. The key lives in the existing ci block, so it inherits that block's global config, trusted-default-branch-only sourcing, and repo-overrides-global precedence with no new mechanism. It is a *bool so an explicit project false overrides a global true rather than reading as unset. Both CI fix paths take the same decision: the automatic auto-fix round and the manual round a person authorizes at the CI gate. No-change retries, the durable fix-attempt budget, duplicate-check suppression, transient reruns, merge-conflict handling, force-push safety, custody, and cancellation are untouched. The CI step logs which policy is in force before its first poll. VISION.md gains the constraint this default follows from: cost is a user-visible property of the gate, so a design that clearly adds significant end-to-end latency or token consumption must be opt-in. * no-mistakes(review): Preserve CI repair continuity across rebases * no-mistakes(document): Correct stale CI repair documentation * no-mistakes(ci): Fixed late gate-mirror failures misreporting published CI repairs. Published repairs now continue CI monitoring with a warning after durable remote binding, while Push-step mirror failures still propagate. Added regression coverage. Verified targeted race tests, rebase tests, make lint, and git diff --check * no-mistakes(document): Document late gate-mirror failure handling * fix(ci): decide repair publication by provable continuity A CI repair was published whenever ci.revalidate_repairs was off, and a merge-conflict repair rebases, so its head is not a descendant of the reviewed head. The publication guard was relaxed for that case with a base-ancestry exception, and a repair that reset to the rebase base satisfied it: reproduced, the reviewed commits were force-pushed away while the pipeline reported success. The actor was the CI repair agent itself, so provenance cannot stand in for the proof either. One rule now decides delivery on every CI-fix path, automatic and manual, CI failure and merge conflict alike: a repair is published without revalidating only when its continuity with the reviewed, published head can be PROVEN; when it cannot, the repair revalidates from Review. Continuity is proven when the repaired head is the run's durable review-approved commit or a descendant of it, read through the same reviewApprovedHead accessor the publication guard enforces, so the decision to publish and the guard that permits the push cannot disagree. Every failure to establish it - unreadable run, missing or malformed approval, unverifiable ancestry - counts as unproven. ci.revalidate_repairs still sets the intent identically on every path: false publishes when it is provable, true revalidates outright. Merge-conflict repairs are not carved out; they simply always land in the cannot-be-proven half, because resolving a conflict changes the commit's patch-id and no content-based guard can separate a resolved rebase from a dropped one. They now revalidate rather than being refused, so conflict repair keeps working. The base-ancestry exception and its rewriteBase plumbing are deleted; assertReviewApprovedPushHead is descendant-only again with no exception. Regressions: a genuine conflict rebase revalidates and succeeds; a reset-to-base conflict repair revalidates, never reaches the remote, and the reviewed work survives; an ordinary provable repair still publishes with the flag off and still restarts with it on; a run with no review authority revalidates rather than publishing. * no-mistakes(review): Correct merge-conflict revalidation guidance across all surfaces * no-mistakes(document): Correct CI repair revalidation documentation * no-mistakes(lint): Regenerate no-mistakes skill documentation * fix(push): record a publication only once all of it has settled The gate-mirror update ran after the push binding and the recorded head, which forced a choice between two wrong answers on a mirror failure. Returning the error made the CI monitor call an already published repair failed, and its next attempt then saw a clean, already-advanced head as producing no changes. Swallowing the error left the gate behind the remote for good, and `no-mistakes rerun` resolves its starting head from the gate, so a later rerun silently omitted the published repair. Settle the gate mirror before recording anything durable. Publication is now all-or-nothing: remote push, gate mirror, push binding, and recorded head all land or none of them are recorded. A partial failure is simply retryable - the next attempt re-enters the same path, finds the remote already at this head via an up-to-date no-op push, and completes the publication once the mirror works. The CI-repair warning special case is gone with it. Also correct four surfaces that still said the default publishes every repair, when it publishes only a repair whose continuity is provable, and make the merge-conflict regression conflict for real: the base and the feature now edit the same line, so the resolved rebase genuinely changes the commit's patch-id rather than replaying cleanly. Regressions: an unsettled publication records nothing and the retry completes it; a real conflict rebase revalidates and succeeds; a reset-to-base conflict repair revalidates and the reviewed content survives on the remote. * no-mistakes(review): Make CI repair publication atomic and retryable * no-mistakes(document): Correct CI publication documentation * fix(ci): fire the unsettled-publication retry only on real evidence The retry added for a part-way publication triggered on any clean worktree whose HEAD differed from the run's recorded head. That is not evidence a publication was attempted: a fix agent that commits and then errors leaves exactly that state, and so does a fixture whose recorded head trails the branch. The retry then ran instead of the fix round and the repair agent was never called, which is what broke TestCIStep_BitbucketAutoFixUsesLivePRHeadSHAForLogs on CI. publishRepair now records the commit whose publication it began and could not finish, and clears it on success or when the repair revalidates instead. The retry fires only for that exact commit, so it still settles a stalled publication without spending another fix attempt, and never swallows a fix round. The marker is in memory only: after a daemon restart the next fix attempt settles it instead, at the cost of one attempt, which is the honest accounting rather than a durable claim the process cannot make. Regression: a differing head with no attempted publication still runs the fix agent and never logs the retry. * no-mistakes(review): Retry unsettled publications before evaluating CI checks * no-mistakes(document): Document CI publication retry evidence * fix(ci): bound the publication retry and order the revalidation write Two defects in the repair-publication path, both reported on the PR. The unsettled-publication retry costs no repair attempt by design, so a gate mirror that could never be settled retried on every poll until the CI idle timeout - seven days by default - while the repaired commit already sat on the remote. Bound it to three attempts, then park for a person with the published head named, because a rerun would resume from the stale gate head and omit the repair. CI is testing the published commit, so no fix agent can help; what a person needs is to know the local gate is behind. recordLocalRepair advanced the in-memory head before its durable write. A failed write left the monitor watching a head the run record did not know about, still carrying its old review approval, with the revalidation the call exists to trigger silently lost. Write durably first, then advance. Regressions: the retry stops at its bound and parks naming the published head while spending only one fix attempt; a failed revalidation write leaves both the live head and the review approval untouched. * no-mistakes(review): Persist and safely settle CI publication retries * no-mistakes(document): Clarify CI publication settlement documentation * no-mistakes(ci): Serialized the process-heavy CI repair fixture tests to prevent macOS subprocess exhaustion and indefinite fake-gh stalls. Verified with 3 repeated focused runs, 25 repetitions of the previously hanging case, and the full internal/pipeline/steps suite (passed in 113.6s) * test(ci): stop paying for a monitor loop to assert a delivery decision The macOS job timed out at the 600s package cap: the repair fixture tests each drove the full CI monitor loop, so every case spent provider polls and several subprocesses on a two-core runner to observe something commitRepair already reports. Serializing them made the wall clock worse, not better. Assert the delivery decision from commitRepair - Revalidate, the remote, the review approval, the push binding - and keep exactly one monitor-loop test for the wiring that is genuinely the monitor's: turning a held repair into a restart at Review and stating the policy in force. Four full monitor loops go away and the cases run in parallel again. Coverage is unchanged: every case still asserts the same observables, and the paths that are about the monitor - the fix round not being swallowed, the bounded publication retry - still drive Execute. * revert(ci): strip the unscoped publication-retry machinery Two things were added to this branch by auto-answered review findings that were never in its scope, and the captain has declined both. Removed, not repaired: - retryPendingRepair and its CI poll-loop call site, its bound, and its exhaustion gate. New runtime behaviour in the monitor. - The durable pending-publication state it needed: SetCIPendingPublication, the ci_pending_publish_head / ci_pending_publish_attempts columns and their migration, the StepResult fields, and the restore-on-CI-entry path. A persistence and schema addition. - publishRunHead's publicationProgress return, which existed only to tell that machinery whether the remote had been verified. Three reported defects went with the code that contained them rather than being fixed: a restored pending publication bypassed by a provider skip path, a marker write that could be lost, and a marker never consumed after a daemon restart. They are not reachable once the machinery is gone. Kept unchanged: the uniform provable-continuity rule, the corrected agent guidance and docs, the single-statement UpdateRunPublication, the reference docs that no longer describe the removed mirror warning, and the real merge-conflict regression. Tests that only exercised the removed machinery go with it. What survives still covers behaviour that survives, including that a partial publication records nothing and the next attempt completes it - which is the atomicity guarantee, not the retry. * no-mistakes(document): Deduplicate CI revalidation configuration guidance
This commit is contained in:
Kun Chen
@@ -42,7 +42,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), and the whole `ci` block 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, raise the maintainer-funded `ci.rerun_transient` budget, or turn off required post-repair revalidation. The operator's global CI settings are a separate, non-contributor surface that trusted repo values override. Enabling the commands opt-in must not drop the maintainer's own trusted values. When the project-settings 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`.
|
||||
|
||||
@@ -62,7 +62,10 @@ Full documentation: <https://kunchenguid.github.io/no-mistakes/>
|
||||
|
||||
Each step either passes on its own or stops with a **finding** for you to act on.
|
||||
Safe, mechanical fixes are applied automatically; anything that touches your intent is escalated for you to **approve**, **fix**, or **skip**.
|
||||
Nothing reaches the configured push target until every check is green.
|
||||
The initial change reaches the configured push target only after every local gate is green.
|
||||
|
||||
When CI itself fails, the pipeline repairs it and publishes that repair through the same guarded force-push path - but only when it can prove the repair builds on the head you already reviewed. When it cannot prove that, the repair goes back through Review before it is published, so unrelated history cannot replace the reviewed commit. Merge-conflict repairs rewrite history, so they always take that safer route.
|
||||
Set [`ci.revalidate_repairs: true`](https://kunchenguid.github.io/no-mistakes/reference/repo-config/#cirevalidate_repairs) if *every* CI repair must itself be reviewed, at the cost of another full pass over your change each time CI is repaired.
|
||||
|
||||
## Install
|
||||
|
||||
|
||||
@@ -13,6 +13,8 @@ Customization is welcome exactly as far as it strengthens what a pass means; no
|
||||
A person may explicitly skip steps for one run; a standing rule may never skip them on anyone's behalf.
|
||||
A gate that cannot run completely refuses loudly with guidance; it never degrades silently into a weaker check.
|
||||
Efficiency never buys itself a skipped check: when a shortcut fails, the gate falls back to the slower correct path instead of skipping the validation.
|
||||
Cost is a user-visible property of the gate, not an implementation detail: pipeline latency and total token spend are already among the first things users raise, so any design that clearly adds significant end-to-end latency or token consumption must be opt-in, or confined to the extremely rare cases where it is genuinely necessary.
|
||||
The default path buys the strongest guarantee it can at a price a user will keep paying; a stronger guarantee that costs another full pass over the change is offered, never imposed.
|
||||
|
||||
## Never lose work
|
||||
|
||||
|
||||
@@ -56,7 +56,7 @@ The pipeline is opinionated so that "passed the gate" has a stable meaning:
|
||||
- **Document after test** so docs are updated against code that's known to work.
|
||||
- **Lint last among local checks** so it doesn't churn over code that may still change.
|
||||
- **Push → PR → CI** happens after all local checks pass.
|
||||
A CI repair restarts the pipeline at Review, so the repaired commit passes the local checks before the Push step publishes it through the same overwrite protection.
|
||||
CI publishes a repair through the Push step's guarded path and keeps monitoring only when it can prove the repair descends from the reviewed head; otherwise the repair revalidates from Review before Push republishes it, which is what a merge-conflict repair always does. [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs) sets that intent: `false` (default) publishes when it is provable, `true` revalidates every repair.
|
||||
CI is the only step that talks to the outside world for validation.
|
||||
|
||||
## What each step can do
|
||||
|
||||
@@ -153,7 +153,7 @@ Successful outcomes also instruct the agent to summarize the run for the user.
|
||||
When the pipeline applied fixes, successful outcomes include a `fixes` table listing each fix so the agent can acknowledge what it missed and the user can review them.
|
||||
|
||||
If that PR later falls behind the default branch or hits a merge conflict - commonly because another PR merged first - the agent runs no command and must never hand-rebase.
|
||||
The CI monitor stays live in the background after checks pass, and when it sees an actual conflict it rebases onto the base, resolves it, restarts validation at Review, and re-pushes the branch through Push, so no agent or user action is needed.
|
||||
The CI monitor stays live in the background after checks pass, and when it sees an actual conflict it rebases onto the base, resolves it, revalidates from Review because rebasing cannot prove continuity with the reviewed head, and re-pushes the branch through Push, so no agent or user action is needed.
|
||||
A PR that is merely behind but still clean needs nothing either, since the platform merges it.
|
||||
The one exception is when that monitor is no longer running - the PR was closed, the run was aborted or superseded, it idle-timed-out, or its auto-fix attempts were exhausted - in which case the agent recovers with `no-mistakes rerun`, which cancels the stale monitor and re-runs the full pipeline including a deterministic rebase step.
|
||||
The agent must not use `no-mistakes axi run` to refresh a still-active PR: after `checks-passed` it reattaches to the running monitor with HEAD unchanged and returns the monitor output without rebasing.
|
||||
|
||||
@@ -91,7 +91,7 @@ git remote set-url origin git@github.com:parent-owner/repo.git
|
||||
no-mistakes init --fork-url git@github.com:your-user/repo.git
|
||||
```
|
||||
|
||||
With this setup, the Push step updates the fork, including after a CI repair restarts validation, while the PR and CI steps stay scoped to the parent repository.
|
||||
With this setup, pipeline pushes update the fork, including CI repairs whether they are published immediately or first revalidated under [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs), while the PR and CI steps stay scoped to the parent repository.
|
||||
The GitHub PR step opens PRs with a fork-qualified head such as `your-user:feature-branch`.
|
||||
Re-running `no-mistakes init` later preserves the stored fork URL unless you pass a new `--fork-url`.
|
||||
|
||||
|
||||
@@ -45,7 +45,7 @@ If the repo still contains a vendored skill copy written by an older no-mistakes
|
||||
The gate advertises Git push-option support, so you can skip steps for one push with `git push -o no-mistakes.skip=test,lint no-mistakes <branch>`.
|
||||
|
||||
For GitHub fork contributions, keep `origin` pointed at the parent repository and pass `--fork-url` with your fork remote URL.
|
||||
The Push step and rebase branch-sync use the fork, including when CI repair restarts validation and reaches Push again, while GitHub PR and CI commands stay scoped to the parent repository and create PRs with `--head <fork-owner>:<branch>`.
|
||||
The Push step, rebase branch-sync, and CI repair publication use the fork, including when [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs) sends a repair back through Push, while GitHub PR and CI commands stay scoped to the parent repository and create PRs with `--head <fork-owner>:<branch>`.
|
||||
Fork routing currently requires both `origin` and `--fork-url` to be GitHub remotes with owner/repo paths.
|
||||
|
||||
`--worktree-root` is for directory-scoped toolchain configuration (mise, direnv), which resolves by path ancestry and so never reaches a run worktree under `NM_HOME`.
|
||||
@@ -128,7 +128,7 @@ Backgrounding a call is fine for an agent harness, but the run never advances pa
|
||||
When the CI step is still monitoring an open PR and checks are green - or the trusted default-branch config declares [`no_ci: true`](/no-mistakes/reference/repo-config/#no_ci) with no registered checks - `axi run` exits successfully with `outcome: checks-passed` instead of waiting for a human merge. A generic empty check list without that declaration is not ready.
|
||||
Treat that as the agent stopping point: ask the user to review and merge the PR from the `help` line.
|
||||
If that PR later falls behind the default branch or hits a merge conflict, do not run `axi run`, `rerun`, or a manual rebase while the CI monitor is still running.
|
||||
The monitor auto-rebases onto the base, resolves actual conflicts, restarts validation at Review, and re-pushes the branch through Push; a PR that is merely behind but clean needs no command.
|
||||
The monitor auto-rebases onto the base, resolves actual conflicts, revalidates from Review because rebasing cannot prove continuity with the reviewed head, and re-pushes the branch through Push; a PR that is merely behind but clean needs no command.
|
||||
Use `no-mistakes rerun` only after that monitor is no longer running, such as a closed PR, aborted or superseded run, idle timeout, or exhausted CI auto-fix attempts.
|
||||
Successful outcomes (`checks-passed` and `passed`) also carry `help` instructions telling the agent to summarize the run.
|
||||
When the pipeline applied fixes, they include a `fixes` table and a `help` instruction to acknowledge the misses and list those fixes for the user's review.
|
||||
|
||||
@@ -75,6 +75,7 @@ auto_fix:
|
||||
|
||||
ci:
|
||||
rerun_transient: 0
|
||||
revalidate_repairs: false
|
||||
|
||||
commit:
|
||||
fix_message: "chore(no-mistakes-{{.Step}}): {{.Summary}}"
|
||||
@@ -363,7 +364,7 @@ Accepts any Go `time.ParseDuration` string: `30m`, `2h`, `4h30m`, etc.
|
||||
|
||||
This is an idle timeout, not an absolute deadline: every time the base branch advances, the monitor re-arms it.
|
||||
So an actively-updated green PR keeps its monitor no matter how long it stays open.
|
||||
If it later develops an actual GitHub, GitLab, Forgejo, or Azure DevOps merge conflict, the CI auto-fix path rebases it, restarts validation at Review, and publishes it through Push, while a clean behind PR needs no command.
|
||||
If it later develops an actual GitHub, GitLab, Forgejo, or Azure DevOps merge conflict, the CI auto-fix path rebases it, revalidates from Review because rebasing cannot prove continuity with the reviewed head, and publishes it through Push, while a clean behind PR needs no command.
|
||||
A genuinely idle/abandoned PR still parks at an approval gate after the timeout elapses.
|
||||
While that CI gate is parked, the daemon continues bounded read-only PR-state checks.
|
||||
If the PR is merged or closed externally, the stale gate completes automatically; an open, unknown, or temporarily unreachable PR remains parked for a user decision.
|
||||
@@ -574,6 +575,22 @@ Set `0` here to never spend someone else's CI minutes; this is the only place to
|
||||
|
||||
The per-repo [`ci.rerun_transient`](/no-mistakes/reference/repo-config/#cirerun_transient) overrides this value and owns the classification, the trust boundary, and every case that skips the rerun.
|
||||
|
||||
### ci.revalidate_repairs
|
||||
|
||||
The operator-level fallback for [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs), whose per-repository reference owns the repair-delivery semantics, safety rationale, and trust boundary.
|
||||
|
||||
| | |
|
||||
|---|---|
|
||||
| Type | `bool` |
|
||||
| Default | `false` |
|
||||
|
||||
```yaml
|
||||
ci:
|
||||
revalidate_repairs: false
|
||||
```
|
||||
|
||||
A value in the trusted repository config overrides this global value in both directions: an explicit repository `true` enables revalidation when this is `false`, and an explicit repository `false` disables opt-in revalidation when this is `true`. When the trusted repository config omits the key, this global value applies.
|
||||
|
||||
### commit.fix_message
|
||||
|
||||
Template for the subject of commits created by the Review, Test, Document, Lint, and CI repair paths.
|
||||
|
||||
@@ -18,7 +18,7 @@ Configured shell commands and one-shot agent subprocesses are scoped to their st
|
||||
When configured Test or Lint command output exceeds 64 KiB, the complete output remains in the authoritative step log while findings, IPC responses, and repair prompts receive a valid-UTF-8 head-and-tail projection capped at 64 KiB. The truncation marker reports the exact original and omitted byte counts and points to `no-mistakes axi logs --step <step> --full` for the complete output.
|
||||
Commits created by the shared Review, Test, Document, and Lint fix path, plus CI repair commits, use the configurable [`commit.fix_message`](/no-mistakes/reference/global-config/#commitfix_message) template.
|
||||
The shared correction commits, and the Push step's commit of leftover changes from a pipeline agent or formatter, are machine-authored records of pipeline output. Each is created with the complete local commit-hook family suppressed by combining `--no-verify` with an empty temporary `core.hooksPath` for that invocation, so `pre-commit`, `prepare-commit-msg`, `commit-msg`, and `post-commit` do not run. This lets a disposable run worktree commit a correction even when a tracked hook depends on generated untracked runtime files that do not exist there - the canonical case is `core.hooksPath=.husky` with a tracked hook that sources the absent `.husky/_/husky.sh`.
|
||||
The suppression is limited to those correction-commit invocations. It does not change the repository, Git configuration, or daemon environment; CI repair commits and all other commit paths keep normal hook behavior. The Review, Test, Document, Lint, Push, PR, and CI gates remain the authoritative checks on what these commits contain.
|
||||
The suppression is limited to those correction-commit invocations. It does not change the repository, Git configuration, or daemon environment; CI repair commits and all other commit paths keep normal hook behavior. Pipeline gates remain authoritative; whether a CI repair returns through the local gates before publication is controlled by [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs).
|
||||
Agent roles that can write, repair, or review tests reject tests whose only evidence is matching implementation source text, tokens, syntax, or incidental snapshots.
|
||||
They instead require an executable interface or a typed or normalized semantic model that proves observable behavior.
|
||||
Reading a file remains valid when that file is itself an owned output or data contract, and deterministic tests may inspect the final emitted agent prompt as a generated interface; model interpretation is reserved for development-only evaluation.
|
||||
@@ -200,8 +200,8 @@ Pushes the validated branch to the configured push target.
|
||||
- Pushes the exact verified commit SHA instead of mutable worktree `HEAD`
|
||||
- Treats the branch as already pushed when the remote already points at that verified commit
|
||||
- Uses regular push for new branches
|
||||
- Updates the run's head SHA in the database to the exact commit delivered
|
||||
- When the local gate mirror exists, advances its branch ref to the delivered commit when that does not rewind a newer gate submission; skips a missing mirror and fails on a divergent ref so subsequent pushes to the gate proxy remain fast-forwardable after pipeline rebases
|
||||
- Only after the remote and gate mirror settle, atomically records the exact delivered commit as both the run head and successful-push binding; until that database write succeeds, the durable database head and binding remain unchanged, so a partial failure records nothing and is safe to re-enter
|
||||
|
||||
A remote branch can move without being rejected when all remote commits are already represented in the validated head, or when a run is intentionally rewriting history it already knew about.
|
||||
Any other out-of-band commit stops the push instead of being overwritten.
|
||||
@@ -298,9 +298,11 @@ Monitors PR health after creation and auto-fixes CI failures. Mergeability polli
|
||||
- When a provider-attributed failure is the only remaining issue, pauses for user approval without spending an auto-fix attempt if no rerun is going to replace it. This includes a check cancelled again after its rerun and a detected GitHub setup failure that persists after its budget. On the default budget of `0`, once the budget is spent, or on a provider with no rerun API, a cancelled or stopped check itself reaches that gate. These outcomes are terminal and will not resolve on their own, there is nothing for the fix agent to repair, and the PR must not look green either
|
||||
- Keeps waiting, rather than pausing, while any check can still finish on its own, so a cancellation observed alongside a running check is decided only once the rollup has stopped moving
|
||||
- Never re-runs checks across a head change: if the published branch head no longer equals the commit the run delivered, the step clears any ready-to-merge signal and pauses for user approval with the expected and observed commits, because re-running checks would certify a revision this run never produced
|
||||
- On CI failure: fetches failed job logs (GitHub via `gh run view --log-failed`, GitLab via `glab ci trace`, Forgejo via the exact native check target plus `forgejo-axi run view --log-failed` when runtime routes are available, Bitbucket Cloud via failed pipeline step logs; Azure DevOps has no first-class build-log command, so the agent fixes from the failing-check list without logs), sends them to the agent with user intent when available, and, if the agent produces changes, commits them locally with [`commit.fix_message`](/no-mistakes/reference/global-config/#commitfix_message), re-runs validation from Review, and publishes them through the Push step's force-push safety guard. Forgejo status gating remains active when logs are unsupported or unavailable
|
||||
- On CI failure: fetches failed job logs (GitHub via `gh run view --log-failed`, GitLab via `glab ci trace`, Forgejo via the exact native check target plus `forgejo-axi run view --log-failed` when runtime routes are available, Bitbucket Cloud via failed pipeline step logs; Azure DevOps has no first-class build-log command, so the agent fixes from the failing-check list without logs), sends them to the agent with user intent when available, and, if the agent produces changes, commits them with [`commit.fix_message`](/no-mistakes/reference/global-config/#commitfix_message). What happens next follows one rule on every CI-fix path: a repair is published without revalidating only when its continuity with the reviewed, published head can be proven, meaning the repaired head is the run's review-approved commit or a descendant of it. A provable repair is published immediately through the Push step's own guarded force-push path and the monitor keeps watching the same run; anything else is held locally, the run's review approval is revoked, and validation restarts from Review so Push republishes it only after Review approves it. [`ci.revalidate_repairs`](/no-mistakes/reference/repo-config/#cirevalidate_repairs) sets the intent identically on every path: `false` (default) publishes when it is provable, `true` revalidates outright. A merge-conflict repair rebases, so its continuity is never provable and it always revalidates. Forgejo status gating remains active when logs are unsupported or unavailable
|
||||
- On GitHub, includes unresolved review-thread comments from supported review bots (currently Greptile) in CI repair prompts when an auto-fix attempt starts; the comments are framed as untrusted external data and the rendered section is capped at 32 KiB
|
||||
- Preserves steps already skipped for the run when restarting validation, including after recovery from a daemon restart
|
||||
- States the configured repair policy in the step log before the first poll, so a run's log says which of the two paths a repair would take without cross-referencing the config in force at the time
|
||||
- Settles the local gate mirror before atomically recording the published head and push binding, so a publication that stalls part way records nothing: the run stays on its pre-repair head and the next fix attempt re-enters the same path, finds the remote already at that commit, and completes it
|
||||
- Whenever a repair revalidates - either because the setting requires it or because continuity cannot be proven - restarts at Review only: Intent and Rebase keep their results, steps already skipped for the run stay skipped, the run id is unchanged, and the durable auto-fix attempt count carries across. Earlier cycles remain in the run's round history; the step's own status shows the latest cycle
|
||||
- Bounds that CI-fix agent with [`agent_timeout`](/no-mistakes/reference/global-config/#agent_timeout): an expired budget cancels the agent and fails the attempt with a timeout diagnostic rather than leaving the run active indefinitely, and a late successful return after the deadline is not committed
|
||||
- If the CI-fix agent exhausts that budget, pauses for user approval instead of re-issuing the same request on the next poll. A budget burn is not transient - repeating it costs another full budget - so the remaining auto-fix attempts are left for the user to spend deliberately with a fix response. The finding carries the measured timeout diagnostic and, when the timed-out agent left uncommitted work in the run worktree, that worktree's path. Ordinary (non-timeout) fix failures keep retrying as before
|
||||
- On GitHub, GitLab, Forgejo, or Azure DevOps merge conflict: asks the agent to rebase onto the latest PR base branch tip and make the smallest correct root-cause fix for the conflicts, using user intent when available
|
||||
|
||||
@@ -8,7 +8,7 @@ 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/<default>` 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`, `ci.revalidate_repairs`, and `test.evidence.branch` only from that trusted copy.
|
||||
`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.
|
||||
@@ -70,9 +70,11 @@ auto_fix:
|
||||
lint: 5
|
||||
ci: 3
|
||||
|
||||
# Read only from the trusted default branch: each rerun is another workflow run.
|
||||
# Read only from the trusted default branch: each rerun is another workflow run,
|
||||
# and revalidation decides whether a CI repair may ship without review.
|
||||
ci:
|
||||
rerun_transient: 0
|
||||
revalidate_repairs: false
|
||||
|
||||
commit:
|
||||
fix_message: "chore(no-mistakes-{{.Step}}): {{.Summary}}"
|
||||
@@ -416,6 +418,59 @@ Reruns are skipped when:
|
||||
- The check's details link names nothing the provider can re-run, for example a third-party status pointing at an external dashboard, or a link under a workflow run that names no job the API accepts. A link naming one job re-runs that job; a cancelled check naming only the workflow run re-runs the whole workflow, while other run-only links re-run failed jobs; an unrecognized link is widened into neither.
|
||||
- The published branch head no longer equals the commit the run delivered. That case terminates with the expected and observed commits instead: re-running checks against a different head would certify a revision this run never produced. See [pipeline steps: CI](/no-mistakes/reference/pipeline-steps/#ci).
|
||||
|
||||
### ci.revalidate_repairs
|
||||
|
||||
Whether every CI repair must re-pass the pipeline before it is published, or only the ones whose continuity with the reviewed head cannot be proven.
|
||||
|
||||
| | |
|
||||
|---|---|
|
||||
| Type | `bool` |
|
||||
| Default | `false` |
|
||||
| Trust | Read only from the trusted default branch |
|
||||
|
||||
```yaml
|
||||
ci:
|
||||
revalidate_repairs: true
|
||||
```
|
||||
|
||||
One rule decides how every CI repair is delivered, on every CI-fix path - automatic and manual, CI failure and merge conflict alike:
|
||||
|
||||
> A repair is published without revalidating only when its continuity with the reviewed, published head can be **proven**. When that continuity cannot be proven, the repair revalidates from Review.
|
||||
|
||||
Continuity is proven when the repaired head is the run's durably review-approved commit or a descendant of it. That is the same fact the Push step's publication guard enforces, so the decision to publish and the guard that permits the push can never disagree.
|
||||
|
||||
`revalidate_repairs` sets the intent, identically on every path:
|
||||
|
||||
- **`false` (default)** asks to publish when it is safe to. A repair that builds on the reviewed head - the ordinary case, where the fix agent adds a commit - is committed and published immediately through the same guarded path the [Push step](/no-mistakes/reference/pipeline-steps/#push) uses (review-approved-head continuity, the force-with-lease anchor, remote verification, and the durable push binding all still apply), and the CI monitor keeps watching the same run for the new head. One repair costs one agent round.
|
||||
- **`true`** asks for revalidation outright: every repair is kept local, the run's review approval is revoked, and validation restarts at Review so the repaired head re-passes Review, Test, Document, and Lint before Push republishes it.
|
||||
|
||||
CI repair publication uses the same settlement order as Push. The [CI step reference](/no-mistakes/reference/pipeline-steps/#ci) owns the publication and retry behavior.
|
||||
|
||||
**Merge-conflict repairs always revalidate, under either setting.** They are not carved out - they simply always land in the cannot-be-proven half. A conflict repair rebases, so the repaired head is never a descendant of the reviewed head; resolving a conflict changes the commit's patch-id; and no content-based guard can separate "rebased and resolved" from "dropped the work". Revalidating is what keeps that safe: the rewritten head is not published until Review has approved it, so the reviewed commits stay on the remote in the meantime.
|
||||
|
||||
Provenance is deliberately not accepted as a substitute for that proof. In the reproduction this rule exists for, the repair that deleted a reviewed commit was authored by no-mistakes' own CI repair agent: it reset to the rebase base, left a clean tree, and the pipeline reported success while the remote lost the work. Who wrote a repair says nothing about what it did to the reviewed commits.
|
||||
|
||||
The tradeoff `true` buys is cost against an unreviewed repair:
|
||||
|
||||
| | `false` (default) | `true` |
|
||||
|---|---|---|
|
||||
| Ordinary repair that builds on the reviewed head | published immediately, one agent round | revalidated: one agent round plus a full Review, Test, Document, Lint, Push, PR pass |
|
||||
| Merge-conflict repair | revalidated | revalidated |
|
||||
| Ordinary repair is reviewed before it reaches the PR | no | yes |
|
||||
| Steps that re-run when a repair revalidates | Review onward; Intent and Rebase do not | same |
|
||||
| Run identity | unchanged; a restart is a same-run rewind | same |
|
||||
|
||||
Turn it on where even an ordinary unreviewed CI repair is unacceptable.
|
||||
The concrete case this exists for: when a review bot posts product-behavior findings as a failing check, the fix agent treats them as CI failures and can reverse what the change was supposed to do.
|
||||
On [firstmate#3250](https://github.com/kunchenguid/firstmate/pull/3250) a CI repair made a `--changed` test run serial by default, contradicting the change's stated intent; the restarted Review caught it and reversed it. Without revalidation that repair would have shipped.
|
||||
That is the safety this option buys, and the reason it is offered rather than removed.
|
||||
|
||||
This value is read only from the trusted default-branch copy of this file, like `ci.rerun_transient` and `disable_project_settings`.
|
||||
A pushed branch cannot turn a maintainer's revalidation requirement off for its own repairs, and cannot turn it on either.
|
||||
|
||||
A value set here always wins over the operator's own [`ci.revalidate_repairs`](/no-mistakes/reference/global-config/#cirevalidate_repairs), in both directions: `true` here enables revalidation even when the global value is `false`, and an explicit `false` here opts out even when the global value is `true`.
|
||||
With no trusted copy of this file, the operator's global value applies, then the built-in default of `false`.
|
||||
|
||||
### commit.fix_message
|
||||
|
||||
Override the auto-fix commit subject template for this repository.
|
||||
|
||||
@@ -4,15 +4,15 @@ package cli
|
||||
// when `axi run` returns `checks-passed`: what to do if that PR later falls
|
||||
// behind the default branch or hits a merge conflict (commonly because another
|
||||
// PR merged first). The live CI monitor keeps running after checks pass and
|
||||
// auto-rebases onto the base, resolves the conflict, revalidates, and re-pushes
|
||||
// itself, so the agent runs no command and never hand-rebases. `no-mistakes
|
||||
// auto-rebases onto the base, resolves the conflict, revalidates from Review,
|
||||
// and re-pushes itself, so the agent runs no command and never hand-rebases. `no-mistakes
|
||||
// rerun` is only the recovery for a monitor that is no longer running.
|
||||
//
|
||||
// This same guidance is mirrored in the skill body (internal/skill/skill.go)
|
||||
// and the published agents guide (docs/.../guides/agents.md); the repo treats
|
||||
// agent-driving guidance as a multi-surface contract, and
|
||||
// TestStaleMonitorGuidance_SyncedAcrossSurfaces keeps the three in sync.
|
||||
const staleMonitorGuidance = "If this PR later falls behind the default branch or hits a merge conflict, the CI monitor rebases onto the base, resolves it, restarts validation at Review, and re-pushes it through Push automatically - run no command and never hand-rebase. Only when that monitor is no longer running (PR closed, run aborted, idle-timeout, or auto-fix exhausted) recover with `no-mistakes rerun`."
|
||||
const staleMonitorGuidance = "If this PR later falls behind the default branch or hits a merge conflict, the CI monitor rebases onto the base, resolves it, revalidates from Review because rebasing cannot prove continuity with the reviewed head, and re-pushes it through Push automatically - run no command and never hand-rebase. Only when that monitor is no longer running (PR closed, run aborted, idle-timeout, or auto-fix exhausted) recover with `no-mistakes rerun`."
|
||||
|
||||
// preserveGateFixCommitsGuidance is the canonical, point-of-use guidance an
|
||||
// agent reads when it needs to make another fix after a gate round already
|
||||
|
||||
@@ -23,6 +23,8 @@ import (
|
||||
// hand-rebases, and `no-mistakes rerun` is only the dead-monitor recovery.
|
||||
var canonicalStaleMonitorPhrases = []string{
|
||||
"never hand-rebase",
|
||||
"revalidates from Review",
|
||||
"cannot prove continuity with the reviewed head",
|
||||
"re-pushes",
|
||||
"no-mistakes rerun",
|
||||
}
|
||||
|
||||
@@ -0,0 +1,157 @@
|
||||
package config
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// resolveRevalidateRepairs runs one global YAML document and one repository
|
||||
// YAML document through the real loaders, the trusted/pushed effective-config
|
||||
// rule, and Merge, and reports the policy the pipeline would actually see.
|
||||
// Going through the loaders (rather than constructing structs) is what makes
|
||||
// these assertions cover YAML key naming, strict-field checking, and the
|
||||
// pointer semantics that let an explicit false override an inherited true.
|
||||
func resolveRevalidateRepairs(t *testing.T, globalYAML, trustedRepoYAML, pushedRepoYAML string) bool {
|
||||
t.Helper()
|
||||
global, err := LoadGlobalFromBytes([]byte(globalYAML))
|
||||
if err != nil {
|
||||
t.Fatalf("LoadGlobalFromBytes(%q): %v", globalYAML, err)
|
||||
}
|
||||
trusted, err := LoadRepoFromBytes([]byte(trustedRepoYAML))
|
||||
if err != nil {
|
||||
t.Fatalf("LoadRepoFromBytes(trusted %q): %v", trustedRepoYAML, err)
|
||||
}
|
||||
pushed, err := LoadRepoFromBytes([]byte(pushedRepoYAML))
|
||||
if err != nil {
|
||||
t.Fatalf("LoadRepoFromBytes(pushed %q): %v", pushedRepoYAML, err)
|
||||
}
|
||||
return Merge(global, EffectiveRepoConfig(pushed, trusted, false)).CI.RevalidateRepairs
|
||||
}
|
||||
|
||||
func TestCIRevalidateRepairs_GlobalAndProjectPrecedence(t *testing.T) {
|
||||
t.Parallel()
|
||||
const on = "ci:\n revalidate_repairs: true\n"
|
||||
const off = "ci:\n revalidate_repairs: false\n"
|
||||
const unset = "{}\n"
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
global string
|
||||
trusted string
|
||||
want bool
|
||||
why string
|
||||
}{
|
||||
{
|
||||
name: "absent_everywhere_defaults_to_publishing_the_repair", global: unset, trusted: unset, want: false,
|
||||
why: "the expensive full revalidation must never be paid for by a config that never asked for it",
|
||||
},
|
||||
{
|
||||
name: "global_true_selects_revalidation", global: on, trusted: unset, want: true,
|
||||
why: "an operator can turn it on machine-wide",
|
||||
},
|
||||
{
|
||||
name: "project_true_selects_revalidation", global: unset, trusted: on, want: true,
|
||||
why: "a repository can require it without the operator configuring anything",
|
||||
},
|
||||
{
|
||||
name: "project_false_overrides_global_true", global: on, trusted: off, want: false,
|
||||
why: "an explicit project false is a real value, not an absent one",
|
||||
},
|
||||
{
|
||||
name: "project_true_overrides_global_false", global: off, trusted: on, want: true,
|
||||
why: "precedence runs in the normal direction in both directions",
|
||||
},
|
||||
{
|
||||
name: "global_true_survives_a_project_that_sets_only_the_rerun_budget", global: on,
|
||||
trusted: "ci:\n rerun_transient: 2\n", want: true,
|
||||
why: "setting one key in the ci block must not silently clear the other",
|
||||
},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
if got := resolveRevalidateRepairs(t, tc.global, tc.trusted, unset); got != tc.want {
|
||||
t.Fatalf("CI.RevalidateRepairs = %v, want %v: %s", got, tc.want, tc.why)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A pushed branch must not be able to turn its maintainer's revalidation
|
||||
// requirement off for its own repairs, so the whole ci block is read from the
|
||||
// trusted default-branch copy. Both directions are pinned: a pushed false
|
||||
// cannot weaken a trusted true, and a pushed true cannot impose cost the
|
||||
// maintainer did not ask for either.
|
||||
func TestCIRevalidateRepairs_TrustedOnly(t *testing.T) {
|
||||
t.Parallel()
|
||||
const on = "ci:\n revalidate_repairs: true\n"
|
||||
const off = "ci:\n revalidate_repairs: false\n"
|
||||
const unset = "{}\n"
|
||||
|
||||
if got := resolveRevalidateRepairs(t, unset, on, off); !got {
|
||||
t.Error("a pushed branch disabled the trusted revalidation requirement")
|
||||
}
|
||||
if got := resolveRevalidateRepairs(t, unset, off, on); got {
|
||||
t.Error("a pushed branch enabled revalidation the trusted config declined")
|
||||
}
|
||||
if got := resolveRevalidateRepairs(t, unset, unset, on); got {
|
||||
t.Error("a pushed branch enabled revalidation with no trusted config at all")
|
||||
}
|
||||
}
|
||||
|
||||
// The opt-in is a security-relevant boundary, so it stays trusted-only even
|
||||
// when a repository has taken the allow_repo_commands opt-in that hands the
|
||||
// pushed branch control of what executes.
|
||||
func TestCIRevalidateRepairs_TrustedOnlyEvenWithRepoCommandsOptIn(t *testing.T) {
|
||||
t.Parallel()
|
||||
trusted, err := LoadRepoFromBytes([]byte("allow_repo_commands: true\nci:\n revalidate_repairs: true\n"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
pushed, err := LoadRepoFromBytes([]byte("ci:\n revalidate_repairs: false\n"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
global, err := LoadGlobalFromBytes([]byte("{}\n"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !Merge(global, EffectiveRepoConfig(pushed, trusted, true)).CI.RevalidateRepairs {
|
||||
t.Error("allow_repo_commands let a pushed branch disable revalidation")
|
||||
}
|
||||
}
|
||||
|
||||
func TestCIRevalidateRepairs_RejectsNonBooleanGlobalValue(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, err := LoadGlobalFromBytes([]byte("ci:\n revalidate_repairs: sometimes\n"))
|
||||
if err == nil {
|
||||
t.Fatal("expected a non-boolean revalidate_repairs to be rejected")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "parse global config") {
|
||||
t.Fatalf("error = %v, want a global config parse failure", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCIRevalidateRepairs_RejectsNonBooleanRepoValue(t *testing.T) {
|
||||
t.Parallel()
|
||||
if _, err := LoadRepoFromBytes([]byte("ci:\n revalidate_repairs: sometimes\n")); err == nil {
|
||||
t.Fatal("expected a non-boolean revalidate_repairs to be rejected")
|
||||
}
|
||||
}
|
||||
|
||||
// The shipped default config must document the key it ships, and must show the
|
||||
// default rather than an aspirational value.
|
||||
func TestCIRevalidateRepairs_ShippedDefaultConfigParsesAndKeepsTheDefault(t *testing.T) {
|
||||
t.Parallel()
|
||||
cfg, err := LoadGlobalFromBytes([]byte(defaultConfigYAML))
|
||||
if err != nil {
|
||||
t.Fatalf("shipped example global config does not parse: %v", err)
|
||||
}
|
||||
repo, err := LoadRepoFromBytes([]byte("{}\n"))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if Merge(cfg, repo).CI.RevalidateRepairs {
|
||||
t.Error("the shipped default config enables revalidation; the documented default is false")
|
||||
}
|
||||
}
|
||||
@@ -76,6 +76,20 @@ const (
|
||||
// with an agent round, but they are not free: each one keeps the monitor
|
||||
// polling the same commit, so the budget stays small by construction.
|
||||
MaxCIRerunTransient = 5
|
||||
// DefaultCIRevalidateRepairs is the policy the CI step uses when
|
||||
// ci.revalidate_repairs is unset. It is false because restarting the whole
|
||||
// pipeline at Review for every CI repair is the single most expensive
|
||||
// thing the pipeline can do to a run: it replays Review, Test, Document,
|
||||
// Lint, Push, and PR against the repaired head, so one repair costs
|
||||
// another full agent pass over the whole change. VISION.md's cost
|
||||
// constraint makes that opt-in.
|
||||
//
|
||||
// False does not mean "always publish". It means "publish when it is
|
||||
// provably safe to": a repair is published only when its head is the run's
|
||||
// review-approved commit or a descendant of it, and any repair that cannot
|
||||
// show that - every merge-conflict repair, since a rebase rewrites the
|
||||
// head - revalidates from Review instead. See CI.RevalidateRepairs.
|
||||
DefaultCIRevalidateRepairs = false
|
||||
// DefaultEvalMaxCases caps the auto-captured local eval corpus. Cases
|
||||
// share one object pool per repository, so the marginal cost of a case is
|
||||
// its JSON records plus the objects its commits actually introduced, not a
|
||||
@@ -454,6 +468,10 @@ type AutoFixRaw struct {
|
||||
// Pointer fields distinguish "not set" (nil) from "set to 0" (disabled).
|
||||
type CIRaw struct {
|
||||
RerunTransient *int `yaml:"rerun_transient"`
|
||||
// RevalidateRepairs is a pointer so an explicit `false` in a repository's
|
||||
// config can override a global `true`, which a plain bool could not
|
||||
// express (it would be indistinguishable from "not set").
|
||||
RevalidateRepairs *bool `yaml:"revalidate_repairs"`
|
||||
}
|
||||
|
||||
// CI holds the resolved CI-step settings.
|
||||
@@ -464,6 +482,33 @@ type CI struct {
|
||||
// an approval gate. 0 disables reruns and restores the behavior of
|
||||
// escalating every failure on sight.
|
||||
RerunTransient int
|
||||
// RevalidateRepairs selects what happens after the CI step's fix agent
|
||||
// produces a real repair commit.
|
||||
//
|
||||
// One rule decides delivery on every CI-fix path, automatic and manual, CI
|
||||
// failure and merge conflict alike: a repair is published without
|
||||
// revalidating only when its continuity with the reviewed, published head
|
||||
// can be PROVEN - the repaired head is the run's review-approved commit or
|
||||
// a descendant of it - and revalidates from Review when it cannot.
|
||||
//
|
||||
// false (default): a provable repair is published through the same guarded
|
||||
// force-push path the Push step uses - review-approved-head continuity, the
|
||||
// force-with-lease anchor, remote verification, the gate mirror, and the
|
||||
// push binding all still apply, and none of it is recorded until all of it
|
||||
// succeeds - and the CI monitor keeps watching the same run for the new
|
||||
// head. The run's review approval stays valid because the repair descends
|
||||
// from the approved head. A repair whose continuity cannot be proven takes
|
||||
// the revalidating path below instead; a merge-conflict repair always does,
|
||||
// because a rebase makes its head a non-descendant and resolving a conflict
|
||||
// changes the commit's patch-id, so no content-based guard can tell a
|
||||
// resolved rebase from one that dropped the work.
|
||||
//
|
||||
// true: the repair is kept local, the run's review approval is revoked,
|
||||
// and the pipeline restarts at Review so the repaired head re-passes
|
||||
// Review, Test, Document, and Lint before Push republishes it. Safer, and
|
||||
// materially more expensive in wall-clock time and tokens - which is why
|
||||
// it is opt-in (see VISION.md).
|
||||
RevalidateRepairs bool
|
||||
}
|
||||
|
||||
// AutoFix holds resolved per-step auto-fix attempt limits.
|
||||
@@ -861,6 +906,18 @@ auto_fix:
|
||||
# default branch overrides this value.
|
||||
ci:
|
||||
rerun_transient: 0
|
||||
# Whether EVERY CI repair must re-pass the whole pipeline before it is
|
||||
# published, or only the ones whose continuity with the reviewed head cannot
|
||||
# be proven. Defaults to false: a repair that descends from the reviewed head
|
||||
# is published through the same guarded force-push path the Push step uses and
|
||||
# CI keeps monitoring, so one repair costs one agent round. A repair that
|
||||
# cannot show that ancestry revalidates from Review anyway - a merge-conflict
|
||||
# repair always does, because rebasing rewrites the head. Set true to restart
|
||||
# validation at Review for every repair - safer, and it pays for another full
|
||||
# pipeline pass in wall clock and tokens every time CI is repaired. A
|
||||
# repository that sets ci.revalidate_repairs on its own default branch
|
||||
# overrides this value.
|
||||
revalidate_repairs: false
|
||||
|
||||
# Auto-fix commit subject template. Available variables: {{.Step}} and {{.Summary}}.
|
||||
# Repo config may override this value.
|
||||
@@ -2121,10 +2178,14 @@ func EffectiveRepoConfig(pushed, trusted *RepoConfig, allowRepoCommands bool) *R
|
||||
// default-branch copy so a pushed branch cannot self-declare no-CI and
|
||||
// bypass checks that the default branch still expects.
|
||||
effective.NoCI = trusted.NoCI
|
||||
// ci.rerun_transient spends the maintainer's resources rather than the
|
||||
// contributor's: every rerun is another provider-side workflow run
|
||||
// billed to the repository. It is trusted-only for that reason, so a
|
||||
// pushed branch cannot raise its own rerun budget to the cap.
|
||||
// The whole ci block is trusted-only. ci.rerun_transient spends the
|
||||
// maintainer's resources rather than the contributor's: every rerun is
|
||||
// another provider-side workflow run billed to the repository, so a
|
||||
// pushed branch must not be able to raise its own rerun budget to the
|
||||
// cap. ci.revalidate_repairs is a validation boundary in the same
|
||||
// sense: it decides whether a CI repair commit must re-pass Review
|
||||
// before it is published, so a pushed branch must not be able to turn
|
||||
// the maintainer's revalidation requirement off for its own repairs.
|
||||
effective.CI = trusted.CI
|
||||
// test.evidence.branch names the git ref evidence commits are pushed
|
||||
// to with the maintainer's credentials. It is trusted-only so a pushed
|
||||
@@ -2386,8 +2447,14 @@ func autoFixDefaults() AutoFix {
|
||||
// safe baseline is to escalate rather than risk restarting a job a maintainer
|
||||
// or a concurrency rule deliberately stopped. Repositories that know their
|
||||
// cancellations are provider-side opt in via ci.rerun_transient.
|
||||
// Post-repair revalidation is off for the reason recorded on
|
||||
// DefaultCIRevalidateRepairs: it is the pipeline's most expensive single
|
||||
// behavior, so it is opted into rather than paid for by default.
|
||||
func ciDefaults() CI {
|
||||
return CI{RerunTransient: DefaultCIRerunTransient}
|
||||
return CI{
|
||||
RerunTransient: DefaultCIRerunTransient,
|
||||
RevalidateRepairs: DefaultCIRevalidateRepairs,
|
||||
}
|
||||
}
|
||||
|
||||
// applyCIOverrides applies non-nil raw values onto resolved defaults, clamping
|
||||
@@ -2395,10 +2462,16 @@ func ciDefaults() CI {
|
||||
// inverting the bound, and anything above MaxCIRerunTransient is capped so a
|
||||
// typo cannot keep a run polling one commit indefinitely.
|
||||
func applyCIOverrides(dst *CI, src *CIRaw) {
|
||||
if src.RerunTransient == nil {
|
||||
return
|
||||
if src.RerunTransient != nil {
|
||||
dst.RerunTransient = min(max(*src.RerunTransient, 0), MaxCIRerunTransient)
|
||||
}
|
||||
// Applied independently of the rerun budget so a config that sets only one
|
||||
// of the two keys does not silently discard the other, and so an explicit
|
||||
// `revalidate_repairs: false` in the later (repository) source overrides an
|
||||
// earlier `true` rather than reading as "unset".
|
||||
if src.RevalidateRepairs != nil {
|
||||
dst.RevalidateRepairs = *src.RevalidateRepairs
|
||||
}
|
||||
dst.RerunTransient = min(max(*src.RerunTransient, 0), MaxCIRerunTransient)
|
||||
}
|
||||
|
||||
// applyAutoFixOverrides applies non-nil raw values onto resolved defaults.
|
||||
|
||||
@@ -408,6 +408,20 @@ func (d *DB) UpdateRunPushBinding(id string, binding PushBinding) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// UpdateRunPublication atomically records the exact published head and its
|
||||
// successful-push provenance.
|
||||
func (d *DB) UpdateRunPublication(id string, binding PushBinding) error {
|
||||
ts := now()
|
||||
_, err := d.sql.Exec(
|
||||
`UPDATE runs SET head_sha = ?, last_pushed_sha = ?, push_target_kind = ?, push_target_fingerprint = ?, push_ref = ?, last_pushed_at = ?, push_generation = COALESCE(push_generation, 0) + 1, updated_at = ? WHERE id = ?`,
|
||||
binding.HeadSHA, binding.HeadSHA, binding.TargetKind, binding.TargetFingerprint, binding.Ref, ts, ts, id,
|
||||
)
|
||||
if err != nil {
|
||||
return fmt.Errorf("update run publication: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// SetRunCustodyReturned stamps the moment a guarded recovery explicitly
|
||||
// returned custody of this run's branch to the operator worktree. Stamping is
|
||||
// idempotent: the first timestamp wins so the record keeps the original
|
||||
|
||||
@@ -589,6 +589,47 @@ func TestRunPushBindingIsForwardOnlyAndLegacyRowsStayNullable(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateRunPublicationIsAtomic(t *testing.T) {
|
||||
d := openTestDB(t)
|
||||
repo, _ := d.InsertRepo("/tmp/repo-publication", "https://example.com/repo.git", "main")
|
||||
run, err := d.InsertRun(repo.ID, "feature", "submitted", "base")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := d.sql.Exec(`CREATE TRIGGER reject_publication_head
|
||||
BEFORE UPDATE OF head_sha ON runs
|
||||
WHEN NEW.head_sha = 'repair'
|
||||
BEGIN
|
||||
SELECT RAISE(FAIL, 'injected publication failure');
|
||||
END`); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
binding := PushBinding{HeadSHA: "repair", TargetKind: "upstream", TargetFingerprint: "digest", Ref: "refs/heads/feature"}
|
||||
if err := d.UpdateRunPublication(run.ID, binding); err == nil {
|
||||
t.Fatal("publication unexpectedly succeeded")
|
||||
}
|
||||
got, err := d.GetRun(run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got.HeadSHA != "submitted" || got.LastPushedSHA != nil || got.PushGeneration != nil {
|
||||
t.Fatalf("failed publication partially changed run: %#v", got)
|
||||
}
|
||||
if _, err := d.sql.Exec(`DROP TRIGGER reject_publication_head`); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := d.UpdateRunPublication(run.ID, binding); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
got, err = d.GetRun(run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got.HeadSHA != "repair" || got.LastPushedSHA == nil || *got.LastPushedSHA != "repair" || got.PushGeneration == nil || *got.PushGeneration != 1 {
|
||||
t.Fatalf("publication was not recorded together: %#v", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateRunPRURL(t *testing.T) {
|
||||
d := openTestDB(t)
|
||||
repo, _ := d.InsertRepo("/home/user/project", "git@github.com:user/project.git", "main")
|
||||
|
||||
@@ -58,8 +58,8 @@ CREATE TABLE IF NOT EXISTS step_results (
|
||||
last_activity_at INTEGER,
|
||||
last_activity TEXT,
|
||||
agent_pid INTEGER,
|
||||
auto_fix_limit INTEGER,
|
||||
ci_fix_attempts INTEGER NOT NULL DEFAULT 0
|
||||
auto_fix_limit INTEGER,
|
||||
ci_fix_attempts INTEGER NOT NULL DEFAULT 0
|
||||
);
|
||||
|
||||
CREATE TABLE IF NOT EXISTS step_rounds (
|
||||
|
||||
+5
-3
@@ -31,11 +31,13 @@ type StepResult struct {
|
||||
const stepResultColumns = `id, run_id, step_name, step_order, status, exit_code, duration_ms, log_path, findings_json, error, started_at, completed_at, last_activity_at, last_activity, agent_pid, auto_fix_limit`
|
||||
|
||||
func (d *DB) readableStepResultColumns() string {
|
||||
columns := stepResultColumns
|
||||
if d.hasColumn("step_results", "ci_fix_attempts") {
|
||||
return stepResultColumns + ", ci_fix_attempts"
|
||||
columns += ", ci_fix_attempts"
|
||||
} else {
|
||||
columns += ", 0 AS ci_fix_attempts"
|
||||
}
|
||||
// Read-only preflight can inspect a database before migrations run.
|
||||
return stepResultColumns + ", 0 AS ci_fix_attempts"
|
||||
return columns
|
||||
}
|
||||
|
||||
// InsertStepResult creates a new step result record.
|
||||
|
||||
@@ -105,7 +105,8 @@ type StepOutcome struct {
|
||||
Skipped bool // mark the step as skipped without failing the run
|
||||
SkipRemaining bool // skip all subsequent steps (e.g. empty diff after rebase)
|
||||
// RestartFrom asks the executor to re-run validation from this earlier step.
|
||||
// CI repairs use it to send the new local head back through review before push.
|
||||
// CI repairs use it when policy requires revalidation or continuity cannot be
|
||||
// proven, sending the new local head back through review before push.
|
||||
RestartFrom types.StepName
|
||||
// FixSummary, when non-empty, is the agent's one-line commit summary for
|
||||
// the fix attempt performed during this round. Steps populate it in fix
|
||||
|
||||
@@ -226,6 +226,11 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
|
||||
} else {
|
||||
sctx.Log(fmt.Sprintf("monitoring CI for PR #%s (timeout: %s)...", prNumber, timeout))
|
||||
}
|
||||
// State the repair policy once, at entry, rather than at every poll: which
|
||||
// of the two very differently priced paths a repair will take is the single
|
||||
// most useful thing to know when reading a CI step log after the fact, and
|
||||
// it cannot be inferred from the repair line alone until a repair happens.
|
||||
sctx.Log(fmt.Sprintf("CI repair policy: %s (ci.revalidate_repairs: %t)", ciRepairPolicyDescription(sctx), ciRevalidatesRepairs(sctx)))
|
||||
now := s.now
|
||||
if now == nil {
|
||||
now = time.Now
|
||||
@@ -258,6 +263,30 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
|
||||
}
|
||||
return ciMonitoringTimeoutOutcome(), nil
|
||||
}
|
||||
waitForPoll := func() error {
|
||||
interval := s.pollIntervalOverride
|
||||
if interval == 0 {
|
||||
interval = pollInterval(now().Sub(started))
|
||||
}
|
||||
if !unlimited {
|
||||
remaining := timeout - now().Sub(timeoutAnchor)
|
||||
if remaining < interval {
|
||||
interval = remaining
|
||||
}
|
||||
}
|
||||
waitForNextPoll := s.waitForNextPoll
|
||||
if waitForNextPoll == nil {
|
||||
waitForNextPoll = func(ctx context.Context, interval time.Duration) error {
|
||||
select {
|
||||
case <-time.After(interval):
|
||||
return nil
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
}
|
||||
}
|
||||
}
|
||||
return waitForNextPoll(ctx, interval)
|
||||
}
|
||||
|
||||
for {
|
||||
if err := ctx.Err(); err != nil {
|
||||
@@ -514,16 +543,21 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
|
||||
manualFixAttempted = true
|
||||
sctx.Log(fmt.Sprintf("issues detected: %s - manual fix requested...", issueDesc))
|
||||
previousHeadSHA := sctx.Run.HeadSHA
|
||||
changed, err := s.autoFixCI(sctx, host, pr, fixTargets, mergeConflict)
|
||||
repair, err := s.autoFixCI(sctx, host, pr, fixTargets, mergeConflict)
|
||||
if outcome := ciFixAgentBudgetOutcome(sctx, issueDesc, err); outcome != nil {
|
||||
return outcome, nil
|
||||
}
|
||||
if err != nil {
|
||||
sctx.Log(fmt.Sprintf("warning: CI manual fix failed: %v", err))
|
||||
} else if changed || sctx.Run.HeadSHA != previousHeadSHA {
|
||||
} else if repair.HeadAdvanced || sctx.Run.HeadSHA != previousHeadSHA {
|
||||
s.lastFixedChecks = fixKey
|
||||
s.lastFixedCompletedAt = fixCompletedAt
|
||||
return &pipeline.StepOutcome{RestartFrom: types.StepReview}, nil
|
||||
if repair.Revalidate {
|
||||
return &pipeline.StepOutcome{RestartFrom: types.StepReview}, nil
|
||||
}
|
||||
// The repair was published, so the monitor stays on
|
||||
// this run and waits for the provider to re-run the
|
||||
// checks against the new head.
|
||||
} else {
|
||||
sctx.Log("CI fix produced no changes, returning for manual intervention...")
|
||||
return ciFailureOutcome(reportedIssues, mergeConflict, "CI fix produced no changes - failures require manual intervention"), nil
|
||||
@@ -548,16 +582,21 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
|
||||
s.ciFixAttempts = nextAttempt
|
||||
sctx.Log(fmt.Sprintf("issues detected: %s - auto-fixing (attempt %d/%d)...", issueDesc, s.ciFixAttempts, ciFixLimit))
|
||||
previousHeadSHA := sctx.Run.HeadSHA
|
||||
changed, err := s.autoFixCI(sctx, host, pr, fixTargets, mergeConflict)
|
||||
repair, err := s.autoFixCI(sctx, host, pr, fixTargets, mergeConflict)
|
||||
if outcome := ciFixAgentBudgetOutcome(sctx, issueDesc, err); outcome != nil {
|
||||
return outcome, nil
|
||||
}
|
||||
if err != nil {
|
||||
sctx.Log(fmt.Sprintf("warning: CI auto-fix failed: %v", err))
|
||||
} else if changed || sctx.Run.HeadSHA != previousHeadSHA {
|
||||
} else if repair.HeadAdvanced || sctx.Run.HeadSHA != previousHeadSHA {
|
||||
s.lastFixedChecks = fixKey
|
||||
s.lastFixedCompletedAt = fixCompletedAt
|
||||
return &pipeline.StepOutcome{RestartFrom: types.StepReview}, nil
|
||||
if repair.Revalidate {
|
||||
return &pipeline.StepOutcome{RestartFrom: types.StepReview}, nil
|
||||
}
|
||||
// The repair was published, so the monitor stays on
|
||||
// this run and waits for the provider to re-run the
|
||||
// checks against the new head.
|
||||
} else {
|
||||
// No changes produced - don't set lastFixedChecks so next
|
||||
// poll treats this as a new failure and retries if attempts remain.
|
||||
@@ -602,29 +641,7 @@ func (s *CIStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
|
||||
}
|
||||
}
|
||||
|
||||
// Sleep for poll interval
|
||||
interval := s.pollIntervalOverride
|
||||
if interval == 0 {
|
||||
interval = pollInterval(now().Sub(started))
|
||||
}
|
||||
if !unlimited {
|
||||
remaining := timeout - now().Sub(timeoutAnchor)
|
||||
if remaining < interval {
|
||||
interval = remaining
|
||||
}
|
||||
}
|
||||
waitForNextPoll := s.waitForNextPoll
|
||||
if waitForNextPoll == nil {
|
||||
waitForNextPoll = func(ctx context.Context, interval time.Duration) error {
|
||||
select {
|
||||
case <-time.After(interval):
|
||||
return nil
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
}
|
||||
}
|
||||
}
|
||||
if err := waitForNextPoll(ctx, interval); err != nil {
|
||||
if err := waitForPoll(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
|
||||
@@ -76,6 +76,7 @@ func TestCIStep_CIFailureAutoFix(t *testing.T) {
|
||||
sctx.UserIntent = "user wanted CI autofix to preserve the extracted intent"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 3}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
@@ -241,6 +242,7 @@ func TestCIStep_CIAutoFixLimitExhausted(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1} // only 1 attempt allowed
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
stepResult, err := sctx.DB.InsertStepResult(sctx.Run.ID, types.StepCI)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
@@ -331,6 +333,7 @@ func TestCIStep_CIAutoFixRetriesAfterChecksRerun(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
var logs []string
|
||||
sctx.Log = func(s string) { logs = append(logs, s) }
|
||||
@@ -404,6 +407,7 @@ func TestCIStep_CIAutoFixRetriesWhenGitHubClockLagsLocalClock(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 5 * time.Minute
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
localNow := start.Add(30 * time.Minute)
|
||||
step := &CIStep{
|
||||
@@ -486,6 +490,7 @@ func TestCIStep_CIAutoFixRetriesWhenFastChecksSkipPendingObservation(t *testing.
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 1 * time.Hour
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
var logs []string
|
||||
sctx.Log = func(s string) { logs = append(logs, s) }
|
||||
@@ -571,6 +576,7 @@ func TestCIStep_CIAutoFixRetriesWhenSomeChecksStayFailing(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
var logs []string
|
||||
sctx.Log = func(s string) { logs = append(logs, s) }
|
||||
@@ -642,6 +648,7 @@ func TestCIStep_DoesNotRetryOnUnrelatedPendingCheck(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
var logs []string
|
||||
sctx.Log = func(s string) { logs = append(logs, s) }
|
||||
@@ -720,6 +727,7 @@ func TestCIStep_RetriesMergeConflictAfterRerun(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 2}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
var logs []string
|
||||
sctx.Log = func(s string) { logs = append(logs, s) }
|
||||
@@ -796,6 +804,7 @@ func TestCIStep_FixMode_ManualInterventionRunsCIFix(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 0}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
sctx.Fixing = true
|
||||
sctx.PreviousFindings = string(findingsJSON)
|
||||
|
||||
|
||||
@@ -237,6 +237,7 @@ func TestCIStep_BitbucketAutoFixIncludesPipelineLogs(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
@@ -318,6 +319,7 @@ func TestCIStep_BitbucketAutoFixUsesLivePRHeadSHAForLogs(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
@@ -402,6 +404,7 @@ func TestCIStep_BitbucketAutoFixUsesMatchingPipelineLogs(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
@@ -49,12 +49,15 @@ func TestCIStep_CommitAndPush_CommitsLocallyWithoutPushing(t *testing.T) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitRepair(sctx, "stabilize Windows path test")
|
||||
repair, err := step.commitRepair(sctx, "stabilize Windows path test")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report committed changes")
|
||||
}
|
||||
|
||||
@@ -120,12 +123,15 @@ func TestCIStep_CommitAndPushDoesNotPushForkWhenConfigured(t *testing.T) {
|
||||
sctx.Repo.ForkURL = fork
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("expected commitAndPush to report committed changes")
|
||||
}
|
||||
|
||||
@@ -147,11 +153,11 @@ func TestCIStep_CommitAndPush_NoChanges(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if changed {
|
||||
if repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report no local changes")
|
||||
}
|
||||
}
|
||||
@@ -196,11 +202,11 @@ func TestCIStep_CommitAndPush_StatusError(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err == nil {
|
||||
t.Fatal("expected status error")
|
||||
}
|
||||
if changed {
|
||||
if repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report no changes on status error")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "git status --porcelain") {
|
||||
@@ -269,12 +275,15 @@ func TestCIStep_CommitAndPush_UsesStepEnvForAllGitCommands(t *testing.T) {
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("expected commitAndPush to report committed changes")
|
||||
}
|
||||
|
||||
@@ -346,12 +355,15 @@ func TestCIStep_CommitAndPush_GitCommandsUseStandardCredentialEnv(t *testing.T)
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("expected commitAndPush to report committed changes")
|
||||
}
|
||||
}
|
||||
@@ -387,12 +399,15 @@ func TestCIStep_CommitAndPush_NoChanges_ReconcilesStaleDatabaseHeadSHA(t *testin
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report the reconciled local head")
|
||||
}
|
||||
|
||||
@@ -450,12 +465,15 @@ func TestCIStep_CommitAndPush_NoChanges_ReconcilesStaleDatabaseHeadSHA_UsesStepE
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report the reconciled local head")
|
||||
}
|
||||
|
||||
@@ -505,12 +523,15 @@ func TestCIStep_CommitAndPush_NoDirtyChangesRecordsAdvancedLocalHead(t *testing.
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("expected commitAndPush to record advanced clean head")
|
||||
}
|
||||
|
||||
@@ -561,12 +582,15 @@ func TestCIStep_CommitAndPush_UpdatesLocalBranchRefWithoutDetachedPush(t *testin
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
if !repair.HeadAdvanced {
|
||||
t.Error("expected commitAndPush to report committed changes")
|
||||
}
|
||||
newHeadSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
|
||||
@@ -15,13 +15,16 @@ import (
|
||||
)
|
||||
|
||||
// autoFixCI runs the agent to fix CI failures and/or merge conflicts, then
|
||||
// commits the repair locally for a new validation cycle.
|
||||
// Returns (true, nil) when the local head changed, (false, nil)
|
||||
// when the agent produced no changes, or (false, err) on failure.
|
||||
func (s *CIStep) autoFixCI(sctx *pipeline.StepContext, host scm.Host, pr *scm.PR, failingNames []string, mergeConflict bool) (bool, error) {
|
||||
// records the repair under the run's uniform continuity rule: published
|
||||
// immediately through the guarded push path when its continuity with the
|
||||
// reviewed head is provable, held for revalidation when it is not or when
|
||||
// ci.revalidate_repairs asks for it outright. See recordRepair.
|
||||
// The result reports whether the recorded head advanced and whether the repair
|
||||
// must revalidate; a zero result means the agent produced no changes.
|
||||
func (s *CIStep) autoFixCI(sctx *pipeline.StepContext, host scm.Host, pr *scm.PR, failingNames []string, mergeConflict bool) (ciRepairResult, error) {
|
||||
ctx := sctx.Ctx
|
||||
if err := sctx.DB.SetRunPushActive(sctx.Run.ID, true); err != nil {
|
||||
return false, err
|
||||
return ciRepairResult{}, err
|
||||
}
|
||||
defer func() { _ = sctx.DB.SetRunPushActive(sctx.Run.ID, false) }()
|
||||
baseBranch := effectivePRBaseBranch(sctx)
|
||||
@@ -132,7 +135,7 @@ CI logs:
|
||||
OnChunk: sctx.LogChunk,
|
||||
})
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("agent CI fix: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("agent CI fix: %w", err)
|
||||
}
|
||||
|
||||
summary, summaryErr := extractCommitSummary(result)
|
||||
@@ -207,23 +210,34 @@ func formatReviewComments(comments []scm.ReviewComment) string {
|
||||
return b.String()
|
||||
}
|
||||
|
||||
// ciRepairResult reports what a repair did to the run. The monitor needs both
|
||||
// facts: whether the recorded head advanced at all, and whether the repair was
|
||||
// held for revalidation instead of published.
|
||||
type ciRepairResult struct {
|
||||
// HeadAdvanced is true when the run's recorded head moved to the repair.
|
||||
HeadAdvanced bool
|
||||
// Revalidate is true when the repair was NOT published and the pipeline
|
||||
// must re-run from Review before Push may publish it.
|
||||
Revalidate bool
|
||||
}
|
||||
|
||||
// commitAndPush remains as the narrow test seam for the default summary.
|
||||
func (s *CIStep) commitAndPush(sctx *pipeline.StepContext) (bool, error) {
|
||||
func (s *CIStep) commitAndPush(sctx *pipeline.StepContext) (ciRepairResult, error) {
|
||||
return s.commitRepair(sctx, "")
|
||||
}
|
||||
|
||||
func (s *CIStep) commitRepair(sctx *pipeline.StepContext, summary string) (bool, error) {
|
||||
func (s *CIStep) commitRepair(sctx *pipeline.StepContext, summary string) (ciRepairResult, error) {
|
||||
status, err := stepGitRun(sctx, "status", "--porcelain")
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("check CI changes: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("check CI changes: %w", err)
|
||||
}
|
||||
if strings.TrimSpace(status) == "" {
|
||||
sctx.Log("no changes to commit")
|
||||
headSHA, err := stepGitHeadSHA(sctx)
|
||||
if err == nil && headSHA != sctx.Run.HeadSHA {
|
||||
return s.recordLocalRepair(sctx, headSHA)
|
||||
return s.recordRepair(sctx, headSHA)
|
||||
}
|
||||
return false, nil
|
||||
return ciRepairResult{}, nil
|
||||
}
|
||||
|
||||
if summary == "" {
|
||||
@@ -231,32 +245,144 @@ func (s *CIStep) commitRepair(sctx *pipeline.StepContext, summary string) (bool,
|
||||
}
|
||||
message, err := sctx.Config.Commit.RenderFixMessage(types.StepCI, summary)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("render CI repair commit message: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("render CI repair commit message: %w", err)
|
||||
}
|
||||
if _, err := stepGitRun(sctx, "add", "-A"); err != nil {
|
||||
return false, fmt.Errorf("stage CI changes: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("stage CI changes: %w", err)
|
||||
}
|
||||
if _, err := stepGitRun(sctx, "commit", "-m", message); err != nil {
|
||||
return false, fmt.Errorf("commit: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("commit: %w", err)
|
||||
}
|
||||
headSHA, err := stepGitHeadSHA(sctx)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("resolve head after commit: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("resolve head after commit: %w", err)
|
||||
}
|
||||
|
||||
return s.recordLocalRepair(sctx, headSHA)
|
||||
return s.recordRepair(sctx, headSHA)
|
||||
}
|
||||
|
||||
func (s *CIStep) recordLocalRepair(sctx *pipeline.StepContext, headSHA string) (bool, error) {
|
||||
// ciRevalidatesRepairs reports whether this run must re-run the whole pipeline
|
||||
// from Review after the CI step repairs a failing check, rather than publishing
|
||||
// the repair and continuing to monitor. It is the resolved ci.revalidate_repairs
|
||||
// policy (global config, overridden by the repository's trusted default-branch
|
||||
// config). The repair recorder uses it to choose immediate publication or
|
||||
// revalidation, and the CI monitor logs the resolved policy.
|
||||
func ciRevalidatesRepairs(sctx *pipeline.StepContext) bool {
|
||||
return sctx.Config != nil && sctx.Config.CI.RevalidateRepairs
|
||||
}
|
||||
|
||||
// ciRepairPolicyDescription names the configured policy in the CI step log, so
|
||||
// an operator reading a run after the fact can tell which of the two paths a
|
||||
// repair took without cross-referencing the config that was in force.
|
||||
func ciRepairPolicyDescription(sctx *pipeline.StepContext) string {
|
||||
if ciRevalidatesRepairs(sctx) {
|
||||
return "always restart validation from Review after a repair"
|
||||
}
|
||||
return "publish a repair whose continuity with the reviewed head is provable, otherwise restart validation from Review"
|
||||
}
|
||||
|
||||
// recordRepair binds a freshly produced CI repair commit to the run.
|
||||
//
|
||||
// One uniform rule decides how, and it applies to every CI-fix path - automatic
|
||||
// and manual alike, CI failure and merge conflict alike:
|
||||
//
|
||||
// A repair is published without revalidating only when its continuity with the
|
||||
// reviewed, published head can be PROVEN. When that continuity cannot be
|
||||
// proven, the repair revalidates from Review.
|
||||
//
|
||||
// ci.revalidate_repairs governs intent identically on every path: true asks for
|
||||
// revalidation outright, false asks to publish when it is safe to do so. Merge
|
||||
// conflict repairs are not carved out - they simply always land in the
|
||||
// cannot-be-proven half, because a rebase makes the repaired head a
|
||||
// non-descendant of the reviewed head, resolving a conflict changes the
|
||||
// commit's patch-id, and no content-based guard can separate "rebased and
|
||||
// resolved" from "dropped the work". Provenance cannot stand in for that proof
|
||||
// either: the repair that deleted a reviewed commit in the reproduction behind
|
||||
// this rule was authored by the CI repair agent itself. Who wrote the repair
|
||||
// says nothing about what it did to the reviewed commits.
|
||||
//
|
||||
// Once recording or publication succeeds, the run's recorded head advances;
|
||||
// the two paths differ in whether the repair is published now or held until
|
||||
// Review has approved it.
|
||||
func (s *CIStep) recordRepair(sctx *pipeline.StepContext, headSHA string) (ciRepairResult, error) {
|
||||
if ciRevalidatesRepairs(sctx) {
|
||||
return s.recordLocalRepair(sctx, headSHA)
|
||||
}
|
||||
if reason := ciRepairContinuityGap(sctx, headSHA); reason != "" {
|
||||
sctx.Log(fmt.Sprintf("cannot prove the repaired head continues the reviewed head: %s; revalidating from Review instead of publishing", reason))
|
||||
return s.recordLocalRepair(sctx, headSHA)
|
||||
}
|
||||
return s.publishRepair(sctx, headSHA)
|
||||
}
|
||||
|
||||
// ciRepairContinuityGap returns why the repaired head cannot be proven to
|
||||
// continue the run's reviewed, published head, or "" when it can. It reads the
|
||||
// same durable review authority the publication guard enforces
|
||||
// (reviewApprovedHead), so the decision to publish and the guard that permits
|
||||
// the push can never disagree.
|
||||
//
|
||||
// Fail closed: an unreadable run, a missing or malformed approval, and an
|
||||
// unverifiable ancestry all count as unproven, because the cost of being wrong
|
||||
// is force-pushing away commits the pipeline was trusted with.
|
||||
func ciRepairContinuityGap(sctx *pipeline.StepContext, headSHA string) string {
|
||||
run, err := sctx.DB.GetRun(sctx.Run.ID)
|
||||
if err != nil {
|
||||
return "the durable review approval could not be read"
|
||||
}
|
||||
approvedHead, reason := reviewApprovedHead(sctx, run)
|
||||
if approvedHead == "" {
|
||||
return reason
|
||||
}
|
||||
if strings.EqualFold(approvedHead, headSHA) {
|
||||
return ""
|
||||
}
|
||||
if _, err := stepGitRun(sctx, "merge-base", "--is-ancestor", approvedHead, headSHA); err != nil {
|
||||
return fmt.Sprintf("repaired head %s does not descend from reviewed head %s", shortObjectID(headSHA), shortObjectID(approvedHead))
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
// recordLocalRepair keeps the repair local because revalidation was requested
|
||||
// or continuity could not be proven. It revokes the run's review authority, so
|
||||
// the Push step's
|
||||
// assertReviewApprovedPushHead guard refuses to publish the repaired head until
|
||||
// Review has approved it again. The CI monitor turns that into a restart at
|
||||
// Review.
|
||||
func (s *CIStep) recordLocalRepair(sctx *pipeline.StepContext, headSHA string) (ciRepairResult, error) {
|
||||
ref := normalizedBranchRef(sctx.Run.Branch)
|
||||
if _, err := stepGitRun(sctx, "update-ref", ref, headSHA); err != nil {
|
||||
return false, fmt.Errorf("update local branch ref: %w", err)
|
||||
return ciRepairResult{}, fmt.Errorf("update local branch ref: %w", err)
|
||||
}
|
||||
// Durable first, then in memory. Advancing the live head before the write
|
||||
// succeeds leaves the monitor watching a head the durable record does not
|
||||
// know about, still holding its old review approval, with the revalidation
|
||||
// this call exists to trigger silently lost.
|
||||
if err := sctx.DB.UpdateRunHeadSHAForRevalidation(sctx.Run.ID, headSHA); err != nil {
|
||||
return ciRepairResult{}, err
|
||||
}
|
||||
sctx.Run.HeadSHA = headSHA
|
||||
if err := sctx.DB.UpdateRunHeadSHAForRevalidation(sctx.Run.ID, headSHA); err != nil {
|
||||
return false, err
|
||||
}
|
||||
sctx.Run.ReviewApprovedHeadSHA = nil
|
||||
sctx.Log("committed CI repair for revalidation")
|
||||
return true, nil
|
||||
return ciRepairResult{HeadAdvanced: true, Revalidate: true}, nil
|
||||
}
|
||||
|
||||
// publishRepair publishes a continuity-proven repair immediately when
|
||||
// ci.revalidate_repairs is false. It uses publishRunHead - the same guarded path
|
||||
// the Push step uses, so force-push lease safety, remote verification, and the
|
||||
// push binding all still apply. Gate-mirror synchronization settles before the
|
||||
// head and push binding are recorded. The run's review approval is deliberately
|
||||
// not revoked: recordRepair has already proven that this head equals or descends
|
||||
// from the approved head,
|
||||
// and publishRunHead enforces the same descendant-only rule. The monitor stays
|
||||
// on this run to watch the checks re-run against the published head.
|
||||
//
|
||||
// publishRunHead records nothing until the remote push, the gate mirror, and
|
||||
// the database write have all succeeded, so a partial failure leaves the run on
|
||||
// the pre-repair head and the next fix attempt re-enters this path.
|
||||
func (s *CIStep) publishRepair(sctx *pipeline.StepContext, headSHA string) (ciRepairResult, error) {
|
||||
if err := publishRunHead(sctx, headSHA, headSHA); err != nil {
|
||||
return ciRepairResult{}, err
|
||||
}
|
||||
sctx.Log("committed and pushed CI repair")
|
||||
return ciRepairResult{HeadAdvanced: true}, nil
|
||||
}
|
||||
|
||||
@@ -36,11 +36,11 @@ func TestCIFixTreatsForgejoLogsAsOptionalEvidence(t *testing.T) {
|
||||
sctx := newTestContext(t, ag, dir, baseSHA, headSHA, config.Commands{})
|
||||
host := &forgejoLogTestHost{logs: tt.logs, err: tt.fetchErr}
|
||||
|
||||
pushed, err := (&CIStep{}).autoFixCI(sctx, host, &scm.PR{Number: "42", URL: "https://forge.example/octo/widgets/pulls/42"}, []string{"CI / test (pull_request)"}, false)
|
||||
repair, err := (&CIStep{}).autoFixCI(sctx, host, &scm.PR{Number: "42", URL: "https://forge.example/octo/widgets/pulls/42"}, []string{"CI / test (pull_request)"}, false)
|
||||
if err != nil {
|
||||
t.Fatalf("autoFixCI() error = %v", err)
|
||||
}
|
||||
if pushed {
|
||||
if repair.HeadAdvanced {
|
||||
t.Fatal("autoFixCI() pushed without agent changes")
|
||||
}
|
||||
if host.calls != 1 {
|
||||
|
||||
@@ -244,6 +244,7 @@ func TestCIStep_GitLabAutoFixIncludesJobTrace(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
|
||||
@@ -289,6 +289,9 @@ func TestCIStep_MergeConflictAutoFixPromptUsesBaseBranchTip(t *testing.T) {
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{}
|
||||
host, skip := buildHost(sctx, scm.ProviderGitHub)
|
||||
if host == nil {
|
||||
@@ -331,6 +334,9 @@ func TestCIStep_AutoFixUsesExistingPRBaseAfterConfigChanges(t *testing.T) {
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Config.PR.BaseBranch = "main"
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
pr := &scm.PR{Number: "42", URL: "https://github.com/test/repo/pull/42", BaseBranch: "develop"}
|
||||
|
||||
var prompt string
|
||||
|
||||
@@ -0,0 +1,598 @@
|
||||
package steps
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kunchenguid/no-mistakes/internal/agent"
|
||||
"github.com/kunchenguid/no-mistakes/internal/branchsync"
|
||||
"github.com/kunchenguid/no-mistakes/internal/config"
|
||||
"github.com/kunchenguid/no-mistakes/internal/db"
|
||||
"github.com/kunchenguid/no-mistakes/internal/pipeline"
|
||||
"github.com/kunchenguid/no-mistakes/internal/types"
|
||||
)
|
||||
|
||||
// ciRepairFixture is one CI monitor run wired to a real git worktree, a real
|
||||
// bare upstream, and a fake gh reporting one failing check, so a test can
|
||||
// observe what a repair does to the local head, the remote, and the run's
|
||||
// review authority. Tests using this process-heavy fixture intentionally run
|
||||
// serially: running all of their git and fake-gh subprocesses in parallel can
|
||||
// exhaust macOS CI process capacity and stall a child indefinitely.
|
||||
type ciRepairFixture struct {
|
||||
sctx *pipeline.StepContext
|
||||
dir string
|
||||
upstream string
|
||||
headSHA string
|
||||
gateDir string
|
||||
logs *[]string
|
||||
}
|
||||
|
||||
func newCIRepairFixture(t *testing.T, revalidate bool, agentAction func(workDir string)) *ciRepairFixture {
|
||||
t.Helper()
|
||||
upstream := t.TempDir()
|
||||
gitCmd(t, upstream, "init", "--bare")
|
||||
|
||||
dir := t.TempDir()
|
||||
gitCmd(t, dir, "init")
|
||||
gitCmd(t, dir, "config", "user.name", "test")
|
||||
gitCmd(t, dir, "config", "user.email", "test@test.com")
|
||||
gitCmd(t, dir, "checkout", "-b", "main")
|
||||
os.WriteFile(filepath.Join(dir, "init.txt"), []byte("init"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "initial")
|
||||
baseSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "remote", "add", "origin", upstream)
|
||||
gitCmd(t, dir, "push", "origin", "main")
|
||||
|
||||
gitCmd(t, dir, "checkout", "-b", "feature")
|
||||
os.WriteFile(filepath.Join(dir, "feature.txt"), []byte("feature"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "feature")
|
||||
headSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "push", "origin", "feature")
|
||||
|
||||
ag := &mockAgent{name: "test", runFn: func(ctx context.Context, opts agent.RunOpts) (*agent.Result, error) {
|
||||
if agentAction != nil {
|
||||
agentAction(opts.CWD)
|
||||
}
|
||||
return &agent.Result{Output: []byte(`{"summary":"repair the failing check"}`)}, nil
|
||||
}}
|
||||
|
||||
prURL := "https://github.com/test/repo/pull/42"
|
||||
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
|
||||
sctx.Env = fakeCIGH(t, "OPEN", `[{"name":"test","state":"FAILURE","bucket":"fail"}]`)
|
||||
sctx.Run.PRURL = &prURL
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Config.CITimeout = 30 * time.Second
|
||||
sctx.Config.AutoFix = config.AutoFix{CI: 1}
|
||||
sctx.Config.CI.RevalidateRepairs = revalidate
|
||||
|
||||
// The CI step only ever runs after Push succeeded, so a run always reaches
|
||||
// it with a durable review approval and a recorded push binding.
|
||||
if err := sctx.DB.UpdateRunReviewApprovedHeadSHA(sctx.Run.ID, headSHA); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
sctx.Run.ReviewApprovedHeadSHA = &headSHA
|
||||
if err := sctx.DB.UpdateRunPushBinding(sctx.Run.ID, db.PushBinding{
|
||||
HeadSHA: headSHA, TargetKind: "upstream",
|
||||
TargetFingerprint: branchsync.TargetFingerprint(upstream), Ref: "refs/heads/feature",
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
logs := &[]string{}
|
||||
sctx.Log = func(s string) { *logs = append(*logs, s) }
|
||||
return &ciRepairFixture{sctx: sctx, dir: dir, upstream: upstream, headSHA: headSHA, gateDir: sctx.GateDir, logs: logs}
|
||||
}
|
||||
|
||||
// run drives the monitor until it returns or the poll budget is spent.
|
||||
func (f *ciRepairFixture) run(t *testing.T) (*pipeline.StepOutcome, error) {
|
||||
t.Helper()
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
f.sctx.Ctx = ctx
|
||||
polls := 0
|
||||
step := &CIStep{waitForNextPoll: func(ctx context.Context, d time.Duration) error {
|
||||
polls++
|
||||
if polls >= 2 {
|
||||
cancel()
|
||||
}
|
||||
return ctx.Err()
|
||||
}}
|
||||
return step.Execute(f.sctx)
|
||||
}
|
||||
|
||||
func (f *ciRepairFixture) localHead(t *testing.T) string {
|
||||
return gitCmd(t, f.dir, "rev-parse", "HEAD")
|
||||
}
|
||||
func (f *ciRepairFixture) remoteHead(t *testing.T) string {
|
||||
return gitCmd(t, f.upstream, "rev-parse", "refs/heads/feature")
|
||||
}
|
||||
func (f *ciRepairFixture) log() string { return strings.Join(*f.logs, "\n") }
|
||||
|
||||
func writeCIFix(workDir string) {
|
||||
os.WriteFile(filepath.Join(workDir, "ci-fix.txt"), []byte("fixed"), 0o644)
|
||||
}
|
||||
|
||||
// TestCIStep_RevalidateRepairsPolicySelectsRepairDelivery is the behavioral
|
||||
// core of ci.revalidate_repairs: the same failing check, the same repair, and
|
||||
// two entirely different deliveries.
|
||||
func TestCIStep_RevalidateRepairsPolicySelectsRepairDelivery(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
revalidate bool
|
||||
wantRestart bool
|
||||
wantRemoteMoved bool
|
||||
wantApprovalKept bool
|
||||
wantLog string
|
||||
}{
|
||||
{
|
||||
name: "default_publishes_the_repair_and_keeps_monitoring",
|
||||
revalidate: false, wantRestart: false, wantRemoteMoved: true, wantApprovalKept: true,
|
||||
wantLog: "committed and pushed CI repair",
|
||||
},
|
||||
{
|
||||
name: "opt_in_holds_the_repair_and_restarts_at_review",
|
||||
revalidate: true, wantRestart: true, wantRemoteMoved: false, wantApprovalKept: false,
|
||||
wantLog: "committed CI repair for revalidation",
|
||||
},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
f := newCIRepairFixture(t, tc.revalidate, nil)
|
||||
writeCIFix(f.dir)
|
||||
// commitRepair, not the whole monitor loop: the delivery decision
|
||||
// is what this table is about, and driving Execute here spends a
|
||||
// provider poll and several subprocesses per case for nothing.
|
||||
// TestCIStep_MonitorRestartsAtReviewForAHeldRepair covers the
|
||||
// monitor turning Revalidate into RestartFrom.
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err != nil {
|
||||
t.Fatalf("CI repair returned error: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("the repair was not recorded as a real change")
|
||||
}
|
||||
if repair.Revalidate != tc.wantRestart {
|
||||
t.Errorf("Revalidate = %v, want %v", repair.Revalidate, tc.wantRestart)
|
||||
}
|
||||
|
||||
localHead := f.localHead(t)
|
||||
if localHead == f.headSHA {
|
||||
t.Fatal("the repair commit was never created")
|
||||
}
|
||||
|
||||
remoteMoved := f.remoteHead(t) != f.headSHA
|
||||
if remoteMoved != tc.wantRemoteMoved {
|
||||
t.Errorf("remote advanced = %v, want %v", remoteMoved, tc.wantRemoteMoved)
|
||||
}
|
||||
if tc.wantRemoteMoved && f.remoteHead(t) != localHead {
|
||||
t.Errorf("remote head = %s, want the repair commit %s", f.remoteHead(t), localHead)
|
||||
}
|
||||
|
||||
run, err := f.sctx.DB.GetRun(f.sctx.Run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
approvalKept := run.ReviewApprovedHeadSHA != nil && strings.TrimSpace(*run.ReviewApprovedHeadSHA) != ""
|
||||
if approvalKept != tc.wantApprovalKept {
|
||||
t.Errorf("review approval retained = %v, want %v", approvalKept, tc.wantApprovalKept)
|
||||
}
|
||||
if run.HeadSHA != localHead {
|
||||
t.Errorf("recorded head = %s, want the repair commit %s", run.HeadSHA, localHead)
|
||||
}
|
||||
|
||||
// A published repair must record the delivery; a held one must not
|
||||
// claim one.
|
||||
publishedSHA := ""
|
||||
if run.LastPushedSHA != nil {
|
||||
publishedSHA = *run.LastPushedSHA
|
||||
}
|
||||
if tc.wantRemoteMoved && publishedSHA != localHead {
|
||||
t.Errorf("push binding = %s, want the published repair %s", publishedSHA, localHead)
|
||||
}
|
||||
if !tc.wantRemoteMoved && publishedSHA == localHead {
|
||||
t.Error("a repair held for revalidation was recorded as published")
|
||||
}
|
||||
|
||||
if !strings.Contains(f.log(), tc.wantLog) {
|
||||
t.Errorf("log missing %q; got:\n%s", tc.wantLog, f.log())
|
||||
}
|
||||
t.Logf("observable delivery: revalidate=%t prior_head=%s local_head=%s remote_head=%s approval_retained=%t published_head=%s\nCI log:\n%s",
|
||||
repair.Revalidate, f.headSHA, localHead, f.remoteHead(t), approvalKept, publishedSHA, f.log())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A repair the agent declined to make is not a repair under either policy: no
|
||||
// commit, no publication, no restart, and the attempt budget still decides
|
||||
// when to stop.
|
||||
func TestCIStep_NoChangeRepairNeitherPublishesNorRestarts(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, revalidate := range []bool{false, true} {
|
||||
revalidate := revalidate
|
||||
name := "publish_policy"
|
||||
if revalidate {
|
||||
name = "revalidate_policy"
|
||||
}
|
||||
t.Run(name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
f := newCIRepairFixture(t, revalidate, nil)
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err != nil {
|
||||
t.Fatalf("CI repair returned error: %v", err)
|
||||
}
|
||||
if repair.HeadAdvanced || repair.Revalidate {
|
||||
t.Errorf("a no-change repair was reported as a delivery: %#v", repair)
|
||||
}
|
||||
if f.localHead(t) != f.headSHA {
|
||||
t.Error("a no-change repair created a commit")
|
||||
}
|
||||
if f.remoteHead(t) != f.headSHA {
|
||||
t.Error("a no-change repair published something")
|
||||
}
|
||||
if !strings.Contains(f.log(), "no changes to commit") {
|
||||
t.Errorf("log missing the no-change outcome; got:\n%s", f.log())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// The agent may commit the repair itself - the merge-conflict and
|
||||
// `git rebase --continue` shape leaves a clean worktree with an advanced HEAD.
|
||||
// Both policies must recognize that as a real repair and deliver it their own
|
||||
// way, rather than reading the clean tree as "nothing happened".
|
||||
func TestCIStep_AgentCommittedRepairFollowsThePolicy(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
revalidate bool
|
||||
wantRestart bool
|
||||
wantRemoteMoved bool
|
||||
}{
|
||||
{name: "publish_policy", revalidate: false, wantRestart: false, wantRemoteMoved: true},
|
||||
{name: "revalidate_policy", revalidate: true, wantRestart: true, wantRemoteMoved: false},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
f := newCIRepairFixture(t, tc.revalidate, nil)
|
||||
// The agent commits the repair itself and leaves a clean tree.
|
||||
os.WriteFile(filepath.Join(f.dir, "resolved.txt"), []byte("resolved"), 0o644)
|
||||
gitCmd(t, f.dir, "add", "-A")
|
||||
gitCmd(t, f.dir, "commit", "-m", "agent resolved the failure")
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err != nil {
|
||||
t.Fatalf("CI repair returned error: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if f.localHead(t) == f.headSHA {
|
||||
t.Fatal("the agent's own commit was not detected")
|
||||
}
|
||||
if repair.Revalidate != tc.wantRestart {
|
||||
t.Errorf("Revalidate = %v, want %v", repair.Revalidate, tc.wantRestart)
|
||||
}
|
||||
if moved := f.remoteHead(t) != f.headSHA; moved != tc.wantRemoteMoved {
|
||||
t.Errorf("remote advanced = %v, want %v", moved, tc.wantRemoteMoved)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Publication is all-or-nothing: the remote push, the gate mirror, the push
|
||||
// binding, and the recorded head either all land or none of them are recorded.
|
||||
// A gate-mirror failure happens after the remote already carries the head, so
|
||||
// the tempting shortcut is to record the publication anyway. Recording it would
|
||||
// leave the gate behind the remote, where `no-mistakes rerun` resolves the
|
||||
// stale gate head and silently omits the repair.
|
||||
//
|
||||
// Nothing is recorded until every part succeeds, so the failure is simply
|
||||
// something the next fix attempt re-enters and completes.
|
||||
func TestCIStep_PartialPublicationRecordsNothing(t *testing.T) {
|
||||
t.Parallel()
|
||||
f := newCIRepairFixture(t, false, nil)
|
||||
writeCIFix(f.dir)
|
||||
brokenGate := filepath.Join(t.TempDir(), "invalid-gate")
|
||||
if err := os.MkdirAll(brokenGate, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
f.sctx.GateDir = brokenGate
|
||||
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err == nil {
|
||||
t.Fatal("a publication that could not settle the gate mirror was reported as complete")
|
||||
}
|
||||
if repair.HeadAdvanced {
|
||||
t.Fatal("an unsettled publication was reported as a delivered repair")
|
||||
}
|
||||
|
||||
repairCommit := f.localHead(t)
|
||||
if repairCommit == f.headSHA {
|
||||
t.Fatal("the repair commit was never created")
|
||||
}
|
||||
run, err := f.sctx.DB.GetRun(f.sctx.Run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if run.HeadSHA != f.headSHA {
|
||||
t.Errorf("recorded head = %s, want the pre-repair head %s until publication settles", run.HeadSHA, f.headSHA)
|
||||
}
|
||||
if run.LastPushedSHA != nil && *run.LastPushedSHA == repairCommit {
|
||||
t.Error("an unsettled publication was recorded in the push binding")
|
||||
}
|
||||
|
||||
// With a working gate the same path completes, and the no-op push over the
|
||||
// already-pushed head is not an obstacle.
|
||||
f.sctx.GateDir = f.gateDir
|
||||
repair, err = (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err != nil {
|
||||
t.Fatalf("the next attempt did not complete the publication: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if !repair.HeadAdvanced || repair.Revalidate {
|
||||
t.Fatalf("result = %#v, want a published repair", repair)
|
||||
}
|
||||
if f.remoteHead(t) != repairCommit {
|
||||
t.Errorf("remote head = %s, want the repair %s", f.remoteHead(t), repairCommit)
|
||||
}
|
||||
run, err = f.sctx.DB.GetRun(f.sctx.Run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if run.HeadSHA != repairCommit || run.LastPushedSHA == nil || *run.LastPushedSHA != repairCommit {
|
||||
t.Errorf("run did not record the settled publication: head=%s pushed=%v", run.HeadSHA, run.LastPushedSHA)
|
||||
}
|
||||
}
|
||||
|
||||
// A merge-conflict repair rewrites history, so its head is never a descendant
|
||||
// of the reviewed head and its continuity can never be proven. The uniform rule
|
||||
// therefore sends every conflict repair down the revalidating path - it is not
|
||||
// carved out, it just always lands in the cannot-be-proven half.
|
||||
//
|
||||
// Both directions matter, and both are load bearing:
|
||||
// - a genuine conflict rebase must still SUCCEED, revalidating rather than
|
||||
// being refused, so conflict repair keeps working;
|
||||
// - a repair that reset to the base instead of replaying the branch must not
|
||||
// reach the remote, so the reviewed commits survive.
|
||||
//
|
||||
// The second case is the reason this rule exists. Reproduced against the
|
||||
// earlier design, that repair force-pushed the reviewed commits away while
|
||||
// reporting success - and the actor was the CI repair agent itself, which is
|
||||
// why provenance cannot substitute for proof.
|
||||
func TestCIStep_ConflictRepairAlwaysRevalidates(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
// rewrite leaves the worktree on the repaired head and returns it.
|
||||
rewrite func(t *testing.T, f *ciRepairFixture, advancedBase string) string
|
||||
// keepsReviewedWork is whether the rewrite actually replayed the
|
||||
// reviewed commit onto the new base.
|
||||
keepsReviewedWork bool
|
||||
}{
|
||||
{
|
||||
name: "genuine_rebase_replaying_the_reviewed_commit",
|
||||
rewrite: func(t *testing.T, f *ciRepairFixture, advancedBase string) string {
|
||||
// Resolve the conflict the way a repair agent would: keep the
|
||||
// feature's intent on top of the base's rewrite. That changes
|
||||
// the commit's patch-id, which is exactly why continuity
|
||||
// cannot be proven for a conflict repair.
|
||||
if err := os.WriteFile(filepath.Join(f.dir, "feature.txt"), []byte("base rewrote this line\nthe user's feature, resolved\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
gitCmd(t, f.dir, "add", "-A")
|
||||
if _, err := stepGitRun(f.sctx, "-c", "core.editor=true", "rebase", "--continue"); err != nil {
|
||||
t.Fatalf("resolve the conflict: %v", err)
|
||||
}
|
||||
return gitCmd(t, f.dir, "rev-parse", "HEAD")
|
||||
},
|
||||
keepsReviewedWork: true,
|
||||
},
|
||||
{
|
||||
name: "reset_to_base_dropping_the_reviewed_commit",
|
||||
rewrite: func(t *testing.T, f *ciRepairFixture, advancedBase string) string {
|
||||
// The repair agent gives up on the conflict and resets to the
|
||||
// base, silently discarding the reviewed commit.
|
||||
gitCmd(t, f.dir, "rebase", "--abort")
|
||||
gitCmd(t, f.dir, "reset", "--hard", advancedBase)
|
||||
return advancedBase
|
||||
},
|
||||
keepsReviewedWork: false,
|
||||
},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
// Publish policy: this is the path that could publish without review.
|
||||
// The base and the feature edit the SAME line of the same file, so
|
||||
// a rebase genuinely conflicts and the repair really is conflict
|
||||
// resolution rather than a clean replay.
|
||||
f := newCIRepairFixture(t, false, nil)
|
||||
gitCmd(t, f.dir, "checkout", "main")
|
||||
if err := os.WriteFile(filepath.Join(f.dir, "feature.txt"), []byte("base rewrote this line\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
gitCmd(t, f.dir, "add", "-A")
|
||||
gitCmd(t, f.dir, "commit", "-m", "advance base over the same line")
|
||||
advancedBase := gitCmd(t, f.dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, f.dir, "checkout", "feature")
|
||||
if _, err := stepGitRun(f.sctx, "rebase", "main"); err == nil {
|
||||
t.Fatal("expected the rebase to conflict; the fixture no longer models a conflict repair")
|
||||
}
|
||||
|
||||
repairedHead := tc.rewrite(t, f, advancedBase)
|
||||
if repairedHead == f.headSHA {
|
||||
t.Fatal("the rewrite did not move the reviewed head")
|
||||
}
|
||||
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "resolve merge conflict")
|
||||
if err != nil {
|
||||
t.Fatalf("a conflict repair must revalidate, not fail: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if !repair.HeadAdvanced {
|
||||
t.Fatal("the conflict repair was not recorded as a real change")
|
||||
}
|
||||
if !repair.Revalidate {
|
||||
t.Fatal("a conflict repair was published without revalidating")
|
||||
}
|
||||
|
||||
// Nothing rewritten reaches the remote. In the reset case this is
|
||||
// exactly what keeps the reviewed commit alive.
|
||||
if f.remoteHead(t) != f.headSHA {
|
||||
t.Fatalf("remote moved to %s; the reviewed head %s must still be published", f.remoteHead(t), f.headSHA)
|
||||
}
|
||||
reviewedContent := gitCmd(t, f.dir, "show", f.headSHA+":feature.txt")
|
||||
published := gitCmd(t, f.upstream, "show", "refs/heads/feature:feature.txt")
|
||||
if published != reviewedContent {
|
||||
t.Fatalf("DATA LOSS: published feature.txt = %q, want the reviewed content %q", published, reviewedContent)
|
||||
}
|
||||
|
||||
// Review authority is revoked so Push cannot publish the rewritten
|
||||
// head until Review approves it again.
|
||||
run, err := f.sctx.DB.GetRun(f.sctx.Run.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if run.ReviewApprovedHeadSHA != nil && strings.TrimSpace(*run.ReviewApprovedHeadSHA) != "" {
|
||||
t.Error("review approval survived a rewritten repair")
|
||||
}
|
||||
if run.HeadSHA != repairedHead {
|
||||
t.Errorf("recorded head = %s, want the repaired head %s", run.HeadSHA, repairedHead)
|
||||
}
|
||||
if !strings.Contains(f.log(), "cannot prove the repaired head continues the reviewed head") {
|
||||
t.Errorf("the log does not say why the repair revalidated:\n%s", f.log())
|
||||
}
|
||||
t.Logf("observable conflict delivery: reviewed_head=%s repaired_head=%s remote_head=%s reviewed_work_retained=%t restart_from=review approval_revoked=true\nCI log:\n%s",
|
||||
f.headSHA, repairedHead, f.remoteHead(t), tc.keepsReviewedWork, f.log())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A manual repair - the one a person authorized by answering the CI gate with
|
||||
// a fix - takes exactly the same delivery decision as an automatic one. The
|
||||
// policy is about the cost of revalidating a repair, not about who asked for
|
||||
// it.
|
||||
func TestCIStep_ManualRepairFollowsTheSamePolicy(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
revalidate bool
|
||||
wantRestart bool
|
||||
wantRemoteMoved bool
|
||||
}{
|
||||
{name: "publish_policy", revalidate: false, wantRestart: false, wantRemoteMoved: true},
|
||||
{name: "revalidate_policy", revalidate: true, wantRestart: true, wantRemoteMoved: false},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
f := newCIRepairFixture(t, tc.revalidate, writeCIFix)
|
||||
// Automatic auto-fix off; the user answered the gate with "fix".
|
||||
f.sctx.Config.AutoFix = config.AutoFix{CI: 0}
|
||||
f.sctx.Fixing = true
|
||||
|
||||
outcome, err := f.run(t)
|
||||
// Under the publish policy the monitor deliberately does NOT
|
||||
// return after a repair, so it is still polling when the test's
|
||||
// poll budget cancels it. That cancellation is the observable
|
||||
// "kept monitoring", and it is the point of this case.
|
||||
if err != nil && !errors.Is(err, context.Canceled) {
|
||||
t.Fatalf("CI step returned error: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if tc.wantRestart && err != nil {
|
||||
t.Fatalf("the revalidation policy must leave the monitor cleanly, got: %v", err)
|
||||
}
|
||||
if !tc.wantRestart && !errors.Is(err, context.Canceled) {
|
||||
t.Fatalf("the publish policy must keep monitoring after a repair, got outcome %#v err %v", outcome, err)
|
||||
}
|
||||
if !strings.Contains(f.log(), "manual fix requested") {
|
||||
t.Fatalf("expected the manual repair path; log:\n%s", f.log())
|
||||
}
|
||||
if f.localHead(t) == f.headSHA {
|
||||
t.Fatal("the manual repair commit was never created")
|
||||
}
|
||||
gotRestart := outcome != nil && outcome.RestartFrom == types.StepReview
|
||||
if gotRestart != tc.wantRestart {
|
||||
t.Errorf("RestartFrom review = %v, want %v (outcome %#v)", gotRestart, tc.wantRestart, outcome)
|
||||
}
|
||||
if moved := f.remoteHead(t) != f.headSHA; moved != tc.wantRemoteMoved {
|
||||
t.Errorf("remote advanced = %v, want %v", moved, tc.wantRemoteMoved)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Continuity is proven against the run's durable review authority, so a run
|
||||
// that has none cannot prove anything: the repair revalidates rather than
|
||||
// publishing. Fail closed is the whole point - a missing approval is not a
|
||||
// reason to skip the check, it is a reason the check cannot pass.
|
||||
func TestCIStep_RepairWithoutReviewAuthorityRevalidatesRatherThanPublishing(t *testing.T) {
|
||||
t.Parallel()
|
||||
f := newCIRepairFixture(t, false, nil)
|
||||
writeCIFix(f.dir)
|
||||
if err := f.sctx.DB.UpdateRunReviewApprovedHeadSHA(f.sctx.Run.ID, ""); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
f.sctx.Run.ReviewApprovedHeadSHA = nil
|
||||
|
||||
repair, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check")
|
||||
if err != nil {
|
||||
t.Fatalf("CI repair returned error: %v", err)
|
||||
}
|
||||
if !repair.Revalidate {
|
||||
t.Fatalf("repair = %#v, want it held for revalidation", repair)
|
||||
}
|
||||
if f.remoteHead(t) != f.headSHA {
|
||||
t.Fatal("a repair was published without a recorded review-approved head")
|
||||
}
|
||||
if !strings.Contains(f.log(), "run has no durably recorded review-approved head") {
|
||||
t.Errorf("the log does not name the missing review authority; log:\n%s", f.log())
|
||||
}
|
||||
}
|
||||
|
||||
// Durable state is written before the live head advances, so a failed write
|
||||
// cannot leave the monitor watching a head the run record does not know about
|
||||
// while its stale review approval still stands.
|
||||
func TestCIStep_FailedRevalidationWriteDoesNotAdvanceTheLiveHead(t *testing.T) {
|
||||
f := newCIRepairFixture(t, true, nil)
|
||||
writeCIFix(f.dir)
|
||||
priorHead := f.sctx.Run.HeadSHA
|
||||
priorApproval := f.sctx.Run.ReviewApprovedHeadSHA
|
||||
|
||||
// Close the database so the durable revalidation write fails.
|
||||
if err := f.sctx.DB.Close(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
if _, err := (&CIStep{}).commitRepair(f.sctx, "repair the failing check"); err == nil {
|
||||
t.Fatal("a failed durable write was reported as a recorded repair")
|
||||
}
|
||||
if f.sctx.Run.HeadSHA != priorHead {
|
||||
t.Errorf("live head advanced to %s despite the failed write; want %s", f.sctx.Run.HeadSHA, priorHead)
|
||||
}
|
||||
if f.sctx.Run.ReviewApprovedHeadSHA != priorApproval {
|
||||
t.Error("review approval was revoked in memory despite the failed write")
|
||||
}
|
||||
}
|
||||
|
||||
// The delivery decision itself is covered by
|
||||
// TestCIStep_RevalidateRepairsPolicySelectsRepairDelivery without paying for a
|
||||
// monitor loop. This one test pays for it once, to pin the remaining wiring:
|
||||
// the monitor turns a held repair into a restart at Review, and states the
|
||||
// policy in force before it does anything.
|
||||
func TestCIStep_MonitorRestartsAtReviewForAHeldRepair(t *testing.T) {
|
||||
t.Parallel()
|
||||
f := newCIRepairFixture(t, true, writeCIFix)
|
||||
outcome, err := f.run(t)
|
||||
if err != nil {
|
||||
t.Fatalf("CI step returned error: %v\nlog:\n%s", err, f.log())
|
||||
}
|
||||
if outcome == nil || outcome.RestartFrom != types.StepReview {
|
||||
t.Fatalf("outcome = %#v, want a restart from Review", outcome)
|
||||
}
|
||||
if !strings.Contains(f.log(), "CI repair policy:") {
|
||||
t.Errorf("CI step did not report its repair policy; log:\n%s", f.log())
|
||||
}
|
||||
}
|
||||
@@ -228,6 +228,9 @@ func TestCIStep_Execute_FixMode_RemoteAlreadyUpdatedDoesNotReturnManualIntervent
|
||||
defer cancel()
|
||||
sctx.Ctx = ctx
|
||||
|
||||
// This test pins the ci.revalidate_repairs: true path, where the
|
||||
// repair is held locally until Review re-approves it.
|
||||
sctx.Config.CI.RevalidateRepairs = true
|
||||
step := &CIStep{
|
||||
waitForNextPoll: func(ctx context.Context, interval time.Duration) error {
|
||||
cancel()
|
||||
|
||||
@@ -219,6 +219,24 @@ func stepGitPush(sctx *pipeline.StepContext, remote, ref, expectedSHA string, fo
|
||||
return err
|
||||
}
|
||||
|
||||
// stepGitPushCommit pushes an explicit commit to a remote ref with the
|
||||
// StepContext's environment, mirroring git.PushCommit's argument assembly. The
|
||||
// explicit source SHA (rather than HEAD) is what lets a caller publish exactly
|
||||
// the commit it verified, even if the worktree moves underneath it.
|
||||
func stepGitPushCommit(sctx *pipeline.StepContext, remote, commitSHA, ref, expectedSHA string, forceWithLease bool) error {
|
||||
args := []string{"push", remote}
|
||||
if forceWithLease {
|
||||
if expectedSHA != "" {
|
||||
args = append(args, fmt.Sprintf("--force-with-lease=%s:%s", ref, expectedSHA))
|
||||
} else {
|
||||
args = append(args, "--force-with-lease")
|
||||
}
|
||||
}
|
||||
args = append(args, commitSHA+":"+ref)
|
||||
_, err := stepGitRun(sctx, args...)
|
||||
return err
|
||||
}
|
||||
|
||||
// stepCLIAvailable checks whether the provider CLI binary is available,
|
||||
// respecting any custom PATH in sctx.Env.
|
||||
func stepCLIAvailable(sctx *pipeline.StepContext, provider scm.Provider) bool {
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/kunchenguid/no-mistakes/internal/config"
|
||||
@@ -23,71 +24,107 @@ func fileAtRef(t *testing.T, dir, ref, path string) bool {
|
||||
// and force-pushes - discarding the origin-only commit. The lease was anchored
|
||||
// to a freshly-read ls-remote SHA, which never refuses.
|
||||
//
|
||||
// CI repairs stay local, so even a stale worktree cannot overwrite a commit
|
||||
// that reached the remote out of band. The later push step owns that check.
|
||||
// The out-of-band commit must survive under BOTH ci.revalidate_repairs
|
||||
// policies, for two different reasons, so the guarantee is pinned on each:
|
||||
// with revalidation the repair never leaves the worktree at all, and without
|
||||
// it the shared guarded publication path refuses the force-push rather than
|
||||
// discarding a commit this run never incorporated.
|
||||
func TestCIStep_CommitAndPush_DoesNotClobberUnseenUpstreamCommit(t *testing.T) {
|
||||
t.Parallel()
|
||||
upstream := t.TempDir()
|
||||
gitCmd(t, upstream, "init", "--bare")
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
revalidateRepairs bool
|
||||
wantChanged bool
|
||||
wantRefusal bool
|
||||
}{
|
||||
{name: "revalidation_keeps_the_repair_local", revalidateRepairs: true, wantChanged: true},
|
||||
{name: "publication_refuses_to_discard_the_unseen_commit", revalidateRepairs: false, wantRefusal: true},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
upstream := t.TempDir()
|
||||
gitCmd(t, upstream, "init", "--bare")
|
||||
|
||||
dir := t.TempDir()
|
||||
gitCmd(t, dir, "init")
|
||||
gitCmd(t, dir, "config", "user.name", "test")
|
||||
gitCmd(t, dir, "config", "user.email", "test@test.com")
|
||||
gitCmd(t, dir, "checkout", "-b", "main")
|
||||
os.WriteFile(filepath.Join(dir, "init.txt"), []byte("init"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "initial")
|
||||
baseSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "remote", "add", "origin", upstream)
|
||||
gitCmd(t, dir, "push", "origin", "main")
|
||||
dir := t.TempDir()
|
||||
gitCmd(t, dir, "init")
|
||||
gitCmd(t, dir, "config", "user.name", "test")
|
||||
gitCmd(t, dir, "config", "user.email", "test@test.com")
|
||||
gitCmd(t, dir, "checkout", "-b", "main")
|
||||
os.WriteFile(filepath.Join(dir, "init.txt"), []byte("init"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "initial")
|
||||
baseSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "remote", "add", "origin", upstream)
|
||||
gitCmd(t, dir, "push", "origin", "main")
|
||||
|
||||
gitCmd(t, dir, "checkout", "-b", "feature")
|
||||
os.WriteFile(filepath.Join(dir, "feature.txt"), []byte("feature"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "feature")
|
||||
headSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "push", "origin", "feature") // origin feature == H1, what no-mistakes last saw
|
||||
gitCmd(t, dir, "checkout", "-b", "feature")
|
||||
os.WriteFile(filepath.Join(dir, "feature.txt"), []byte("feature"), 0o644)
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "feature")
|
||||
headSHA := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
gitCmd(t, dir, "push", "origin", "feature") // origin feature == H1, what no-mistakes last saw
|
||||
|
||||
// Out-of-band: a reviewed commit is pushed to origin only, via a separate
|
||||
// clone, so the gate worktree never sees it.
|
||||
other := t.TempDir()
|
||||
gitCmd(t, other, "clone", upstream, ".")
|
||||
gitCmd(t, other, "config", "user.name", "other")
|
||||
gitCmd(t, other, "config", "user.email", "other@test.com")
|
||||
gitCmd(t, other, "checkout", "feature")
|
||||
os.WriteFile(filepath.Join(other, "approved.txt"), []byte("approved review fix"), 0o644)
|
||||
gitCmd(t, other, "add", "-A")
|
||||
gitCmd(t, other, "commit", "-m", "approved review fix")
|
||||
approvedSHA := gitCmd(t, other, "rev-parse", "HEAD")
|
||||
gitCmd(t, other, "push", "origin", "feature") // origin feature == H2 (has approved.txt)
|
||||
// Out-of-band: a reviewed commit is pushed to origin only, via a separate
|
||||
// clone, so the gate worktree never sees it.
|
||||
other := t.TempDir()
|
||||
gitCmd(t, other, "clone", upstream, ".")
|
||||
gitCmd(t, other, "config", "user.name", "other")
|
||||
gitCmd(t, other, "config", "user.email", "other@test.com")
|
||||
gitCmd(t, other, "checkout", "feature")
|
||||
os.WriteFile(filepath.Join(other, "approved.txt"), []byte("approved review fix"), 0o644)
|
||||
gitCmd(t, other, "add", "-A")
|
||||
gitCmd(t, other, "commit", "-m", "approved review fix")
|
||||
approvedSHA := gitCmd(t, other, "rev-parse", "HEAD")
|
||||
gitCmd(t, other, "push", "origin", "feature") // origin feature == H2 (has approved.txt)
|
||||
|
||||
// The CI auto-fix agent produces a new head in the worktree that does NOT
|
||||
// contain the approved commit (simulating a rebase from stale local state).
|
||||
os.WriteFile(filepath.Join(dir, "ci-fix.txt"), []byte("ci fix"), 0o644)
|
||||
// The CI auto-fix agent produces a new head in the worktree that does NOT
|
||||
// contain the approved commit (simulating a rebase from stale local state).
|
||||
os.WriteFile(filepath.Join(dir, "ci-fix.txt"), []byte("ci fix"), 0o644)
|
||||
|
||||
ag := &mockAgent{name: "test"}
|
||||
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Run.HeadSHA = headSHA // gate's last-recorded head == H1
|
||||
ag := &mockAgent{name: "test"}
|
||||
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
|
||||
sctx.Repo.UpstreamURL = upstream
|
||||
sctx.Run.Branch = "refs/heads/feature"
|
||||
sctx.Run.HeadSHA = headSHA // gate's last-recorded head == H1
|
||||
sctx.Config.CI.RevalidateRepairs = tc.revalidateRepairs
|
||||
// Review approved H1, so the publication path's review-continuity
|
||||
// guard passes and the force-push lease is the check under test.
|
||||
if err := sctx.DB.UpdateRunReviewApprovedHeadSHA(sctx.Run.ID, headSHA); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
step := &CIStep{}
|
||||
changed, err := step.commitAndPush(sctx)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !changed {
|
||||
t.Fatal("expected the CI repair to be committed locally")
|
||||
}
|
||||
step := &CIStep{}
|
||||
repair, err := step.commitAndPush(sctx)
|
||||
switch {
|
||||
case tc.wantRefusal:
|
||||
if err == nil {
|
||||
t.Fatal("expected the publication path to refuse the force-push")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "refusing to force-push") {
|
||||
t.Fatalf("error = %v, want a force-push refusal", err)
|
||||
}
|
||||
if repair.HeadAdvanced {
|
||||
t.Fatal("a refused publication must not report the repair as delivered")
|
||||
}
|
||||
default:
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if repair.HeadAdvanced != tc.wantChanged {
|
||||
t.Fatalf("HeadAdvanced = %v, want %v", repair.HeadAdvanced, tc.wantChanged)
|
||||
}
|
||||
}
|
||||
|
||||
// The approved commit must still be on origin.
|
||||
originSHA := gitCmd(t, upstream, "rev-parse", "refs/heads/feature")
|
||||
if originSHA != approvedSHA {
|
||||
t.Fatalf("origin feature SHA = %s, want %s (approved commit must be preserved)", originSHA, approvedSHA)
|
||||
}
|
||||
if !fileAtRef(t, upstream, "refs/heads/feature", "approved.txt") {
|
||||
t.Fatalf("approved.txt was discarded from origin - data loss")
|
||||
// The approved commit must still be on origin.
|
||||
originSHA := gitCmd(t, upstream, "rev-parse", "refs/heads/feature")
|
||||
if originSHA != approvedSHA {
|
||||
t.Fatalf("origin feature SHA = %s, want %s (approved commit must be preserved)", originSHA, approvedSHA)
|
||||
}
|
||||
if !fileAtRef(t, upstream, "refs/heads/feature", "approved.txt") {
|
||||
t.Fatalf("approved.txt was discarded from origin - data loss")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+147
-89
@@ -61,6 +61,41 @@ func (s *PushStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, e
|
||||
newHeadSHA = headSHA
|
||||
}
|
||||
|
||||
headBeingPushed, err := git.HeadSHA(ctx, sctx.WorkDir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("resolve head before push: %w", err)
|
||||
}
|
||||
if err := publishRunHead(sctx, headBeingPushed, newHeadSHA); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
sctx.Log("pushed successfully")
|
||||
return &pipeline.StepOutcome{}, nil
|
||||
}
|
||||
|
||||
// publishRunHead is the single guarded publication path for a run's head. Both
|
||||
// the Push step and a CI repair published without revalidation
|
||||
// (ci.revalidate_repairs: false) go through it, so the review-approved-head
|
||||
// continuity check, the force-with-lease anchor, the remote verification, the
|
||||
// push binding, and the gate-mirror update are written once and can never
|
||||
// drift apart between the two callers.
|
||||
//
|
||||
// localRefUpdate, when non-empty, is the SHA the run's local branch ref is
|
||||
// moved to after a verified push. Callers that already advanced the ref with
|
||||
// their commit pass "".
|
||||
//
|
||||
// Every worktree git call here is step-scoped (stepGitRun), not git.Run,
|
||||
// because the CI step runs with a step-local PATH and credential environment
|
||||
// that a plain runner would not see. Gate-mirror calls stay on git.Run: they
|
||||
// operate on the bare gate directory, not the run worktree.
|
||||
// Publication becomes durable only after the remote and gate mirror settle;
|
||||
// the push binding and recorded head then land in one database update.
|
||||
//
|
||||
// It deliberately does not relax the review-approved-head check for anyone.
|
||||
// Whether a CI repair may be published at all is decided before publication, by
|
||||
// ciRepairContinuityGap.
|
||||
func publishRunHead(sctx *pipeline.StepContext, headBeingPushed, localRefUpdate string) error {
|
||||
ctx := sctx.Ctx
|
||||
ref := normalizedBranchRef(sctx.Run.Branch)
|
||||
branch := strings.TrimPrefix(ref, "refs/heads/")
|
||||
|
||||
@@ -74,12 +109,8 @@ func (s *PushStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, e
|
||||
sctx.Log(fmt.Sprintf("pushing to %s (%s)...", safeurl.Redact(pushURL), ref))
|
||||
}
|
||||
|
||||
headBeingPushed, err := git.HeadSHA(ctx, sctx.WorkDir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("resolve head before push: %w", err)
|
||||
}
|
||||
if err := assertReviewApprovedPushHead(sctx, headBeingPushed); err != nil {
|
||||
return nil, err
|
||||
return err
|
||||
}
|
||||
|
||||
// Decide whether force-pushing would discard commits the pipeline never saw.
|
||||
@@ -90,128 +121,155 @@ func (s *PushStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, e
|
||||
// A bare --force-with-lease offers no protection when pushing to a URL (no
|
||||
// remote-tracking refs), so the anchor is explicit.
|
||||
lastSeen := lastKnownBranchTip(ctx, sctx, branch, usingFork)
|
||||
gitRun := func(args ...string) (string, error) { return git.Run(ctx, sctx.WorkDir, args...) }
|
||||
gitRun := func(args ...string) (string, error) { return stepGitRun(sctx, args...) }
|
||||
decision, err := resolveForcePushDecision(gitRun, pushURL, ref, headBeingPushed, lastSeen, sctx.Run.BaseSHA)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
return fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
}
|
||||
switch {
|
||||
case decision.newBranch:
|
||||
// New branch: regular push (no force needed).
|
||||
if err := git.PushCommit(ctx, sctx.WorkDir, pushURL, headBeingPushed, ref, "", false); err != nil {
|
||||
return nil, fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
if err := stepGitPushCommit(sctx, pushURL, headBeingPushed, ref, "", false); err != nil {
|
||||
return fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
}
|
||||
case decision.upToDate:
|
||||
// Remote already at this exact head. This freshly verified equality is a
|
||||
// successful binding even though no objects needed to move.
|
||||
default:
|
||||
// Existing branch: force-with-lease anchored to the verified remote head.
|
||||
if err := git.PushCommit(ctx, sctx.WorkDir, pushURL, headBeingPushed, ref, decision.remoteSHA, true); err != nil {
|
||||
return nil, fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
if err := stepGitPushCommit(sctx, pushURL, headBeingPushed, ref, decision.remoteSHA, true); err != nil {
|
||||
return fmt.Errorf("push to %s: %w", pushTarget, err)
|
||||
}
|
||||
}
|
||||
verifiedRemote, err := git.LsRemote(ctx, sctx.WorkDir, pushURL, ref)
|
||||
verifiedRemote, err := lsRemoteSHA(gitRun, pushURL, ref)
|
||||
if err != nil || verifiedRemote != headBeingPushed {
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("verify successful push to %s: %w", pushTarget, err)
|
||||
return fmt.Errorf("verify successful push to %s: %w", pushTarget, err)
|
||||
}
|
||||
return nil, fmt.Errorf("verify successful push to %s: remote head %s does not equal pushed head %s", pushTarget, verifiedRemote, headBeingPushed)
|
||||
return fmt.Errorf("verify successful push to %s: remote head %s does not equal pushed head %s", pushTarget, verifiedRemote, headBeingPushed)
|
||||
}
|
||||
if err := sctx.DB.UpdateRunPushBinding(sctx.Run.ID, db.PushBinding{
|
||||
// Settle the gate mirror BEFORE recording the publication. The remote
|
||||
// already has the head, but a run is only "published" once the gate mirror
|
||||
// carries it too: `no-mistakes rerun` resolves its starting head from the
|
||||
// gate, so a head recorded as published while the gate is behind is a head
|
||||
// a later rerun silently omits.
|
||||
//
|
||||
// Ordering it here is what makes a mirror failure retryable instead of
|
||||
// having to choose between two wrong answers. Nothing durable has been
|
||||
// written yet, so the caller's next attempt re-enters this path, finds the
|
||||
// remote already at this head (an up-to-date no-op push), and retries the
|
||||
// mirror. The alternative orderings both lose: recording first and
|
||||
// returning the error makes the CI monitor treat an already published
|
||||
// repair as a failed one, and recording first and swallowing the error
|
||||
// strands the gate behind the remote for good.
|
||||
if err := updateGateMirrorAfterPush(ctx, sctx, ref, headBeingPushed); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if localRefUpdate != "" {
|
||||
if _, err := stepGitRun(sctx, "update-ref", ref, localRefUpdate); err != nil {
|
||||
return fmt.Errorf("update local branch ref: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
if err := sctx.DB.UpdateRunPublication(sctx.Run.ID, db.PushBinding{
|
||||
HeadSHA: headBeingPushed,
|
||||
TargetKind: pushTarget,
|
||||
TargetFingerprint: branchsync.TargetFingerprint(pushURL),
|
||||
Ref: ref,
|
||||
}); err != nil {
|
||||
return nil, err
|
||||
return err
|
||||
}
|
||||
|
||||
if newHeadSHA != "" {
|
||||
if _, err := git.Run(ctx, sctx.WorkDir, "update-ref", ref, newHeadSHA); err != nil {
|
||||
return nil, fmt.Errorf("update local branch ref: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Persist the immutable source that was verified and delivered, never a
|
||||
// fresh read of mutable worktree HEAD after the push.
|
||||
if headBeingPushed != sctx.Run.HeadSHA {
|
||||
sctx.Run.HeadSHA = headBeingPushed
|
||||
if err := sctx.DB.UpdateRunHeadSHA(sctx.Run.ID, headBeingPushed); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
|
||||
// Update the gate mirror's ref so follow-up pushes to the gate proxy
|
||||
// remain fast-forwardable after pipeline rebases.
|
||||
if sctx.Repo != nil && strings.TrimSpace(sctx.GateDir) != "" {
|
||||
gateDir := strings.TrimSpace(sctx.GateDir)
|
||||
if _, statErr := os.Stat(gateDir); statErr != nil {
|
||||
if !os.IsNotExist(statErr) {
|
||||
return nil, fmt.Errorf("stat gate mirror repository: %w", statErr)
|
||||
}
|
||||
} else {
|
||||
if err := git.ValidateBareRepository(ctx, gateDir); err != nil {
|
||||
return nil, fmt.Errorf("update gate mirror ref %s: validate repository: %w", ref, err)
|
||||
}
|
||||
|
||||
if fetchErr := git.FetchRemoteRef(ctx, gateDir, sctx.WorkDir, headBeingPushed, headBeingPushed); fetchErr != nil {
|
||||
return nil, fmt.Errorf("update gate mirror ref %s: fetch pushed head: %w", ref, fetchErr)
|
||||
}
|
||||
|
||||
gateTip, _ := git.Run(ctx, gateDir, "rev-parse", "--verify", ref)
|
||||
gateTip = strings.TrimSpace(gateTip)
|
||||
|
||||
submittedHead := ""
|
||||
if sctx.Run.SubmittedHeadSHA != nil {
|
||||
submittedHead = strings.TrimSpace(*sctx.Run.SubmittedHeadSHA)
|
||||
}
|
||||
|
||||
shouldUpdate := gateTip == "" || gateTip == headBeingPushed || (submittedHead != "" && gateTip == submittedHead)
|
||||
if !shouldUpdate {
|
||||
if _, err := git.Run(ctx, gateDir, "merge-base", "--is-ancestor", headBeingPushed, gateTip); err == nil {
|
||||
// Preserve a newer descendant.
|
||||
shouldUpdate = false
|
||||
} else if _, err := git.Run(ctx, gateDir, "merge-base", "--is-ancestor", gateTip, headBeingPushed); err == nil {
|
||||
// Fast-forward advance from an older ancestor.
|
||||
shouldUpdate = true
|
||||
} else {
|
||||
return nil, fmt.Errorf("gate mirror ref %s at %s diverged from pushed head %s", ref, gateTip, headBeingPushed)
|
||||
}
|
||||
}
|
||||
if shouldUpdate {
|
||||
if _, updateErr := git.Run(ctx, gateDir, "update-ref", ref, headBeingPushed, gateTip); updateErr != nil {
|
||||
return nil, fmt.Errorf("update gate mirror ref %s to %s: %w", ref, headBeingPushed, updateErr)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
sctx.Log("pushed successfully")
|
||||
return &pipeline.StepOutcome{}, nil
|
||||
sctx.Run.HeadSHA = headBeingPushed
|
||||
return nil
|
||||
}
|
||||
|
||||
func updateGateMirrorAfterPush(ctx context.Context, sctx *pipeline.StepContext, ref, headBeingPushed string) error {
|
||||
if sctx.Repo == nil || strings.TrimSpace(sctx.GateDir) == "" {
|
||||
return nil
|
||||
}
|
||||
gateDir := strings.TrimSpace(sctx.GateDir)
|
||||
if _, statErr := os.Stat(gateDir); statErr != nil {
|
||||
if os.IsNotExist(statErr) {
|
||||
return nil
|
||||
}
|
||||
return fmt.Errorf("stat gate mirror repository: %w", statErr)
|
||||
}
|
||||
if err := git.ValidateBareRepository(ctx, gateDir); err != nil {
|
||||
return fmt.Errorf("update gate mirror ref %s: validate repository: %w", ref, err)
|
||||
}
|
||||
|
||||
if fetchErr := git.FetchRemoteRef(ctx, gateDir, sctx.WorkDir, headBeingPushed, headBeingPushed); fetchErr != nil {
|
||||
return fmt.Errorf("update gate mirror ref %s: fetch pushed head: %w", ref, fetchErr)
|
||||
}
|
||||
|
||||
gateTip, _ := git.Run(ctx, gateDir, "rev-parse", "--verify", ref)
|
||||
gateTip = strings.TrimSpace(gateTip)
|
||||
|
||||
submittedHead := ""
|
||||
if sctx.Run.SubmittedHeadSHA != nil {
|
||||
submittedHead = strings.TrimSpace(*sctx.Run.SubmittedHeadSHA)
|
||||
}
|
||||
|
||||
shouldUpdate := gateTip == "" || gateTip == headBeingPushed || (submittedHead != "" && gateTip == submittedHead)
|
||||
if !shouldUpdate {
|
||||
if _, err := git.Run(ctx, gateDir, "merge-base", "--is-ancestor", headBeingPushed, gateTip); err == nil {
|
||||
// Preserve a newer descendant.
|
||||
shouldUpdate = false
|
||||
} else if _, err := git.Run(ctx, gateDir, "merge-base", "--is-ancestor", gateTip, headBeingPushed); err == nil {
|
||||
// Fast-forward advance from an older ancestor.
|
||||
shouldUpdate = true
|
||||
} else {
|
||||
return fmt.Errorf("gate mirror ref %s at %s diverged from pushed head %s", ref, gateTip, headBeingPushed)
|
||||
}
|
||||
}
|
||||
if shouldUpdate {
|
||||
if _, updateErr := git.Run(ctx, gateDir, "update-ref", ref, headBeingPushed, gateTip); updateErr != nil {
|
||||
return fmt.Errorf("update gate mirror ref %s to %s: %w", ref, headBeingPushed, updateErr)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// assertReviewApprovedPushHead refuses to publish a head that is not the
|
||||
// durably review-approved commit or a descendant of it. There is no exception:
|
||||
// a head that cannot show that ancestry has not been reviewed, and the CI
|
||||
// repair path answers that case by revalidating instead of publishing.
|
||||
func assertReviewApprovedPushHead(sctx *pipeline.StepContext, proposedHead string) error {
|
||||
run, err := sctx.DB.GetRun(sctx.Run.ID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("load durable review approval before push: %w", err)
|
||||
}
|
||||
approvedHead, reason := reviewApprovedHead(sctx, run)
|
||||
if approvedHead == "" {
|
||||
return fmt.Errorf("refusing to push: %s", reason)
|
||||
}
|
||||
if proposedHead == approvedHead {
|
||||
return nil
|
||||
}
|
||||
if _, err := stepGitRun(sctx, "merge-base", "--is-ancestor", approvedHead, proposedHead); err != nil {
|
||||
return fmt.Errorf("refusing to push: proposed head %s violates continuity with review-approved head %s (it is not an equal or descendant commit)", shortObjectID(proposedHead), shortObjectID(approvedHead))
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// reviewApprovedHead returns the run's durable review-approved commit, or ""
|
||||
// plus the reason it is unusable. It is the single reader of that authority, so
|
||||
// the pre-publication continuity decision and the publication guard itself can
|
||||
// never disagree about what "reviewed" means.
|
||||
func reviewApprovedHead(sctx *pipeline.StepContext, run *db.Run) (string, string) {
|
||||
if run == nil || run.ReviewApprovedHeadSHA == nil || strings.TrimSpace(*run.ReviewApprovedHeadSHA) == "" {
|
||||
return fmt.Errorf("refusing to push: run has no durably recorded review-approved head")
|
||||
return "", "run has no durably recorded review-approved head"
|
||||
}
|
||||
approvedHead := strings.TrimSpace(*run.ReviewApprovedHeadSHA)
|
||||
if !isFullGitObjectID(approvedHead) {
|
||||
return fmt.Errorf("refusing to push: durable review-approved head is malformed")
|
||||
return "", "durable review-approved head is malformed"
|
||||
}
|
||||
resolved, err := git.Run(sctx.Ctx, sctx.WorkDir, "rev-parse", "--verify", approvedHead+"^{commit}")
|
||||
resolved, err := stepGitRun(sctx, "rev-parse", "--verify", approvedHead+"^{commit}")
|
||||
if err != nil || !strings.EqualFold(strings.TrimSpace(resolved), approvedHead) {
|
||||
return fmt.Errorf("refusing to push: durable review-approved head is unreachable")
|
||||
return "", "durable review-approved head is unreachable"
|
||||
}
|
||||
if proposedHead != approvedHead {
|
||||
if _, err := git.Run(sctx.Ctx, sctx.WorkDir, "merge-base", "--is-ancestor", approvedHead, proposedHead); err != nil {
|
||||
return fmt.Errorf("refusing to push: proposed head %s violates continuity with review-approved head %s (it is not an equal or descendant commit)", shortObjectID(proposedHead), shortObjectID(approvedHead))
|
||||
}
|
||||
}
|
||||
return nil
|
||||
return approvedHead, ""
|
||||
}
|
||||
|
||||
func isFullGitObjectID(value string) bool {
|
||||
|
||||
@@ -183,6 +183,44 @@ func TestAssertReviewApprovedPushHead_RefusesMissingLegacyState(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestAssertReviewApprovedPushHead_UsesStepScopedGit(t *testing.T) {
|
||||
dir, baseSHA, approvedHead := setupGitRepo(t)
|
||||
sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, baseSHA, approvedHead, config.Commands{})
|
||||
recordReviewApproval(t, sctx, approvedHead)
|
||||
if err := os.WriteFile(filepath.Join(dir, "descendant.txt"), []byte("descendant\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
gitCmd(t, dir, "add", "-A")
|
||||
gitCmd(t, dir, "commit", "-m", "descendant")
|
||||
proposedHead := gitCmd(t, dir, "rev-parse", "HEAD")
|
||||
|
||||
realGit, err := exec.LookPath("git")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
binDir := fakeCLIBinDir(t)
|
||||
linkTestBinary(t, binDir, "git")
|
||||
logFile := filepath.Join(t.TempDir(), "git.log")
|
||||
sctx.Env = []string{
|
||||
"PATH=" + binDir + string(os.PathListSeparator) + os.Getenv("PATH"),
|
||||
"FAKE_CLI_MODE=git-passthrough",
|
||||
"FAKE_CLI_REAL_GIT=" + realGit,
|
||||
"FAKE_CLI_LOG=" + logFile,
|
||||
}
|
||||
|
||||
if err := assertReviewApprovedPushHead(sctx, proposedHead); err != nil {
|
||||
t.Fatalf("expected descendant approval, got %v", err)
|
||||
}
|
||||
logBytes, err := os.ReadFile(logFile)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
logText := string(logBytes)
|
||||
if !strings.Contains(logText, "rev-parse --verify") || !strings.Contains(logText, "merge-base --is-ancestor") {
|
||||
t.Fatalf("step-scoped git did not run both continuity checks; log:\n%s", logText)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPushStep_BindsRemoteAndDatabaseToVerifiedCommitWhenHEADMovesDuringPush(t *testing.T) {
|
||||
upstream := t.TempDir()
|
||||
gitCmd(t, upstream, "init", "--bare")
|
||||
|
||||
@@ -255,8 +255,9 @@ Never treat "no CI checks reported" alone as green.
|
||||
Because that monitor stays live, a PR that falls behind the default branch or
|
||||
hits a merge conflict after checks pass - commonly because another PR merged
|
||||
first - needs **no command from you**: never hand-rebase. When the CI monitor
|
||||
sees an actual conflict it **rebases onto the base, resolves it, restarts
|
||||
validation at Review, and re-pushes the branch through Push**; a PR that is merely behind but still clean needs nothing
|
||||
sees an actual conflict it **rebases onto the base, resolves it, revalidates from Review
|
||||
because rebasing cannot prove continuity with the reviewed head, and re-pushes
|
||||
the branch through Push**; a PR that is merely behind but still clean needs nothing
|
||||
either, since the platform merges it. The one exception is when that monitor is
|
||||
no longer running - the PR was closed, the run was aborted or superseded, it
|
||||
idle-timed-out, or its auto-fix attempts were exhausted - in which case recover
|
||||
|
||||
@@ -255,8 +255,9 @@ Never treat "no CI checks reported" alone as green.
|
||||
Because that monitor stays live, a PR that falls behind the default branch or
|
||||
hits a merge conflict after checks pass - commonly because another PR merged
|
||||
first - needs **no command from you**: never hand-rebase. When the CI monitor
|
||||
sees an actual conflict it **rebases onto the base, resolves it, restarts
|
||||
validation at Review, and re-pushes the branch through Push**; a PR that is merely behind but still clean needs nothing
|
||||
sees an actual conflict it **rebases onto the base, resolves it, revalidates from Review
|
||||
because rebasing cannot prove continuity with the reviewed head, and re-pushes
|
||||
the branch through Push**; a PR that is merely behind but still clean needs nothing
|
||||
either, since the platform merges it. The one exception is when that monitor is
|
||||
no longer running - the PR was closed, the run was aborted or superseded, it
|
||||
idle-timed-out, or its auto-fix attempts were exhausted - in which case recover
|
||||
|
||||
Reference in New Issue
Block a user