diff --git a/assets/land-fix.md b/assets/land-fix.md index fd45f767..df03c4bf 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -28,7 +28,8 @@ For each PR, requeue it unchanged if healthy (if the failure was flaky, name the flaky test in its `failure` so it can be followed up), commit a mechanical fix on its own branch, or hand it back with diagnosis and recommendation. Allowed fixes: preserve both sides' intent in conflicts, format/lint, regenerate -lockfiles/generated files, or follow a base rename. Merge base where practical. +generated files, rebuild lockfiles as [Lockfiles](#lockfiles) says, or follow a +base rename. Merge base where practical. flake.nix, flake.lock, shell.nix, default.nix, nix/, .envrc, .github/, .gitmodules, and gitlinks (submodule entries) anywhere are protected. Merging the base may carry the base's own changes to those paths: that is no edit of yours when they come out @@ -79,6 +80,47 @@ A fixed head must be the tip of pr-, descend from its pinned head, and contain only that PR's fix and base history. Leave all other PR branches intact. Keep the result file uncommitted. +## Lockfiles + +When merging the base leaves a lockfile (`bun.lock`, `package-lock.json`, +`pnpm-lock.yaml`, `yarn.lock`, `Cargo.lock`, `uv.lock`, `poetry.lock`, `go.sum`, +and the like) conflicted or out of date with the merged manifests, rebuild it from +the base's copy, never from the PR's side: the base's copy carries dependency +changes the base made, such as a security bump, that the PR's copy would revert. + +1. Resolve the manifests first (`package.json`, `Cargo.toml`, `pyproject.toml`, + `go.mod`, and the like), preserving both sides' intent. +2. Take the base's lockfile: `git checkout base -- `. +3. Run the package manager's minimal resolve against the merged manifests, such + as `bun install`, `npm install`, `cargo update --workspace` or a `cargo build`, + `uv lock`, or `go mod tidy`. Never run `cargo generate-lockfile`, `bun update`, + `uv lock --upgrade`, a `cargo update` without `--workspace`, or another update + that moves versions neither side asked for. +4. Reapply any lockfile change the PR made without a manifest change, such as a + `cargo update -p`, with a pinned targeted update such as + `cargo update -p --precise `. Where the base also moved that + package, keep the base's version if it is at or past the PR's. +5. Compare the result with the base's copy (`git diff base -- `) and + with the PR's own changes (`git diff base...pr- -- `): + every lockfile change, and any manifest change the package manager made itself + (such as `go mod tidy` editing `go.mod`), must trace to a dependency or import + change in the PR. + +Name the lockfiles you rebuilt in the reason. Hand back instead of taking either +side's copy, quoting the command's output or the offending diff in the diagnosis, +when: + +- the package manager cannot rebuild the lockfile, or cannot reapply the PR's own + lockfile change with a pinned update; +- a change does not trace to the PR; +- the PR's manifest no longer admits a version the base moved to, such as a pin + below the base's security bump, or the base moved a package below the PR's + version: keeping either version is a behavior decision, not a mechanical fix. + +`flake.lock` is a protected path. When the fix must edit it, as the protected-path +rule above allows, rebuild it the same way from the base's copy, with +`nix flake lock` as the minimal resolve. + ## Flaky tests A failure is flaky when it comes and goes with no cause in the PR or the base, diff --git a/quest/m0/README.md b/quest/m0/README.md index 399586ab..e6e1f0cf 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -84,7 +84,7 @@ ownership-manifest design: - [Land through the merge queue with an async fixer](/quest/m0/queue-watcher/README.md) - GitHub's merge queue lands one PR at a time, and a host watcher fixes and re-enqueues each PR it drops - [Merge through a host gate](/quest/m0/host-land/README.md) - the host's fixer containers, and the frozen batched gate for the repositories not yet switched -- [The land fixer resolves every mechanical failure](/quest/m0/land-fixer/README.md) - lockfile merges, protected edits as ready commits, workflow pushes, and the PR's own CI as context +- [The land fixer resolves every mechanical failure](/quest/m0/land-fixer/README.md) - workflow pushes and the PR's own CI as context - [Checks are judged by their latest run](/quest/m0/latest-check-runs.md) - `quest status`, `quest wait`, and the gate count each check's latest run on the head, as GitHub does, so a superseded cancelled or failed run stops failing them or refusing release-plz PRs GitHub would merge - [Current landing status](/quest/m0/status-landing.md) - `quest status` clears a stale `landing-stopped` reason once the PR is queued again - [Agents don't stall](/quest/m0/agent-stalls.md) - long waits block in the same turn, pre-existing failures don't stop review, and reviewer outages become waits diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index 69578152..39a800bb 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -124,8 +124,11 @@ Decision prompts, selected options marked ✅: closed unmerged by the merge-queue switch below. - Amended 2026-11-01 (maintainer, /quest-plan in moq-dev/moq): a fixer merge on [moq-dev/moq#7318](https://github.com/moq-dev/moq/pull/7318) rebuilt - `bun.lock` from the PR's side and undid the base's advisory fix; see - [Lockfile merges](/quest/m0/land-fixer/lockfile.md). + `bun.lock` from the PR's side and undid the base's advisory fix. + [moq-dev/quest#410](https://github.com/moq-dev/quest/pull/410) gave + `assets/land-fix.md` a Lockfiles procedure: rebuild from the base's copy with + the package manager's minimal resolve, and hand back any change that does not + trace to the PR. - Superseded 2026-11-01 (maintainer, /quest-plan; see the [queue watcher](/quest/m0/queue-watcher/README.md)): repositories land @@ -151,7 +154,6 @@ work, done once the Required quests below have merged. ## Required - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted -- [Lockfile merges](/quest/m0/land-fixer/lockfile.md) - a conflicted lockfile is rebuilt from the base's copy, so a fix never reverts the base's dependency changes - [PR CI as a signal](/quest/m0/land-fixer/pr-signal.md) - `/quest-merge` and the fixer read a PR's own CI without gating on it; public repos may run it, private ones need not ## Related diff --git a/quest/m0/land-fixer/lockfile.md b/quest/m0/land-fixer/lockfile.md deleted file mode 100644 index f09b7ef7..00000000 --- a/quest/m0/land-fixer/lockfile.md +++ /dev/null @@ -1,64 +0,0 @@ -# [S] The fixer rebuilds a conflicted lockfile from the base's copy - -## Goal - -When the fixer resolves a lockfile while merging the base (`bun.lock`, -`Cargo.lock`, `uv.lock`, `go.sum`, and the like), it starts -from the base's copy and runs the package manager, so the result adds only -the PR's own dependency changes. A fixer merge never reverts a dependency -change the base made, such as a security bump. - -Non-goals: running `bun audit` or another advisory scan in a repository's -land check; that is each repository's CI choice. - -## Plan - -Decided 2026-11-01 (maintainer, /quest-plan in moq-dev/moq). After `quest land` -removed [moq-dev/moq#7318](https://github.com/moq-dev/moq/pull/7318) with -`merge_conflict`, the fixer pushed its own merge of main, -[229973492b](https://github.com/moq-dev/moq/commit/229973492ba403cd414161c69c98e67680907e23), -which rebuilt `bun.lock` from the PR's side instead of main's. That undid -[moq-dev/moq#7443](https://github.com/moq-dev/moq/pull/7443)'s glob/minimatch -advisory fix: `glob@10.5.0` (with `jackspeak` and `path-scurry@1.11.1`) and -`minimatch@9.0.9` came back, and `bun audit` reported two moderate advisories, -GHSA-3g3m-2fjr-6pw4 and GHSA-g3hv-75wg-m249. The land check missed it because -moq runs `bun audit` only in Nightly. Redoing the merge from main's `bun.lock` -plus `bun install` added only the PR's packages and audited clean -([evidence](https://github.com/moq-dev/moq/pull/7318#issuecomment-6558969580)). - -- ✅ Plan it: start from the base's lockfile and regenerate, size S, in this - epic (m0) - -The fixer's merge resolution is Codex following `assets/land-fix.md`, whose -only lockfile rule is "regenerate lockfiles/generated files"; nothing says -which side to start from. Replace it with a procedure: for each lockfile the -merge touches, check out the `base` branch's copy (`git checkout base -- -`), apply the PR's manifest changes (merged `package.json`, -`Cargo.toml`, and so on), and run the package manager's minimal resolve -(`bun install`, a `cargo` build or `cargo update --workspace`, `uv lock`, -`go mod tidy`; never `cargo generate-lockfile` or an unpinned update). Then -compare the result with the base's copy and the merged manifests: every -lockfile change, and any manifest change the package manager itself made -(such as `go mod tidy` editing `go.mod`), must trace to a dependency or import -change in the PR. The fix reason names the lockfiles it rebuilt. - -Hand back, quoting the command's output or the offending diff, instead of -taking either side's copy when: - -- the package manager cannot rebuild the lockfile; -- a change does not trace to the PR; -- the PR's manifest no longer admits a version the base moved to, such as a - pin below the base's security bump. Keeping either version is a behavior - decision, not a mechanical fix. - -The prompt already lets build tools fetch from package registries (from -[moq-dev/quest#356](https://github.com/moq-dev/quest/pull/356)), which -regeneration needs. `flake.lock` stays a protected path in `validateFix`, and -this quest does not open it: a fixer-written `flake.lock` comes back as a -ready commit through -[moq-dev/quest#409](https://github.com/moq-dev/quest/pull/409), -and that edit follows the same rule, rebuilt from the base's copy with -`nix flake lock`. That PR also edited `assets/land-fix.md`: merge its wording. - -Cover the rule in `test/land.test.ts`, which already asserts the prompt's -contents. diff --git a/test/land.test.ts b/test/land.test.ts index 6ef0724b..b22a7915 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -3275,6 +3275,29 @@ ${requeueScript}`); ]) expect(fixerPrompt).toContain(text); }); + test("rebuilds a conflicted lockfile from the base's copy", () => { + const prompt = fixerPrompt.replace(/\s+/g, " "); + expect(prompt).not.toContain("regenerate lockfiles"); + for (const text of [ + "rebuild lockfiles as [Lockfiles](#lockfiles) says", + "## Lockfiles", + "rebuild it from the base's copy, never from the PR's side", + "`git checkout base -- `", + "minimal resolve against the merged manifests", + "Never run `cargo generate-lockfile`", + "`cargo update -p --precise `", + "keep the base's version if it is at or past the PR's", + "must trace to a dependency or import change in the PR", + "Name the lockfiles you rebuilt in the reason.", + "Hand back instead of taking either side's copy", + "the package manager cannot rebuild the lockfile", + "a change does not trace to the PR", + "below the base's security bump", + "`flake.lock` is a protected path", + "rebuild it the same way from the base's copy, with `nix flake lock` as the minimal resolve", + ]) + expect(prompt).toContain(text); + }); test("has the fixer put its comments in the result instead of posting them", () => { for (const text of [ "You have no GitHub credential and must not seek one.",