diff --git a/README.md b/README.md index 71d8dd6d..95ad7a6e 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ There are two audiences and two surfaces: ## Requirements - [Bun](https://bun.sh) 1.4.4 or later -- git 2.28 or later +- git 2.28 or later (2.38 or later to keep a change that waits for a person mergeable while main moves on; see Conflicts with main) - Claude Code CLI (`claude` on PATH), signed in with a subscription. Loopstra drives the CLI, not the API, so no API key is needed. - GitHub CLI (`gh`), signed in, when the repo has a remote. `loopstra start` refuses to run otherwise. - Optional, for chat on Slack or Discord: a Slack app or a Discord bot of your own (see Chat). @@ -239,7 +239,7 @@ The `status` and `note` lines at the top of `intent.md` say what to do; `intent/ - `blocked`: read the `note`. It says in plain words what went wrong and which status to set to try again (for example back to `plan-approved`), or you can set `closed`. A passing hiccup (the assistant crashed or took too long) says "To try again, set status to ..."; anything else says "When that is sorted out, ...". Loops are bounded (test-fix 3, review-revise 2). A step that hits its spending limit (`claude.max_budget_usd`, if set) blocks, and its note says an engineer can raise or remove it with `loopstra setup budgets`; so does a step that, on two days at the same status, used (nearly) the whole of the loop's daily budget itself and still ran out partway. - `spec-review`, `plan-review`: the automatic checks passed and a gate is set to wait for a person. Read `spec.md` or `plan.md`, then set `spec-approved` or `plan-approved`, or say what is wrong in the note and set the earlier status. Stepping a review status never advances it. -- `merge-review`: checks passed and the change is waiting to go into the main code. Follow the note: with nobody on the merge gate it waits for the checks on GitHub (nothing to do; it merges once they pass, even while another change is being worked on, within about `poll_seconds`); with `pr`, approve the pull request on GitHub; with `status`, read `review.md` and set `merge-approved`. +- `merge-review`: checks passed and the change is waiting to go into the main code. Follow the note: with nobody on the merge gate it waits for the checks on GitHub (nothing to do; it merges once they pass, even while another change is being worked on, within about `poll_seconds`); with `pr`, approve the pull request on GitHub; with `status`, read `review.md` and set `merge-approved`. If other work merged meanwhile conflicts with it, it goes back to `reviewing` on its own ("Other work merged into main meanwhile conflicted with this change…") and comes back to you once the resolution is tested and reviewed; with `pr`, approve it again, since an approval from before the resolution does not count. - `merged`: the change is in the main code. - `verifying`: with a person on the done gate, look at the result and set `done`. - `question: open`: an agent asked you something, and the change waits for your answer, whatever its status. See Steering. @@ -382,12 +382,15 @@ flowchart LR nReview -.->|important findings| nRevise["revise
up to 2 rounds"] -->|test again| nReview nReview -->|none| nMergeChecks{"merge checks"} nMergeChecks -.->|checks made fixes| nReview + nMergeChecks -.->|"conflict with main:
resolve, test"| nReview nMergeChecks -->|pass| nMergeGate{"merge gate"} ``` -Every loop has a limit: `stages.build.max_fix_loops` (3) fixes while the tests fail, one fix when `verify` fails, and `stages.review.max_rounds` (2) revisions after review. Past a limit the change is blocked, and its note says so. `reconcile`, `fix` and `revise` continue the build's own session, so they know what the build did. With `speed.overlap_judges`, round 1's review runs beside `verify` on the same commit, and is used only if verify passes. +Every loop has a limit: `stages.build.max_fix_loops` (3) fixes while the tests fail, one fix when `verify` fails, `stages.review.max_rounds` (2) revisions after review, and `stages.review.max_resolutions` (3) conflict resolutions per build (each adds a review round of its own, on top of `max_rounds`). Past a limit the change is blocked, and its note says so. `reconcile`, `fix` and `revise` continue the build's own session, so they know what the build did. With `speed.overlap_judges`, round 1's review runs beside `verify` on the same commit, and is used only if verify passes. -**Merge checks** bring the branch up to date with `main`, make sure the tests pass (not run again when they already passed on the same code, since `main` only gained records), and read the newest review. Files changed in the change's worktree and not committed (a review `after` command, or a `lint --fix` in the build's own test runs, that rewrote them) are committed to the branch and reviewed once more first, so a formatting fix still reaches review. What the merge checks' own commands rewrite in tracked files (a `lint --fix`, a report with a timestamp) is put back, never kept, so it cannot start one review round after another; fixes the checks commit are reviewed once more. A review `after` command that changes the same files again right after they were kept blocks the change with a note naming them. New files the test, lint and build commands write (a coverage folder, a test report) are removed after each run, during the build as well; a fix session that runs the tests itself can still leave one, and the fix's commit takes it, so list such outputs in `.gitignore`. +**Merge checks** bring the branch up to date with `main` (resolving conflicts; see below), make sure the tests pass (not run again when they already passed on the same code, since `main` only gained records), and read the newest review. Files changed in the change's worktree and not committed (a review `after` command, or a `lint --fix` in the build's own test runs, that rewrote them) are committed to the branch and reviewed once more first, so a formatting fix still reaches review. What the merge checks' own commands rewrite in tracked files (a `lint --fix`, a report with a timestamp) is put back, never kept, so it cannot start one review round after another; fixes the checks commit are reviewed once more. A review `after` command that changes the same files again right after they were kept blocks the change with a note naming them. New files the test, lint and build commands write (a coverage folder, a test report) are removed after each run, during the build as well; a fix session that runs the tests itself can still leave one, and the fix's commit takes it, so list such outputs in `.gitignore`. + +**Conflicts with main.** When the branch no longer applies cleanly to `main` (another change merged into the same lines), Loopstra resolves it rather than blocking. It happens in three places: when review starts and `main` has gained code since the build; in the merge checks; and while the change waits for a person at the merge gate (status `merge-review`, or a pull request waiting for approval), as soon as it no longer merges cleanly (with git 2.38 or later; older git leaves it to the merge checks). A branch that only falls behind without a conflict is left alone until the merge checks. The change's commits are squashed into one on top of `main`, so every conflict is met once, against the final code, and the `resolve` step (`strong` by default) resolves them in the change's worktree: it sees both changes' intent (this change's spec and plan, and the specs of the changes merged on `main` that touched the same files) and each conflict with both sides. The squashed commit lists the commits it replaces, with a `Co-authored-by` line for each person whose commits were among them. A resolution is then tested, and reviewed in one more round whose prompt shows the conflicts as they were and what the agent kept (`{{resolution}}` in `review.md`; a repository's own copy without it gets it added at the end). A change waiting for a person goes back to `reviewing` and returns to them afterwards; with `merge.human: pr`, only an approval given after the resolution reached GitHub counts, and the pull request's wait says so ("It was approved on GitHub before conflicts with main were resolved; approve the pull request again…"). Before resolving a change that waits as a pull request, `main` is synced and commits a person added to the pull request are taken into the branch, so the resolution starts from what would merge. A binary file both sides changed is left to a person, and new files the session's own commands wrote (a coverage folder) are not kept. If the agent cannot resolve it safely, leaves conflict markers, or the change has used its `stages.review.max_resolutions` (3; 0 turns resolution off), the branch is left exactly as it was and the change is blocked: "The change overlaps with other recent changes, and resolving the conflict automatically did not work. … The details are in conflict.md." `conflict.md`, beside `review.md`, lists the files, the conflicts with both sides, the commits involved and why it stopped. A stop, an outage or a crash part-way puts the branch back where it was; a resolution cut short does not count against the limit. Without a remote, the merge checks run under the merge lock, so a resolution there holds up other changes' merges while it runs, as a fix in the merge checks does. ### Where sessions read and write @@ -399,7 +402,7 @@ flowchart TB main -->|copied for each step| snap main -->|branched| wt snap -->|read| ro["read-only steps
intake, design, spec-check,
plan, plan-challenge, lessons"] - wr["writing steps
build, fix, reconcile, revise"] -->|edit| wt + wr["writing steps
build, fix, reconcile, revise, resolve"] -->|edit| wt wt -->|read, run checks| chk["review, verify"] main -.->|"main's CLAUDE.md, settings,
skills, MCP servers, from its commit"| wr main -.-> chk @@ -424,10 +427,10 @@ A change that edits those files takes effect once it is merged. The limits: a `C - **After a merge**, the done-when criteria are checked, `outcome.md` and `lessons.md` are written, and main's tests are run. If main goes from green to red, a new draft intent describing the breach is opened. - **The trace** is in `.loopstra/` (gitignored): `trace.db` (SQLite), and per change `runs//events.jsonl` plus a folder per phase holding the prompt, the result envelope and the raw session. `status`, `tail` and `ui` read it, and it stays on your machine. Chat sessions are traced there too, under `_chat`, with their state in `.loopstra/chat/`. The only things that leave it are what you set up: pull requests on GitHub, and chat messages and announcements posted to Slack or Discord. - **Design and plan beside the lanes** (`speed.think_ahead`, on): while any change runs a step, design and plan steps of other changes run beside it, one at a time, in queue order, so a change accepted during another's build is designed and planned by the time the build ends. Those steps only read code and run none of the project's commands, so they do not count against `max_parallel_changes`; a design or plan stage with `before` or `after` commands is left to the lanes. The lanes take build, review, merge and verify steps first. It starts nothing once every lane has ended (the next tick takes over), and only with room in the day's budget for its session beside the others, so never with a daily budget and no session limit. -- **More than one change at a time** (`speed.max_parallel_changes` above 1): each tick runs a step for up to that many changes at once, each in its own worktree, and waits for all of them before the next tick. Fewer run when what is left of the day's budget covers fewer whole sessions (`claude.max_budget_usd`); the trace says so. Loopstra's own writes to the main checkout (its records, moving `main`, a local merge) go one at a time, and nothing slower waits with them: checking out a change's worktree and talking to GitHub happen beside them. Changes merge one at a time: without GitHub, a change that merges after another is brought up to date and tested again first; with GitHub, a pull request that another change's merge left out of date is brought up to date and waits for GitHub's checks again. A change whose plan lists a file another change in progress also lists (in its plan, or in the files its build added to the plan) waits until that change has merged, and `queue.md` says so ("Waits for add-numbers to be merged first, because both change the same files."), so the two do not clash when they merge. That wait holds at one change at a time too, since a pull request can wait to merge while the one lane works on the next change. The loop line names every change being worked on ("working on add-numbers and say-goodbye"). +- **More than one change at a time** (`speed.max_parallel_changes` above 1): each tick runs a step for up to that many changes at once, each in its own worktree, and waits for all of them before the next tick. Fewer run when what is left of the day's budget covers fewer whole sessions (`claude.max_budget_usd`); the trace says so. Loopstra's own writes to the main checkout (its records, moving `main`, a local merge) go one at a time, and nothing slower waits with them: checking out a change's worktree and talking to GitHub happen beside them. Changes merge one at a time: without GitHub, a change that merges after another is brought up to date and tested again first; with GitHub, a pull request that another change's merge left out of date is brought up to date and waits for GitHub's checks again. Where the two changed the same lines, the conflict is resolved, tested and reviewed (see Conflicts with main). A change whose plan lists a file another change in progress also lists (in its plan, or in the files its build added to the plan) waits until that change has merged, and `queue.md` says so ("Waits for add-numbers to be merged first, because both change the same files."), so the two do not clash when they merge. That wait holds at one change at a time too, since a pull request can wait to merge while the one lane works on the next change. The loop line names every change being worked on ("working on add-numbers and say-goodbye"). - **Chat** is a separate process (`loopstra chat`, or the console). It never runs a stage or writes the main checkout: what it needs there (starting a draft, acting on a change, or adding written intents when there is no remote) it leaves as a request in `.loopstra/requests/`, as the console does, and the loop applies it at the start of a tick and every 2 seconds beside the running steps. -The full design is in `docs/superpowers/specs/2026-09-28-loopstra-design.md`, with chat in `docs/superpowers/specs/2026-10-01-orchestrator-chat-design.md`, more than one change at a time in `docs/superpowers/specs/2026-10-16-parallel-changes-design.md`, how `main` is synced, pull requests are watched, sessions read `main` and the checkout is locked in `docs/superpowers/specs/2026-10-16-simpler-core-design.md`, packaging and `loopstra upgrade` in `docs/superpowers/specs/2026-10-18-packaging-upgrade-design.md`, and agents for each step in `docs/superpowers/specs/2026-10-26-agent-setup-design.md`; `docs/decisions.md` records why. +The full design is in `docs/superpowers/specs/2026-09-28-loopstra-design.md`, with chat in `docs/superpowers/specs/2026-10-01-orchestrator-chat-design.md`, more than one change at a time in `docs/superpowers/specs/2026-10-16-parallel-changes-design.md`, how `main` is synced, pull requests are watched, sessions read `main` and the checkout is locked in `docs/superpowers/specs/2026-10-16-simpler-core-design.md`, packaging and `loopstra upgrade` in `docs/superpowers/specs/2026-10-18-packaging-upgrade-design.md`, agents for each step in `docs/superpowers/specs/2026-10-26-agent-setup-design.md`, and per-change resources and conflict resolution in `docs/superpowers/specs/2026-10-27-conflict-free-concurrency-design.md`; `docs/decisions.md` records why. ## Configuration @@ -439,7 +442,7 @@ The full design is in `docs/superpowers/specs/2026-09-28-loopstra-design.md`, wi - `agents`: which agents, subagents, teams, skills, tools and MCP tools each step's session gets (see Agents for each step) - `gates`: spec, plan, merge, done, each with `human` (`status` or `none`; merge also `pr`) and `agent` (independent reviewer); merge also `method` (`squash` or `merge`) - `stages`: per-stage model, `steps` (a model for single steps; see Models for each step), skills, `before`/`after` commands, and loop limits -- `speed`: work that runs at the same time to save waiting; only `cheap_judges: first` changes who decides (once earned, a cheaper judge's pass stands). `overlap_judges` (on) runs the first review alongside the verifier (in a copy of the tested commit), and lessons alongside the final check; when the verifier fails, the review running beside it is stopped at once and round 1 runs after the fix. With `claude.max_budget_usd_per_day` set, they overlap only when what is left of the day covers two whole sessions (`claude.max_budget_usd`), so never without a session limit. `parallel_checks` (off) starts test, lint and build together, and stops the rest once one fails; leave it off if they write the same folder. `plan_ahead` (on) writes the first plan while the spec is being checked. The plan is stopped if the spec does not pass, and it is used only if the code on `main` it read (outside `intent/` and `loopstra/config.yaml`) is still the same commit's code when planning starts. It follows the same daily budget rule, and does not run with a person on the spec gate, without a spec judge, with plan `before` commands, or while another change is about to merge. `cheap_judges` (off) asks a cheaper model (`claude.models.default`) the same question as the spec check, the plan challenge and the review: `shadow` only records whether it agreed with the strong judge, which still decides, and `first` lets it pass something alone once it has also caught the strong judge's last 30 rejections, sending anything it rejects or is unsure of to the strong judge (`loopstra judges` shows the record). In `first`, every 5th call still runs both judges as an audit, with the strong judge deciding, so a later miss sends it back to `shadow`. Like the overlaps, `shadow` and the audits run only with room for every session at once (three when the early review runs beside the verifier, or the spec check beside a plan written ahead), so never with a daily budget and no session limit; then the strong judge decides alone, and in `first` the cheaper judge never decides alone. `think_ahead` (on) runs design and plan steps beside the changes running a step (see How it works). `max_parallel_changes` (1, up to 4) is how many changes may each run a step at the same time. Raise it only if the project's commands (test, lint, build, install) can run twice at once on this machine: no fixed ports, no shared test database, no output outside the worktree. Each command, and each session that may run commands, gets `LOOPSTRA_SLUG` and `LOOPSTRA_SLOT`: a small number (0, 1, ...) that no other change running at the same time has, so it can pick its own ports, database and Docker project names (port `3100 + LOOPSTRA_SLOT`, say). Main's health check runs alone and gets no slot. More changes at once also spend the daily budget and the subscription's usage limits faster. With `claude.max_budget_usd_per_day` set, only as many run at once as what is left of the day covers whole sessions (`claude.max_budget_usd`), so one at a time without a session limit; each overlap above also counts the sessions of the other changes running. +- `speed`: work that runs at the same time to save waiting; only `cheap_judges: first` changes who decides (once earned, a cheaper judge's pass stands). `overlap_judges` (on) runs the first review alongside the verifier (in a copy of the tested commit), and lessons alongside the final check; when the verifier fails, the review running beside it is stopped at once and round 1 runs after the fix. With `claude.max_budget_usd_per_day` set, they overlap only when what is left of the day covers two whole sessions (`claude.max_budget_usd`), so never without a session limit. `parallel_checks` (off) starts test, lint and build together, and stops the rest once one fails; leave it off if they write the same folder. `plan_ahead` (on) writes the first plan while the spec is being checked. The plan is stopped if the spec does not pass, and it is used only if the code on `main` it read (outside `intent/` and `loopstra/config.yaml`) is still the same commit's code when planning starts. It follows the same daily budget rule, and does not run with a person on the spec gate, without a spec judge, with plan `before` commands, or while another change is about to merge. `cheap_judges` (off) asks a cheaper model (`claude.models.default`) the same question as the spec check, the plan challenge and the review: `shadow` only records whether it agreed with the strong judge, which still decides, and `first` lets it pass something alone once it has also caught the strong judge's last 30 rejections, sending anything it rejects or is unsure of to the strong judge (`loopstra judges` shows the record). In `first`, every 5th call still runs both judges as an audit, with the strong judge deciding, so a later miss sends it back to `shadow`. Like the overlaps, `shadow` and the audits run only with room for every session at once (three when the early review runs beside the verifier, or the spec check beside a plan written ahead), so never with a daily budget and no session limit; then the strong judge decides alone, and in `first` the cheaper judge never decides alone. `think_ahead` (on) runs design and plan steps beside the changes running a step (see How it works). `max_parallel_changes` (1, up to 4) is how many changes may each run a step at the same time. Raise it only if the project's commands (test, lint, build, install) can run twice at once on this machine: no fixed ports, no shared test database, no output outside the worktree. Each command, and each session that may run commands, gets `LOOPSTRA_SLUG` and `LOOPSTRA_SLOT`: a small number (0, 1, ...) that no other change running at the same time has, so it can pick its own database and Docker project names. It also gets its own temp folder (`TMPDIR`, `TMP` and `TEMP` set to a short folder of its own in the system's temp folder, `loopstra-<8 hex>/s`, so Unix socket paths stay short; removed when the step ends), and a block of ports no other running change has, checked free as it is handed out: `LOOPSTRA_PORT_BASE` and `LOOPSTRA_PORT_COUNT` (`ports_per_change`, 10; 0 for none; blocks start at `ports_from`, 20000). `own_caches` (off) also gives each slot its own tool caches (`XDG_CACHE_HOME`, `npm_config_cache`, `BUN_INSTALL_CACHE_DIR`, `YARN_CACHE_FOLDER`, `PIP_CACHE_DIR`, `GOCACHE`, under `.loopstra/cache/slot-/`), kept between changes so the next change on that slot finds them warm; turn it on when two installs or builds at once trip over a shared cache. Main's health check runs alone and gets none of these. More changes at once also spend the daily budget and the subscription's usage limits faster. With `claude.max_budget_usd_per_day` set, only as many run at once as what is left of the day covers whole sessions (`claude.max_budget_usd`), so one at a time without a session limit; each overlap above also counts the sessions of the other changes running. - `signals`: how often main's health check runs - `chat`: the orchestrator's model (`model`), the model of the write-up (`steps.write-intent`), optional spending limits (`max_budget_usd_per_day`, `max_budget_usd_per_session`), and the Slack and Discord bots under `transports` (see Chat) @@ -459,6 +462,7 @@ The full design is in `docs/superpowers/specs/2026-09-28-loopstra-design.md`, wi | `fix` | build | the build stage's model | | `review` | review | the review stage's model | | `revise` | review | the build stage's model | +| `resolve` | review | `strong` | | `verify` | verify | the verify stage's model | | `done-check` | verify | `strong` | | `lessons` | verify | `cheap` | diff --git a/docs/decisions.md b/docs/decisions.md index 14d28af6..89c865d3 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -888,3 +888,24 @@ A new `agents` block in `loopstra/config.yaml` gives each step an agent to play, So these sessions get main's setup, read from the step's commit of main (`git ls-tree` and `cat-file`) into a temporary folder of the session's own, removed when it ends: its settings merged into `--settings` (lists and maps joined, so both Loopstra's hook and main's run), its CLAUDE.md files and `.claude/rules` (which an added dir loads too, checked live) as a read-only `--add-dir`, its `.claude/skills`, `agents` and `commands` as the plugin `project`, and its `.mcp.json` servers less those its settings disable. A first version read them from the step's snapshot, a checkout: the review of this change found that a build can write the snapshot with any command it may run (only the edit tools are denied there), which is why agent files were already read from the commit, and it made every branch step check out all of main. The limits: main's subfolder CLAUDE.md files, and files a CLAUDE.md imports from elsewhere in the repository, do not load in these sessions; a hook or MCP server of main's that runs a script from the project folder runs the branch's copy; and main's settings can still turn hooks off or allow more, as they always could. - **A session runs on the config its prompt was rendered from.** A phase read loopstra/config.yaml three times (prompt, model, agents), so a setting saved in between could give a session agents its prompt did not name. The step now remembers the config each recent prompt was rendered from, and the session that sends the prompt uses it, as do the keys that match a prompt to earlier work (the judge comparison, the plan written ahead, the early review); a retry renders and reads again. - **Shared helpers, once.** `readText` is in `fsutil.ts`, frontmatter parsing (`splitFrontmatter`) in `agent-files.ts`, the agent and skill name rule (`ITEM_NAME`) in `agent-library.ts`, `TEAM_NAME` and `once` in `agents.ts` (which the page can import), and the empty-map cleanup is `ConfigDocument.dropEmptyMaps`. + +## Changes running at the same time without getting in each other's way (2026-10-27) + +- **Per-change temp folder, always, with a short path.** Every claimed change's commands and command-running sessions get `TMPDIR`/`TMP`/`TEMP` set to `/loopstra-<8 hex of the repository's path>/s`, removed when the step releases the change. Not under the repository: tools make Unix sockets under `TMPDIR`, and a socket path may be at most about 104 bytes, which a deep repository path and a long slug would pass. Named by the slot, which no other running change has. A crashed step's folder is swept at the next tick's start. Claude Code itself runs with it too, which does no harm. +- **Ports handed out and checked free, 10 by default.** A change gets the lowest block (`ports_from + i × ports_per_change`) that no other claimed change holds and whose ports all bind on 127.0.0.1 at that moment. At most 50 blocks are tried; with none free the change gets no port variables and runs as before. Kept for the step, like the slot. The check is only a snapshot: something else can take a port after it, which the project's own retry or the fix loop meets as it would today. +- **Caches per slot, not per change, and opt-in.** Per-change caches would make every change fill its own install cache. A slot belongs to one running change at a time, so per-slot caches never collide and stay warm for the next change on that slot. Off by default, because sharing a cache is usually fine and isolating it costs disk. +- **`lanes.ts` still imports nothing from the rest of Loopstra.** It gained `onRelease` hooks and `isClaimed`; `src/resources.ts` registers its own clean-up there. +- **Conflicts are resolved, not blocked.** A rebase conflict used to block the change for an engineer. Now `catchUp` (src/stages/catch-up.ts) resolves it: at review start (only when main gained code, so a branch behind by records keeps the early review), in the merge checks' `up-to-date` check, and while a change waits for a person at the merge gate. +- **Squash, then replay, only on a conflict.** A clean rebase keeps the branch's commits. On a conflict the commits are squashed into one on top of main (`merge --squash` with diff3 markers), so the agent meets every conflict once against the final code, not once per commit with the same lines conflicting again; a conflict that only existed in an intermediate commit goes away. This applies with `gates.merge.method: merge` too: the branch's commits are mostly the loop's own build, fix and revise commits, and the squashed commit lists their subjects. Authors other than the runtime keep a `Co-authored-by` trailer. +- **The resolve step runs on `strong`, fresh, in the build tool set.** It does not resume the build session (that runs on the build model, and a resolution needs both sides' intent, which its prompt carries: this change's spec and plan, the conflicts with both sides, and the main commits involved with the specs of local merges named by their `: ` message). It must not run git that changes history or the index; the runtime stages and commits. A resolution passes only when the session says `resolved` and no conflicted file has a `<<<<<<<`, `|||||||` or `>>>>>>>` line (a bare `=======` is also a Markdown heading underline, so it is not checked). +- **Every resolution is tested, then reviewed.** At review start and while waiting: the test loop (`resolve-test-N`) runs before the review. In the merge checks the tests check runs after the resolution as usual, and the gate reports `changed`, which sends it back for a review round. The reviewer gets `{{resolution}}`: the conflicts as they were and the agent's summary. It is cleared when a review passes. +- **Resolution reviews do not use up `max_rounds`.** `roundsAllowed` is `max_rounds` plus the resolutions tried in this build; `stages.review.max_resolutions` (3) caps them, so a change cannot chase a busy main forever. +- **An interrupted resolution is undone, and does not count.** The old head is written to `catching-up` before the squash. A stop, an outage, the day's budget or a crash part-way puts the branch back (in a `finally`, and the next time the worktree is opened, or a build starts). The count goes up only once the resolve session has answered, whatever it said. +- **Failures write `conflict.md` beside `review.md`**, with the files, the conflicts with both sides, the commits involved and the reason, and leave the branch exactly as it was. +- **Waiting changes are kept mergeable only when they conflict.** A change at `merge-review` with a person on the gate (or a pull request waiting for approval) is checked with `git merge-tree --write-tree` (git 2.38+); a branch that merges cleanly is left alone even when behind, since the merge checks bring it up to date anyway. With an older git the check says nothing and resolution waits for the merge checks. Without a remote the tick makes such a change runnable (it is not otherwise); with one the watcher does it, within `max_parallel_changes` as `mergeWhenUpToDate` does. +- **An approval from before a resolution reached GitHub does not count.** GitHub keeps an approval through a push unless the repository dismisses stale ones. With `merge.human: pr`, `resolved-at` is `pending` from the resolution until the push that carries it, then that push's time; only an approval submitted after it (`latestReviews[].submittedAt` of someone with a say) merges the pull request, and `reviewDecision: APPROVED` alone is not enough then. Comparing with the resolution's own time would let an approval of the old head, given while the resolution was being reviewed, count. The wait says why (`PR_WAIT`), so the pull request does not look stuck. +- **A waiting pull request is caught up from GitHub's copy.** Before resolving, the watcher syncs main and takes commits a person added to the pull request (as `mergeWhenUpToDate` does, under the merge lock), so the resolution includes them and its push goes over them rather than blocking as "both changed". +- **Binary conflicts go to a person.** git leaves main's copy of a binary file in place with no markers, so a resolution that did not touch it would keep main's version unseen. A conflicted file with a NUL byte blocks the resolution before the session runs. +- **New files the resolve session leaves are removed** before the resolution is saved (`git clean -fd`, not `-x`), as after the test loop's runs: the change's own new files are already in the index, and the prompt says not to add files. Commits are allowed empty: a resolution may keep main's side of everything. +- **The waiting check is cached by commit.** `conflictsWhileWaiting` keeps its last answer per branch with main's and the branch's commits, so ticks and watcher looks that find neither moved run no `merge-tree`. +- **A resolution in the local merge checks holds the merge lock.** Without a remote, the merge checks run under it, so other changes' merges wait while a resolution and its tests run. Fix sessions in the merge checks already did this; moving the catch-up before the lock would leave a window where main moves again. The catch-up rebase inside `mergeIntoMain` still blocks on a conflict: under the lock, main can only have gained records there. diff --git a/docs/superpowers/specs/2026-10-27-conflict-free-concurrency-design.md b/docs/superpowers/specs/2026-10-27-conflict-free-concurrency-design.md new file mode 100644 index 00000000..c5568f2d --- /dev/null +++ b/docs/superpowers/specs/2026-10-27-conflict-free-concurrency-design.md @@ -0,0 +1,289 @@ +# Changes that run at the same time without getting in each other's way + +`speed.max_parallel_changes` lets up to four changes run a step at once. Each already has its own +branch and worktree (`.loopstra/worktrees/<slug>`). Design and plan sessions read a snapshot of +main, and the early review reads a throwaway copy of the tested commit. The "same files" rule keeps +a change from building into files another change is still working on. Two gaps are left: + +1. **Shared machine resources.** Two changes' test commands can still collide outside their + worktrees: on `/tmp`, on tool caches, and on ports. Today loopstra hands out `LOOPSTRA_SLOT`, + but each project has to turn it into ports and folders itself. +2. **Merge conflicts.** Before a merge, the merge checks rebase the branch onto main. A conflict + aborts the rebase and blocks the change ("An engineer needs to look at it"). Nothing tries to + resolve it, and a change waiting for a person never finds out it no longer merges. + +This change closes both gaps. + +## Goals + +- Every change that runs commands gets its own temp folder, and its own block of free ports. + Its own cache folders are available when the project turns them on. +- A merge conflict with main is resolved by an agent rather than blocking the change. The + resolution is tested and reviewed before anything merges. +- A change waiting at the merge gate (for a person, or as an open pull request) is kept mergeable. + When main moves on and the change no longer applies cleanly, it is resolved, tested and reviewed + again, so the person always approves code that will merge. +- A failed resolution blocks with enough detail for an engineer to pick it up: the files, what the + agent tried, and why it stopped. + +## Non-goals + +- Changing the "same files" rule. It stays as it is: avoiding a conflict we can see coming is + better than resolving it. Resolution covers what the plans did not predict. +- Resolving the catch-up rebase inside the local merge (`mergeIntoMain`). It runs under the merge + lock, after the merge checks brought the branch up to date. Main can only have gained records + under `intent/` since then, so it cannot conflict with the change's code. It keeps blocking as + it does today. +- Isolating anything the project's own configuration shares (a database server, Docker networks). + `LOOPSTRA_SLOT`, `LOOPSTRA_SLUG` and the new port block are what a project uses for those. + +## Anti-goals + +- A resolution that merges unseen. Every resolution is tested and then reviewed. The reviewer sees + each conflicting section as it was, with both sides, and the agent's account of what it chose. +- A change chasing a busy main forever. Resolutions per build are capped. +- Losing a person's commits, or overwriting a pull request branch that someone else pushed to. The + existing lease rules still apply to every push. + +## Part 1: Per-change resources + +All of this goes in a new module, `src/resources.ts`. It owns the per-change environment that every +project command, hook command and command-running session of a claimed change receives. Today +those three call sites (`projectCommand` and `runHookCommands`, plus `tracedSession` for the +`build` and `read+commands` tool sets) each add `slotEnv(slug)`. They switch to one async +`changeEnv(ctx)` that returns: + +| Variable | Value | When | +|---|---|---| +| `LOOPSTRA_SLOT` | as today (`slotEnv`) | the change is claimed | +| `TMPDIR`, `TMP`, `TEMP` | `<system temp>/loopstra-<8 hex of the repo path>/s<slot>/` (created on first use; short, for Unix socket paths) | the change is claimed | +| `LOOPSTRA_PORT_BASE`, `LOOPSTRA_PORT_COUNT` | the first port and the size of the change's block | `speed.ports_per_change` > 0 and the change is claimed | +| `XDG_CACHE_HOME`, `npm_config_cache`, `BUN_INSTALL_CACHE_DIR`, `YARN_CACHE_FOLDER`, `PIP_CACHE_DIR`, `GOCACHE` | folders under `.loopstra/cache/slot-<n>/` | `speed.own_caches: true` and the change is claimed | + +- **Temp folder:** one per change, always on. It is removed when the step releases the change (the + scheduler's `release`). A step that crashes leaves it behind, so cleanup at tick start removes + leftover temp folders of changes that are not claimed. Claude Code itself also runs with this + `TMPDIR`, which does no harm. +- **Caches:** off by default (`speed.own_caches: false`). They are per *slot*, not per change, and + are kept between changes. A slot belongs to one running change at a time, so two changes never + share a cache folder, and the next change on the same slot finds it warm. *(My call: per slot + rather than per change. The user chose "opt-in" because of slow installs, and per-slot keeps + most of the warmth.)* +- **Ports:** `speed.ports_per_change` (default 10, 0 = none, at most 100). Block `i` starts at + `speed.ports_from + i × ports_per_change` (`ports_from` default 20000). A change gets the lowest + block that no other claimed change holds and whose ports all bind on `127.0.0.1` right now (the + check binds each port and closes it straight away). It keeps the block until its step ends, like + its slot. At most 50 blocks are tried; if none of them is free (or the ports run out at 65535), + the change gets no port variables. +- Main's health check runs alone and gets none of this, as it gets no slot today. +- `lanes.ts` keeps holding the claims and slots. The port blocks and temp folders live in + `resources.ts`, and are released from the same `release(slug)` call through a hook that + `resources.ts` registers. That way `lanes.ts` still imports nothing from the rest of Loopstra. + +Config (`speed:`): + +```yaml +speed: + ports_per_change: 10 # each running change gets LOOPSTRA_PORT_BASE..+count-1, checked free; 0: none + ports_from: 20000 # where the first block starts + own_caches: false # true: tool caches per slot (XDG_CACHE_HOME, npm, bun, yarn, pip, go) +``` + +## Part 2: Resolving conflicts + +### Where the branch is brought up to date + +There are three places. All of them use one function, `catchUp(ctx, where)` in a new +`src/stages/catch-up.ts`: + +1. **Review start** (`runReviewRounds`, only for a fresh set of rounds: no round marker yet). This + runs when main has code outside `intent/` that the branch lacks (the same test as `outOfDate`). + A branch behind only by records is left alone, so the early review the build step kept still + matches the commit. +2. **The merge checks' `up-to-date` check** (`checkMerge`). Today a conflicting rebase fails this + check. It now calls `catchUp`. +3. **While waiting at the merge gate**: `merge-review` with a person on the gate (no remote), or + an open pull request waiting for a person (`gates.merge.human` `status` before `merge-approved`, + or `pr` before it is approved). The step checks whether the branch still merges cleanly into + main (`git merge-tree --write-tree`, git 2.38+; on an older git it never finds a conflict, and + resolution waits for the merge checks). A branch that merges cleanly is left alone, even when + it is behind. A conflicting branch is brought up to date with `catchUp`, and its status goes to + `reviewing` with the note "Brought up to date with main; it is reviewed again before it comes + back to you." From there the usual path reviews it, runs the merge checks, and (with a remote) + pushes it to the same pull request and waits at `merge-review` again. + - Without a remote, `merge-review` with a person on the gate is not runnable today. The tick + works out which of these changes conflict with main, the same way it works out the same-file + holds, and makes only those runnable. + - With a remote, the watcher's look at a pull request waiting for a person does the check. It + does the resolution only while no more than `speed.max_parallel_changes` changes run a step + that may run commands (the same rule `mergeWhenUpToDate` uses); otherwise a later look does it. + +### What `catchUp` does + +In the change's worktree, which is clean (each caller saves or refuses unsaved edits first): + +1. The branch already contains main → `"current"`. +2. `rebaseOnto(main)` succeeds → `"rebased"`. +3. The rebase conflicted, and this build has already used `stages.review.max_resolutions` + (default 3) resolutions → fail with the conflict note (below). +4. Otherwise **squash, then replay**. The branch is moved to main (`reset --keep`), and the + whole change since it parted from main is applied on top in one go (`merge --squash`, with + `merge.conflictStyle=diff3` so each conflict shows main's side, the base and the change's side). + The commit it makes (runtime identity) lists the original subjects, plus a `Co-authored-by:` + trailer for each author who is not the runtime, so a person's commits taken from a pull request + keep their credit. + - Squashing means the agent resolves every conflict once, against the final code, instead of + once per commit. It also often makes a conflict that only existed in an intermediate commit + go away. *(My call. Per-commit resolution would run a session per conflicting commit, and the + same lines often conflict again in each one.)* + - If the squash applies cleanly → `"rebased"` (the history changed, but nothing was resolved). +5. It conflicted: the **resolve** session runs (below). Then the runtime checks its work: + - the session reported `resolved: true`; + - no file is still unmerged, and none of the conflicted files has a conflict marker line + (`<<<<<<< `, `=======`, `>>>>>>> ` at the start of a line). + + If all of that holds, the runtime stages everything (`git add -A`), checks nothing is left + unmerged, and commits → `"resolved"`. Otherwise it puts the branch back where it was before the + squash (`reset --hard` to the old head, then `clean -fd`: the worktree was clean when it + started), and fails with the conflict note. +6. Each resolve session that answers, whatever it said, counts against `max_resolutions` + (run-folder marker `resolutions`, cleared by a new build). One cut short does not. +7. **Interruptions.** The old head is written to the run-folder marker `catching-up` before the + branch moves. A stop, an outage, the day's budget or a crash part-way puts the branch back: + in a `finally` around the resolution, and otherwise the next time the worktree is opened + (`openBranchWorktree`) or a build starts. + +The result: `{ ok: true, outcome: "current" | "rebased" | "resolved" } | Failure`. + +### The resolve session + +- Step key `resolve`, in the review group, default model `strong` (a new `STEPS` entry, so + `stages.review.steps.resolve` can change it, and it shows up in setup and the console like the + other steps). +- A fresh session (it does not resume the build session, which runs on the build model). It uses + the `build` tool set, `acceptEdits`, and runs in the worktree, mid-rebase. +- Prompt template `templates/prompts/resolve.md`. It gets: + - the change's spec and plan; + - the conflicted files, each with its conflicting sections as they are in the file (markers and + both sides), capped at 200 lines per file and 40 000 characters in all, with a note that the + agent should read the file when the cap cuts it; + - the commits on main since the branch parted from it that touched those files (`%h %s`). Where + a commit's subject starts with another change's slug (`<slug>: <title>`, the local merge's + message) and `intent/<slug>/spec.md` exists, it names that spec and plan so the agent can read + the other change's intent; + - the test command, which the agent may run. + It is told to keep both changes' intent, to edit only what the conflict needs, never to run git + commands that change history (commit, rebase, reset, checkout), and to answer `resolved: false` + with a reason when it cannot do this safely. +- Envelope `resolve`: `{ status, resolved: boolean, summary_markdown: string, reason: string }`. + +### After a resolution: tests, then review + +- **Review start:** a `"resolved"` catch-up runs the test loop (`testLoop(ctx, "resolve-test")`, + with its usual fix budget) before round 1, so the reviewer reads tested code. +- **Merge checks:** a `"resolved"` up-to-date check passes with `changed: true`. The tests check + runs after it as usual. The gate then reports `changed`, which sends the change back for one + more review round (`anotherRound`). Today's rule that nothing merges unseen does the rest. +- **Waiting:** the test loop (`resolve-test`) runs, then the change takes one more review round + (as `anotherRound`, counted through `roundsAllowed`) and its status goes to `reviewing`. The review + rounds and then the merge checks run, and it comes back to `merge-review`. +- **Approvals on GitHub:** with `merge.human: pr`, an approval from before the latest resolution + reached GitHub does not count. The run-folder marker `resolved-at` is `pending` until the push + that carries the resolution, then that push's time; only an approval whose `submittedAt` (from + gh's `latestReviews`, of someone with a say) is newer merges it, and the wait says why. +- **Pull requests waiting for a person:** before resolving, the watcher syncs main and takes + commits a person added to the pull request (`upToDate`, under the merge lock), so the resolution + includes them. +- **Binary conflicts** block before the session runs (git leaves main's copy with no markers). + New files the session's commands wrote are removed before saving; commits may be empty. GitHub keeps an approval through a + push unless the repository dismisses stale ones, and the resolution is code the person has not + seen. `PrInfo` gains `approvedAt`. +- **What the reviewer sees:** the resolution's record (run-folder marker `resolution.md`) goes + into the review prompt as a new variable `{{resolution}}`. It holds the conflicting sections as + they were (both sides), the main commits involved, and the agent's summary. It is cleared when a + review passes, and by a new build. A repository's own `review.md` without `{{resolution}}` + still gets it, appended as a note, the way `build.md` gets `{{feedback}}` (`phasePrompt`'s + `note` fallback). +- **Rounds:** resolution reviews do not use up `review.max_rounds`. Every limit check on the rounds + (`runReviewRounds`, `anotherRound`) uses `max_rounds + resolutions used`, through one helper, + `roundsAllowed(ctx)`. + +### When it fails + +The change blocks, as today, but with a new note and detail: + +- Note: "The change overlaps with other recent changes, and resolving the conflict automatically + did not work. Nothing was merged. The details are in conflict.md. An engineer needs to look at + it." +- `conflict.md` (an artifact under `intent/<slug>/`, so the person sees it beside `review.md`) + lists the conflicted files, their conflicting sections (both sides), the main commits involved, + and either the agent's summary and reason, or the runtime's reason (markers left, files still + unmerged, the session failed, the resolutions used up). The block's `detail` names the files + and the reason. +- The branch is left exactly as it was before the attempt. +- When the cap is reached, the agent does not run. The note says so ("…has already been brought + up to date as many times as allowed…"). + +### Observability + +- The catch-up is a code phase (`catch-up`), so it gets its own row. Its trace event records the + outcome, the main commit, and the files. +- The resolve session is an agent phase (`resolve`), with its own row, cost and folder, like any + other. +- The merge gate's `up-to-date` row says `rebased onto main`, `resolved conflicts in a.ts, b.ts`, + or the failure. +- `loopstra status`, the dashboard and `queue.md` need no changes. They show `reviewing` with the + new status note, and the block note when it fails. + +### Questions the design review raised, and the answers (my calls) + +1. *A resolve cut short leaves the worktree mid-resolution.* Undone, via `catching-up` (step 7 + above), and not counted against the limit. +2. *An old GitHub approval could cover a resolution.* It does not (see Approvals on GitHub). +3. *Squashing with `gates.merge.method: merge`.* Accepted: squashing happens only on a conflict, + the branch's commits are mostly the loop's own, and the squashed commit lists their subjects + and keeps people's credit. Recorded in `docs/decisions.md`. +4. *Is a waiting change's resolution tested before its review, and which round is it?* Tested + first; it is one more round, which the resolution allowance pays for. +5. *Without a remote, the resolution runs under the merge lock.* Accepted: fix sessions in the + merge checks already do, and resolving before the lock would leave a window for main to move + again. + +## Config summary + +```yaml +speed: + ports_per_change: 10 + ports_from: 20000 + own_caches: false +stages: + review: + max_resolutions: 3 # conflict resolutions per build; their reviews do not count against max_rounds + # steps: { resolve: strong } # the default +``` + +## Tests + +- `resources`: temp folder per change, created and removed on release. Ports: blocks never overlap + between claimed changes, a busy port skips its block, none when the setting is 0. Caches only + with `own_caches`, per slot. The env reaches project commands, hook commands and `build` / + `read+commands` sessions, and not `read` / `read+git` sessions. +- `catchUp` against real git repositories (the existing helpers make them): current, a clean + rebase, a conflict that squashing removes, a conflict resolved by a fake session (fake-claude + writes the resolved file), a session that says `resolved: false`, a session that leaves markers, + a cap that is used up. Failure leaves the branch at its old head, and `conflict.md` is written. + Co-authored-by trailers. +- Review start: main gained code and the branch conflicts → resolve, test, review with + `{{resolution}}` filled in. Main gained only records → no rebase, and the early review is used. +- Merge checks: a conflict is resolved, then another review round runs that does not use up + `max_rounds`. +- Waiting: local `merge-review` with a person; main moves with a conflicting change → the tick + makes it runnable, it is resolved, and it goes to `reviewing`. A clean one stays put. Remote: a + pull request waiting for approval that conflicts → resolved, reviewed, and pushed to the same + pull request. + +## Docs + +The README's `speed` and `stages.review` sections, "How it works" (what happens on a conflict), +`templates/config.yaml`, and `docs/decisions.md`: squash-then-replay, per-slot caches, the +resolution allowance on top of `max_rounds`, and git 2.38 for the waiting check. diff --git a/src/config.ts b/src/config.ts index 74cd7c7a..9f3ad58d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -213,7 +213,11 @@ export const ConfigSchema = z.object({ design: stage("design").extend({ model: modelRef.default("strong") }).prefault({}), plan: stage("plan").extend({ model: modelRef.default("strong") }).prefault({}), build: stage("build").extend({ model: modelRef.default("default"), max_fix_loops: z.number().int().min(1).default(3) }).prefault({}), - review: stage("review").extend({ model: modelRef.default("strong"), max_rounds: z.number().int().min(1).default(2) }).prefault({}), + review: stage("review").extend({ + model: modelRef.default("strong"), max_rounds: z.number().int().min(1).default(2), + /** Conflict resolutions with main per build; the review round each one adds does not count against max_rounds. 0: a conflict blocks. */ + max_resolutions: z.number().int("must be a whole number").min(0, "must be 0 or more").default(3), + }).prefault({}), verify: stage("verify").extend({ model: modelRef.default("default") }).prefault({}), }).strict().prefault({}), // Work that runs at the same time to save waiting. Only cheap_judges: first changes who decides: once earned, a cheaper judge's pass stands. @@ -230,6 +234,12 @@ export const ConfigSchema = z.object({ think_ahead: z.boolean().default(true), /** How many changes may each run a step at the same time. 1 = one at a time (as before). */ max_parallel_changes: z.number().int("must be a whole number").min(1, "must be 1 or more").max(4, "must be 4 or less").default(1), + /** How many ports each running change gets (LOOPSTRA_PORT_BASE, LOOPSTRA_PORT_COUNT), checked free. 0 = none. */ + ports_per_change: z.number().int("must be a whole number").min(0, "must be 0 or more").max(100, "must be 100 or less").default(10), + /** The first port of the first block. */ + ports_from: z.number().int("must be a whole number").min(1024, "must be 1024 or more").max(65535, "must be 65535 or less").default(20000), + /** Tool caches (XDG_CACHE_HOME, npm, bun, yarn, pip, go) per slot, so changes running at the same time never share one. */ + own_caches: z.boolean().default(false), }).strict().prefault({}), signals: z.object({ main_health: z.object({ every_minutes: z.number().positive().default(30) }).strict().prefault({}), diff --git a/src/envelopes.ts b/src/envelopes.ts index e719d519..c1b89aa8 100644 --- a/src/envelopes.ts +++ b/src/envelopes.ts @@ -66,6 +66,12 @@ export const Envelopes = { review_markdown: z.string(), }), revise: askingCodeChange, + /** resolved: every conflict is resolved in the files; false with a reason when it could not be done safely. */ + resolve: base.extend({ + resolved: z.boolean(), + summary_markdown: z.string(), + reason: z.string(), + }), "done-check": base.extend({ /** One item per Done when criterion. needs-person: it cannot be judged from the repository. */ evidence: z.array(z.object({ criterion: z.string(), result: z.enum(["met", "unmet", "needs-person"]), evidence: z.string() })), diff --git a/src/git.ts b/src/git.ts index c1a88393..8a10ff99 100644 --- a/src/git.ts +++ b/src/git.ts @@ -510,6 +510,20 @@ export class Git { return false; } + /** + * Whether `branch` merges into `into` without a conflict, worked out without touching any checkout + * (`git merge-tree --write-tree`). Null when git cannot say: before git 2.38, or no common history. + */ + async mergesCleanly(into: string, branch: string): Promise<boolean | null> { + const r = await this.run(["merge-tree", "--write-tree", "--no-messages", "--name-only", into, branch], true); + return r.code === 0 ? true : r.code === 1 ? false : null; + } + + /** The files a merge left unmerged in this checkout (conflicts). */ + async unmergedFiles(): Promise<string[]> { + return (await this.run(["diff", "--name-only", "--diff-filter=U", "-z"])).out.split("\0").filter(Boolean); + } + /** Replays this checkout's branch onto `base`, as the runtime. A conflict is aborted and reported as false. */ async rebaseOnto(base: string): Promise<boolean> { // A rebase left over from an interrupted step is the runtime's own (this is its worktree). diff --git a/src/github.ts b/src/github.ts index d34812ee..5587cb4e 100644 --- a/src/github.ts +++ b/src/github.ts @@ -42,7 +42,8 @@ export function recordGitHub(trace: Trace, error: string | null): void { * `head`: the commit at the tip of the pull request's branch on GitHub (null when gh does not say). * `draft`: a draft pull request, which GitHub refuses to merge until it is marked ready for review. */ -export interface PrInfo { number: number; state: "OPEN" | "MERGED" | "CLOSED"; approved: boolean; merged: boolean; mergeCommit: string | null; url: string; createdAt: string; head: string | null; draft: boolean } +/** `approvedAt`: when the newest approval that counts was given (see approvedAt), null when none or GitHub gives no time. */ +export interface PrInfo { number: number; state: "OPEN" | "MERGED" | "CLOSED"; approved: boolean; approvedAt: string | null; merged: boolean; mergeCommit: string | null; url: string; createdAt: string; head: string | null; draft: boolean } /** `none`: GitHub reports no checks on the pull request (none configured, or not registered yet). */ type ChecksState = "pass" | "fail" | "pending" | "none" | "unknown"; @@ -67,7 +68,7 @@ const ghUser = z.looseObject({ login: z.string().nullish() }).nullish(); /** The fields of a pull request gh pr view --json prints that approved reads (see lookupPr). */ const REVIEW_FIELDS = { reviewDecision: z.string().nullish(), - latestReviews: z.array(z.looseObject({ author: ghUser, authorAssociation: z.string().nullish(), state: z.string().nullish() })).nullish(), + latestReviews: z.array(z.looseObject({ author: ghUser, authorAssociation: z.string().nullish(), state: z.string().nullish(), submittedAt: z.string().nullish() })).nullish(), author: ghUser, }; @@ -112,10 +113,21 @@ const TRUSTED = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); */ function approved(j: PrJson): boolean { if (j.reviewDecision) return j.reviewDecision === "APPROVED"; - const reviews = (j.latestReviews ?? []).filter((r) => r.authorAssociation ? TRUSTED.has(r.authorAssociation) : r.author?.login !== j.author?.login); + const reviews = withSay(j); return reviews.some((r) => r.state === "APPROVED") && !reviews.some((r) => r.state === "CHANGES_REQUESTED"); } +/** The latest reviews of those with a say in the repository (see approved). */ +function withSay(j: PrJson): NonNullable<PrJson["latestReviews"]> { + return (j.latestReviews ?? []).filter((r) => r.authorAssociation ? TRUSTED.has(r.authorAssociation) : r.author?.login !== j.author?.login); +} + +/** When the newest approval among the latest reviews of those with a say was given; null when none, or GitHub gives no time. */ +function approvedAt(j: PrJson): string | null { + const times = withSay(j).filter((r) => r.state === "APPROVED" && r.submittedAt).map((r) => r.submittedAt!).filter((t) => Number.isFinite(Date.parse(t))); + return times.sort((a, b) => Date.parse(a) - Date.parse(b)).at(-1) ?? null; +} + export class GitHub { private readonly exe: string | null; private readonly env: Record<string, string>; @@ -170,7 +182,7 @@ export class GitHub { return { error: reviewsOnly ? "gh pr view printed reviews in a shape Loopstra does not read" : "gh pr view printed something that is not a pull request" }; } const p = parsed.data; - return { pr: { number: p.number, state: p.state, approved: approved(p), merged: !!p.mergedAt, mergeCommit: p.mergeCommit?.oid || null, url: p.url, createdAt: p.createdAt, head: p.headRefOid || null, draft: p.isDraft === true } }; + return { pr: { number: p.number, state: p.state, approved: approved(p), approvedAt: approvedAt(p), merged: !!p.mergedAt, mergeCommit: p.mergeCommit?.oid || null, url: p.url, createdAt: p.createdAt, head: p.headRefOid || null, draft: p.isDraft === true } }; } async createPr(p: { head: string; base: string; title: string; body: string }): Promise<{ number: number; url: string }> { diff --git a/src/lanes.ts b/src/lanes.ts index 96cb6a1b..0f3d6898 100644 --- a/src/lanes.ts +++ b/src/lanes.ts @@ -199,11 +199,25 @@ export function slotEnv(slug: string): { LOOPSTRA_SLOT?: string } { return { LOOPSTRA_SLOT: String(slot) }; } +/** What else is let go of when a change is released (see onRelease). */ +const releaseHooks: Array<(slug: string) => void> = []; + +/** + * Runs `fn` whenever a change is released, after its claim is gone: the module that hands out more + * per-change resources (src/resources.ts) lets go of them, while this one imports nothing of it. + */ +export function onRelease(fn: (slug: string) => void): void { + releaseHooks.push(fn); +} + /** Releases a change's claim once its step has ended. */ export function release(slug: string): void { claimed.delete(slug); noCommands.delete(slug); slots.delete(slug); + for (const fn of releaseHooks) { + try { fn(slug); } catch { /* best effort: a temp folder that cannot be removed now is swept at a later tick's start */ } + } } /** How many claimed changes there are besides this one. */ diff --git a/src/models.ts b/src/models.ts index 264afd00..be3857ee 100644 --- a/src/models.ts +++ b/src/models.ts @@ -12,7 +12,7 @@ export type StepGroup = StageName | "chat"; export const STEP_KEYS = [ "intake", "design", "spec-check", "plan", "plan-challenge", "build", "reconcile", "fix", - "review", "revise", "verify", "done-check", "lessons", "orchestrator", "write-intent", + "review", "revise", "resolve", "verify", "done-check", "lessons", "orchestrator", "write-intent", ] as const; export type StepKey = (typeof STEP_KEYS)[number]; @@ -39,6 +39,7 @@ export const STEPS: readonly StepInfo[] = [ { key: "fix", group: "build", what: "fixes after failed checks, and after a failed verify", byDefault: { follows: "build" } }, { key: "review", group: "review", what: "reviews the code (also the early review)", byDefault: { follows: "review" } }, { key: "revise", group: "review", what: "changes the code after review findings", byDefault: { follows: "build" } }, + { key: "resolve", group: "review", what: "resolves conflicts with main", byDefault: { ref: "strong" } }, { key: "verify", group: "verify", what: "runs and reads the checks", byDefault: { follows: "verify" } }, { key: "done-check", group: "verify", what: "judges whether the change did what it said", byDefault: { ref: "strong" } }, { key: "lessons", group: "verify", what: "notes lessons from the change", byDefault: { ref: "cheap" } }, diff --git a/src/phases.ts b/src/phases.ts index 41ddcc4e..54c7df4d 100644 --- a/src/phases.ts +++ b/src/phases.ts @@ -15,7 +15,8 @@ import { promptPath, renderPrompt, type PromptVars } from "./prompts"; import { issuesText, tracedSession } from "./sessions"; import { commandTimeoutMs, errorText, runCommand, type CommandResult } from "./shell"; import { clearPause } from "./heartbeat"; -import { assertNoCheckoutLock, assertNoLock, slotEnv } from "./lanes"; +import { assertNoCheckoutLock, assertNoLock } from "./lanes"; +import { changeEnv } from "./resources"; import { AssistantUnavailable, Cancelled, endsStepQuietly, LoopBudgetReached, QuestionAsked, SessionStopped, StopRequested, throwIfCancelled, throwIfStopping } from "./stop"; import { answerFor, answerPrompt, askedAt, closeQuestion, MAX_QUESTIONS, NO_MORE_QUESTIONS, questionIn, recordQuestion, TOO_MANY_NOTE, type Question } from "./questions"; import { readConversation } from "./conversation"; @@ -559,10 +560,10 @@ async function attempt<N extends PhaseName>(ctx: StepContext, spec: AgentPhaseSp timeoutMs: ctx.cfg.claude.timeout_minutes * 60_000, maxBudgetUsd: capUsd, resume: spec.resume, - // A session that may run the project's commands gets the change's slot, as its commands do. + // A session that may run the project's commands gets the change's slot, temp folder and ports, as its commands do. env: { LOOPSTRA_PHASE: spec.name, LOOPSTRA_SLUG: ctx.slug, LOOPSTRA_BASE: ctx.cfg.main_branch, LOOPSTRA_PROTECTED_LOG: protectedLog, - ...(RUNS_COMMANDS.has(spec.tools) ? slotEnv(ctx.slug) : {}), ...main?.env, ...(spec.env ?? {}), + ...(RUNS_COMMANDS.has(spec.tools) ? await changeEnv(ctx) : {}), ...main?.env, ...(spec.env ?? {}), }, signal, // Live: what people send this session while it runs reaches it (see src/steering.ts). @@ -701,17 +702,18 @@ export function summarize(e: Record<string, unknown>, cwd?: string): Record<stri /** * Runs a configured project command in `cwd` with the configured time limit, and traces it as a - * `command` event of `on.slug` (under phase `seq` when given). `env` defaults to LOOPSTRA_SLUG. + * `command` event of `on.slug` (under phase `seq` when given). `env` defaults to LOOPSTRA_SLUG and + * the change's own resources (see changeEnv). * `signal` cancels it (Cancelled is thrown); it defaults to the one for work run alongside. */ export async function projectCommand( - on: { trace: Trace; cfg: Config; slug: string }, command: string, cwd: string, + on: { root: string; trace: Trace; cfg: Config; slug: string }, command: string, cwd: string, opts: { env?: Record<string, string>; seq?: number; where?: string; signal?: AbortSignal } = {}, ): Promise<CommandResult> { const where = opts.where ? { where: opts.where } : {}; let r: CommandResult; try { - r = await runCommand(command, cwd, { env: opts.env ?? { LOOPSTRA_SLUG: on.slug, ...slotEnv(on.slug) }, timeoutMs: commandTimeoutMs(on.cfg), signal: opts.signal ?? cancelSignal() }); + r = await runCommand(command, cwd, { env: opts.env ?? { LOOPSTRA_SLUG: on.slug, ...(await changeEnv(on)) }, timeoutMs: commandTimeoutMs(on.cfg), signal: opts.signal ?? cancelSignal() }); } catch (e) { if (e instanceof Cancelled) on.trace.event(on.slug, "command", { command, note: e.message, ...where }, opts.seq); throw e; diff --git a/src/prompts.ts b/src/prompts.ts index d2857de0..51e78b21 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -9,7 +9,7 @@ import { templatePath } from "./package"; export const PROMPT_VARS = [ "slug", "main_branch", "intent", "priority", "spec", "plan", "review", "previous", "failure_output", "done_when", "concerns", "findings", "test_command", "test_result", "run_command", "commands", "feedback", - "conversation", + "conversation", "conflicts", "resolution", ] as const; export type PromptVars = Partial<Record<(typeof PROMPT_VARS)[number], string>>; diff --git a/src/resources.ts b/src/resources.ts new file mode 100644 index 00000000..7513d83a --- /dev/null +++ b/src/resources.ts @@ -0,0 +1,154 @@ +/** + * What a running change gets of the machine, so changes running at the same time do not share it: + * its slot (see slotEnv), its own temp folder, a block of free ports, and (speed.own_caches) its + * slot's own tool caches. Every project command, hook command and session that may run commands + * gets them through changeEnv. A change that is not claimed (main's health check) gets none. + */ +import { createHash } from "node:crypto"; +import { mkdirSync, readdirSync, rmSync } from "node:fs"; +import { createServer } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { Config } from "./config"; +import { onRelease, slotEnv } from "./lanes"; +import { loopstraDir } from "./paths"; + +/** + * The folder of this repository's temp folders: `loopstra-<8 hex of its path>` in the system's temp + * folder, not under the repository, whose path can be long: a Unix socket's path (which tools make + * under TMPDIR) may be at most about 104 bytes. + */ +export function tempRoot(root: string): string { + return join(tmpdir(), `loopstra-${createHash("sha256").update(root).digest("hex").slice(0, 8)}`); +} + +/** The folder of each slot's caches, `.loopstra/cache/`. */ +function cacheRoot(root: string): string { + return join(loopstraDir(root), "cache"); +} + +/** How many blocks of ports are tried before a change goes without (each try binds every port of the block). */ +const PORT_TRIES = 50; + +/** Each claimed change's temp folder (named by its slot, which is short), removed when its step releases it. */ +const temps = new Map<string, string>(); +/** Each claimed change's port block (its first port), or null when none was free; a promise while it is looked for. */ +const ports = new Map<string, Promise<number | null>>(); +/** The block index each claimed change holds or is trying, so two changes never try the same one. */ +const blocks = new Map<string, number>(); +/** The look for a block in progress for each claimed change (see portBlock). */ +const looks = new Map<string, symbol>(); + +onRelease((slug) => { + const temp = temps.get(slug); + temps.delete(slug); + ports.delete(slug); + blocks.delete(slug); + looks.delete(slug); + if (temp) rmSync(temp, { recursive: true, force: true, maxRetries: 2 }); +}); + +/** The tool caches a project may share between changes, each set to its own folder under the slot's cache folder. */ +const CACHE_VARS: ReadonlyArray<[string, string]> = [ + ["XDG_CACHE_HOME", "xdg"], ["npm_config_cache", "npm"], ["BUN_INSTALL_CACHE_DIR", "bun"], + ["YARN_CACHE_FOLDER", "yarn"], ["PIP_CACHE_DIR", "pip"], ["GOCACHE", "go"], +]; + +/** + * The environment a claimed change's commands and command-running sessions get (on top of + * LOOPSTRA_SLUG): LOOPSTRA_SLOT; TMPDIR, TMP and TEMP set to its own folder (see tempRoot, one per slot); + * LOOPSTRA_PORT_BASE and LOOPSTRA_PORT_COUNT when speed.ports_per_change is above 0 and a block of + * ports is free; and with speed.own_caches, the CACHE_VARS under `.loopstra/cache/slot-<n>/`. The + * same for the rest of the step. Empty for a change that is not claimed. + */ +export async function changeEnv(on: { root: string; cfg: Config; slug: string }): Promise<Record<string, string>> { + const slot = slotEnv(on.slug).LOOPSTRA_SLOT; + if (slot === undefined) return {}; + const env: Record<string, string> = { LOOPSTRA_SLOT: slot }; + const temp = tempFolder(on.root, on.slug, slot); + Object.assign(env, { TMPDIR: temp, TMP: temp, TEMP: temp }); + const count = on.cfg.speed.ports_per_change; + if (count > 0) { + const base = await portBlock(on.slug, count, on.cfg.speed.ports_from); + if (base !== null) Object.assign(env, { LOOPSTRA_PORT_BASE: String(base), LOOPSTRA_PORT_COUNT: String(count) }); + } + if (on.cfg.speed.own_caches) { + for (const [name, folder] of CACHE_VARS) { + const dir = join(cacheRoot(on.root), `slot-${slot}`, folder); + mkdirSync(dir, { recursive: true }); + env[name] = dir; + } + } + return env; +} + +/** The change's temp folder, made on first use: `s<slot>`, which no other running change has. */ +function tempFolder(root: string, slug: string, slot: string): string { + let dir = temps.get(slug); + if (!dir) { + dir = join(tempRoot(root), `s${slot}`); + temps.set(slug, dir); + } + mkdirSync(dir, { recursive: true }); + return dir; +} + +/** + * The first port of the change's block: the lowest block (`from + i × count`) that no other claimed + * change holds and whose ports all bind on 127.0.0.1 now. Null when none of the first PORT_TRIES free + * blocks are, or the ports run out. Looked for once per step; asking again meanwhile waits for it. + */ +function portBlock(slug: string, count: number, from: number): Promise<number | null> { + const known = ports.get(slug); + if (known) return known; + // Which look this is: one the change was released during (its step ended) stops. + const look = Symbol(slug); + looks.set(slug, look); + const found = (async () => { + let tried = 0; + for (let i = 0; from + (i + 1) * count - 1 <= 65535 && tried < PORT_TRIES; i++) { + if ([...blocks.values()].includes(i)) continue; + blocks.set(slug, i); + tried++; + if (await allFree(from + i * count, count)) return from + i * count; + // Released meanwhile: the step ended, so it needs no ports. + if (looks.get(slug) !== look) return null; + blocks.delete(slug); + } + blocks.delete(slug); + return null; + })(); + ports.set(slug, found); + return found; +} + +/** True when every port from `base` (`count` of them) can be bound on 127.0.0.1 now. */ +async function allFree(base: number, count: number): Promise<boolean> { + for (let p = base; p < base + count; p++) { + if (!(await bindable(p))) return false; + } + return true; +} + +function bindable(port: number): Promise<boolean> { + return new Promise((resolve) => { + const server = createServer(); + server.once("error", () => resolve(false)); + server.listen({ port, host: "127.0.0.1", exclusive: true }, () => server.close(() => resolve(true))); + }); +} + +/** + * Removes this repository's temp folders that no running change has: a step that crashed, or a loop + * that was killed, left them. Best effort; run at a tick's start. + */ +export function sweepTempFolders(root: string): void { + let names: string[]; + try { names = readdirSync(tempRoot(root)); } catch { return; } + const inUse = new Set(temps.values()); + for (const name of names) { + const dir = join(tempRoot(root), name); + if (inUse.has(dir)) continue; + try { rmSync(dir, { recursive: true, force: true }); } catch { /* the next tick tries again */ } + } +} diff --git a/src/scheduler.ts b/src/scheduler.ts index 6e811877..96b8b0b3 100644 --- a/src/scheduler.ts +++ b/src/scheduler.ts @@ -20,9 +20,10 @@ import { parsePlanFiles } from "./checks"; import { mainHealthDue, runMainHealth } from "./signals"; import { runBuildStep } from "./stages/build"; import { runDesignStep } from "./stages/design"; -import { cleanupChange, runMergeStep } from "./stages/merge"; +import { cleanupChange, conflictsWhileWaiting, runMergeStep } from "./stages/merge"; import { runPlanStep } from "./stages/plan"; import { RECONCILED_PLAN } from "./stages/shared"; +import { sweepTempFolders } from "./resources"; import { runVerifyStep } from "./stages/verify"; import { sweepInbox } from "./steering"; import { clearChangeStop } from "./inbox"; @@ -189,7 +190,11 @@ export async function tick(root: string): Promise<TickResult> { // With speed.think_ahead, design and plan steps also run beside the lanes (see thinkAhead), so // the lanes take the steps that may run the project's commands first. const human = humanGates(cfg); - const runnable = ordered.filter((i) => forLane(i, human, hasRemote) && !waitingOn(i, ordered).length && !holds.has(i.slug)); + // Without a remote, a change waiting for a person at the merge gate runs only when it no longer + // merges cleanly into main, to be brought up to date (with a remote, the watcher looks at it). + const stale = hasRemote ? new Set<string>() : await staleWhileWaiting(root, cfg, ordered, human); + const lane = (i: Intent) => forLane(i, human, hasRemote) || (stale.has(i.slug) && !waiting(i)); + const runnable = ordered.filter((i) => lane(i) && !waitingOn(i, ordered).length && !holds.has(i.slug)); // Not when the day's budget has no room for a second session (never with a daily budget and no // session limit): the thinker could not start, and design and plan steps would only wait longer. const thinking = cfg.speed.think_ahead && roomFor(cfg, trace, 2); @@ -201,7 +206,7 @@ export async function tick(root: string): Promise<TickResult> { const shared: TickLanes = { taken: new Set(), halt: false, problems: [], lanesRunning: 0 }; const started = Date.now(); // Settled, not all: the tick goes on only once every lane has ended, even one that failed outright. - const running = Promise.allSettled(Array.from({ length: lanes }, () => runLane(root, cfg, trace, candidates, shared, human, hasRemote))); + const running = Promise.allSettled(Array.from({ length: lanes }, () => runLane(root, cfg, trace, candidates, shared, lane))); // Started after the lanes, which take their changes at once: the thinker gets what they left. const thinker = thinking ? thinkAhead(root, cfg, trace, shared, human, hasRemote, running) : Promise.resolve([]); // Watching until the thinker's step has ended too: the tick waits for it. @@ -250,7 +255,7 @@ export async function tick(root: string): Promise<TickResult> { * (afterLanes). It rejects only when the trace cannot be written while it records a problem (a full * disk, a database held too long); see laneSteps. */ -async function runLane(root: string, cfg: Config, trace: Trace, candidates: Intent[], shared: TickLanes, human: HumanGates, hasRemote: boolean): Promise<LaneStep[]> { +async function runLane(root: string, cfg: Config, trace: Trace, candidates: Intent[], shared: TickLanes, lane: (i: Intent) => boolean): Promise<LaneStep[]> { // Counted from the moment the lane is started until it ends (see thinkAhead). shared.lanesRunning++; try { @@ -259,7 +264,7 @@ async function runLane(root: string, cfg: Config, trace: Trace, candidates: Inte if (shared.halt || stopRequested() || readLoopHold(root)) return []; const next = candidates.find((i) => takeChange(shared, i.slug)); if (!next) return []; - const ran = await runClaimed(root, cfg, trace, shared, next.slug, (i) => forLane(i, human, hasRemote)); + const ran = await runClaimed(root, cfg, trace, shared, next.slug, lane); if (ran) return [ran]; } } finally { @@ -267,6 +272,26 @@ async function runLane(root: string, cfg: Config, trace: Trace, candidates: Inte } } +/** + * Without a remote: the changes waiting for a person at the merge gate (merge-review) that no longer + * merge cleanly into main (see conflictsWhileWaiting), so a lane brings them up to date. A git + * failure leaves the change out: the merge checks bring it up to date later. + */ +async function staleWhileWaiting(root: string, cfg: Config, ordered: Intent[], human: HumanGates): Promise<Set<string>> { + const stale = new Set<string>(); + if (human.merge === "none") return stale; + const git = new Git(root); + for (const i of ordered) { + if (i.file.frontmatter.status !== "merge-review") continue; + try { + if (await conflictsWhileWaiting(git, cfg.main_branch, `intent/${i.slug}`)) stale.add(i.slug); + } catch (e) { + if (e instanceof StopRequested) throw e; + } + } + return stale; +} + /** True when a change is a lane's to run now: runnable, not held, and (with a remote) not watching its pull request. */ function forLane(i: Intent, human: HumanGates, hasRemote: boolean): boolean { return isRunnable(i, human, hasRemote) && !waiting(i) && !(hasRemote && WATCHING_PR.has(i.file.frontmatter.status)); @@ -802,6 +827,8 @@ const HANDOFF_LEFT_MS = 60 * 60_000; * never blocks anything. */ async function cleanupLeftovers(root: string, cfg: Config, trace: Trace, intents: Intent[]): Promise<void> { + // Temp folders of steps that crashed or of a loop that was killed (nothing is claimed at a tick's start). + sweepTempFolders(root); if (existsSync(snapshotsDir(root))) { for (const name of readdirSync(snapshotsDir(root))) { try { diff --git a/src/signals.ts b/src/signals.ts index af6b5eec..43191909 100644 --- a/src/signals.ts +++ b/src/signals.ts @@ -76,7 +76,7 @@ export async function runMainHealth(root: string, cfg: Config, trace: Trace, aft const git = new Git(root); const dir = join(loopstraDir(root), "health", "main"); // Traced on the loop; the commands get no change's slug. - const on = { trace, cfg, slug: "_loop" }; + const on = { root, trace, cfg, slug: "_loop" }; let result: HealthResult; let output: string; try { diff --git a/src/stages/build.ts b/src/stages/build.ts index 5beadbed..df7c5959 100644 --- a/src/stages/build.ts +++ b/src/stages/build.ts @@ -5,7 +5,8 @@ import { agentPhase, alongside, codePhase, mayOverlap, phasePrompt, projectComma import { EARLY_REVIEW, keepEarlyReview, reviewAhead } from "./early-review"; import { errorText, type CommandResult } from "../shell"; import { StopRequested } from "../stop"; -import { artifacts, bullets, buildSession, MERGING, PR_LEASE, PR_NUMBER, PR_PUSHING, PR_WAIT, RECONCILED_PLAN, REMOTE_AHEAD, REVIEW_ROUND, runHookCommands, saveWork, TESTED, testedMarker, testResult } from "./shared"; +import { RESOLUTION, RESOLUTIONS, RESOLVED_AT } from "./catch-up"; +import { artifacts, bullets, buildSession, MERGING, PR_LEASE, PR_NUMBER, PR_PUSHING, PR_WAIT, RECONCILED_PLAN, REMOTE_AHEAD, restoreCatchUp, REVIEW_ROUND, runHookCommands, saveWork, TESTED, testedMarker, testResult } from "./shared"; /** Stage 3 build half plus Stage 4. Called for plan-approved and building. Ends at reviewing or blocked. */ export async function runBuildStep(ctx: StepContext): Promise<StepResult> { @@ -18,12 +19,17 @@ export async function runBuildStep(ctx: StepContext): Promise<StepResult> { clearMarker(ctx, PR_LEASE); // nor tied to what was pushed for it (a person may have deleted that branch on GitHub) clearMarker(ctx, PR_PUSHING); // nor an earlier build's push that was cut short writeMarker(ctx, PR_NUMBER, ""); // nor an earlier build's pull request, closed or merged (see ownPr) + clearMarker(ctx, RESOLUTIONS); // a new build gets its own conflict resolutions + clearMarker(ctx, RESOLUTION); // and no reviewer is shown an earlier build's + clearMarker(ctx, RESOLVED_AT); // nor does an earlier build's resolution ask for a fresh approval const branch = await codePhase(ctx, "branch", async () => { // Branches and worktrees take no lock (see Git.run): another change's commits and checkouts go on meanwhile. if (!(await ctx.git.branchExists(ctx.branch))) await ctx.git.createBranch(ctx.branch, ctx.cfg.main_branch); // Never trust a leftover directory: a plain folder here would send commits to the main checkout. await ctx.git.ensureWorktree(ctx.worktreeDir, ctx.branch); + // A conflict resolution an earlier step left part-way is undone before anything builds on it. + await restoreCatchUp(ctx); if (ctx.cfg.commands.install) { const r = await projectCommand(ctx, ctx.cfg.commands.install, ctx.worktreeDir); if (r.code !== 0) throw new Error(`install failed: ${r.lastLine}`); diff --git a/src/stages/catch-up.ts b/src/stages/catch-up.ts new file mode 100644 index 00000000..c22590b8 --- /dev/null +++ b/src/stages/catch-up.ts @@ -0,0 +1,283 @@ +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { clearMarker, readMarker, writeArtifact, writeMarker, type Failure, type StepContext } from "../context"; +import { Git, NO_AUTO_MAINTENANCE, passOn, RUNTIME_COMMIT_CONFIG, RUNTIME_EMAIL } from "../git"; +import { agentPhase, codePhase } from "../phases"; +import { errorText } from "../shell"; +import { artifacts, bullets, CATCHING_UP, restoreCatchUp } from "./shared"; + +/** Run-folder marker: how many conflict resolutions this build has tried (see roundsAllowed). A new build clears it. */ +export const RESOLUTIONS = "resolutions"; +/** Run-folder marker: the latest resolution, as the reviewer reads it (`{{resolution}}`). Cleared when a review passes, and by a new build. */ +export const RESOLUTION = "resolution.md"; +/** + * Run-folder marker: a resolution this build made, for the approval on GitHub: PUSH_PENDING until the + * pull request's branch has it, then when it was pushed (an ISO time). An approval from before that + * does not count (see freshApproval in merge.ts). + */ +export const RESOLVED_AT = "resolved-at"; +/** RESOLVED_AT while the resolution is not on GitHub yet. */ +export const PUSH_PENDING = "pending"; + +/** Every conflict section a prompt shows, at most this many lines per file. */ +const LINES_PER_FILE = 200; +/** All conflicts a prompt shows, at most this many characters. */ +const CONFLICT_CHARS = 40_000; +/** Lines of context shown around each conflict section. */ +const CONTEXT = 3; +/** A conflict marker line the resolution must not leave behind. `=======` alone is left out: it is also a Markdown heading underline. */ +const MARKER = /^(<{7}|\|{7}|>{7})( |$)/m; + +export const CONFLICT_NOTE = "The change overlaps with other recent changes, and resolving the conflict automatically did not work. Nothing was merged. The details are in conflict.md. An engineer needs to look at it."; +const USED_UP_NOTE = "The change overlaps with other recent changes, and it has already been brought up to date as many times as allowed. Nothing was merged. The details are in conflict.md. An engineer needs to look at it."; +const OFF_NOTE = "The change overlaps with other recent changes, and resolving conflicts automatically is turned off (stages.review.max_resolutions is 0). Nothing was merged. The details are in conflict.md. An engineer needs to look at it."; + +/** How the branch was brought up to date: it had main already, it rebased cleanly, or conflicts were resolved. */ +export type CatchUpOutcome = "current" | "rebased" | "resolved"; + +/** The resolutions this build has tried. */ +function resolutionsUsed(ctx: StepContext): number { + const n = Number.parseInt(readMarker(ctx, RESOLUTIONS) ?? "", 10); + return Number.isInteger(n) && n > 0 ? n : 0; +} + +/** The review rounds this build may run: review.max_rounds, plus one for each resolution, whose review does not count against them. */ +export function roundsAllowed(ctx: StepContext): number { + return ctx.cfg.stages.review.max_rounds + resolutionsUsed(ctx); +} + +/** + * True when `ref` has code (outside intent/) that `branch` lacks: it changed files since they + * parted. A comparison git cannot make (no common history) counts as out of date. + */ +export async function outOfDate(git: Git, ref: string, branch: string): Promise<boolean> { + if (await git.isAncestor(ref, branch)) return false; + const gained = await git.run(["diff", "--name-only", "-z", `${branch}...${ref}`], true); + if (gained.code !== 0) return true; + return gained.out.split("\0").some((f) => f && !f.startsWith("intent/")); +} + +/** + * Brings the change's branch up to date with main, in its worktree, which must be clean (each + * caller saves or refuses unsaved edits first). The branch has main → current; it rebases cleanly → + * rebased. On a conflict, while resolutions are left (stages.review.max_resolutions per build), the + * branch's commits are squashed into one on top of main (see resolveConflicts): a squash that + * applies cleanly → rebased; one that conflicts → the resolve session resolves it → resolved. A + * failure leaves the branch where it was, writes conflict.md, and is returned for the caller to + * block with. + */ +export async function catchUp(ctx: StepContext): Promise<{ ok: true; outcome: CatchUpOutcome; files: string[] } | Failure> { + const main = ctx.cfg.main_branch; + if (await ctx.git.isAncestor(main, ctx.branch)) return { ok: true, outcome: "current", files: [] }; + const wt = new Git(ctx.worktreeDir); + if (await wt.rebaseOnto(main)) return { ok: true, outcome: "rebased", files: [] }; + const max = ctx.cfg.stages.review.max_resolutions; + if (resolutionsUsed(ctx) >= max) { + const reason = max === 0 ? "resolving conflicts automatically is turned off" : `this build has already had ${max} conflict resolutions`; + const files = await conflictingFiles(ctx.git, main, ctx.branch); + return { ...(await failed(ctx, null, files, "", reason)), note: max === 0 ? OFF_NOTE : USED_UP_NOTE }; + } + return resolveConflicts(ctx, wt); +} + +/** + * The branch's commits since it parted from main, squashed into one on top of main (`merge + * --squash`): the resolve session then meets every conflict once, against the final code, not once + * per commit. The old head is kept in CATCHING_UP until the branch is saved again, so a stop, an + * outage or a crash meanwhile puts it back (here, or the next time the worktree is opened). + */ +async function resolveConflicts(ctx: StepContext, wt: Git): Promise<{ ok: true; outcome: CatchUpOutcome; files: string[] } | Failure> { + const main = ctx.cfg.main_branch; + const old = await wt.headSha(); + const base = (await wt.run(["merge-base", main, "HEAD"])).out.trim(); + const message = await squashMessage(ctx, wt, base); + writeMarker(ctx, CATCHING_UP, old); + try { + const squashed = await codePhase(ctx, "catch-up", async () => { + await wt.run([...NO_AUTO_MAINTENANCE, "reset", "-q", "--keep", main]); + const merged = await wt.run([...RUNTIME_COMMIT_CONFIG, "-c", "merge.conflictStyle=diff3", "merge", "--no-verify", "-q", "--squash", old], true); + const files = await wt.unmergedFiles(); + if (merged.code !== 0 && !files.length) throw new Error(`merge --squash of ${old} onto ${main}: ${(merged.err || merged.out).trim()}`); + // Allowed empty: main may already have everything the change did. + if (!files.length) await wt.runtime("commit", ["-q", "--allow-empty", "-m", message]); + ctx.trace.event(ctx.slug, "command", { command: "catch up", main, files, note: files.length ? "the change was squashed onto main, and conflicts are left for the resolve session" : "the change was squashed onto main without conflicts" }); + return { ok: true as const, files }; + }); + if (!squashed.ok) return await failed(ctx, old, [], "", squashed.detail); + if (!squashed.files.length) { + clearMarker(ctx, CATCHING_UP); + return { ok: true, outcome: "rebased", files: [] }; + } + const files = squashed.files; + const conflicts = await conflictsText(ctx, wt, base, files); + // git leaves main's copy of a binary file with no markers, so a resolution could keep it unseen: a person chooses. + const binary = files.filter((f) => isBinary(join(ctx.worktreeDir, f))); + if (binary.length) return await failed(ctx, old, files, conflicts, `both sides changed ${binary.join(", ")}, which ${binary.length === 1 ? "is a binary file" : "are binary files"} a person needs to choose between`); + const a = await artifacts(ctx); + const session = await agentPhase(ctx, { + name: "resolve", permissionMode: "acceptEdits", tools: "build", cwd: ctx.worktreeDir, + vars: { spec: a.spec, plan: a.currentPlan, conflicts, test_command: ctx.cfg.commands.test }, + }); + // Counted once the session has answered: one a stop, an outage or the day's budget cut short is not. + writeMarker(ctx, RESOLUTIONS, String(resolutionsUsed(ctx) + 1)); + if (!session.ok) return await failed(ctx, old, files, conflicts, `the resolve session did not finish: ${session.note}`); + const { resolved, reason, summary_markdown: summary } = session.envelope; + if (!resolved) return await failed(ctx, old, files, conflicts, `the resolve session could not resolve it: ${reason || "no reason given"}`, summary); + const marked = files.filter((f) => hasMarkers(join(ctx.worktreeDir, f))); + if (marked.length) return await failed(ctx, old, files, conflicts, `conflict markers were left in ${marked.join(", ")}`, summary); + const saved = await codePhase(ctx, "save-resolution", async () => { + // New files are the output of commands the session ran (a coverage folder, a test report), as + // after the test loop's runs: the change's own new files are already in the index. + await wt.run(["clean", "-f", "-d", "-q"], true); + await wt.run(["add", "-A"]); + const left = await wt.unmergedFiles(); + if (left.length) throw new Error(`still unmerged: ${left.join(", ")}`); + // Allowed empty: the resolution may keep main's side of everything (main already did what the change did). + await wt.runtime("commit", ["-q", "--allow-empty", "-m", message]); + return { ok: true as const }; + }); + if (!saved.ok) return await failed(ctx, old, files, conflicts, saved.detail, summary); + clearMarker(ctx, CATCHING_UP); + const sha = (await ctx.git.run(["rev-parse", "--short", main])).out.trim(); + writeMarker(ctx, RESOLUTION, resolutionRecord(main, sha, files, summary, conflicts)); + writeMarker(ctx, RESOLVED_AT, PUSH_PENDING); + ctx.trace.event(ctx.slug, "command", { command: "catch up", main, files, note: `conflicts with ${main} were resolved; the resolution is tested and reviewed before it merges` }); + return { ok: true, outcome: "resolved", files }; + } finally { + // A stop, an outage, the day's budget or a crash part-way: the branch goes back to where it was. + if (readMarker(ctx, CATCHING_UP) !== null) await restoreCatchUp(ctx); + } +} + +/** The squashed commit's message: the subjects it replaces, and a Co-authored-by line for each author who is not the runtime. */ +async function squashMessage(ctx: StepContext, wt: Git, base: string): Promise<string> { + const log = (await wt.run(["log", "--reverse", "--format=%s%x00%an <%ae>", `${base}..HEAD`])).out.split("\n").filter(Boolean); + const subjects = log.map((l) => l.split("\0")[0] ?? ""); + const authors = [...new Set(log.map((l) => l.split("\0")[1] ?? "").filter((a) => a && !a.endsWith(`<${RUNTIME_EMAIL}>`)))]; + return [ + `loopstra(${ctx.slug}): brought up to date with ${ctx.cfg.main_branch}`, + "", + "Squashes:", + bullets(subjects), + ...(authors.length ? ["", ...authors.map((a) => `Co-authored-by: ${a}`)] : []), + ].join("\n"); +} + +/** True when the file looks binary (a NUL byte in its first 8000 bytes, as git judges). */ +function isBinary(path: string): boolean { + if (!existsSync(path)) return false; + try { return readFileSync(path).subarray(0, 8000).includes(0); } catch { return false; } +} + +/** The files that conflict merging `branch` into `into` (git 2.38+), for conflict.md when no resolution is tried; none when git cannot say. */ +async function conflictingFiles(git: Git, into: string, branch: string): Promise<string[]> { + const r = await git.run(["merge-tree", "--write-tree", "--name-only", "--no-messages", into, branch], true); + return r.code === 1 ? r.out.split("\n").slice(1).map((l) => l.trim()).filter(Boolean) : []; +} + +/** True when the file has a conflict marker line (a missing file has none: deleting it is a resolution). */ +function hasMarkers(path: string): boolean { + if (!existsSync(path)) return false; + try { return MARKER.test(readFileSync(path, "utf8")); } catch { return false; } +} + +/** + * What the resolve session and the reviewer are shown: for each file, its conflict sections with a + * few lines around them (at most LINES_PER_FILE lines per file, CONFLICT_CHARS in all), then the + * commits on main since the branch parted from it that touched those files, naming the spec and plan + * of each change merged locally (`<slug>: <title>`) that has them. + */ +async function conflictsText(ctx: StepContext, wt: Git, base: string, files: string[]): Promise<string> { + const parts: string[] = []; + let left = CONFLICT_CHARS; + for (const file of files) { + let text = `### ${file}\n\n`; + const path = join(ctx.worktreeDir, file); + const content = existsSync(path) ? readFileSync(path, "utf8") : null; + const sections = content === null ? null : conflictSections(content); + if (content === null) text += "Deleted on one side and changed on the other.\n"; + else if (!sections) text += "No conflict sections in the text (a binary file, or deleted on one side and changed on the other).\n"; + else text += `\`\`\`\n${sections.text}\n\`\`\`\n${sections.cut ? "\nThe file has more conflicts than are shown here; read it in full.\n" : ""}`; + if (text.length > left) { + parts.push(`### ${file}\n\nNot shown here (too long); read the file.\n`); + continue; + } + left -= text.length; + parts.push(text); + } + const log = (await wt.run(["log", "--format=%h %s", `${base}..${ctx.cfg.main_branch}`, "--", ...files], true)).out.split("\n").filter(Boolean); + const commits = log.map((line) => { + const slug = /^\S+ ([a-z0-9][a-z0-9-]*): /.exec(line)?.[1]; + const other = slug && slug !== ctx.slug && existsSync(join(ctx.worktreeDir, "intent", slug, "spec.md")); + return other ? `${line} (its spec: intent/${slug}/spec.md, its plan: intent/${slug}/plan.md)` : line; + }); + return `${parts.join("\n")}\n### Commits on ${ctx.cfg.main_branch} that touched these files\n\n${commits.length ? bullets(commits) : "(none found)"}\n`; +} + +/** The conflict sections of a file, each with CONTEXT lines around it, at most LINES_PER_FILE lines. Null when it has none. */ +function conflictSections(content: string): { text: string; cut: boolean } | null { + const lines = content.split("\n"); + const keep = new Set<number>(); + let open = -1; + lines.forEach((line, i) => { + if (line.startsWith("<<<<<<<")) open = i; + else if (line.startsWith(">>>>>>>") && open >= 0) { + for (let j = Math.max(0, open - CONTEXT); j <= Math.min(lines.length - 1, i + CONTEXT); j++) keep.add(j); + open = -1; + } + }); + if (!keep.size) return null; + const out: string[] = []; + let last = -2; + let cut = false; + for (const i of [...keep].sort((x, y) => x - y)) { + if (out.length >= LINES_PER_FILE) { cut = true; break; } + if (i > last + 1 && out.length) out.push("..."); + out.push(`${i + 1}: ${lines[i] ?? ""}`); + last = i; + } + return { text: out.join("\n"), cut }; +} + +/** The resolution as the reviewer reads it. */ +function resolutionRecord(main: string, sha: string, files: string[], summary: string, conflicts: string): string { + return [ + `This change was brought up to date with ${main} (at ${sha}), and conflicts in ${files.join(", ")} were resolved by an agent. Check that the resolution keeps what both sides meant.`, + "", + "## What the agent did", + "", + summary.trim() || "(no summary)", + "", + "## The conflicts as they were", + "", + conflicts, + ].join("\n"); +} + +/** + * A resolution that did not work: the branch goes back to where it was, conflict.md records what + * happened (written on main beside review.md), and the failure says so in plain words. + */ +async function failed(ctx: StepContext, old: string | null, files: string[], conflicts: string, reason: string, summary = ""): Promise<Failure> { + await restoreCatchUp(ctx); + ctx.trace.event(ctx.slug, "command", { command: "catch up", files, note: `the conflicts with ${ctx.cfg.main_branch} were not resolved: ${reason}`, head: old }); + const record = [ + "# Conflict with main", + "", + `Bringing this change up to date with ${ctx.cfg.main_branch} hit conflicts that were not resolved automatically. The change's branch was left as it was.`, + "", + `Why: ${reason}`, + ...(summary.trim() ? ["", "## What the agent said", "", summary.trim()] : []), + "", + "## The conflicts", + "", + conflicts || (files.length ? bullets(files) : "(not known)"), + ].join("\n"); + try { + await writeArtifact(ctx, "conflict.md", record); + } catch (e) { + passOn(e); + ctx.trace.event(ctx.slug, "error", { where: "catch up", what: "conflict.md", error: errorText(e) }); + } + return { ok: false, note: CONFLICT_NOTE, detail: `${reason}${files.length ? ` (conflicts in ${files.join(", ")})` : ""}` }; +} diff --git a/src/stages/early-review.ts b/src/stages/early-review.ts index fb62564e..51e05a6c 100644 --- a/src/stages/early-review.ts +++ b/src/stages/early-review.ts @@ -1,6 +1,7 @@ import { createHash } from "node:crypto"; import { join } from "node:path"; -import { writeMarker, type StepContext } from "../context"; +import { readMarker, writeMarker, type StepContext } from "../context"; +import { RESOLUTION } from "./catch-up"; import { Git, withDetachedWorktree } from "../git"; import { cheapMayDecide, runJudge, type JudgeResult } from "../judges"; import { loopstraDir } from "../paths"; @@ -20,10 +21,20 @@ export async function reviewPhase(ctx: StepContext, round: number, spec: string, const stage = ctx.cfg.stages.review; return { name: "review", traceName: `review-${round}`, permissionMode: "default", tools: "read+git", cwd: ctx.worktreeDir, - vars: { spec, plan, test_result: await testResult(ctx, new Git(ctx.worktreeDir)) }, skills: stage.skills, + vars: { spec, plan, test_result: await testResult(ctx, new Git(ctx.worktreeDir)), resolution: readMarker(ctx, RESOLUTION) ?? "" }, skills: stage.skills, }; } +/** + * A round's review prompt (the early review's too, so the two match). A conflict resolution since + * the last passing review reaches the reviewer even through a repository's own review.md without + * `{{resolution}}`. + */ +export function reviewPrompt(ctx: StepContext, phase: AgentPhaseSpec & { name: "review" }): Promise<string | null> { + const note = phase.vars.resolution ?? ""; + return phasePrompt(ctx, phase, { variable: "resolution", note, text: `This change was brought up to date with ${ctx.cfg.main_branch}, and conflicts were resolved; review the resolution too:\n\n${note}` }); +} + /** * What a review is asked about, hashed: the commit, the model (the CLI name it ran on, or would run on * now), and the whole rendered prompt. @@ -43,7 +54,7 @@ function reviewKey(sha: string, model: string, prompt: string): string { */ export async function reviewAhead(ctx: StepContext, spec: string, plan: string): Promise<string | null> { const phase = { ...(await reviewPhase(ctx, 1, spec, plan)), traceName: `${EARLY}review` }; - const prompt = await phasePrompt(ctx, phase); + const prompt = await reviewPrompt(ctx, phase); if (prompt === null) return null; const sha = await new Git(ctx.worktreeDir).headSha(); const dir = join(loopstraDir(ctx.root), "review", ctx.slug); diff --git a/src/stages/merge.ts b/src/stages/merge.ts index 0bfe6241..bd154333 100644 --- a/src/stages/merge.ts +++ b/src/stages/merge.ts @@ -11,7 +11,8 @@ import { errorText } from "../shell"; import { markHealthPending } from "../signals"; import { endsStepQuietly, notTheStepsFault } from "../stop"; import { testLoop } from "./build"; -import { REVIEW_PASSED, runReviewRounds } from "./review"; +import { catchUp, outOfDate, PUSH_PENDING, RESOLVED_AT, roundsAllowed } from "./catch-up"; +import { catchUpTested, REVIEW_PASSED, runReviewRounds } from "./review"; import { alreadyTested, KEEP_UNFINISHED, MERGE_WAIT_NOTE, MERGING, openBranchWorktree, PR_APPROVE_NOTE, PR_CHECKS_NOTE, PR_LEASE, PR_MERGED_ELSEWHERE, PR_NUMBER, PR_PUSHING, PR_WAIT, readRound, REMOTE_AHEAD, REVIEW_ROUND, saveWork, writeRound } from "./shared"; export const DIRTY_ROOT_NOTE = "The main checkout has unsaved changes or is on another branch; an engineer needs to tidy it up before this can merge. Then set status to merge-approved."; @@ -88,11 +89,11 @@ async function checkMerge(ctx: StepContext): Promise<MergeVerdict> { name: "up-to-date", run: async () => { if (await ctx.git.isAncestor(main, ctx.branch)) return { result: "pass", evidence: "branch contains main" }; - if (await wt.rebaseOnto(main)) return { result: "pass", evidence: "rebased onto main" }; - return { - result: "fail", evidence: "rebase onto main hit conflicts", - payload: { ok: false, note: UPDATE_FAILED_NOTE, detail: `rebase of ${ctx.branch} onto ${main} hit conflicts and was aborted` }, - }; + // A conflict is resolved (see catchUp): the tests run on it next, and a review round sees it before it merges. + const up = await catchUp(ctx); + if (!up.ok) return { result: "fail", evidence: `bringing the branch up to date with main failed: ${up.detail}`, payload: up }; + if (up.outcome === "resolved") return { result: "pass", evidence: `resolved conflicts with main in ${up.files.join(", ")}`, payload: { ok: true, changed: true } }; + return { result: "pass", evidence: "rebased onto main" }; }, }, { @@ -203,6 +204,8 @@ async function openPullRequest(ctx: StepContext, newReview: boolean): Promise<{ writeMarker(ctx, PR_LEASE, head); clearMarker(ctx, PR_PUSHING); writeMarker(ctx, PR_PUSHED, new Date().toISOString()); + // A resolution is on GitHub from now: only an approval given after this covers it (see freshApproval). + if (readMarker(ctx, RESOLVED_AT)?.trim() === PUSH_PENDING) writeMarker(ctx, RESOLVED_AT, new Date().toISOString()); const created = !pr; if (!pr) { const dir = `intent/${ctx.slug}`; @@ -369,8 +372,9 @@ async function watchPullRequest(ctx: StepContext, gh: GitHub, unanswered: (error } const status = ctx.intent.file.frontmatter.status; - // A person decides on the status line and has not yet: only a merge or close on GitHub counts. - if (ctx.cfg.gates.merge.human === "status" && status !== "merge-approved") return { ok: true, waiting: true }; + // A person decides on the status line and has not yet: only a merge or close on GitHub counts, + // and the change is kept mergeable meanwhile. + if (ctx.cfg.gates.merge.human === "status" && status !== "merge-approved") return keepCurrentWhileWatching(ctx); // A draft: GitHub refuses to merge it until a person marks it ready for review, so it is not // approved yet, whatever its checks say (traced once). if (pr.draft) { @@ -387,10 +391,20 @@ async function watchPullRequest(ctx: StepContext, gh: GitHub, unanswered: (error clearMarker(ctx, REMOTE_AHEAD); return block(ctx, PR_CHECKS_FAILED_NOTE, { detail: { pr: pr.number, url: pr.url }, retryFrom: mergeRetry(ctx) }); } - // merge.human pr: approved on GitHub, or a person set merge-approved on the status line. - if (ctx.cfg.gates.merge.human === "pr" && !pr.approved && status !== "merge-approved") return { ok: true, waiting: true }; + // merge.human pr: approved on GitHub, or a person set merge-approved on the status line. An + // approval from before a conflict resolution approved other code (see freshApproval). + const approved = pr.approved && freshApproval(ctx, pr); + if (ctx.cfg.gates.merge.human === "pr" && !approved && status !== "merge-approved") { + if (pr.approved) { + // Approved on GitHub, but before a conflict resolution: say why it waits, or it would look stuck. + if (onceMarker(ctx, STALE_APPROVAL)) ctx.trace.event(ctx.slug, "command", { command: "merge", waiting: STALE_APPROVAL_TEXT, pr: pr.number }); + waits(STALE_APPROVAL_TEXT); + } else clearMarker(ctx, STALE_APPROVAL); + return keepCurrentWhileWatching(ctx); + } + clearMarker(ctx, STALE_APPROVAL); ctx.trace.gate(ctx.slug, "merge", "pr-checks", "pass", checks === "none" ? `${where} (no checks reported)` : where); - if (ctx.cfg.gates.merge.human === "pr") ctx.trace.gate(ctx.slug, "merge", "pr-approved", "pass", pr.approved ? "approved on GitHub" : "merge-approved on the status line"); + if (ctx.cfg.gates.merge.human === "pr") ctx.trace.gate(ctx.slug, "merge", "pr-approved", "pass", approved ? "approved on GitHub" : "merge-approved on the status line"); // At every speed.max_parallel_changes: the watcher runs beside the lanes even at one, so another // pull request may merge while this one waits, and it is then out of date. return mergeWhenUpToDate(ctx, gh, pr, waits); @@ -493,8 +507,8 @@ async function upToDate(ctx: StepContext): Promise<Freshness | Failure> { took = true; } if (!(await ctx.git.isAncestor(ctx.branch, pushed))) return "not pushed"; - if (await outOfDate(ctx, main, pushed)) return "main ahead"; - if (await outOfDate(ctx, `${remote}/${main}`, pushed)) return "remote ahead"; + if (await outOfDate(ctx.git, main, pushed)) return "main ahead"; + if (await outOfDate(ctx.git, `${remote}/${main}`, pushed)) return "remote ahead"; return took ? "taken" : "up to date"; } @@ -517,17 +531,6 @@ async function takePersonCommits(ctx: StepContext, sha: string): Promise<{ ok: t return { ok: true }; } -/** - * True when `ref` has code (outside intent/) that `branch` lacks: it changed files since they - * parted. A comparison git cannot make (no common history) counts as out of date. - */ -async function outOfDate(ctx: StepContext, ref: string, branch: string): Promise<boolean> { - if (await ctx.git.isAncestor(ref, branch)) return false; - const gained = await ctx.git.run(["diff", "--name-only", "-z", `${branch}...${ref}`], true); - if (gained.code !== 0) return true; - return gained.out.split("\0").some((f) => f && !f.startsWith("intent/")); -} - /** GitHub may not have registered the workflow runs of a pull request opened, or pushed to (by Loopstra or a person), moments ago. */ async function checksMayStillStart(ctx: StepContext, pr: PrInfo): Promise<boolean> { const since = Math.max(0, ...[Date.parse(pr.createdAt), Date.parse(readMarker(ctx, PR_PUSHED) ?? "")].filter(Number.isFinite)); @@ -643,6 +646,95 @@ function withReason(text: string, synced: SyncOutcome | null): string { return synced?.behind ? `${text} ${synced.text}` : text; } +/** The note of a change sent back to review after it was brought up to date while it waited for a person. */ +export const CAUGHT_UP_NOTE = "Other work merged into main meanwhile conflicted with this change, so it was brought up to date. It is tested and reviewed again before it comes back to you."; + +/** + * Whether a change waiting at the merge gate no longer merges cleanly into main: main gained code + * since it parted, and git finds a conflict merging the two. False when it merges cleanly (even if + * it is behind), and when git cannot say (before git 2.38): the merge checks resolve it later. + */ +export async function conflictsWhileWaiting(git: Git, main: string, branch: string): Promise<boolean> { + const [at, head] = [await git.commitOf(main), await git.commitOf(`refs/heads/${branch}`)]; + if (at === null || head === null) return false; + // Asked every tick and every look of the watcher: worked out again only once either side moved. + const where = `${git.cwd}\0${branch}`; + const known = conflictsSeen.get(where); + if (known?.at === at && known.head === head) return known.conflicts; + const conflicts = (await outOfDate(git, main, branch)) && (await git.mergesCleanly(main, branch)) === false; + conflictsSeen.set(where, { at, head, conflicts }); + return conflicts; +} + +/** What conflictsWhileWaiting last found for each repository and branch, with main's commit and the branch's then. */ +const conflictsSeen = new Map<string, { at: string; head: string; conflicts: boolean }>(); + +/** + * Keeps a change that waits for a person at the merge gate mergeable (see conflictsWhileWaiting): + * one that no longer merges cleanly is brought up to date (see catchUp), tested, and sent back to + * reviewing for one more round, so the person approves code that will merge. Null when there was + * nothing to do. + */ +async function keepCurrent(ctx: StepContext): Promise<StepResult | null> { + if (!(await conflictsWhileWaiting(ctx.git, ctx.cfg.main_branch, ctx.branch))) return null; + const open = await openBranchWorktree(ctx); + if (!open.ok) return block(ctx, open.note, open); + const wt = new Git(ctx.worktreeDir); + if (await wt.isDirty()) { + // As the merge checks do: edits nobody reviewed are kept on the branch, so the next review sees them. + const kept = await saveWork(ctx, wt, KEEP_UNFINISHED); + if (!kept.ok) return block(ctx, kept.note, kept); + } + const up = await catchUpTested(ctx); + if (!up.ok) return block(ctx, up.note, up); + const again = await anotherRound(ctx); + if (again !== REVIEW_AGAIN) return again; + await setStatus(ctx, "reviewing", CAUGHT_UP_NOTE); + return { ok: true }; +} + +/** + * keepCurrent for a pull request the watcher looks at while it waits for a person. It may run tests + * and sessions, so only while no more than speed.max_parallel_changes changes run a step that may + * run commands, counting this one (as mergeWhenUpToDate); otherwise a later look does it. + */ +async function keepCurrentWhileWatching(ctx: StepContext): Promise<StepResult> { + const waiting: StepResult = { ok: true, waiting: true }; + if (othersRunningCommands(ctx.slug) >= ctx.cfg.speed.max_parallel_changes) return waiting; + if (!(await conflictsWhileWaiting(ctx.git, ctx.cfg.main_branch, ctx.branch))) return waiting; + // What would merge on GitHub first, as mergeWhenUpToDate does: main as it is there, and commits a + // person added to the pull request, so the resolution starts from them and its push goes over them. + const seen = await oneMergeAtATime(ctx, async () => { + await syncMain(ctx.root, ctx.cfg, ctx.trace); + return upToDate(ctx); + }); + if (typeof seen !== "string") return block(ctx, seen.note, { ...seen, retryFrom: mergeRetry(ctx) }); + if (seen === "both changed") return block(ctx, PR_BRANCH_MOVED_NOTE, { detail: `${ctx.branch} here and on GitHub each have commits the other lacks`, retryFrom: mergeRetry(ctx) }); + if (seen === "unreachable") return waiting; + return (await keepCurrent(ctx)) ?? waiting; +} + +/** Run-folder marker: the pull request's approval on GitHub came before a conflict resolution (traced once). */ +const STALE_APPROVAL = "stale-approval"; +/** Why a pull request approved before a conflict resolution waits, for PR_WAIT. */ +export const STALE_APPROVAL_TEXT = "It was approved on GitHub before conflicts with main were resolved; approve the pull request again to merge the resolved code."; + +/** + * True when an approval on GitHub covers the code as it is: there was no conflict resolution in this + * build, or a person approved after the latest one reached GitHub (RESOLVED_AT). Not while the + * resolution is still waiting to be pushed. GitHub keeps an approval through a push unless the + * repository dismisses stale ones, and the resolution is code they have not seen. + */ +function freshApproval(ctx: StepContext, pr: PrInfo): boolean { + const marker = readMarker(ctx, RESOLVED_AT)?.trim(); + if (marker === undefined) return true; + if (marker === PUSH_PENDING) return false; + const pushed = Date.parse(marker); + // A marker that cannot be read says nothing: as before resolutions, GitHub's approval stands. + if (!Number.isFinite(pushed)) return true; + return pr.approvedAt !== null && Date.parse(pr.approvedAt) > pushed; +} + /** The merge gate sent the change back for one more review round (the round marker names it). */ const REVIEW_AGAIN = "review-again"; @@ -711,7 +803,7 @@ function oneMergeAtATime<T>(ctx: StepContext, fn: () => Promise<T>): Promise<T> */ async function anotherRound(ctx: StepContext): Promise<StepResult | typeof REVIEW_AGAIN> { const round = readRound(ctx) ?? 1; - if (round > ctx.cfg.stages.review.max_rounds) { + if (round > roundsAllowed(ctx)) { clearMarker(ctx, REVIEW_ROUND); return block(ctx, REVIEWS_USED_UP, { detail: { rounds: round, reason: "fixes were committed after the last review" } }); } @@ -740,7 +832,9 @@ export async function runMergeStep(ctx: StepContext): Promise<StepResult> { } } if ((await ctx.git.remoteName()) !== null) return runRemoteMerge(ctx); - if (status === "merge-review" && ctx.cfg.gates.merge.human !== "none") return { ok: true }; + // A person decides and has not yet: the change is kept mergeable meanwhile (the scheduler runs + // this step only when it no longer merges cleanly; see conflictsWhileWaiting). + if (status === "merge-review" && ctx.cfg.gates.merge.human !== "none") return (await keepCurrent(ctx)) ?? { ok: true }; // A person may have set merge-approved (or edited the change's other files) without committing: // record the change's own folder first, so their edit is part of main before the checks run. diff --git a/src/stages/review.ts b/src/stages/review.ts index 7fc7b5af..081d9840 100644 --- a/src/stages/review.ts +++ b/src/stages/review.ts @@ -1,22 +1,31 @@ -import { block, clearMarker, writeArtifact, type StepContext, type StepResult } from "../context"; +import { block, clearMarker, writeArtifact, type Failure, type StepContext, type StepResult } from "../context"; import { Git } from "../git"; import { modelFor } from "../config"; import { passes, runJudge } from "../judges"; -import { phasePrompt } from "../phases"; import { testLoop } from "./build"; -import { reviewPhase, takeEarlyReview } from "./early-review"; +import { catchUp, outOfDate, RESOLUTION, roundsAllowed } from "./catch-up"; +import { reviewPhase, reviewPrompt, takeEarlyReview } from "./early-review"; import { artifacts, bullets, buildSession, KEEP_UNFINISHED, openBranchWorktree, readRound, REVIEW_ROUND, runHookCommands, saveWork, writeRound } from "./shared"; /** The review rounds ended with a review that had no important findings (and the after commands ran). */ export const REVIEW_PASSED = "review-passed"; +/** + * Brings the branch up to date with main (see catchUp), and runs the test loop on a resolution, so + * whoever reviews it next reads tested code. A failure to block with. + */ +export async function catchUpTested(ctx: StepContext): Promise<{ ok: true } | Failure> { + const up = await catchUp(ctx); + if (!up.ok) return up; + return up.outcome === "resolved" ? testLoop(ctx, "resolve-test") : { ok: true }; +} + /** * Stage 5 review rounds, run by the merge step for reviewing (see runMergeStep): review, and on * important findings revise and test again, until a review passes or the rounds run out (block). * The round in progress is kept in the run folder, so a restart continues it. */ export async function runReviewRounds(ctx: StepContext): Promise<StepResult | typeof REVIEW_PASSED> { - const stage = ctx.cfg.stages.review; const wt = new Git(ctx.worktreeDir); // The change must be on its branch, in a real worktree, before anyone reviews it. @@ -34,6 +43,13 @@ export async function runReviewRounds(ctx: StepContext): Promise<StepResult | ty const resumed = readRound(ctx); if (resumed === null) { + // Main gained code since the build: the review reads the change on top of it. A conflict is + // resolved, and tested before anyone reviews it. Records alone leave the branch (and the early + // review kept for its commit) as they are; the merge checks bring those in. + if (await outOfDate(ctx.git, ctx.cfg.main_branch, ctx.branch)) { + const up = await catchUpTested(ctx); + if (!up.ok) return block(ctx, up.note, up); + } const before = await runHookCommands(ctx, "before", "review", ctx.worktreeDir); if (!before.ok) return before; } @@ -42,7 +58,7 @@ export async function runReviewRounds(ctx: StepContext): Promise<StepResult | ty for (let round = resumed ?? 1; ; round++) { writeRound(ctx, round); const phase = await reviewPhase(ctx, round, a.spec, a.currentPlan); - const prompt = await phasePrompt(ctx, phase); + const prompt = await reviewPrompt(ctx, phase); const review = (await takeEarlyReview(ctx, phase, prompt)) ?? await runJudge(ctx, phase, { prompt }); if (!review.ok) return block(ctx, review.note); // review.md, the gate row and the findings all come from the review that decided. @@ -57,11 +73,13 @@ export async function runReviewRounds(ctx: StepContext): Promise<StepResult | ty ctx.trace.gate(ctx.slug, "review", "findings", pass ? "pass" : "fail", summary); if (pass) { + // Seen: a later review is not shown it again. + clearMarker(ctx, RESOLUTION); const after = await runHookCommands(ctx, "after", "review", ctx.worktreeDir); return after.ok ? REVIEW_PASSED : after; } - if (round > stage.max_rounds) { + if (round > roundsAllowed(ctx)) { clearMarker(ctx, REVIEW_ROUND); return block(ctx, "The reviewer still found important problems after the change was revised. The details are in review.md. An engineer needs to look at the change.", { detail: { rounds: round, important } }); } diff --git a/src/stages/shared.ts b/src/stages/shared.ts index f8535a17..e2574dd6 100644 --- a/src/stages/shared.ts +++ b/src/stages/shared.ts @@ -2,8 +2,9 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; import { block, clearMarker, clearSession, loadSessions, readArtifact, readMarker, saveSession, writeMarker, type Failure, type StepContext, type StepResult } from "../context"; import { Envelopes, type Envelope, type PhaseName } from "../envelopes"; -import { Git, passOn } from "../git"; -import { onCheckout, slotEnv } from "../lanes"; +import { Git, NO_AUTO_MAINTENANCE, passOn } from "../git"; +import { onCheckout } from "../lanes"; +import { changeEnv } from "../resources"; import { agentPhase, codePhase, projectCommand, type AgentPhaseResult, type AgentPhaseSpec } from "../phases"; import { WATCHING_PR, type Intent, type Status } from "../intents"; import { runDir } from "../paths"; @@ -21,7 +22,7 @@ export async function runHookCommands(ctx: StepContext, which: "before" | "after let timedOut = false; const runAll = async () => { for (const cmd of cmds) { - const res = await projectCommand(ctx, cmd, cwd, { env: { LOOPSTRA_SLUG: ctx.slug, LOOPSTRA_STAGE: stage, ...slotEnv(ctx.slug) } }); + const res = await projectCommand(ctx, cmd, cwd, { env: { LOOPSTRA_SLUG: ctx.slug, LOOPSTRA_STAGE: stage, ...(await changeEnv(ctx)) } }); if (res.code !== 0) { timedOut = res.timedOut; throw new Error(`\`${cmd}\` failed: ${res.lastLine || `exit ${res.code}`}`); } } return { ok: true as const }; @@ -241,8 +242,34 @@ export function writeRound(ctx: StepContext, round: number): void { } /** - * Makes sure the intent branch exists and has its own worktree checked out on it. Returns a - * failure for the caller to record; it never blocks by itself. + * Run-folder marker: the branch's head before a conflict resolution squashed it onto main (see + * catchUp), until the resolution is saved. While it is here, the branch is mid-resolution, and + * restoreCatchUp puts it back. + */ +export const CATCHING_UP = "catching-up"; + +/** + * Puts the branch back where it was before a conflict resolution that did not finish (CATCHING_UP), + * with the files the resolution left removed, and clears the marker. The worktree was clean when + * the resolution started, so nothing of anyone's is lost. Runs even after a stop request. + */ +export async function restoreCatchUp(ctx: StepContext): Promise<void> { + const old = readMarker(ctx, CATCHING_UP)?.trim(); + if (old === undefined) return; + if (old) { + const wt = new Git(ctx.worktreeDir); + await wt.run(["merge", "--abort"], { allowFail: true, cleanup: true }); + await wt.run([...NO_AUTO_MAINTENANCE, "reset", "-q", "--hard", old], { cleanup: true }); + await wt.run(["clean", "-f", "-d", "-q"], { allowFail: true, cleanup: true }); + ctx.trace.event(ctx.slug, "command", { command: "catch up", note: "a conflict resolution that did not finish was undone; the branch is back where it was", head: old }); + } + clearMarker(ctx, CATCHING_UP); +} + +/** + * Makes sure the intent branch exists and has its own worktree checked out on it, and puts back a + * conflict resolution that did not finish (see restoreCatchUp). Returns a failure for the caller + * to record; it never blocks by itself. */ export async function openBranchWorktree(ctx: StepContext): Promise<{ ok: true } | Failure> { if (!(await ctx.git.branchExists(ctx.branch))) { @@ -251,6 +278,7 @@ export async function openBranchWorktree(ctx: StepContext): Promise<{ ok: true } try { await ctx.git.ensureWorktree(ctx.worktreeDir, ctx.branch); await new Git(ctx.worktreeDir).assertBranch(ctx.branch); + await restoreCatchUp(ctx); } catch (e) { passOn(e); return { ok: false, note: "The workspace for this change could not be prepared. An engineer needs to look at it.", detail: errorText(e) }; diff --git a/templates/config.yaml b/templates/config.yaml index 49055824..ddf927de 100644 --- a/templates/config.yaml +++ b/templates/config.yaml @@ -35,6 +35,9 @@ gates: # Per-stage model, skills to load, and deterministic commands to run before and after. # A step of a stage can have its own model too, for example under a stage: # steps: { done-check: max } # a step's own model; see the step table in the README +# review can also take max_resolutions (3): how many times a build may have conflicts with main +# resolved (each is tested and reviewed in a round of its own, on top of max_rounds); past it, a +# conflict blocks. stages: design: { model: strong, skills: [], before: [], after: [] } plan: { model: strong, skills: [], before: [], after: [] } @@ -86,8 +89,12 @@ stages: # only if the project's commands (test, lint, build, install) can run twice at once on this # machine: no fixed ports, no shared test database, no output outside the worktree. Each command # gets LOOPSTRA_SLUG and LOOPSTRA_SLOT (0, 1, ...: no two changes running at once share one), so it -# can pick its own ports, database and Docker project names. More changes at once also +# can pick its own database and Docker project names. More changes at once also # spend the daily budget and the subscription's usage limits faster. +# Each running change also gets its own TMPDIR (in the system's temp folder), and ports_per_change ports +# no other running change has, checked free: LOOPSTRA_PORT_BASE up to LOOPSTRA_PORT_BASE + +# LOOPSTRA_PORT_COUNT - 1 (blocks start at ports_from; 0 ports: none). own_caches gives each slot +# its own tool caches (XDG_CACHE_HOME, npm, bun, yarn, pip, go), kept warm between changes. speed: parallel_checks: false overlap_judges: true @@ -95,6 +102,9 @@ speed: cheap_judges: off think_ahead: true max_parallel_changes: 1 + ports_per_change: 10 + ports_from: 20000 + own_caches: false signals: main_health: { every_minutes: 30 } diff --git a/templates/prompts/resolve.md b/templates/prompts/resolve.md new file mode 100644 index 00000000..84b7063b --- /dev/null +++ b/templates/prompts/resolve.md @@ -0,0 +1,21 @@ +This change's branch no longer applies cleanly to `{{main_branch}}`: other work merged into `{{main_branch}}` since it was built changed the same lines. The runtime has applied the whole change on top of `{{main_branch}}` in this workspace, and git left the conflicts in the files below. Your job is to resolve them. + +This change's spec: + +{{spec}} + +This change's plan: + +{{plan}} + +The conflicts, with the commits on `{{main_branch}}` that touched these files: + +{{conflicts}} + +Each conflict shows three parts: between `<<<<<<<` and `|||||||` is `{{main_branch}}`'s version, between `|||||||` and `=======` is the version both started from, and between `=======` and `>>>>>>>` is this change's version. Read each conflicted file in full, and the other changes' specs where they are named, before you edit. + +Keep what both sides meant: `{{main_branch}}`'s work must keep working, and this change must still do what its spec says. Edit only what resolving the conflicts needs, add no new files (new untracked files are removed before your work is saved), and leave no conflict marker lines in any file. A file deleted on one side and changed on the other: keep it or delete it, whichever keeps both changes working. You may run the test command (`{{test_command}}`) to check your work. The shell commands you may run, with any arguments: {{commands}}. Never run git commands that change the history or the index (commit, add, rebase, reset, checkout, merge, stash): the runtime saves your work. + +If you cannot resolve a conflict safely (the two changes want opposite things, or you would have to guess what a person meant), stop, leave the files as they are, set `resolved` to false, and say why in `reason`. Otherwise set `resolved` to true and `reason` to an empty string. + +Write `summary_markdown` for the reviewer who reads the change next: one short paragraph per file, saying what each side wanted and what you kept. diff --git a/templates/prompts/review.md b/templates/prompts/review.md index efb0064b..bfedccca 100644 --- a/templates/prompts/review.md +++ b/templates/prompts/review.md @@ -12,4 +12,8 @@ Plan: {{plan}} +If this change was brought up to date with `{{main_branch}}` and an agent resolved conflicts on the way, this is the record: the conflicts as they were (both sides), and what the agent kept. Check that the resolution keeps what both sides meant; a resolution that drops either side's behavior is an important finding. + +{{resolution}} + Report findings with `severity` `important` only for things that break behavior, leak data, breach policy, or contradict the spec or plan. Everything else is a `nit`; report at most five nits. For a finding about a whole file, use `line` 0. Set `approved` to true when there are no important findings. Write `review_markdown` as a short document with a `## Summary` heading and a `## Findings` heading listing each finding with its severity and location. diff --git a/tests/fake-claude/claude.ts b/tests/fake-claude/claude.ts index f58ede84..33713377 100644 --- a/tests/fake-claude/claude.ts +++ b/tests/fake-claude/claude.ts @@ -130,6 +130,8 @@ if (process.env.LOOPSTRA_FAKE_ARGS) { CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: process.env.CLAUDE_CODE_DISABLE_BACKGROUND_TASKS ?? null, LOOPSTRA_BASE: process.env.LOOPSTRA_BASE ?? null, LOOPSTRA_SLOT: process.env.LOOPSTRA_SLOT ?? null, + LOOPSTRA_PORT_BASE: process.env.LOOPSTRA_PORT_BASE ?? null, + TMPDIR: process.env.TMPDIR ?? null, LOOPSTRA_PROTECTED_LOG: process.env.LOOPSTRA_PROTECTED_LOG ?? null, GIT_OPTIONAL_LOCKS: process.env.GIT_OPTIONAL_LOCKS ?? null, CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD: process.env.CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD ?? null, diff --git a/tests/integration/lanes-helpers.ts b/tests/integration/lanes-helpers.ts index 2ae9dbd9..2b931b49 100644 --- a/tests/integration/lanes-helpers.ts +++ b/tests/integration/lanes-helpers.ts @@ -53,6 +53,21 @@ export async function writeChange(path: string, fixtures: string, c: Change): Pr writeFileSync(join(fixtures, c.slug, "plan.jsonl"), planFixture(c)); } +/** + * The change's resolve session, as a fake fixture: it writes `writes` (path → content) in the + * worktree, and says whether it `resolved` the conflicts. + */ +export function resolveFixture(fixtures: string, slug: string, writes: Record<string, string>, resolved = true): void { + const lines: unknown[] = [{ type: "system", subtype: "init", session_id: `fake-resolve-${slug}`, cwd: "/", tools: [], model: "fake" }]; + for (const [path, content] of Object.entries(writes)) lines.push({ type: "fake_action", write: { path, content } }); + lines.push({ + type: "result", subtype: "success", is_error: false, session_id: `fake-resolve-${slug}`, total_cost_usd: 0.1, usage: {}, result: "ok", + structured_output: { status: "success", summary: "done", resolved, reason: resolved ? "" : "the two changes want different things", summary_markdown: "Kept both." }, + }); + mkdirSync(join(fixtures, slug), { recursive: true }); + writeFileSync(join(fixtures, slug, "resolve.jsonl"), lines.map((l) => JSON.stringify(l)).join("\n") + "\n"); +} + /** The change's plan session, as a fake fixture: it writes planFor(c), so changes planned in a test list their own files. */ function planFixture(c: Change): string { return fixture("plan").split("\n").filter((l) => l.trim()).map((l) => { diff --git a/tests/integration/lanes.test.ts b/tests/integration/lanes.test.ts index fb913d05..92bf934e 100644 --- a/tests/integration/lanes.test.ts +++ b/tests/integration/lanes.test.ts @@ -10,7 +10,7 @@ import { PROBE_AFTER, tick } from "../../src/scheduler"; import { requestStop, resetStop } from "../../src/stop"; import { Trace } from "../../src/trace"; import { fixture, TEMPLATES, tempDir, withEnv } from "../helpers"; -import { lanesRepo, statusOf, inTrace, expirePause, payloadOf, numbered, phaseEndId, mergeOrder, holdSession, traceShows, statusShows } from "./lanes-helpers"; +import { lanesRepo, resolveFixture, statusOf, inTrace, expirePause, payloadOf, numbered, phaseEndId, mergeOrder, holdSession, traceShows, statusShows } from "./lanes-helpers"; afterEach(() => resetStop()); @@ -70,11 +70,11 @@ describe("more than one change at a time (speed.max_parallel_changes)", () => { r.cleanup(); }, 120_000); - test("two builds running at once get different slots, so their commands can pick their own ports", async () => { + test("two builds running at once get different slots, ports and temp folders", async () => { const out = tempDir("loopstra-slots-"); const script = join(out.path, "slot.ts").replaceAll("\\", "/"); const log = join(out.path, "slots.log").replaceAll("\\", "/"); - writeFileSync(script, `import { appendFileSync } from "node:fs";\nappendFileSync(process.argv[2]!, process.env.LOOPSTRA_SLUG + " " + process.env.LOOPSTRA_SLOT + "\\n");\n`); + writeFileSync(script, `import { appendFileSync } from "node:fs";\nappendFileSync(process.argv[2]!, [process.env.LOOPSTRA_SLUG, process.env.LOOPSTRA_SLOT, process.env.LOOPSTRA_PORT_BASE, process.env.TMPDIR].join("\\t") + "\\n");\n`); const r = await lanesRepo(2, [{ slug: "alpha", status: "plan-approved" }, { slug: "beta", status: "plan-approved" }], `stages:\n build:\n before:\n - 'bun "${script}" "${log}"'\n`); // alpha's step holds at its verifier until beta's build has started: the two run at once. @@ -87,8 +87,12 @@ describe("more than one change at a time (speed.max_parallel_changes)", () => { expect(await statusOf(r.path, "alpha")).toBe("reviewing"); expect(await statusOf(r.path, "beta")).toBe("reviewing"); }); - const slots = (await Bun.file(log).text()).trim().split("\n").map((l) => l.split(" ")[1]).sort(); - expect(slots).toEqual(["0", "1"]); + const lines = (await Bun.file(log).text()).trim().split("\n").map((l) => l.split("\t")); + expect(lines.map((l) => l[1]).sort()).toEqual(["0", "1"]); + // Ten ports each, no overlap, and each its own temp folder. + const ports = lines.map((l) => Number(l[2])).sort((a, b) => a - b); + expect(ports[1]! - ports[0]!).toBeGreaterThanOrEqual(10); + expect(new Set(lines.map((l) => l[3])).size).toBe(2); r.cleanup(); out.cleanup(); }, 120_000); @@ -322,12 +326,38 @@ describe("more than one change at a time (speed.max_parallel_changes)", () => { r.cleanup(); }, 180_000); - test("two changes that clash: the first merges, the second is blocked as out of date, and main keeps only the first", async () => { + test("two changes that clash: the first merges, and the second's conflict is resolved, tested and reviewed before it merges too", async () => { + // As below, beta's plan lists no files: the same-file wait would otherwise keep the two apart. + const r = await lanesRepo(2, [ + { slug: "alpha", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "alpha" }) } }, + { slug: "beta", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "beta" }) }, planFiles: null }, + ], "", { "src/shared.ts": numbered() }); + // Whichever merges second resolves line 1 to keep both. + for (const slug of ["alpha", "beta"]) resolveFixture(r.fixtures, slug, { "src/shared.ts": numbered({ 1: "alpha and beta" }) }); + await withEnv(r.env, async () => { + for (let i = 0; i < 4 && ((await statusOf(r.path, "alpha")) !== "merged" || (await statusOf(r.path, "beta")) !== "merged"); i++) await tick(r.path); + expect(await statusOf(r.path, "alpha")).toBe("merged"); + expect(await statusOf(r.path, "beta")).toBe("merged"); + expect((await new Git(r.path).run(["show", "main:src/shared.ts"])).out).toBe(numbered({ 1: "alpha and beta" })); + inTrace(r.path, (trace) => { + const [, second] = mergeOrder(trace, ["alpha", "beta"]) as [string, string]; + const names = trace.phases(second).map((p) => p.name); + // Resolved, then tested, then reviewed again, all before it merged. + expect(names.indexOf("resolve")).toBeGreaterThan(-1); + expect(names.indexOf("merge-test-1")).toBeGreaterThan(names.indexOf("resolve")); + expect(names.lastIndexOf("review-2")).toBeGreaterThan(names.indexOf("merge-test-1")); + }); + }); + r.cleanup(); + }, 180_000); + + test("two changes that clash and cannot be resolved: the first merges, the second is blocked as out of date, and main keeps only the first", async () => { // As above, beta's plan lists no files: the same-file wait would otherwise keep the two apart. const r = await lanesRepo(2, [ { slug: "alpha", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "alpha" }) } }, { slug: "beta", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "beta" }) }, planFiles: null }, ], "", { "src/shared.ts": numbered() }); + for (const slug of ["alpha", "beta"]) resolveFixture(r.fixtures, slug, {}, false); await withEnv(r.env, async () => { await tick(r.path); await tick(r.path); @@ -338,11 +368,39 @@ describe("more than one change at a time (speed.max_parallel_changes)", () => { expect((await readIntent(r.path, second)).file.frontmatter.note).toMatch(/^The change overlaps with other recent changes/); const git = new Git(r.path); expect((await git.run(["show", "main:src/shared.ts"])).out).toBe(numbered({ 1: first })); + expect((await git.run(["show", `main:intent/${second}/conflict.md`])).out).toContain("the two changes want different things"); expect((await git.run(["status", "--porcelain"])).out.trim()).toMatch(/^(M intent\/queue\.md)?$/); }); r.cleanup(); }, 180_000); + test("without a remote, a change waiting for a person that another change's merge conflicts with is brought up to date and reviewed again", async () => { + const r = await lanesRepo(2, [ + { slug: "alpha", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "alpha" }) } }, + { slug: "beta", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "beta" }) }, planFiles: null }, + ], "gates:\n merge:\n human: status\n", { "src/shared.ts": numbered() }); + for (const slug of ["alpha", "beta"]) resolveFixture(r.fixtures, slug, { "src/shared.ts": numbered({ 1: "alpha and beta" }) }); + await withEnv(r.env, async () => { + await tick(r.path); // both build + await tick(r.path); // both are reviewed and wait for a person + expect(await statusOf(r.path, "alpha")).toBe("merge-review"); + expect(await statusOf(r.path, "beta")).toBe("merge-review"); + // A person lets alpha in. + const alpha = await readIntent(r.path, "alpha"); + await Bun.write(join(alpha.dir, "intent.md"), (await Bun.file(join(alpha.dir, "intent.md")).text()).replace("status: merge-review", "status: merge-approved")); + await new Git(r.path).commitAll("person: merge-approved"); + await tick(r.path); // alpha merges + expect(await statusOf(r.path, "alpha")).toBe("merged"); + await tick(r.path); // beta no longer merges cleanly: resolved, tested, back to review + expect(await statusOf(r.path, "beta")).toBe("reviewing"); + expect((await readIntent(r.path, "beta")).file.frontmatter.note).toMatch(/^Other work merged into main meanwhile conflicted/); + await tick(r.path); // reviewed again, and back with the person + expect(await statusOf(r.path, "beta")).toBe("merge-review"); + expect((await new Git(r.path).run(["show", "intent/beta:src/shared.ts"])).out).toBe(numbered({ 1: "alpha and beta" })); + }); + r.cleanup(); + }, 180_000); + test("two plan-approved changes that change the same file: only the first builds, the second waits with a note and builds once it merged", async () => { const r = await lanesRepo(2, [ { slug: "alpha", status: "plan-approved", writes: { "src/shared.ts": numbered({ 1: "alpha" }) } }, diff --git a/tests/integration/remote.test.ts b/tests/integration/remote.test.ts index 1e5e61dd..f9fe5382 100644 --- a/tests/integration/remote.test.ts +++ b/tests/integration/remote.test.ts @@ -9,7 +9,7 @@ import { runStepGuarded, tick } from "../../src/scheduler"; import { renderStatus } from "../../src/commands/status"; import { runDir } from "../../src/paths"; import { SYNC_TEXT } from "../../src/remote"; -import { GITHUB_MERGING, MERGED_NOT_HERE, READY_BEHIND, RECORDING_FAILED } from "../../src/stages/merge"; +import { GITHUB_MERGING, MERGED_NOT_HERE, READY_BEHIND, RECORDING_FAILED, STALE_APPROVAL_TEXT } from "../../src/stages/merge"; import { PR_CHECKS_NOTE, PR_WAIT } from "../../src/stages/shared"; import { resetStop } from "../../src/stop"; import { Trace } from "../../src/trace"; @@ -19,6 +19,65 @@ import { SLUG, BRANCH, PLAN, remoteSetup, intentOf, elsewhere, headOn, onRemoteM afterEach(() => resetStop()); describe("the loop with a GitHub remote", () => { + test("a pull request waiting for approval that main conflicts with is resolved, reviewed and pushed again, and only a newer approval merges it", async () => { + const s = await remoteSetup("gates:\n merge:\n human: pr\n"); + const fixtures = tempDir("loopstra-fixtures-"); + const resolved = "// added on main by a person\nexport const ZERO = 0;\nexport function add(a: number, b: number): number {\n return a + b;\n}\n"; + writeFileSync(join(fixtures.path, "resolve.jsonl"), [ + { type: "system", subtype: "init", session_id: "fake-resolve", cwd: "/", tools: [], model: "fake" }, + { type: "fake_action", write: { path: "src/add.ts", content: resolved } }, + { type: "result", subtype: "success", is_error: false, session_id: "fake-resolve", total_cost_usd: 0.1, usage: {}, result: "ok", structured_output: { status: "success", summary: "done", resolved: true, reason: "", summary_markdown: "Kept both." } }, + ].map((l) => JSON.stringify(l)).join("\n") + "\n"); + try { + await withEnv({ ...s.env, LOOPSTRA_FAKE_FIXTURE_DIR: fixtures.path }, async () => { + await tick(s.repo); // build + await tick(s.repo); // review, merge checks, pull request + expect((await intentOf(s.repo)).status).toBe("merge-review"); + const firstHead = await headOn(s.remote); + // A person approved it already, and someone else adds src/add.ts on GitHub's main meanwhile. + const approvedBefore = { author: { login: "ann" }, authorAssociation: "MEMBER", state: "APPROVED", submittedAt: new Date(Date.now() - 60_000).toISOString() }; + await s.setPr({ checks: "pass" }); + // A person adds a commit to the pull request too: the resolution starts from it, and it keeps their credit. + const person = await elsewhere(s.remote); + await run(["git", "checkout", "-q", "-b", BRANCH, `origin/${BRANCH}`], person.path); + await Bun.write(join(person.path, "NOTES.md"), "from a person\n"); + await run(["git", "add", "-A"], person.path); + await run(["git", "commit", "-q", "-m", "docs: a note"], person.path); + await run(["git", "push", "-q", "origin", BRANCH], person.path); + person.cleanup(); + const other = await elsewhere(s.remote); + await Bun.write(join(other.path, "src", "add.ts"), "// added on main by a person\nexport const ZERO = 0;\n"); + await run(["git", "add", "-A"], other.path); + await run(["git", "commit", "-q", "-m", "a person adds src/add.ts"], other.path); + await run(["git", "push", "-q", "origin", "main"], other.path); + other.cleanup(); + await tick(s.repo); // waiting for approval: it no longer merges cleanly, so it is resolved and tested + expect((await intentOf(s.repo)).status).toBe("reviewing"); + await tick(s.repo); // reviewed again, pushed to the same pull request + expect((await intentOf(s.repo)).status).toBe("merge-review"); + expect(await headOn(s.remote)).not.toBe(firstHead); + expect((await run(["git", "show", `${BRANCH}:src/add.ts`], s.remote)).out).toBe(resolved); + expect((await run(["git", "show", `${BRANCH}:NOTES.md`], s.remote)).out).toBe("from a person\n"); + expect((await run(["git", "log", "--format=%B", `main..${BRANCH}`], s.remote)).out).toContain("Co-authored-by: Person <person@example.com>"); + // The approval from before the resolution does not cover it. + const raw = await Bun.file(s.env.LOOPSTRA_FAKE_GH_STATE).json(); + Object.assign(raw.prs[BRANCH], { reviewDecision: "APPROVED", latestReviews: [approvedBefore], checks: "pass" }); + await Bun.write(s.env.LOOPSTRA_FAKE_GH_STATE, JSON.stringify(raw)); + await tick(s.repo); + expect((await intentOf(s.repo)).status).toBe("merge-review"); + expect(readFileSync(join(runDir(s.repo, SLUG), PR_WAIT), "utf8")).toBe(STALE_APPROVAL_TEXT); + // A newer one does. + await s.setPr({ latestReviews: [{ ...approvedBefore, submittedAt: new Date(Date.now() + 1000).toISOString() }] } as never); + await tick(s.repo); + expect((await intentOf(s.repo)).status).toBe("merged"); + expect(await onRemoteMain(s.remote, "src/add.ts")).toBe(true); + expect((await run(["git", "show", "main:src/add.ts"], s.remote)).out).toBe(resolved); + }); + } finally { + fixtures.cleanup(); s.cleanup(); + } + }, 120_000); + test("merge approved on GitHub: opens a pull request, waits for checks and approval, merges through gh, and syncs main", async () => { const s = await remoteSetup("gates:\n merge:\n human: pr\n"); await withEnv(s.env, async () => { diff --git a/tests/unit/config.test.ts b/tests/unit/config.test.ts index a1ff2f3c..bb1b731f 100644 --- a/tests/unit/config.test.ts +++ b/tests/unit/config.test.ts @@ -29,7 +29,23 @@ describe("loadConfig", () => { expect(cfg.signals.main_health.every_minutes).toBe(30); // Judges overlap by default; project commands share folders in some projects, so they do not. // One change at a time unless the owner says the project's commands can run side by side. - expect(cfg.speed).toEqual({ parallel_checks: false, overlap_judges: true, plan_ahead: true, cheap_judges: "off", think_ahead: true, max_parallel_changes: 1 }); + // Each running change gets ten ports checked free; caches are shared unless the project says otherwise. + expect(cfg.speed).toEqual({ + parallel_checks: false, overlap_judges: true, plan_ahead: true, cheap_judges: "off", think_ahead: true, max_parallel_changes: 1, + ports_per_change: 10, ports_from: 20000, own_caches: false, + }); + t.cleanup(); + }); + + test("speed.ports_per_change takes 0 to 100 and ports_from 1024 to 65535, in plain words otherwise", async () => { + const t = tempDir(); + await writeConfig(t.path, "version: 1\ncommands:\n test: x\nspeed:\n ports_per_change: 0\n ports_from: 30000\n own_caches: true\n"); + const speed = (await loadConfig(t.path)).speed; + expect([speed.ports_per_change, speed.ports_from, speed.own_caches]).toEqual([0, 30000, true]); + for (const [line, error] of [["ports_per_change: -1", "must be 0 or more"], ["ports_per_change: 101", "must be 100 or less"], ["ports_from: 80", "must be 1024 or more"], ["ports_per_change: 2.5", "must be a whole number"]]) { + await writeConfig(t.path, `version: 1\ncommands:\n test: x\nspeed:\n ${line}\n`); + await expect(loadConfig(t.path)).rejects.toThrow(error); + } t.cleanup(); }); diff --git a/tests/unit/conflicts.test.ts b/tests/unit/conflicts.test.ts new file mode 100644 index 00000000..d09d658a --- /dev/null +++ b/tests/unit/conflicts.test.ts @@ -0,0 +1,378 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { existsSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { FAKE_CLAUDE_ENV } from "../../src/claude"; +import { readMarker, writeMarker, type StepContext } from "../../src/context"; +import { Git } from "../../src/git"; +import { readIntent } from "../../src/intents"; +import { runBuildStep } from "../../src/stages/build"; +import { catchUp, CONFLICT_NOTE, outOfDate, RESOLUTION, RESOLUTIONS, roundsAllowed } from "../../src/stages/catch-up"; +import { EARLY_REVIEW } from "../../src/stages/early-review"; +import { CAUGHT_UP_NOTE, conflictsWhileWaiting, runMergeStep } from "../../src/stages/merge"; +import { CATCHING_UP, openBranchWorktree } from "../../src/stages/shared"; +import type { Trace } from "../../src/trace"; +import { copyRepo, FAKE_CLAUDE, fixtureDir, openRepo, setEnv, setupRepo, withEnv, writeIntentOnDisk } from "../helpers"; + +const PLAN = "# Plan: add\n\n## Files that change\n- src/add.ts (new)\n- tests/add.test.ts (new)\n\n## Order of work\n1. x\n\n## Risks\nNone.\n\n## Proof\nbun test.\n"; +/** A person on the merge gate, and one review round: a resolution's review must not need a second one. */ +const PERSON_ONE_ROUND = "gates:\n merge:\n human: status\nstages:\n review:\n max_rounds: 1\n"; + +/** What the build writes to src/add.ts (tests/fake-claude/fixtures/build.jsonl). */ +const BUILT = "export function add(a: number, b: number): number {\n return a + b;\n}\n"; +/** What main gets meanwhile: another change that also added src/add.ts. */ +const MAINS = "// added on main by another change\nexport const ZERO = 0;\n"; +/** Both kept. */ +const RESOLVED = `${MAINS}${BUILT}`; + +/** A resolve session's fixture: it writes `content` to src/add.ts (unless null) and answers `resolved`. */ +function resolveFixture(content: string | null, resolved: boolean, reason = ""): string { + const lines: unknown[] = [{ type: "system", subtype: "init", session_id: "fake-resolve", cwd: "/", tools: [], model: "fake" }]; + if (content !== null) lines.push({ type: "fake_action", write: { path: "src/add.ts", content } }); + lines.push({ + type: "result", subtype: "success", is_error: false, session_id: "fake-resolve", total_cost_usd: 0.1, usage: {}, result: "ok", + structured_output: { status: "success", summary: "resolved", resolved, reason, summary_markdown: "Kept main's ZERO and this change's add." }, + }); + return lines.map((l) => JSON.stringify(l)).join("\n") + "\n"; +} + +const templates = new Map<string, { path: string; cleanup: () => void }>(); +let restoreEnv = () => {}; + +beforeAll(async () => { + restoreEnv = setEnv({ [FAKE_CLAUDE_ENV]: FAKE_CLAUDE }); + // Built (status reviewing), and the same after its first review (merge-review, a person on the gate). + const s = await build(PERSON_ONE_ROUND); + s.trace.close(); + templates.set("built", s.repo); + const r = await copy("built"); + await runMergeStep(r.ctx); + r.trace.close(); + templates.set("reviewed", r.repo); +}, 120_000); + +afterAll(() => { + for (const t of templates.values()) t.cleanup(); + restoreEnv(); +}); + +async function build(config: string) { + const s = await setupRepo("plan-approved", { commands: { test: "bun test" }, config }); + await Bun.write(join(s.repo.path, "intent", "add-numbers", "spec.md"), "# Spec\n\n## Summary\ns\n"); + await Bun.write(join(s.repo.path, "intent", "add-numbers", "plan.md"), PLAN); + await Bun.write(join(s.repo.path, "package.json"), JSON.stringify({ name: "target", type: "module" })); + await new Git(s.repo.path).commitAll("artifacts"); + await s.ctx.reload(); + await runBuildStep(s.ctx); + await s.ctx.reload(); + return s; +} + +async function copy(state: "built" | "reviewed") { + const repo = await copyRepo(templates.get(state)!.path); + return { repo, ...(await openRepo(repo.path)) }; +} + +/** Another change lands on main: `src/add.ts` with other content (a conflict), or another file (none). */ +async function mainMoves(repo: string, conflicting = true): Promise<void> { + await Bun.write(join(repo, "src", conflicting ? "add.ts" : "other.ts"), MAINS); + await new Git(repo).commitAll("other-change: something else on main"); +} + +/** Runs the change's steps with these resolve fixtures; the prompts each session got are returned. */ +async function withResolve<T>(fixtures: Record<string, string>, fn: () => Promise<T>): Promise<{ result: T; prompts: Array<{ phase: string; prompt: string }> }> { + const dir = fixtureDir(fixtures); + const promptsFile = join(dir.path, "prompts.jsonl"); + try { + const result = await withEnv({ LOOPSTRA_FAKE_FIXTURE_DIR: dir.path, LOOPSTRA_FAKE_PROMPTS: promptsFile }, fn); + const prompts = existsSync(promptsFile) ? readFileSync(promptsFile, "utf8").split("\n").filter(Boolean).map((l) => JSON.parse(l) as { phase: string; prompt: string }) : []; + return { result, prompts }; + } finally { + dir.cleanup(); + } +} + +async function status(repo: string): Promise<{ status: string; note: string }> { + const fm = (await readIntent(repo, "add-numbers")).file.frontmatter; + return { status: fm.status, note: fm.note }; +} + +async function branchFile(repo: string, path: string): Promise<string> { + return (await new Git(repo).run(["show", `intent/add-numbers:${path}`])).out; +} + +function phaseNames(trace: Trace): string[] { + return trace.phases("add-numbers").map((p) => p.name); +} + +async function personSets(ctx: StepContext, to: "merge-approved"): Promise<void> { + await ctx.reload(); + await writeIntentOnDisk(ctx.intent, { status: to, note: "" }); + await ctx.git.commitPaths([`intent/${ctx.slug}`], `person: ${to}`); + await ctx.reload(); +} + +describe("the merge checks resolve a conflict with main", () => { + test("resolved, tested, then reviewed once more with the resolution in the prompt, without using up the review rounds", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + await mainMoves(repo.path); + await personSets(ctx, "merge-approved"); + const { prompts } = await withResolve({ resolve: resolveFixture(RESOLVED, true) }, async () => { + await runMergeStep(ctx); // the merge checks resolve it and send it back to review + expect((await status(repo.path)).status).toBe("reviewing"); + await ctx.reload(); + await runMergeStep(ctx); // the review round that sees the resolution, then the gate again + }); + // Back with the person: what they approve now is what will merge. + expect(await status(repo.path)).toMatchObject({ status: "merge-review" }); + expect(await branchFile(repo.path, "src/add.ts")).toBe(RESOLVED); + // Main moved on since only by records (the status changes); its code is all on the branch. + expect(await outOfDate(new Git(repo.path), "main", "intent/add-numbers")).toBe(false); + const names = phaseNames(trace); + expect(names).toContain("resolve"); + expect(names.indexOf("merge-test-1")).toBeGreaterThan(names.indexOf("resolve")); + // max_rounds is 1, and round 2 ran: the resolution's review is on top of them. + expect(names).toContain("review-2"); + const resolve = prompts.find((p) => p.phase === "resolve")!; + expect(resolve.prompt).toContain("### src/add.ts"); + expect(resolve.prompt).toMatch(/<<<<<<< /); + expect(resolve.prompt).toContain("other-change: something else on main"); + const review = prompts.filter((p) => p.phase === "review").at(-1)!; + expect(review.prompt).toContain("conflicts in src/add.ts were resolved by an agent"); + expect(review.prompt).toContain("Kept main's ZERO and this change's add."); + // Seen by a review that passed: not shown again. + expect(readMarker(ctx, RESOLUTION)).toBeNull(); + expect(trace.gates("add-numbers").some((g) => g.check === "up-to-date" && g.evidence.includes("resolved conflicts with main in src/add.ts"))).toBe(true); + // The branch's commits were squashed into one on top of main. + const log = (await new Git(repo.path).run(["log", "--format=%s", "main..intent/add-numbers"])).out.trim().split("\n"); + expect(log).toEqual(["loopstra(add-numbers): brought up to date with main"]); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("a session that cannot resolve it blocks with conflict.md, and leaves the branch and main as they were", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + const before = (await new Git(repo.path).run(["rev-parse", "intent/add-numbers"])).out.trim(); + await mainMoves(repo.path); + await personSets(ctx, "merge-approved"); + await withResolve({ resolve: resolveFixture(null, false, "the two changes want different things from add.ts") }, () => runMergeStep(ctx)); + const s = await status(repo.path); + expect(s.status).toBe("blocked"); + expect(s.note).toContain("conflict.md"); + expect(s.note).not.toMatch(/<<<<|CONFLICT \(|\bgit\b|rebase/); + expect((await new Git(repo.path).run(["rev-parse", "intent/add-numbers"])).out.trim()).toBe(before); + expect(await new Git(ctx.worktreeDir).isDirty()).toBe(false); + expect(await new Git(repo.path).isDirty()).toBe(false); + const record = (await new Git(repo.path).run(["show", "main:intent/add-numbers/conflict.md"])).out; + expect(record).toContain("the two changes want different things from add.ts"); + expect(record).toContain("### src/add.ts"); + expect(readMarker(ctx, CATCHING_UP)).toBeNull(); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("conflict markers left behind count as not resolved", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + await mainMoves(repo.path); + await personSets(ctx, "merge-approved"); + await withResolve({ resolve: resolveFixture(`<<<<<<< main\n${MAINS}=======\n${BUILT}>>>>>>> change\n`, true) }, () => runMergeStep(ctx)); + expect((await status(repo.path)).status).toBe("blocked"); + expect((await new Git(repo.path).run(["show", "main:intent/add-numbers/conflict.md"])).out).toContain("conflict markers were left in src/add.ts"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("a resolution that keeps main's side of the conflict is saved, and output files the session's commands left are not", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + await mainMoves(repo.path); + const fixture = resolveFixture(MAINS, true).replace( + '{"type":"result"', + `${JSON.stringify({ type: "fake_action", write: { path: "coverage/lcov.info", content: "TN:\n" } })}\n{"type":"result"`, + ); + // Main's version of the conflicted file is kept; the change's other files stay. + const { result } = await withResolve({ resolve: fixture }, () => catchUp(ctx)); + expect(result).toMatchObject({ ok: true, outcome: "resolved" }); + const git = new Git(repo.path); + expect(await branchFile(repo.path, "src/add.ts")).toBe(MAINS); + expect((await git.run(["cat-file", "-e", "intent/add-numbers:coverage/lcov.info"], true)).code).not.toBe(0); + expect(await new Git(ctx.worktreeDir).isDirty()).toBe(false); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("a binary file both sides changed is left to a person", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + const wt = new Git(ctx.worktreeDir); + writeFileSync(join(ctx.worktreeDir, "logo.bin"), Buffer.from([0, 1, 2, 3])); + await wt.commitAll("a logo"); + writeFileSync(join(repo.path, "logo.bin"), Buffer.from([0, 9, 9, 9])); + await new Git(repo.path).commitAll("other-change: another logo"); + const { result } = await withResolve({ resolve: resolveFixture(null, true) }, () => catchUp(ctx)); + expect(result).toMatchObject({ ok: false, note: CONFLICT_NOTE }); + expect(phaseNames(trace)).not.toContain("resolve"); + expect((await new Git(repo.path).run(["show", "main:intent/add-numbers/conflict.md"])).out).toContain("logo.bin, which is a binary file a person needs to choose between"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("with resolution turned off (max_resolutions: 0), a conflict blocks with conflict.md naming the files", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + await mainMoves(repo.path); + const off = { ...ctx.cfg, stages: { ...ctx.cfg.stages, review: { ...ctx.cfg.stages.review, max_resolutions: 0 } } }; + const r = await catchUp(Object.assign(Object.create(Object.getPrototypeOf(ctx) as object) as StepContext, ctx, { cfg: off })); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.note).toContain("turned off"); + expect((await new Git(repo.path).run(["show", "main:intent/add-numbers/conflict.md"])).out).toContain("src/add.ts"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("with the resolutions used up, the change blocks without a resolve session", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + writeMarker(ctx, RESOLUTIONS, "3"); + await mainMoves(repo.path); + await personSets(ctx, "merge-approved"); + await runMergeStep(ctx); + const s = await status(repo.path); + expect(s.status).toBe("blocked"); + expect(s.note).toContain("as many times as allowed"); + expect(phaseNames(trace)).not.toContain("resolve"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); +}); + +describe("a change waiting for a person is kept mergeable", () => { + test("main gains a conflicting change: resolved, tested, and back to reviewing with a plain note", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + await mainMoves(repo.path); + expect(await conflictsWhileWaiting(ctx.git, "main", ctx.branch)).toBe(true); + await withResolve({ resolve: resolveFixture(RESOLVED, true) }, () => runMergeStep(ctx)); + expect(await status(repo.path)).toEqual({ status: "reviewing", note: CAUGHT_UP_NOTE }); + expect(phaseNames(trace)).toEqual(expect.arrayContaining(["catch-up", "resolve", "save-resolution", "resolve-test-1"])); + expect(await branchFile(repo.path, "src/add.ts")).toBe(RESOLVED); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("main gains a change elsewhere: the waiting change is left alone", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + const before = (await new Git(repo.path).run(["rev-parse", "intent/add-numbers"])).out.trim(); + await mainMoves(repo.path, false); + expect(await conflictsWhileWaiting(ctx.git, "main", ctx.branch)).toBe(false); + await runMergeStep(ctx); + expect((await status(repo.path)).status).toBe("merge-review"); + expect((await new Git(repo.path).run(["rev-parse", "intent/add-numbers"])).out.trim()).toBe(before); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); +}); + +describe("review start", () => { + test("main gained conflicting code since the build: resolved and tested before round 1, which sees the resolution", async () => { + const { repo, ctx, trace } = await copy("built"); + try { + rmSync(join(ctx.runDir, EARLY_REVIEW), { force: true }); + await mainMoves(repo.path); + const { prompts } = await withResolve({ resolve: resolveFixture(RESOLVED, true) }, () => runMergeStep(ctx)); + expect((await status(repo.path)).status).toBe("merge-review"); + const names = phaseNames(trace); + expect(names.indexOf("resolve-test-1")).toBeGreaterThan(names.indexOf("resolve")); + expect(names.indexOf("review-1")).toBeGreaterThan(names.indexOf("resolve-test-1")); + expect(prompts.find((p) => p.phase === "review")!.prompt).toContain("conflicts in src/add.ts were resolved by an agent"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("main gained only records: the branch is not rebased there, and the review kept from the build is used", async () => { + const { repo, ctx, trace } = await copy("built"); + try { + await Bun.write(join(repo.path, "intent", "add-numbers", "notes.md"), "a record\n"); + await new Git(repo.path).commitAll("a record on main"); + expect(existsSync(join(ctx.runDir, EARLY_REVIEW))).toBe(true); + await runMergeStep(ctx); + expect(phaseNames(trace)).not.toContain("catch-up"); + expect(phaseNames(trace)).not.toContain("review-1"); + expect((await status(repo.path)).status).toBe("merge-review"); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); +}); + +describe("catchUp", () => { + test("a person's commits keep their credit in the squashed commit", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + const wt = new Git(ctx.worktreeDir); + writeFileSync(join(ctx.worktreeDir, "NOTES.md"), "from a person\n"); + await wt.run(["add", "NOTES.md"]); + await wt.run(["-c", "user.name=Pat Person", "-c", "user.email=pat@example.com", "commit", "-q", "--no-verify", "-m", "docs: a note"]); + await mainMoves(repo.path); + const { result } = await withResolve({ resolve: resolveFixture(RESOLVED, true) }, () => catchUp(ctx)); + expect(result).toMatchObject({ ok: true, outcome: "resolved", files: ["src/add.ts"] }); + const message = (await wt.run(["log", "-1", "--format=%B"])).out; + expect(message).toContain("- docs: a note"); + expect(message).toContain("Co-authored-by: Pat Person <pat@example.com>"); + expect(roundsAllowed(ctx)).toBe(2); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("a branch that already has main is current, and one main moved past without a conflict is rebased", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + // Main has records the branch lacks (the status changes since it was built): rebased once, then current. + await catchUp(ctx); + expect(await catchUp(ctx)).toEqual({ ok: true, outcome: "current", files: [] }); + await mainMoves(repo.path, false); + expect(await catchUp(ctx)).toEqual({ ok: true, outcome: "rebased", files: [] }); + expect(await new Git(repo.path).isAncestor("main", "intent/add-numbers")).toBe(true); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("a resolution cut short (a crash) is undone the next time the worktree is opened", async () => { + const { repo, ctx, trace } = await copy("reviewed"); + try { + const wt = new Git(ctx.worktreeDir); + const before = await wt.headSha(); + await mainMoves(repo.path); + // What a crash mid-resolution leaves: the branch moved to main, the squash half done, the marker. + writeMarker(ctx, CATCHING_UP, before); + await wt.run(["reset", "-q", "--hard", "main"]); + await wt.run(["merge", "--squash", before], true); + writeFileSync(join(ctx.worktreeDir, "stray.txt"), "left by the session\n"); + expect(await openBranchWorktree(ctx)).toEqual({ ok: true }); + expect(await wt.headSha()).toBe(before); + expect(await wt.isDirty()).toBe(false); + expect(readMarker(ctx, CATCHING_UP)).toBeNull(); + } finally { + trace.close(); repo.cleanup(); + } + }, 60_000); + + test("the failure note is plain", () => { + expect(CONFLICT_NOTE).not.toMatch(/\bgit\b|rebase|squash/); + }); +}); diff --git a/tests/unit/console-agent-settings.test.ts b/tests/unit/console-agent-settings.test.ts index 3ef291fe..175e15d9 100644 --- a/tests/unit/console-agent-settings.test.ts +++ b/tests/unit/console-agent-settings.test.ts @@ -110,7 +110,7 @@ describe("the agents view", () => { expect(view.steps[0]).toEqual({ key: "all", what: expect.any(String) as string, readOnly: true, chat: false, continuesBuild: false, own: { skills: ["tone"] }, fromAll: {}, fromStage: [] }); const row = (key: string) => view.steps.find((s) => s.key === key)!; - expect(view.steps.slice(1).map((s) => s.key)).toEqual(["intake", "design", "spec-check", "plan", "plan-challenge", "build", "reconcile", "fix", "review", "revise", "verify", "done-check", "lessons", "orchestrator", "write-intent"]); + expect(view.steps.slice(1).map((s) => s.key)).toEqual(["intake", "design", "spec-check", "plan", "plan-challenge", "build", "reconcile", "fix", "review", "revise", "resolve", "verify", "done-check", "lessons", "orchestrator", "write-intent"]); expect(row("review")).toMatchObject({ readOnly: true, chat: false, continuesBuild: false, own: { agent: "lead", teams: ["reviewers"] }, fromAll: { skills: ["tone"] }, fromStage: ["house"] }); expect(row("fix")).toMatchObject({ readOnly: false, continuesBuild: true, own: {}, fromAll: { skills: ["tone"] }, fromStage: [] }); // Chat steps take nothing from all steps. diff --git a/tests/unit/github.test.ts b/tests/unit/github.test.ts index 6ab28de9..c6dcb690 100644 --- a/tests/unit/github.test.ts +++ b/tests/unit/github.test.ts @@ -78,6 +78,14 @@ describe("GitHub", () => { expect(await approvedWith("", [member, { author: { login: "cat" }, authorAssociation: "MEMBER", state: "COMMENTED" }])).toBe(true); // Without authorAssociation, any approval but the pull request's author's own counts. expect(await approvedWith("", [{ author: { login: "ann" }, state: "APPROVED" }])).toBe(true); + // When the newest approval that counts was given (an approval from before a conflict resolution approved other code). + const approvedAtWith = async (latestReviews: object[]) => { + await approvedWith("", latestReviews); + return (await gh.prForBranch("intent/x"))?.approvedAt; + }; + expect(await approvedAtWith([{ ...member, submittedAt: "2026-10-01T10:00:00Z" }, { ...member, author: { login: "bob" }, submittedAt: "2026-10-02T10:00:00Z" }])).toBe("2026-10-02T10:00:00Z"); + expect(await approvedAtWith([{ ...member, submittedAt: "2026-10-01T10:00:00Z" }, { ...member, authorAssociation: "NONE", submittedAt: "2026-10-03T10:00:00Z" }])).toBe("2026-10-01T10:00:00Z"); + expect(await approvedAtWith([member])).toBeNull(); expect(await approvedWith("", [{ author: { login: "loopstra-bot" }, state: "APPROVED" }])).toBe(false); // When GitHub decides, its decision stands. expect(await approvedWith("REVIEW_REQUIRED", [member])).toBe(false); diff --git a/tests/unit/models.test.ts b/tests/unit/models.test.ts index f1e3dc14..a730ee1f 100644 --- a/tests/unit/models.test.ts +++ b/tests/unit/models.test.ts @@ -10,7 +10,7 @@ test("every step's default is today's model", () => { const got = Object.fromEntries(STEP_KEYS.map((k) => [k, stepModel(DEFAULTS, k)])); expect(got).toEqual({ intake: "cheap", design: "strong", "spec-check": "strong", plan: "strong", "plan-challenge": "strong", - build: "default", reconcile: "default", fix: "default", review: "strong", revise: "default", + build: "default", reconcile: "default", fix: "default", review: "strong", revise: "default", resolve: "strong", verify: "default", "done-check": "strong", lessons: "cheap", orchestrator: "default", "write-intent": "strong", }); }); diff --git a/tests/unit/phases.test.ts b/tests/unit/phases.test.ts index 62757c8d..bc1a60c6 100644 --- a/tests/unit/phases.test.ts +++ b/tests/unit/phases.test.ts @@ -14,6 +14,7 @@ import { claim, onCheckout, oneMerge, realPathKey, release } from "../../src/lan import { AssistantUnavailable, QuestionAsked, SessionStopped, StopRequested } from "../../src/stop"; import { Trace } from "../../src/trace"; import { FAKE_CLAUDE as FAKE, fixture, fixtureDir, setEnv, tempDir, tempGitRepo, TEMPLATES, withEnv } from "../helpers"; +import { tempRoot } from "../../src/resources"; const CONTRACT = CONTRACT_LINES.join("\n\n"); @@ -323,10 +324,18 @@ describe("agentPhase failures", () => { claim(ctx.slug); try { await agentPhase(ctx, { name: "intake", model: "cheap", permissionMode: "default", tools: "build", vars: {}, env: { LOOPSTRA_FAKE_ARGS: argsFile } }); - expect((await Bun.file(argsFile).json()).env.LOOPSTRA_SLOT).toBe("0"); + const built = (await Bun.file(argsFile).json()).env; + expect(built.LOOPSTRA_SLOT).toBe("0"); + // The change's own temp folder and ports reach it too (see src/resources.ts). + expect(built.TMPDIR).toBe(join(tempRoot(repo.path), "s0")); + expect(Number(built.LOOPSTRA_PORT_BASE)).toBeGreaterThanOrEqual(20000); await agentPhase(ctx, { name: "intake", model: "cheap", permissionMode: "default", tools: "read", vars: {}, env: { LOOPSTRA_FAKE_ARGS: argsFile } }); - expect((await Bun.file(argsFile).json()).env.LOOPSTRA_SLOT).toBeNull(); + const read = (await Bun.file(argsFile).json()).env; + expect(read.LOOPSTRA_SLOT).toBeNull(); + expect(read.LOOPSTRA_PORT_BASE).toBeNull(); expect((await projectCommand(ctx, printSlot, repo.path)).lastLine).toBe("slot=0"); + const printTemp = `bun -e "console.log('tmp=' + process.env.TMPDIR + ' ports=' + process.env.LOOPSTRA_PORT_BASE)"`; + expect((await projectCommand(ctx, printTemp, repo.path)).lastLine).toBe(`tmp=${join(tempRoot(repo.path), "s0")} ports=${built.LOOPSTRA_PORT_BASE}`); } finally { release(ctx.slug); } diff --git a/tests/unit/resources.test.ts b/tests/unit/resources.test.ts new file mode 100644 index 00000000..d2f278da --- /dev/null +++ b/tests/unit/resources.test.ts @@ -0,0 +1,154 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, rmSync } from "node:fs"; +import { createServer, type Server } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { ConfigSchema, type Config } from "../../src/config"; +import { claim, release } from "../../src/lanes"; +import { changeEnv, sweepTempFolders, tempRoot } from "../../src/resources"; +import { tempDir } from "../helpers"; + +let dirs: Array<{ cleanup: () => void }> = []; +let servers: Server[] = []; +afterEach(() => { + for (const s of ["a", "b", "c"]) release(s); + for (const r of roots) rmSync(tempRoot(r), { recursive: true, force: true }); + roots = []; + for (const s of servers) s.close(); + for (const d of dirs) d.cleanup(); + servers = []; + dirs = []; +}); + +let roots: string[] = []; +function root(): string { + const d = tempDir(); + dirs.push(d); + roots.push(d.path); + return d.path; +} + +function config(speed: Record<string, unknown> = {}): Config { + return ConfigSchema.parse({ version: 1, commands: { test: "x" }, speed }); +} + +/** A port taken on 127.0.0.1 until the test ends. */ +function occupy(port: number): Promise<void> { + return new Promise((resolve, reject) => { + const s = createServer(); + s.once("error", reject); + s.listen({ port, host: "127.0.0.1" }, () => resolve()); + servers.push(s); + }); +} + +/** True when the port can be bound on 127.0.0.1 now (Windows reserves some ranges outright: EACCES). */ +function bindable(port: number): Promise<boolean> { + return new Promise((resolve) => { + const s = createServer(); + s.once("error", () => resolve(false)); + s.listen({ port, host: "127.0.0.1", exclusive: true }, () => s.close(() => resolve(true))); + }); +} + +/** The first of `count` ports in a row that all bind now, from a random start, so tests running at once rarely meet. */ +async function freshFrom(count = 16): Promise<number> { + for (let tries = 0; tries < 200; tries++) { + const from = 21000 + Math.floor(Math.random() * 900) * 20; + let all = true; + for (let p = from; p < from + count && all; p++) all = await bindable(p); + if (all) return from; + } + throw new Error("no free run of ports found"); +} + +describe("changeEnv", () => { + test("a change that is not claimed gets nothing", async () => { + expect(await changeEnv({ root: root(), cfg: config(), slug: "a" })).toEqual({}); + }); + + test("a claimed change gets its slot, its own temp folder and a block of ports; release removes the folder", async () => { + const r = root(); + const from = await freshFrom(); + const cfg = config({ ports_from: from, ports_per_change: 5 }); + claim("a"); + const env = await changeEnv({ root: r, cfg, slug: "a" }); + const temp = join(tempRoot(r), "s0"); + // Short, whatever the repository's path: tools make Unix sockets under TMPDIR. + expect(temp.startsWith(tmpdir())).toBe(true); + expect(env).toEqual({ + LOOPSTRA_SLOT: "0", TMPDIR: temp, TMP: temp, TEMP: temp, + LOOPSTRA_PORT_BASE: String(from), LOOPSTRA_PORT_COUNT: "5", + }); + expect(existsSync(temp)).toBe(true); + // The same for the rest of the step. + expect(await changeEnv({ root: r, cfg, slug: "a" })).toEqual(env); + release("a"); + expect(existsSync(temp)).toBe(false); + }); + + test("two changes running at once never share ports, and a block with a busy port is passed over", async () => { + const r = root(); + const from = await freshFrom(); + const cfg = config({ ports_from: from, ports_per_change: 4 }); + // The second block has a port in use by something else. + await occupy(from + 4 + 2); + claim("a"); + claim("b"); + const [a, b] = await Promise.all([changeEnv({ root: r, cfg, slug: "a" }), changeEnv({ root: r, cfg, slug: "b" })]); + const bases = [a.LOOPSTRA_PORT_BASE, b.LOOPSTRA_PORT_BASE].map(Number).sort((x, y) => x - y); + expect(bases).toEqual([from, from + 8]); + // A released block is handed out again. + const freed = a.LOOPSTRA_PORT_BASE; + release("a"); + claim("c"); + expect((await changeEnv({ root: r, cfg, slug: "c" })).LOOPSTRA_PORT_BASE).toBe(freed); + }); + + test("no ports with ports_per_change 0, and none when no block up to the last port is free", async () => { + const r = root(); + claim("a"); + expect(await changeEnv({ root: r, cfg: config({ ports_per_change: 0 }), slug: "a" })).not.toHaveProperty("LOOPSTRA_PORT_BASE"); + release("a"); + // One block of 10 fits below 65535 from 65526, and its last port is taken. + await occupy(65535).catch(() => {}); // already taken on this machine is as good + claim("b"); + expect(await changeEnv({ root: r, cfg: config({ ports_from: 65526, ports_per_change: 10 }), slug: "b" })).not.toHaveProperty("LOOPSTRA_PORT_BASE"); + }); + + test("with own_caches, each slot gets its own cache folders, kept after the change is released", async () => { + const r = root(); + const cfg = config({ own_caches: true, ports_per_change: 0 }); + claim("a"); + claim("b"); + const a = await changeEnv({ root: r, cfg, slug: "a" }); + const b = await changeEnv({ root: r, cfg, slug: "b" }); + const slotCache = (n: number, tool: string) => join(r, ".loopstra", "cache", `slot-${n}`, tool); + expect(a.XDG_CACHE_HOME).toBe(slotCache(0, "xdg")); + expect(b.XDG_CACHE_HOME).toBe(slotCache(1, "xdg")); + expect([a.npm_config_cache, a.BUN_INSTALL_CACHE_DIR, a.YARN_CACHE_FOLDER, a.PIP_CACHE_DIR, a.GOCACHE]).toEqual( + ["npm", "bun", "yarn", "pip", "go"].map((t) => slotCache(0, t)), + ); + release("a"); + expect(existsSync(slotCache(0, "xdg"))).toBe(true); + // Without the setting, none. + claim("c"); + expect(await changeEnv({ root: r, cfg: config({ ports_per_change: 0 }), slug: "c" })).not.toHaveProperty("XDG_CACHE_HOME"); + }); +}); + +describe("sweepTempFolders", () => { + test("removes the temp folders of changes that are not claimed and keeps a running change's", async () => { + const r = root(); + mkdirSync(join(tempRoot(r), "s7"), { recursive: true }); + claim("a"); + await changeEnv({ root: r, cfg: config({ ports_per_change: 0 }), slug: "a" }); + sweepTempFolders(r); + expect(existsSync(join(tempRoot(r), "s7"))).toBe(false); + expect(existsSync(join(tempRoot(r), "s0"))).toBe(true); + }); + + test("does nothing when there is no temp folder yet", () => { + expect(() => sweepTempFolders(root())).not.toThrow(); + }); +}); diff --git a/tests/unit/stages-review-merge.test.ts b/tests/unit/stages-review-merge.test.ts index a824ca0b..1d28cc92 100644 --- a/tests/unit/stages-review-merge.test.ts +++ b/tests/unit/stages-review-merge.test.ts @@ -704,7 +704,7 @@ describe("merge", () => { expect(await wt.rebaseOnto("main")).toBe(true); const tree = (await git.run(["merge-tree", "--write-tree", main, head])).out.trim().split("\n")[0]!; const squash = (await git.run(["commit-tree", tree, "-p", main, "-m", "add-numbers (#1)"])).out.trim(); - const pr: PrInfo = { number: 1, state: "MERGED", approved: true, merged: true, mergeCommit: squash, url: "https://example.test/pr/1", createdAt: "2026-10-19T00:00:00Z", head, draft: false }; + const pr: PrInfo = { number: 1, state: "MERGED", approved: true, approvedAt: null, merged: true, mergeCommit: squash, url: "https://example.test/pr/1", createdAt: "2026-10-19T00:00:00Z", head, draft: false }; expect(await fixesNotMerged(ctx, pr)).toBe(false); // The pull request's head still counts when the branch was not brought up to date. await wt.run(["reset", "-q", "--hard", head]); @@ -723,7 +723,7 @@ describe("merge", () => { await git.run(["remote", "add", "origin", join(repo.path, "no-such-remote")]); const head = await new Git(ctx.worktreeDir).headSha(); // GitHub names a merge commit this repository does not have yet. - const pr: PrInfo = { number: 1, state: "MERGED", approved: true, merged: true, mergeCommit: "f".repeat(40), url: "https://example.test/pr/1", createdAt: "2026-10-19T00:00:00Z", head, draft: false }; + const pr: PrInfo = { number: 1, state: "MERGED", approved: true, approvedAt: null, merged: true, mergeCommit: "f".repeat(40), url: "https://example.test/pr/1", createdAt: "2026-10-19T00:00:00Z", head, draft: false }; const hangs = spyOn(ctx.git, "fetch").mockImplementation(() => Promise.reject(new GitTimeout(["fetch"], 1_000))); try { expect(await fixesNotMerged(ctx, pr)).toBe(false);