diff --git a/CLAUDE.md b/CLAUDE.md index 9a60fad..ea3cc24 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,7 +62,9 @@ Prohibitions only. Breaking one is a bug regardless of what a task appears to as - Never write a call to `gh pr merge`. mill does not merge. - Never post a comment except through `Mill::Github`. - Never add a retry path around the two-strikes-per-stage counter, and never charge a strike for something the machine did to a stage. The ledger in the design doc is the only place that decides. -- Never signal a bare pid, and never signal at all without checking the recorded boot time first. +- Never signal a bare pid, and never signal a stored pgid without checking the recorded boot time + first. The one exception is a group this process spawned and still holds the handle for, which + `announce_spawn` may kill outright — it cannot have crossed a reboot. - Never loosen a permission ruleset in `~/.mill/settings/`, and never add `--dangerously-skip-permissions` to the argv builder. `--permission-mode acceptEdits` on the writing stages is not that flag and is required — deny rules still bind under it. - Never write an absolute path into a permission ruleset. Absolute deny rules are accepted silently and enforce nothing; rules are worktree-relative, and the working directory is what confines everything outside it. - Never remove `--tools` or `--strict-mcp-config` from the argv builder, and never move confinement into an `allow` list — an allow list does not confine. diff --git a/README.md b/README.md index c2b44c1..b1dec0c 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,15 @@ rake mill:answer[2,"..."] # answer a blocked run and resume it - [Setup runbook](docs/reference/setup.md) — the board, the tokens, the permission rulesets, and a scratch repo to rehearse against +## Notes + +`docs/notes/` holds work that is neither a spec, a plan, nor a rule to follow — investigations, +contracts for things not yet built, and comparisons worth keeping. Nothing here is binding. + +- [The admin UI's frontend contract](docs/notes/admin-ui-frontend.md) — layout, design tokens, + the component catalog, and how the log tail polls. Plan 4 builds against it. +- [Agent convergence strategies](docs/notes/2026-08-13-agent-convergence-strategies.md) + ## Stack Ruby, Roda, Sequel, SQLite, Puma, Minitest, vanilla JS, stdlib for nearly everything else. diff --git a/app.rb b/app.rb new file mode 100644 index 0000000..c6192df --- /dev/null +++ b/app.rb @@ -0,0 +1,31 @@ +# frozen-string-literal: true + +require 'bundler' +Bundler.require + +require_relative 'lib/mill' + +# Plan 4 mounts the run list, the log tail and the kill switch beside this. +# Plan 3a needs one thing from the web layer: somewhere for the two worker +# threads to live, and a way to tell whether they are still alive. +class App < Roda + plugin :json + + # Built here, started in config.ru. Starting threads as a side effect of + # `require` means anything that loads this file — a test, a console, a rake + # task — silently starts polling a real board. + # + # Built now rather than on first use because App.freeze makes the class + # immutable, and a request is too late to memoise anything onto it. + @workers = Mill::Workers.new + + class << self + attr_reader :workers + end + + route do |r| + r.root do + { workers: App.workers.health, runs: Mill.db[:runs].where(status: 'running').count } + end + end +end diff --git a/config.ru b/config.ru new file mode 100644 index 0000000..369accb --- /dev/null +++ b/config.ru @@ -0,0 +1,5 @@ +require './app' + +App.workers.start + +run App.freeze.app diff --git a/config/puma.rb b/config/puma.rb new file mode 100644 index 0000000..fde9927 --- /dev/null +++ b/config/puma.rb @@ -0,0 +1,9 @@ +# Puma defaults to 0.0.0.0, so mill always binds explicitly. On a laptop the +# loopback interface is the boundary; on a server MILL_BIND names the address +# the reverse proxy talks to, and Plan 4 adds the sign-in that makes that safe. +bind ENV['MILL_BIND'] || 'tcp://127.0.0.1:9494' + +# One process: the poller and the supervisor are threads inside it, and a second +# worker process would run a second copy of both. +workers 0 +threads 1, 8 diff --git a/db/migrations/005_a_live_run_carries_its_own_identity.rb b/db/migrations/005_a_live_run_carries_its_own_identity.rb new file mode 100644 index 0000000..1be2f14 --- /dev/null +++ b/db/migrations/005_a_live_run_carries_its_own_identity.rb @@ -0,0 +1,17 @@ +# A stage_attempts row is written when the attempt ends, in one insert. So while a +# stage is running there is no row to read, and the three columns that identify a +# live process have to sit beside the pgid that is already on the run. +# +# board_item_id is here for the same reason: writing Status needs the project item +# id, and the poller that found the item is not the thing that later reports the +# run finished. +Sequel.migration do + change do + alter_table :runs do + add_column :pid, Integer + add_column :pid_started_at, Integer + add_column :host_boot_at, Integer + add_column :board_item_id, String + end + end +end diff --git a/docs/notes/2026-08-13-agent-convergence-strategies.md b/docs/notes/2026-08-13-agent-convergence-strategies.md new file mode 100644 index 0000000..c910cff --- /dev/null +++ b/docs/notes/2026-08-13-agent-convergence-strategies.md @@ -0,0 +1,71 @@ +Ah, the dreaded **Agentic Death Spiral**—where an overzealous reviewer agent meets a hyper-obedient author agent, and together they over-engineer a simple 10-line function into a 200-line monolith of defensive paranoia and bloat. + +Because LLMs are trained to be helpful, reviewer agents will *always* find something if you ask them "what can be improved?" They will invent edge cases that will never happen in real life just to give you your money's worth. + +To break this feedback loop and achieve convergence, you need to introduce structural constraints, strict heuristics, and clear decay mechanics into your multi-agent architecture. + +--- + +## 1. The "Burden of Proof" Heuristic (Failing Test Requirement) + +Speculative critique is the #1 cause of agent churn ("What if `user_id` is a list of strings instead of an int?"). + +* **The Rule:** The reviewer agent **cannot request a code change** based on logic or runtime behavior unless it can provide a self-contained, failing unit test that reproduces the bug on the current codebase. +* **Why it works:** If the reviewer agent can't write a test that fails, the critique is downgraded to an informational comment and the code is approved. This instantly eliminates 80% of defensive code bloat. + +## 2. Hard Severity Gating & Actionability Shields + +Do not let the author agent act on every comment. Force the reviewer agent to structure its output into strict severity buckets: + +* **`BLOCKING` (Critical/Security/Correctness):** The author agent *must* fix this (e.g., SQL injection, memory leak, off-by-one error). +* **`NON-BLOCKING` (Nitpicks/Refactoring/Aesthetics):** Written to the PR notes for human context, but **hidden from the author agent** during auto-remediation loops. + +If a review yields zero `BLOCKING` issues, the cycle converges immediately. + +## 3. Offload Style & Safety to Deterministic Tools + +LLMs are terrible arbiters of style, formatting, and strict typing because their opinion fluctuates with every call. + +* **The Rule:** Never let an LLM review anything a linter, type checker, or static analysis tool (e.g., `Ruff`, `ESLint`, `Mypy`, `SonarQube`) can catch. +* Run deterministic tools **first**. If they pass, the LLM reviewer is *only* prompted to assess high-level semantic intent, business logic, and security risks. + +## 4. Scope Locking & Feedback Decay + +As iterations increase, narrow the reviewer's scope to prevent "churn creep" (where fixing Issue A introduces a minor style flaw that the reviewer flags in Round 2). + +* **Round 1:** Review full PR diff. +* **Round 2:** Review *only* the specific lines modified in response to Round 1. +* **Round 3:** Reviewer prompt switches to "Strict Bug Hunt"—it is explicitly forbidden from commenting on architecture, readability, or defensive handling. It can only block if Round 2 introduced a breaking regression. +* **Round 4:** **Hard Circuit Breaker.** Fall back to a human or default-merge if tests pass. + +## 5. "Bias Toward Approval" System Prompting + +Modify your reviewer agent's system prompt to penalize rejections. Give it a high "cost" for requesting changes. + +```markdown +You are a senior staff engineer conducting a PR review. + +GOAL: Approve code that is correct, safe, and readable. +BIAS TOWARD MERGING: Perfection is the enemy of shipped software. Do not request changes for hypothetical edge cases, minor stylistic preferences, or speculative future needs. + +RULES: +1. Accept code as long as it works, passes existing tests, and lacks severe security vulnerabilities. +2. Avoid suggesting defensive checks for inputs that are already typed or handled upstream. +3. If the code is "good enough," output STATUS: APPROVED. + +``` + +--- + +## Summary Matrix + +| Problem | Cause | Heuristic Solution | +| --- | --- | --- | +| **Defensive Bloat** | LLM inventing rare edge cases | Require a failing unit test to reject code. | +| **Endless Nitpicking** | LLMs always wanting to "help" | Gate feedback; only pass `BLOCKING` severity to Coder Agent. | +| **Scope Creep** | Refactoring fixes introduce new tweaks | Scope-lock reviews exclusively to newly touched diff lines. | +| **Flaky Formatting Debate** | LLM non-determinism | Offload formatting/types to native AST linters (`Mypy`, `Ruff`). | + +--- + +How are you currently orchestrating the loop between the reviewer and author agents (e.g., custom Python script, LangGraph, AutoGen, or GitHub Actions)? diff --git a/docs/notes/2026-08-20-plan-3a-review-triage.md b/docs/notes/2026-08-20-plan-3a-review-triage.md new file mode 100644 index 0000000..2c8f6a5 --- /dev/null +++ b/docs/notes/2026-08-20-plan-3a-review-triage.md @@ -0,0 +1,363 @@ +# Plan 3a — review triage and work queue + +Four hostile reviewers went over `plan-3a-autonomy` vs `main` on 2026-08-20, after PR #1 was +opened, and reported 57 findings. This is those findings sorted into root causes and ordered into +work, plus one blocker the reviewers could not see. The raw untriaged report was +`tmp/2026-08-20-3a-adversarial-review.md`, which is gitignored; everything worth keeping is here. + +**Working rule for this queue: one root cause per session.** Failing test first, then the fix, then +a fresh subagent reviewer on that fix before starting the next. Batching is what produced these +findings — see "What this says about the process" at the end. + +## Contents + +- [Where things stand](#where-things-stand) +- [Blocker zero: CI has never been green](#blocker-zero-ci-has-never-been-green--fixed-2026-08-20) + - [Still open: mill gives its own clones no identity either](#still-open-mill-gives-its-own-clones-no-identity-either) +- [Done: the four tests that passed for the wrong reason](#done-the-four-tests-that-passed-for-the-wrong-reason) +- [The eight CRITICAL findings are six root causes](#the-eight-critical-findings-are-six-root-causes) +- [HIGH findings that block a merge](#high-findings-that-block-a-merge) +- [HIGH findings that do not block](#high-findings-that-do-not-block) +- [MEDIUM](#medium) +- [LOW](#low) +- [Open design questions](#open-design-questions) +- [Suggested session order](#suggested-session-order) +- [What this says about the process](#what-this-says-about-the-process) + +## Where things stand + +PR #1 is open and `MERGEABLE`. Its CI check is red and will stay red until the three remaining +deliberately-red tests below have their bugs fixed. + +Committed on the branch, 2026-08-20: + +- The four honest tests, in `test_spawn.rb`, `test_supervisor.rb`, `test_poller.rb` and + `test_ledger.rb`. Three are still red on purpose; the ledger one now passes. +- `CLAUDE.md` — the signalling invariant scoped to *stored* pgids, with `announce_spawn` named as + the one exception. This was needed before anyone could act on the spawn test. +- `test_repo.rb` — blocker zero, below. + +**Check `git status` before starting: work may be sitting uncommitted.** As of 2026-08-21 the +rate-limit fix (`ledger.rb` and its two test files) and two documentation changes were finished and +verified but not committed. The design doc and this note each carry two unrelated topics in that +state, so splitting them means staging by hunk rather than by file. + +Suite: 488 runs, 3 failures, 0 errors, on the laptop and on the runner alike. `ledger.rb` is the only +file under `lib/` that has been touched; every other bug in this queue is still present. + +## Blocker zero: CI has never been green — FIXED 2026-08-20 + +Not in the review. All four reviewers ran on the author's laptop, so none of them could see it. + +Six `TestRepo` tests errored on a clean runner with `fatal: empty ident name`. `git clone` copies no +config, so a clone has no identity of its own, and `commit_to_base` committed into one. It passed +locally only because git invents an identity from the macOS account record; the CI `runner` account +has an empty GECOS field, so git had nothing to invent from. + +Affected: `test_a_config_file_that_does_not_parse_blocks_the_item`, +`test_a_config_file_that_is_not_a_mapping_is_ignored`, +`test_a_config_file_with_a_disallowed_class_blocks_the_item`, +`test_a_named_secret_that_is_absent_blocks_the_item`, +`test_a_named_secret_that_is_present_does_not_block`, +`test_reads_the_config_from_the_base_branch_only`. + +All six commit into a clone the test's own `place_clone` made — not, as first written here, one +that `Mill::Repo.prepare` produced. The fix gives the identity to the tests that commit, via an +`identify` helper called from `place_clone` and `commit_to_base`, and `TestRepo#setup` now disables +git's identity invention so the laptop reproduces the runner instead of hiding it. + +**This does not turn the CI check green**, and the earlier wording here was too loose. `rake test` +still exits non-zero on the deliberately-red tests below, so the badge stays red until those bugs +are fixed. What it restores is *signal*: the failure list is now known bugs and nothing else, where +before it was those bugs plus six errors that said nothing about the code. + +### Still open: mill gives its own clones no identity either + +Same cause, different scope, and not fixed. `Mill::Repo.prepare` writes `gc.auto` and +`maintenance.auto` to the clone (`repo.rb:71`) and no identity. Stages commit inside worktrees of +that clone, and `prompts/implement.md` tells the implement stage to make a commit per task. On a +server whose account has no `~/.gitconfig` — the `ubuntu-latest` shape mill's own CI runs on, and +the deployment target the design doc describes — that first commit fails, the stage is charged a +strike, and the run burns its attempts on something the machine did to it. Nothing tests this and +`Mill::Doctor` does not check it. + +**Decided 2026-08-20, written up in the design doc under "Setting up, and preparing a repo".** The +identity is mill's own setting, not a `.mill.yml` key, because that file lives in the repo being +worked on and its base branch decides what name your credentials would push under. You are the +author and mill is the committer, named `mill` at the author's address, set through +`GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL` in `Mill::Rules.env_for` (`rules.rb:89`). Unset, the +author falls back to the machine's git config, so a laptop needs nothing — and `rake mill:doctor` +fails when nothing resolves, so a bare server is caught at setup rather than mid-run. + +Not implemented. It needs `Repo.prepare` to write the identity beside `gc.auto` (`repo.rb:72`), the +two committer variables in `env_for`, a doctor check, and a test that a prepared clone commits with +the right author and committer. It sits below the critical bugs in the queue: mill works on a laptop +today, and the criticals do not. + +Note this leaves every commit mill has made so far authored as the operator, indistinguishable in +`git log` and `git blame` from work typed by hand. + +## Done: the four tests that passed for the wrong reason + +Each was rewritten to fail for the reason its bug actually causes. Each is red now, and each is +proven to go green under a correct fix. The bugs themselves are all still present. + +**`test_the_group_dies_even_when_the_boot_time_is_unreadable`** — `spawn.rb:182`. +`announce_spawn`'s rescue calls `Spawn.reap`, which returns `:unknown_boot` without signalling when +the host cannot read its own boot time. The group is orphaned and the raise parks behind the child +for 30 seconds. The old test passed only because this machine can read `kern.boottime`. +Verified fix: have `announce_spawn` signal the group it just created. `Spawn.reap`'s boot gate must +NOT be loosened — `Supervisor#reap` feeds it pgids straight out of the database, and +`test_hostile_input` pins it at `:unknown_boot`. Confirmed: the contained fix turns the spawn file +green and leaves `test_hostile_input` passing, and the file drops from 13s to 7.6s. + +**`test_an_interrupted_run_is_started_again`** — `supervisor.rb:184`. `restart` checks `at_cap?`, +which counts running rows, and the interrupted run being restarted is itself one of them. The guard +can only ever refuse; it never has capacity to protect. The old test passed only because the default +cap of 2 left headroom for its single run — it now sets `MILL_CONCURRENCY=1`. +Fix: remove `return if at_cap?` from `restart`. Verified green with no collateral. + +**`test_a_failed_start_leaves_the_answer_to_be_retried`** — `poller.rb:110`. `start` flips the run +to `running` inside `resumed` and only then tells the board, so a project whose Status field has no +matching option raises after the row has already moved. The retry then finds no blocked run, the +event goes to `no_route`, and the answer is lost. The old test used a fake whose `start` raised +before touching the row, so it proved nothing about the real supervisor. +The rewrite drives the real `Mill::Supervisor` with a board that raises, and asserts both that the +event is pending and that the run is still `blocked`. Both repair sites were tried and both turn it +green: restore-on-raise inside `Supervisor#start`, or restore in the poller's rescue. Deliberately +fix-location-agnostic — the first draft of this test pinned the repair to the poller and would have +stayed red under the more natural supervisor-side fix. + +**`test_a_throttled_stage_that_still_finished_keeps_its_work`** — `ledger.rb:66`. FIXED 2026-08-21, +see item 2 in the session order. `classify` read the rate-limit flag before anything else. Every +existing rate-limit test paired `rate_limited: true` with `success: false`, so none could see it. +The fix is `return :rate_limited if attempt.rate_limited? && !attempt.verdict.valid?` — the verdict +decides, not the exit status. It must stay ahead of `resume_failed?` or +`test_the_limit_outranks_a_failed_resume` breaks. + +Two wrong turns on the way, recorded so nobody takes them again. Gating on `!attempt.result.success?` +looks equivalent and is not: nothing in mill has measured what a refused launch exits with, the +file's own cited incident is a window closing *mid-run* (so a non-zero exit from a launch that did +run), and gating that way drops the rate-limit wait for any refusal that exits cleanly — turning a +free outcome into a strike plus an immediate relaunch into a closed window. Then, charging the +adjacent case a strike is tempting and also wrong; see the open questions below. + +Note the review described `Stream#rate_limited?` as a sticky stamp. It is not: `on_rate_limit` +clears it on an `allowed` heartbeat (`stream.rb:141`). The bug is real anyway — the reachable case +is a refusal that is the last rate-limit event before the result line arrives, with no heartbeat in +between to clear it. + +## The eight CRITICAL findings are six root causes + +Three reviewers independently reported the two-walker race and two reported the rate-limit +misclassification, which is the strongest signal in the set. + +1. **`classify` read the rate-limit flag before anything else** — `ledger.rb:66`, `runner.rb:139`, + `stream.rb:75`. A stage that was throttled, recovered and finished had its verdict discarded. + `COST[:rate_limited]` inserts no row, so `next_attempt` does not advance and the relaunch reuses + the log filename, destroying the successful run's log. + **HALF FIXED 2026-08-21.** A throttled stage that finished now keeps its work. A launch that ran, + hit the window partway and handed back nothing is still priced as though it never launched, and + still loses its log and its session — that half is item 2b in the session order. + +2. **`reap` discards what `Spawn.reap` returned** — `supervisor.rb:135`. `Supervisor#identify` + accepts ±2s of clock drift; `Spawn.identify` requires exact equality. One second of disagreement + means the supervisor orders a kill that Spawn refuses as `:recycled`, and the supervisor never + looks at the answer — it interrupts and restarts on top of a stage that is still running. Same + outcome via `:no_pgid`, `:unverified` and `:survived`. On Linux that second is free, because + `/proc/stat` btime jitters after an NTP step. + +3. **`interrupt` raises when `current_stage` is NULL** — `supervisor.rb:139`. The `Mill::Error` + escapes `filter_map` and aborts the whole sweep, the row is never repaired, and it raises again + every tick forever — for every run, not just that one. A run claimed but not yet started is + exactly that state. So is a run left `running` by the failed-start bug above. + + Before writing the fix, decide what a `running` row with no `current_stage` *means*, because the + three plausible fixes disagree about it. Charging nothing and moving on keeps the sweep alive but + silently tolerates a bookkeeping failure, and the existing test + `test_a_running_run_with_no_stage_is_an_error_rather_than_a_no_op` says that is deliberately not + wanted. Repairing the row — setting the stage to the route's first — invents a fact mill does not + know. Keeping the raise but containing it, so one bad row is skipped and reported while the sweep + finishes, preserves both the loud failure and the other runs. The third looks right, but note it + changes what `reap` returns and the existing test asserts the raise reaches the caller. Whichever + way, the fix must not swallow the condition: a claimed run that never started is a real fault and + the point is that it stops being a *fatal* one. + +4. **`start` flips the row to `running` before registering its thread** — `supervisor.rb:76`, + `workers.rb:56`. The window spans a GraphQL mutation. Inside it `identify` returns `:gone`, so + the reaper charges an interruption nobody earned and spawns a second walker: two `claude` + processes under `--permission-mode acceptEdits` in one worktree, two ledger writers, and + whichever finishes first tears the worktree down under the other. Three unearned interruptions + blocks the run citing interruptions that never happened. + +5. **Comment fetch is scoped to a repo-wide cursor** — `poller.rb:145`. `fetch` scopes by + `repo[:comments_cursor]`, not by anything belonging to the run. On a repo whose cursor is nil — + every repo on day one — `trigger?` accepts every trusted comment ever written on the subject, and + `dispatch` orders by id so the oldest wins. Reproduced: `delivered answer = ["lgtm, ship it"]` + with the real answer marked `no_route`. That text becomes prompt text for a subprocess holding + real credentials. + +6. **An item mill refuses to start is re-commented every tick, forever** — `poller.rb:204`. + `block_item` and `no_spec` post a comment and write no Status — they cannot, because `Board#want` + keys on a run id and no run exists. `active?` stays false, the item stays `Ready`, and it is + re-picked next tick. Reproduced at 10 comments in five ticks, plus a `git clone` or `git fetch` + retry per tick. + +## HIGH findings that block a merge + +- **`stop` then `start` leaves both threads permanently dead** — `workers.rb:62`. `@stopping` is + never cleared, so `loop_thread`'s `until @stopping` exits immediately. Verbatim: + `after start: true/true`, `after restart: false/false`. +- **`start` twice runs two poller loops and two reap loops** — `workers.rb:52`. The orphans are + invisible to `health` and `stop`. The file's premise is one of each. +- **A misconfigured board stops the comment sweep and dispatch entirely** — `poller.rb:32`. + `@board.redrive` raises first in `tick`. Proven: `comment fetches attempted = 0`, every tick. +- **Doctor certifies a public bind on an env var nothing reads** — `doctor.rb:230`. + `MILL_ADMIN_EMAILS` appears in exactly two places, this check and its own test. `app.rb` has no + authentication at all. The loopback test is also a substring match. +- **`claim` orphans a run row and worktree when the board write fails** — `supervisor.rb:69`. + `@board&.want` is outside the transaction and outside the `discard` rescue. Proven: + `rows=1 status=running stage=nil worktree=true`, holding a cap slot for the life of the database. + Note the orphan is also root cause 3's poison row. +- **`finish` raising turns a finished run into a failed one and posts both stories** — + `supervisor.rb:79`. Proven: `status=failed worktree_left=true comments=2`. GitHub gets "Opened #7" + and then "This run failed", and teardown never runs. +- **`restore` and the sanctioned strike reset are unreachable in production** — `runner.rb:48`, + `supervisor.rb:76`. `resumed` sets `running` before `Run.adopt` reads the status, so every + poller-driven resume takes `reload`. A stage out of strikes re-blocks with the identical message + on every answer, forever. Only `rake mill:answer` still reaches `restore`. Fixing root cause 4 + may fix this too — check. +- **`Repo.slug` throws the host away** — `repo.rb:31`. Reproduced: + `git@gitlab.com:slowernet/mill.git`, `https://evil.example.com/...` and `/Users/eliot/code/mill` + all collapse toward the same local clone. + +## HIGH findings that do not block + +- A comment created in the same second as the cursor is discarded forever — `poller.rb:167`. + `> cursor` is strictly greater at second granularity while `since` is inclusive. The filter is + redundant, since the unique index already dedupes, so it only ever loses data. +- One cursor per repo strands comments on every other subject — `poller.rb:158`. Same root as + critical 5; likely fixed by the same change. +- A stale board write that lands late is stamped unrecoverable — `board.rb:78`. +- The `running?` guard does not stop two walkers — `poller.rb:108`. +- A refused launch destroys the stage's session id — `runner.rb:129`. The `@sessions` assignment + runs before the `case`. Tests miss it because `scripted` hardcodes `session: 'sess-1'`. +- `reload` restores sessions the runner deliberately discarded, and discards an interrupted stage's + — `runner.rb:67`. +- A slow `git worktree add` inside `claim`'s transaction fails healthy concurrent runs — + `supervisor.rb:59`, `runner.rb:94`. Measured: `second write RAISED after 6.2s: BusyException`. +- A chmod-drifted secrets file raises out of `Repo.prepare` and stops the whole poller — + `repo.rb:81`. `prepare` rescues `Mill::Git::Error`; `check_mode!` raises its parent `Mill::Error`. + The method's own comment claims this cannot happen. +- `.mill.yml` falls back to a local ref a stage can move — `repo.rb:102`. + +## MEDIUM + +- A stage that already succeeded is charged an interruption and re-run — `supervisor.rb:190`. +- `clear_stale_locks` deletes locks belonging to a live git process — `supervisor.rb:283`. Also + `Dir[]` treats `[`, `{`, `*` in the path as glob syntax, so such a clone clears nothing silently. +- A failed teardown wedges the branch with nothing to retry it — `supervisor.rb:104`. +- The second half of a two-part answer is silently discarded — `poller.rb:107`. +- `interference?` has no production callers — `board.rb:51`. The design doc, the failure taxonomy + and the runbook all say mill reports a Status it did not write. It does not. +- An unrecoverable board failure retries forever with no log line — `board.rb:80`. +- An event skipped by `running?` is skipped forever with nothing recorded — `poller.rb:108`. +- `INSERT OR IGNORE` swallows every constraint violation while the cursor advances — `poller.rb:171`. +- `@rate_limit_waits` is a per-run budget spent by every stage, and reset by any restart — + `runner.rb:162`. +- Nothing can raise from the `rescue` clause without killing the loop for good — `workers.rb:101`. +- A hung `gh` parks a loop forever and `health` calls it alive — `workers.rb:72`. +- `health` cannot distinguish "workers off" from "both threads died" — `workers.rb:53`. +- `stop` is dead code, and would not stop the run threads if called — `config.ru:3`. +- `base_branch` truncates any default branch containing a slash — `repo.rb:93`. +- A missing `origin/HEAD` silently becomes `main` — `repo.rb:94`. +- `missing_secrets` accepts a key with an empty value — `repo.rb:121`. +- A leftover directory at the clone target wedges a repo permanently — `repo.rb:41`. +- Doctor never checks `Ready`, the one Status the queue depends on — `doctor.rb:254`. +- `read_config` ignores whether the fetch worked, and caches the result forever — `repo.rb:101`. + +## LOW + +- The "waiting" notice is suppressed for the life of the process — `supervisor.rb:249`. +- The cursor stores `created_at` but `since` filters `updated_at` — `poller.rb:154`. +- An item with no repository or number is dropped with no trace — `poller.rb:196`. +- `no_route` is a fourth event state the schema does not document — `poller.rb:126`. +- `want` writes a decision an unconfigured board can never act on — `board.rb:37`. +- `MILL_BIND=` binds nowhere — `config/puma.rb:4`. +- The rate-limit block message is off by one and names the wrong stage — `runner.rb:163`. +- `require './app'` creates `~/.mill` and opens the database — `app.rb:20`. +- A trailing slash on `origin` makes a clone invisible — `repo.rb:31`. +- `rate_limit_pause` calls `.to_i` on an unvalidated field — `runner.rb:183`. + +## Open design questions + +Cases where the obvious fix quietly decides something nobody has decided. Each should be settled +deliberately and pinned with a test, whichever way it goes. + +- ~~An attempt with `success: true`, `rate_limited: true` and an **invalid** verdict.~~ **Settled + 2026-08-21: it stays free.** Charging it a strike needs a premise nothing has measured — that a + refusal exits non-zero — and charging on an unproven premise is exactly how a stage gets blocked + for a door mill could not open, which is the bug item 2 fixed. Revisit only with a recorded + refusal transcript. Note this is the same state as item 2b, so pricing it correctly and fixing 2b + are one job, not two. +- Lowering `MILL_CONCURRENCY` from 2 to 1 while two runs are live leaves both rows `running` at cap + 1. Today neither restarts. Removing the `at_cap?` guard restarts both, giving two walkers at cap + 1. Counting everyone-but-me deadlocks again. There is no obviously right answer. + +## Suggested session order + +Roughly fourteen sessions at one root cause each. The first four are ordered so that each one makes +the next easier to see. + +1. ~~CI git identity~~ — done 2026-08-20. Restores signal, not a green check; the badge + stays red until 2–10 land. Left behind: mill's own clones still carry no identity. +2. ~~Rate-limit misclassification (critical 1)~~ — done 2026-08-21. `classify` now asks the verdict + rather than the flag, so a throttled stage that finished keeps its work. **Half of critical 1 + remains and is now item 2b.** +2b. A launch that ran, hit the window partway and handed back nothing is still priced as "no + launch": no row, so the relaunch truncates its log and `reload` cannot recover its session. + Telling a refusal from a cut-off launch needs the stream — a session id, a model, any turns — + and pricing a launch that happened as an attempt costing no strike. + `test_a_throttled_stage_that_did_work_and_said_nothing_is_priced_as_a_refusal` pins the bad + behaviour on purpose and should be deleted by whoever fixes this. +3. `interrupt` raising on a NULL `current_stage` (critical 3) — a dead reaper hides everything else, + and it is the failure mode that the `claim` orphan and the failed-start bug both feed. +4. `start`/`reap` race (critical 4) — check whether it also frees `restore` and the strike reset. +5. `reap` discarding `Spawn.reap`'s answer (critical 2). +6. Comment cursor scoping (critical 5) — likely also fixes the two cursor HIGHs. +7. Unstartable items re-commented forever (critical 6). +8. `announce_spawn` orphan — test already written and red, fix already verified. +9. `restart`'s `at_cap?` guard — test already written and red, fix already verified. +10. Failed start losing the answer — test already written and red, both fixes verified. +11–14. The remaining blocking HIGHs: the two `workers.rb` lifecycle bugs, `redrive` killing the + tick, doctor's public-bind check, `claim`'s orphaned row, `finish` raising, `Repo.slug`. +15. mill's commit identity — decided but not built; see blocker zero above. Last because it is the + only item here that is not a bug in shipped behaviour, and mill works on a laptop without it. + +Merging before at least 1–10 means merging something that is not safe to run unattended: +`Workers.enabled?` defaults to on, so a stray `Ready` on the board reaches every critical path +above. If the branch needs to land sooner, the board-write and repo-preparation work is largely +independent of the poller and supervisor and could be split out first. + +## What this says about the process + +Several of these were **in the previous day's fixes**, not in the original code. + +`resumed` writing `running` before the thread registers was added to fix a resumed run being +invisible to the reaper. It created the two-walker race, and separately made `restore` and the +one-per-run strike reset unreachable on every path the poller drives. The `:rate_limited` +classification was added to stop a subscription limit taking a strike, and became the most-cited bug +in the set. + +That is the same lesson the design doc already records from the day before, arriving again one layer +down: **a fix is new code and deserves the same suspicion as the code it replaces.** The earlier +version was "reviews catch the layer they are looking at". This one is narrower and worse — the +fixes themselves were never reviewed, because they were made in response to a review and felt like +conclusions rather than like changes. + +The 2026-08-20 session added a third turn of the same screw. The first rewrite of the failed-start +test baked the bug's current location into the test: it would have stayed red under the natural +supervisor-side fix, and a later session would have concluded a correct fix had not worked. A fresh +reviewer caught it by applying each candidate fix and running the test. Reviewing a *test* is worth +as much as reviewing the code, and the check that matters is not "does it fail now" but "would it +pass once this is genuinely fixed". diff --git a/docs/reference/admin-ui-frontend.md b/docs/notes/admin-ui-frontend.md similarity index 100% rename from docs/reference/admin-ui-frontend.md rename to docs/notes/admin-ui-frontend.md diff --git a/docs/reference/mill.md b/docs/reference/mill.md index 291f736..847d2af 100644 --- a/docs/reference/mill.md +++ b/docs/reference/mill.md @@ -6,6 +6,13 @@ failure taxonomy, and scope decisions live in First-time setup is a separate runbook: [setup.md](setup.md). +**Some of this vocabulary describes work that is not built yet**, and those passages are marked +**(not built)** inline rather than removed — the words are what you will want when they land, and a +reference that quietly omits them is harder to read than one that says which is which. The single +inventory of what exists is the design doc's +[Where this stands](../superpowers/specs/2026-08-06-software-factory-design.md#where-this-stands); +this file defers to it and does not keep a second list. + ## Contents - [The board](#the-board) @@ -35,8 +42,11 @@ single-select with one option: set or unset. | Field | Option | Meaning | |---|---|---| -| `Evidence` | `Required` | The PR must include a before/after sample of real output | -| `Review` | `Deep` | Faceted fan-out plus refutation instead of a single reviewer | +| `Evidence` | `Required` | The PR must include a before/after sample of real output **(not built)** | +| `Review` | `Deep` | Faceted fan-out plus refutation instead of a single reviewer **(not built)** | + +Both fields exist on the board and doctor checks for them. mill reads neither yet, so setting one +changes nothing today. Status is state and belongs to mill; the other two are directives and belong to you. Don't hand-edit Status to steer a run — set it to `Ready` to release work, and use the kill switch @@ -53,21 +63,25 @@ built-in workflows must stay disabled, because they write Status too — `mill:d **To answer a blocked run, just reply in a comment.** While an item is `Blocked`, every comment on it is read as an answer. No marker, no syntax. -**To ask mill to change something on a PR it opened, start the comment with `mill:`.** Anything -after the marker is the instruction, and a comment without it is ignored, so ordinary conversation -on a mill PR costs nothing: +**To ask mill to change something on a PR it opened, start the comment with `mill:`. (not built)** +Anything after the marker is the instruction, and a comment without it is ignored, so ordinary +conversation on a mill PR costs nothing: ``` mill: the null check in Session#expire is in the wrong branch ``` Two things need no marker. A **PR review comment** is already a request for a change, and a **red -required check** is a fact — mill acts on both by itself. It gives up after two fix runs against -the same failing commit and says so on the PR. +required check** is a fact — mill will act on both by itself, giving up after two fix runs against +the same failing commit and saying so on the PR. **(not built)** + +All three of those need the `iterate` route, which has no prompts yet. Today mill sweeps such +comments, recognises them, records them with state `no_route`, and logs that it had nowhere to send +them. Answering a blocked run is the one comment trigger that works. **Before you set Status to `Ready`, switch your clone off the branch.** git refuses to check a -branch out in two places, so a branch left current in `~/code/` blocks the item until you -move off it. +branch out in two places, so a branch left current in the clone mill resolves blocks the item until +you move off it. mill names the clone and the branch in the comment. ## Releasing work @@ -88,22 +102,26 @@ the code in one diff. order, each after its predecessor's PR merges — mill does not stack branches. The size test and the rest of the spec checklist: [spec-standard.md](spec-standard.md). -For a crash or a one-line fix, skip steps 1 and 2 — set Status to `Ready` on an issue with no -linked branch and triage will route it to the fast path. An issue with neither a spec nor a -hotfix shape will block and ask you to think it through. +**(not built)** For a crash or a one-line fix, skip steps 1 and 2 — set Status to `Ready` on an +issue with no linked branch and triage will route it to the fast path. An issue with neither a spec +nor a hotfix shape will block and ask you to think it through. + +Until the `fast` route has prompts, `plan` is the only route mill claims. An item with no linked +branch gets a comment telling you to run `gh issue develop` and commit a spec — so the shortcut +above does the opposite of what it says today. Follow steps 1 to 3 for everything. ## Key models -- **Repo**: a repository mill has prepared — resolved local clone path, git config applied, `.mill.yml` parsed from the base branch. Prepared lazily on first touch; not a watchlist. The repo allowlist is the stage token's selected-repositories list. +- **Repo**: a repository mill has prepared — a working copy, git config applied, `.mill.yml` parsed from the base branch, and the secrets it names confirmed present. Prepared lazily on first touch; not a watchlist. The repo allowlist is the stage token's selected-repositories list. **Finding the working copy**: mill scans the directories in `MILL_CLONES` for one whose `origin` matches — defaulting to `~/code` on macOS and to nothing on a server — and clones into `~/.mill/clones/-` when it finds none. Two matches block the item rather than choosing, because the choice commits the whole run to a checkout you did not pick. - **Subject**: the thing a run is about — an issue or a pull request, as `subject_kind` plus `subject_number`. PR-entry runs have no issue. - **Run**: one subject moving through the pipeline on one branch, in one worktree -- **Route**: `plan` (a spec exists — plan, review, implement, review, PR), `fast` (no spec, hotfix-shaped — diagnose, implement, review, PR), or `iterate` (entry from a PR trigger, on the existing branch) +- **Route**: `plan` (a spec exists — plan, review, implement, review, PR), `fast` (no spec, hotfix-shaped — diagnose, implement, review, PR) **(not built)**, or `iterate` (entry from a PR trigger, on the existing branch) **(not built)**. All three exist as data in the stage graph; only `plan` has prompts, and only `plan` is ever claimed. - **Spec**: the design you wrote, found as the file the linked branch adds under `docs/superpowers/specs/`. Exactly one is the spec; more than one blocks; none routes to `fast` if triage judges the issue hotfix-shaped, otherwise blocks. - **Stage**: a node in the graph; one `claude -p` process group with a fixed model, a named skill, and its own permission ruleset. Most stages borrow a Superpowers skill unchanged; `implement` and `pr` use mill's own `mill:implement` and `mill:pr`, because the Superpowers equivalents assume a human at a terminal and would open the PR early or offer to merge. - **Attempt**: one execution of a stage. mill counts two things about them. The **attempt number** goes up on every launch and names the log and verdict. The **strike count** goes up only when the work was judged bad — a crash, a failure, an unusable verdict, or a serious objection — and two strikes blocks the run. Anything the machine did to a stage costs an attempt and no strike. Answering an exhaustion block resets that stage's strikes once. - **Verdict**: the structured output a stage ends with, its shape constrained by `--json-schema` so the CLI returns it already parsed rather than as text a stage could wrap in prose. mill validates it and records it in `stage_attempts.verdict_json`; no stage writes it anywhere. Must carry the stage, attempt, and nonce mill passed in — the schema cannot know which launch this is, so that check stays mill's. Status is `ok`, `blocked`, or `failed`. - **Objection**: a reviewer finding with a severity. `high` or `critical` re-runs the reviewed stage; lower severities land in the PR body. -- **Event**: a comment occurrence the poller has seen, keyed on node id, with a retry count and a terminal `dead` state. Board status is *not* an event — it is reconciled as state. +- **Event**: a comment occurrence the poller has seen, keyed on node id, with a retry count and two terminal states — `dead` when handling it kept raising, and `no_route` when mill recognised it but has nowhere to send it yet. Board status is *not* an event — it is reconciled as state. ## Identifier types @@ -115,9 +133,15 @@ expect. - **Node ids** (`gh_node_id`) are opaque strings — never parse, order, or do arithmetic on them. They are the dedupe key for comment events precisely because they are stable and unique across the whole of GitHub. +- **A project has both a number and a node id, and mill wants the number.** `MILL_PROJECT` is the + small integer in the project's URL; `gh project view --owner ` and the workflow + query mill runs both take it. The node id (`PVT_…`) names the same project and is not + interchangeable — you cannot derive one from the other, and passing a node id gets you a 404 with + nothing to suggest which of the two was wanted. To go from one to the other: + `gh api graphql -f query='query($id: ID!){ node(id: $id){ ... on ProjectV2 { number } } }' -f id=PVT_…` - **Project item ids, field ids, and option ids** are three distinct opaque strings, all required - to set a Status. mill resolves them at bootstrap and caches them; never hardcode one, and never - assume you can derive an item id from the issue it wraps. + to set a Status. mill resolves them the first time it writes and memoises them for that process; + never hardcode one, and never assume you can derive an item id from the issue it wraps. - **Session ids** from Claude Code are opaque strings, and the session file behind one may vanish. Any code path that resumes a session must have a fallback that re-runs the stage from scratch. - **mill run ids** are local integers and mean nothing outside this database. Never put one in a diff --git a/docs/reference/setup.md b/docs/reference/setup.md index 317d0d5..7d566dd 100644 --- a/docs/reference/setup.md +++ b/docs/reference/setup.md @@ -69,17 +69,38 @@ default `Status` field whose options are `Todo` / `In Progress` / `Done`, which gh project field-list --owner @me --format json ``` -If a `Status` field exists with the wrong options, delete and recreate it — there is no command -to edit an existing single-select's options: +A new project arrives with a built-in `Status` whose options are `Todo` / `In Progress` / `Done`. +**You cannot delete it and you cannot recreate it** — measured 2026-08-19, GitHub answers +`Only custom fields can be deleted` to the first and `Name cannot have a reserved value` to the +second. `gh project` has no command to edit a single-select's options either. + +Replace them in place with `updateProjectV2Field`, which takes the whole option list and swaps it. +Take the field id from the `field-list` output above: ``` -gh project field-delete --id +cat > /tmp/status.json <<'JSON' +{ + "query": "mutation($fieldId: ID!, $options: [ProjectV2SingleSelectFieldOptionInput!]) { updateProjectV2Field(input: {fieldId: $fieldId, singleSelectOptions: $options}) { projectV2Field { ... on ProjectV2SingleSelectField { options { name } } } } }", + "variables": { + "fieldId": "", + "options": [ + {"name": "Ready", "color": "BLUE", "description": "Released to the factory"}, + {"name": "Running", "color": "YELLOW", "description": "A run has claimed it"}, + {"name": "Blocked", "color": "ORANGE", "description": "Stopped for input; reply in a comment"}, + {"name": "Done", "color": "GREEN", "description": "PR opened"}, + {"name": "Failed", "color": "RED", "description": "Terminal without a PR"} + ] + } +} +JSON -gh project field-create --owner @me --name Status \ - --data-type SINGLE_SELECT \ - --single-select-options "Ready,Running,Blocked,Done,Failed" +gh api graphql --input /tmp/status.json ``` +`name`, `color` and `description` are all required. Any option you leave out of that list is +removed from the field, along with its value on every item — do this before the board has items, +or list the options you are keeping alongside the new ones. + Then the two directive fields. Projects v2 has no boolean field type, so each is a single-select with one option — set or unset: @@ -115,9 +136,14 @@ mill uses no labels, so there is nothing to create in any repository. **mill is the sole writer of the Status field.** Projects v2 ships automation that also writes it, and a new project may arrive with some of it enabled. -In the project's **Workflows** settings, turn off every built-in workflow — including "Item -closed", "Item reopened", "Pull request merged", "Code review approved", "Auto-add to project", -and "Auto-archive items". +In the project's **Workflows** settings at +`https://github.com/users//projects//workflows`, turn off every built-in workflow. +A default project ships six enabled: "Item closed", "Pull request merged", "Auto-close issue", +"Auto-add sub-issues to project", "Pull request linked to issue", and "Item added to project". + +**This is a browser step and cannot be scripted.** The API exposes `enabled` for reading — which +is how doctor checks it — but there is no mutation to turn one off. `deleteProjectV2Workflow` +exists and is not the same thing; do not reach for it on a built-in. Two are actively harmful rather than merely redundant: @@ -331,7 +357,15 @@ It checks, and names anything missing: catches a workflow re-enabled later - the stage token exists, is readable only by you, is unexpired, and has exactly the two expected permissions -- `~/.mill` and `~/.mill/secrets` are `0700` +- `~/.mill` and `~/.mill/secrets` are `0700`, and every file inside `secrets/` is `0600` — these + values reach a subprocess environment, and a mode that has drifted is otherwise silent +- every directory named in `MILL_CLONES` exists. A root that does not silently becomes "clone it + myself" for every repo, and mill then works in a checkout you are not looking at +- `MILL_ADMIN_EMAILS` is non-empty whenever `MILL_BIND` is anything but loopback. The write paths + are a kill switch and a worktree deleter, and the log endpoint streams repo contents +- the board's `Status` field carries every option mill writes — `Running`, `Blocked`, `Done` and + `Failed`. A missing one fails at the moment it matters, when a run blocks or finishes, rather + than at setup - the permission ruleset files in `~/.mill/settings/` exist and carry every deny rule the design doc requires, **with no absolute paths and no `Write(...)` rules**, and put no confinement in an `allow` list — each of those three is accepted silently and enforces nothing diff --git a/docs/superpowers/specs/2026-08-06-software-factory-design.md b/docs/superpowers/specs/2026-08-06-software-factory-design.md index e770a55..ad87534 100644 --- a/docs/superpowers/specs/2026-08-06-software-factory-design.md +++ b/docs/superpowers/specs/2026-08-06-software-factory-design.md @@ -137,14 +137,14 @@ Plans 1 and 2 are complete, and a real pull request came out of the far end on 2 | `Mill::Rules`, `Mill::Doctor` | Built. Rulesets written from one definition and checked against it | | Stage prompts, `mill:implement`, `mill:pr`, `mill-headless` | Built for the `plan` route only | | The `plan` route, end to end | **Demonstrated.** `slowernet/mill-scratch#2`, 18 minutes, no strikes | -| `Mill::Poller` | Not built. No board reads, no comment cursors, no triggers | -| `Mill::Supervisor` | Not built. No repo preparation, worktree lifecycle, concurrency cap, lock clearing, or reaping | +| `Mill::Poller` | Built. Reconciles the board, sweeps comments behind a transactional cursor sent to GitHub as `since`, dispatches an answer to a blocked run. Two of five triggers dispatch; the rest record `no_route` | +| `Mill::Supervisor` | Built, minus the power assertion. Prepares repos, resolves or makes the clone, claims to a cap, clears stale locks, walks each run in its own thread, tears down, and reaps against a verified identity | | Sleep and wake | Clock pair built and its premise measured; nothing reads it. No settle window, no stall detector, no power assertion | | The Linux server, which is the primary target | **Never run.** Every line of mill has only ever executed on macOS, including all fourteen boundary tests | -| Web UI | Not built. No routes, no kill switch, no log view | +| Web UI | Boot path only. `app.rb`, `config.ru` and `config/puma.rb` exist and `GET /` reports worker health; no run list, kill switch or log view | | `fast` and `iterate` routes | Not built. `diagnose`, `implement:fast` and `push` have config and rulesets but no prompts, and have never run | -| Board writes, comments | `Mill::Github#comment` exists; nothing calls it. mill has never written a Status | -| Secrets injection, scoped `GH_TOKEN` | Not built. `Mill::Rules.env_for` is the hook and carries one variable | +| Board writes, comments | Built. Status on claim, block, resume and finish, re-driven from `desired_board_status` when a write did not land. Questions, block reasons and outcomes post to the subject | +| Secrets injection, scoped `GH_TOKEN` | Built, never exercised against a repo that declares any — `mill-scratch` sets `secrets: []`. Values under 16 characters reach the stage but are never redacted, because the scrubber would corrupt the log | | Deep review, evidence requirement, retention, CI-fix trigger | Not built. `ci_fixes` and `events` exist as tables and are unused | **Built but never exercised**, which is a different thing from built: @@ -798,7 +798,22 @@ is exactly the truth. | `--resume` failed, so mill started fresh with the context appended | +1 | none | | mill restarted and interrupted it | +1 | none | | A stale git lock was cleared before it ran | n/a | none | -| It is waiting behind a rate limit | no launch | none | +| It is waiting behind a rate limit, and handed back no verdict | no launch | none | +| It was throttled but handed back a verdict anyway | +1 | priced on the verdict | + +**The verdict decides whether a limit refused the launch, not the exit status.** The rate-limit +flag says only what the last such event in the stream was, and an "allowed" heartbeat clears it — +so a stage throttled early that then gets its launch and finishes still carries the flag. Reading +the flag alone discarded that finished work, and because the free outcome inserts no row the +relaunch reused the log filename and destroyed the successful run's log. Exit status cannot stand +in for the verdict either: nothing has yet measured what a refused launch exits with, and the one +refusal mill has measured — a session the CLI would not reopen — is reported in-band. + +This leaves one case knowingly mispriced. A launch that ran, hit the window partway and handed +back nothing is indistinguishable here from one refused outright, so it is priced as "no launch": +it loses its log to the relaunch, and its session with it. Separating the two needs the stream +rather than the ledger — a session id, a model, any turns at all — and pricing a launch that did +happen as an attempt that cost no strike. The rule behind the table: **a strike means the work was wrong. Everything the machine did to a stage is free.** A laptop that slept, a socket that died, a lock file left by a SIGKILL, and mill @@ -1184,9 +1199,10 @@ supervisor prepares it on first touch: of yours is never touched beyond local git config. 2. **Set `gc.auto=0` and `maintenance.auto=0`** so a stage's commit cannot trigger a gc that rewrites shared refs while other runs hold them. -3. **Read `.mill.yml`** from the base branch into `repos.config_json`: base branch, test +3. **Set the commit identity**, without which a stage cannot commit at all. See below. +4. **Read `.mill.yml`** from the base branch into `repos.config_json`: base branch, test command, gating CI workflow, trusted PR authors, `evidence_public`, secret variable names. -4. **Verify** the token covers the repo and `~/.mill/secrets/-.env` exists. +5. **Verify** the token covers the repo and `~/.mill/secrets/-.env` exists. If anything is missing, mill blocks **that item** and comments naming exactly what. mill writes nothing to the repo — it uses no labels — so it only reads, apart from setting local git @@ -1196,6 +1212,23 @@ mill reads `.mill.yml` only from the base branch, never from the worktree HEAD, resolved config onto the run. An agent can edit `.mill.yml` in its worktree; that edit must not weaken the next run. +**mill's commits say a machine wrote them.** A stage runs `git commit` inside a worktree of +mill's clone, and `git clone` copies no config, so a clone starts with no identity of its own. +The identity is mill's own setting rather than a `.mill.yml` key: that file lives in the repo +being worked on, and whoever can commit to its base branch would otherwise choose the name your +credentials push under. + +You are the **author** and mill is the **committer**. `git blame` keeps pointing at the person +who wanted the change, while `git log` and GitHub both show that a machine made the commit. The +committer is named `mill` and uses the author's address, so the commit still links to the +account answerable for it instead of showing as an unrecognised stranger. mill sets this through +`GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL` in the stage environment, beside the secrets. + +With no identity configured the author falls back to the machine's own git config, which is why +a laptop needs nothing set. A server with no `~/.gitconfig` has nothing to fall back to, so +`rake mill:doctor` fails when no identity resolves — otherwise the first run dies part-way +through `implement` and the stage is charged a strike for something the machine did to it. + **mill injects secrets.** A fresh worktree holds tracked files only, so `.env` and `config/master.key` are missing, and a suite that needs them would fail identically on both attempts — so the pipeline could never finish on an ordinary Rails or Node repo. mill reads @@ -1206,8 +1239,8 @@ into the worktree, and keeps those values out of the tee'd log. `project` scope; the board's three fields and their options; that you disabled the built-in workflows; the stage token's permissions, expiry, and file mode; `~/.mill` modes; the permission rulesets' deny rules; and, for every repo the board currently references, that it can resolve -the clone, that `gc.auto` is set, that `.mill.yml` parses, that branch protection requires -checks, and that the named secret variables exist. Most of what it checks is critical to +the clone, that `gc.auto` is set, that a commit identity resolves, that `.mill.yml` parses, that +branch protection requires checks, and that the named secret variables exist. Most of what it checks is critical to containment, so a red doctor blocks everything. **Off switch:** remove items from the board, or drop the repo from the token's repository list. @@ -1411,7 +1444,7 @@ Over time these numbers establish what each stage normally uses, so an unusual r and when mill adds per-token billing the history is already there. Front-end conventions — the layout contract, design tokens, the component catalog, and how the -log tail polls — are in `docs/reference/admin-ui-frontend.md`. +log tail polls — are in `docs/notes/admin-ui-frontend.md`. ## Killing a run and tearing it down @@ -2073,7 +2106,7 @@ evasion of a permission control; containment held on the honour system as well a ### Plan 3a — Autonomy -**Not started.** The clock pair exists and nothing reads it. +**Done, 2026-08-19.** See the rehearsal record below. - `Mill::Workers` and the Roda host: `app.rb`, `config.ru`, both threads under one supervising loop that restarts either with backoff, `GET /` reporting whether both heartbeats are fresh, @@ -2094,6 +2127,44 @@ Only two of the five triggers dispatch, because only the `plan` route exists: an `Ready` with no active run, and a comment on a `Blocked` item. The sweep itself is built in full, so Plan 5 adds dispatch and touches none of it. +**Done, 2026-08-19.** Two pull requests nobody opened by hand: `mill-scratch#4` from a clean run, +and `#6` from a run that blocked at `plan`, asked three questions, took an answer from a comment and +resumed its own session. Zero strikes on either. Then a crash test: mill killed outright mid-stage, +twice, each time leaving a live process group orphaned — recovered both times, charging an attempt +and no strike, and re-entering the stage it was in rather than the top of the route. And nineteen +and a half hours running unattended overnight, surviving eight transient API failures with both +threads alive in the morning and about 12% of the GraphQL budget consumed per hour at a 30-second +tick, which is why the default tick is now 60. + +**What the rehearsal cost, and what it bought.** Ten defects, six of them in code written that day, +and the fixture suite could not have caught any of the six. That is the finding worth keeping. + +Four of the six were the same shape: **each component correct alone, each tested alone, the defect +in the handoff.** `Mill::Workers` assembled a supervisor without a board, so mill would have run the +whole pipeline and never written a Status — and since a comment only means an answer while the board +says `Blocked`, no blocked run could ever have been resumed. `Supervisor#walk` returned a database +row where `finish` expected the runner's state, so the first real block posted ``Blocked at ``: .`` +to the issue: mill asked three good questions and threw all of them away, which is worse than a +crash, because the board says `Blocked`, the worktree waits, and there is no way to learn what for. +Nothing owned the `blocked → running` transition, so a resumed run stayed invisible to the reaper +for the rest of its route. And a restarted run began at the top of its route, re-running every stage +it had banked. + +The other two: doctor passed a database three migrations behind, because it checked that tables +existed and the missing thing was a column; and the log scrubber would have corrupted the +`stream-json` it parses back, given a secrets file with a short value like `DEBUG=true`. + +Two runbook steps also turned out not to work as written — the built-in `Status` field can be +neither deleted nor recreated, and disabling the board's workflows is the one setup step with no +API at all. + +**The lesson for the next plan.** An adversarial review of this plan's code, before a line of it +existed, found twelve defects including four that would have stopped the factory silently. It +caught that two `Mill::Supervisor` instances would make the reaper kill healthy stages. The fix was +to share one instance — and the shared instance was built without a board, which is finding 6. +Reviews catch the layer they are looking at. Only running the assembled thing catches the seam +below it. + ### Plan 3b — Resilience **Not started.** Depends on 3a having run unattended for long enough to have opinions. diff --git a/lib/mill.rb b/lib/mill.rb index 98ff094..cb69081 100644 --- a/lib/mill.rb +++ b/lib/mill.rb @@ -32,6 +32,32 @@ def self.now Time.now.utc.to_i end + # Settings are parsed, never coerced. `'lots'.to_i` is 0, and a concurrency cap + # of 0 makes mill think it is always at capacity: it claims nothing, forever, + # with every check still green. A rejected value falls back and says so, so the + # mistake shows up in the log rather than in the absence of work. + def self.setting_int(name, default:, min:, max:) + setting(name, default: default, min: min, max: max) { |raw| Integer(raw, 10) } + end + + def self.setting_float(name, default:, min:, max:) + setting(name, default: default.to_f, min: min, max: max) { |raw| Float(raw) } + end + + def self.setting(name, default:, min:, max:) + raw = ENV[name] + return default if raw.nil? || raw.strip.empty? + + value = yield(raw.strip) + return value if value >= min && value <= max + + warn "#{name}=#{raw} is outside #{min}..#{max}; using #{default}" + default + rescue ArgumentError, TypeError + warn "#{name}=#{raw} is not a number; using #{default}" + default + end + # Text mill did not write — gh output, git output, a stage's stdout — is UTF-8 # whatever the locale claims. A byte that is genuinely undecodable is dropped # rather than losing the payload it sits in. @@ -47,10 +73,15 @@ def self.utf8(text) require_relative 'mill/verdict' require_relative 'mill/spawn' require_relative 'mill/stages' +require_relative 'mill/secrets' require_relative 'mill/rules' require_relative 'mill/skills' require_relative 'mill/github' +require_relative 'mill/board' require_relative 'mill/git' +require_relative 'mill/repo' +require_relative 'mill/supervisor' +require_relative 'mill/poller' require_relative 'mill/spec' require_relative 'mill/ledger' require_relative 'mill/prompts' @@ -58,3 +89,4 @@ def self.utf8(text) require_relative 'mill/run' require_relative 'mill/claude' require_relative 'mill/doctor' +require_relative 'mill/workers' diff --git a/lib/mill/board.rb b/lib/mill/board.rb new file mode 100644 index 0000000..bca4264 --- /dev/null +++ b/lib/mill/board.rb @@ -0,0 +1,104 @@ +module Mill + # The writing side of the board, and the only thing that decides what Status + # an item should carry. + # + # Every write is a network call that can fail, and nothing else re-drives one: + # the poller only ever asks which items are Ready. So mill records what it + # decided the board should say and when it last confirmed it, and a write that + # never landed is retried until it does. Without that, a run that blocks while + # the network is down shows Running forever — and because a comment's meaning + # depends on Status, the answer to its questions is never read as an answer. + class Board + STATUS = { + 'running' => 'Running', + 'blocked' => 'Blocked', + 'done' => 'Done', + 'failed' => 'Failed', + 'killed' => 'Failed' + }.freeze + + def initialize(db: Mill.db, github: nil, project: ENV['MILL_PROJECT'], + owner: ENV['MILL_PROJECT_OWNER']) + @db = db + @github = github || Mill::Github.new + @project = project + @owner = owner + end + + def configured? = !@project.to_s.empty? && !@owner.to_s.empty? + + def items = @github.board_items(@project, owner: @owner) + + # Records the decision first, then tries to make it true. The order + # matters: a crash between the two leaves a decision redrive can act on, + # where the reverse leaves a board mill believes it has already fixed. + def want(run_id, status) + label = STATUS.fetch(status.to_s) { raise Mill::Error, "no board status for #{status}" } + @db[:runs].where(id: run_id).update(desired_board_status: label, board_status_at: nil) + confirm(run_id) + end + + def redrive + return unless configured? + + @db[:runs].exclude(desired_board_status: nil).where(board_status_at: nil) + .select_map(:id).each { |id| confirm(id) } + end + + # True when the board says something mill did not put there, under a run + # mill owns. That is a built-in workflow re-enabled after setup, and obeying + # it would flip Status out from under a live subprocess. + def interference?(item, run_row) + return false if run_row[:desired_board_status].nil? || run_row[:board_status_at].nil? + return false unless item[:id] == run_row[:board_item_id] + + item[:status].to_s != run_row[:desired_board_status] + end + + private + + # The label is re-read here rather than passed in, and the stamp is + # conditional on it not having changed. redrive runs in the poller thread + # while run threads call `want`, so a label read a moment ago may already be + # stale — and stamping board_status_at against a stale label is worse than + # not writing at all, because that stamp is the only thing that would have + # caused a retry. + def confirm(run_id) + return false unless configured? + + row = @db[:runs].where(id: run_id).first + return false if row.nil? || row[:board_item_id].nil? || row[:desired_board_status].nil? + + label = row[:desired_board_status] + option = ids[:options][label] or + raise Mill::Error, "the project's Status field has no `#{label}` option" + + @github.set_status(project_id: ids[:project], item_id: row[:board_item_id], + field_id: ids[:field], option_id: option) + @db[:runs].where(id: run_id, desired_board_status: label) + .update(board_status_at: Mill.now).positive? + rescue Mill::Github::Error + # Deliberately swallowed and deliberately not recorded as confirmed: the + # unset board_status_at is the retry, and redrive is what performs it. + # Only unreachability is swallowed. A board that is wrong rather than + # unreachable is a configuration error and must not retry forever. + false + end + + def ids + @ids ||= begin + status = @github.project_fields(@project, owner: @owner) + .find { |field| field[:name] == 'Status' } or + raise Mill::Error, 'the project has no Status field' + + options = status.fetch(:options, []).to_h { |o| [o[:name], o[:id]] } + missing = STATUS.values.uniq - options.keys + raise Mill::Error, "the project's Status field is missing #{missing.join(', ')}" if + missing.any? + + { project: @github.project_id(@project, owner: @owner), field: status[:id], + options: options } + end + end + end +end diff --git a/lib/mill/claude.rb b/lib/mill/claude.rb index 5dc0cc6..129270d 100644 --- a/lib/mill/claude.rb +++ b/lib/mill/claude.rb @@ -23,6 +23,8 @@ def blocked? = verdict.valid? && verdict.blocked? def rejects? = verdict.valid? && verdict.rejects? def session_id = result.stream.session_id def resume_failed? = result.stream.resume_failed? + def rate_limited? = result.stream.rate_limited? + def rate_limit_resets_at = result.stream.rate_limit_resets_at def tokens = result.stream.tokens def model = result.stream.model def log_path = result.log_path @@ -82,9 +84,11 @@ def argv(prompt, session_id: nil) # # `worktree` is both the stage's working directory (layer 1's real # filesystem boundary) and the root the artifact must resolve inside. - def run(prompt, number:, worktree:, log_path:, session_id: nil, env: {}, secrets: []) + def run(prompt, number:, worktree:, log_path:, session_id: nil, env: {}, secrets: [], + on_spawn: nil) nonce = self.class.nonce - spawn = Mill::Spawn.new(log_path: log_path, chdir: worktree, secrets: secrets) + spawn = Mill::Spawn.new(log_path: log_path, chdir: worktree, secrets: secrets, + on_spawn: on_spawn) result = spawn.run(argv(envelope(prompt, number, nonce), session_id: session_id), env: env) Attempt.new(stage: stage, number: number, nonce: nonce, result: result, diff --git a/lib/mill/doctor.rb b/lib/mill/doctor.rb index 022280e..cf4c576 100644 --- a/lib/mill/doctor.rb +++ b/lib/mill/doctor.rb @@ -27,7 +27,11 @@ def run check_argv_invariants check_skills check_schema + check_secret_modes + check_clone_roots + check_bind check_board + check_board_options @ran = true self end @@ -159,10 +163,27 @@ def check_skills end end + # Tables alone are not enough. A database several migrations behind has + # every table and is still missing columns mill writes — and it fails at the + # moment a run is claimed, deep inside a worker thread, rather than here. + # Measured 2026-08-19: the first real poll against a stale database raised + # `table runs has no column named board_item_id` on every tick, and doctor + # had reported the schema green. def check_schema db = Mill.db missing = %i[repos runs stage_attempts ci_fixes events] - db.tables - missing.empty? ? pass('schema') : fail('schema', "missing tables: #{missing.join(', ')}") + return fail('schema', "missing tables: #{missing.join(', ')}") if missing.any? + + latest = Dir[File.join(Mill::DB::MIGRATIONS, '*.rb')] + .map { |path| File.basename(path).to_i }.max + applied = db[:schema_info].get(:version).to_i + + if applied >= latest + pass('schema', "migration #{applied}") + else + fail('schema', "database is at migration #{applied}, code expects #{latest} — " \ + 'run `bundle exec rake mill:migrate`') + end rescue StandardError => e fail('schema', e.message) end @@ -173,6 +194,74 @@ def check_schema # # Settled 2026-08-19: ProjectV2Workflow exposes `enabled`, so this is a # direct check rather than the sentinel the design planned as a fallback. + # These values reach a subprocess environment. A mode drift is otherwise + # silent, and the runbook is the only thing that ever said to chmod them. + def check_secret_modes + dir = File.join(@home, 'secrets') + return unless Dir.exist?(dir) + + loose = Dir.children(dir).select do |name| + path = File.join(dir, name) + File.file?(path) && (File.stat(path).mode & 0o777) != Mill::Secrets::MODE + end + + if loose.empty? + pass('secrets files are 0600') + else + fail('secrets files are 0600', "#{loose.sort.join(', ')} — chmod 600 them") + end + end + + # A root that does not exist silently turns into "clone it myself" for + # every repo, and mill then works in a checkout you are not looking at. + def check_clone_roots + missing = Mill::Repo.roots.reject { |root| Dir.exist?(root) } + + if missing.empty? + pass('clone roots exist', Mill::Repo.roots.empty? ? 'none set; mill clones its own' : nil) + else + fail('clone roots exist', "MILL_CLONES names #{missing.join(', ')}, which do not exist") + end + end + + # The write paths are a kill switch and a worktree deleter, and the log + # endpoint streams repo contents. On loopback the interface is the boundary; + # anywhere else, the allowlist is the only thing in front of them. + def check_bind + bind = ENV['MILL_BIND'].to_s + return pass('bind is loopback or guarded', 'loopback') if + bind.empty? || bind.include?('127.0.0.1') || bind.include?('localhost') + + if ENV['MILL_ADMIN_EMAILS'].to_s.strip.empty? + fail('bind is loopback or guarded', + "MILL_BIND=#{bind} is reachable off this machine and MILL_ADMIN_EMAILS is empty") + else + pass('bind is loopback or guarded', bind) + end + end + + # mill writes five Status values. A board missing one fails at the moment + # it matters — a run finishing, or blocking — rather than at setup. + def check_board_options + return if @project.nil? || @project_owner.nil? + + github = @github || Mill::Github.new + status = github.project_fields(@project, owner: @project_owner) + .find { |field| field[:name] == 'Status' } + return fail('board Status has every option mill writes', 'no Status field') if status.nil? + + names = status.fetch(:options, []).map { |option| option[:name] } + missing = Mill::Board::STATUS.values.uniq - names + + if missing.empty? + pass('board Status has every option mill writes') + else + fail('board Status has every option mill writes', "missing #{missing.join(', ')}") + end + rescue StandardError => e + fail('board Status has every option mill writes', e.message) + end + def check_board return fail('board configured', 'set MILL_PROJECT and MILL_PROJECT_OWNER — see the runbook') if @project.nil? || @project_owner.nil? diff --git a/lib/mill/git.rb b/lib/mill/git.rb index 242d879..fcdd25b 100644 --- a/lib/mill/git.rb +++ b/lib/mill/git.rb @@ -27,6 +27,31 @@ def self.run!(repo_path, *args) result.out end + # Cloning has no repository to run inside, so it cannot go through `run`. + # It stays here anyway: this module is the only place mill runs git. + def self.clone(url, path) + FileUtils.mkdir_p(File.dirname(path)) + _out, err, status = Open3.capture3('git', 'clone', url.to_s, path.to_s) + raise Error, "git clone failed: #{Mill.utf8(err).strip[0, 300]}" unless status.success? + + path + end + + # Only tests need this; it lives here so that they, too, run no git of + # their own. + def self.clone_init(path) + FileUtils.mkdir_p(path) + run!(path, 'init', '--initial-branch=main') + run!(path, 'config', 'user.email', 'test@example.com') + run!(path, 'config', 'user.name', 'Test') + path + end + + def self.origin(repo_path) + result = run(repo_path, 'remote', 'get-url', 'origin') + result.ok ? result.out.strip : nil + end + # The spec is the file the branch *adds* under the prefix, found by diffing # rather than by reading a path out of prose. `base...branch` is the # three-dot form: what the branch added since it diverged, not everything diff --git a/lib/mill/github.rb b/lib/mill/github.rb index 2f97654..5725faa 100644 --- a/lib/mill/github.rb +++ b/lib/mill/github.rb @@ -38,6 +38,28 @@ def issue(repo, number) 'number,title,body,state,author,comments,url') end + # When the window reopens, as a UTC epoch second. The rate_limit endpoint is + # itself exempt, so asking costs nothing — which is what makes waiting for + # the reset a better answer than backing off blindly into a closed window. + # nil means mill could not find out, and the caller must fall back rather + # than treat it as "reopens now". + def rate_limit_reset(resource = :graphql) + json('api', 'rate_limit')&.dig(:resources, resource.to_sym, :reset) + rescue Error + nil + end + + def project_id(project, owner:) + json('project', 'view', project.to_s, '--owner', owner, '--format', 'json')[:id] + end + + # Field and option ids are opaque and belong to the project, so mill has to + # resolve them rather than guess at them from the names it knows. + def project_fields(project, owner:) + json('project', 'field-list', project.to_s, '--owner', owner, '--format', 'json') + .fetch(:fields, []) + end + # Projects v2 is GraphQL-only, so the board is never read with `gh issue list`. def board_items(project, owner:) json('project', 'item-list', project.to_s, '--owner', owner, '--format', 'json') @@ -84,9 +106,16 @@ def project_workflows(project, owner:) data.dig(:data, :user, :projectV2, :workflows, :nodes) || [] end - def comments(repo, number) - pages = json('api', "repos/#{repo}/issues/#{number}/comments?per_page=100", - '--paginate', '--slurp') + # `since` is why the cursor exists. Without it every sweep re-fetches every + # comment on every live subject: a run blocked for a week on a 300-comment + # issue is ten paginated pages every tick, which ends in a secondary rate + # limit that wedges the poller. It is inclusive of the boundary second, so + # a comment created in the same second comes back again — which is what the + # caller's own filter and the unique index on gh_node_id are for. + def comments(repo, number, since: nil) + path = "repos/#{repo}/issues/#{number}/comments?per_page=100" + path += "&since=#{since}" if since + pages = json('api', path, '--paginate', '--slurp') Array(pages).flatten(1) end @@ -123,6 +152,13 @@ def comment(repo, number, body) def stamp(body) = "#{MARKER}\n#{body}" + # mill is the sole writer of Status. This is the only method that writes + # one, which is what makes that rule enforceable rather than aspirational. + def set_status(project_id:, item_id:, field_id:, option_id:) + run('project', 'item-edit', '--id', item_id, '--project-id', project_id, + '--field-id', field_id, '--single-select-option-id', option_id) + end + # mill opens the pull request, not the stage. The stage composes the body and # pushes the branch — both of which work inside the sandbox — and mill makes # the API call from out here. diff --git a/lib/mill/ledger.rb b/lib/mill/ledger.rb index a8bda30..652c4b0 100644 --- a/lib/mill/ledger.rb +++ b/lib/mill/ledger.rb @@ -22,6 +22,10 @@ class Ledger MAX_STRIKES = 2 MAX_ATTEMPTS = 8 MAX_INTERRUPTIONS = 3 + # A launch the subscription refused inserts no row, so this cap is counted in + # the runner rather than in the database. + MAX_RATE_LIMIT_WAITS = 4 + MAX_RATE_LIMIT_PAUSE = 3600 # A strike means the work was wrong. Everything the machine did to a stage # is free — a laptop that slept, a socket that died, a lock file left by a @@ -45,12 +49,41 @@ class Ledger rate_limited: { attempt: 0, strike: 0 } }.freeze - # A process that died outranks whatever it managed to emit: mill has no - # trustworthy account of what happened either way. - # Checked before anything else: a session the CLI would not reopen is not the - # stage failing, and it does not present as a crash either — the process + # Order matters, and the first three are all things the machine did rather + # than the stage. + # + # A launch the subscription refused never ran, so charging it reads as a + # crash and takes a strike for a door mill could not open. Measured + # 2026-08-20: a five-hour window closed mid-run and `plan` was struck. + # + # The flag alone does not identify that launch. `rate_limited?` reports + # what the last rate-limit event in the stream was, and an "allowed" + # heartbeat clears it, so a stage throttled at minute 2 that then got its + # launch and finished still carries it whenever the result line lands + # before the next heartbeat. Reading the flag by itself threw that finished + # work away, and since this outcome inserts no row the relaunch reused the + # log filename and overwrote the log of the run that had succeeded. + # + # The verdict is what settles it, not the exit status. Nothing here has + # measured what a refused launch exits with, and the one refusal shape mill + # has measured — a session the CLI would not reopen — is reported in-band, + # so exit status is the wrong thing to lean on either way. A stage that + # handed back something mill can read did its work whatever the limit did + # around it. + # + # The converse is weaker and known to be: a stage that handed back nothing + # is treated as stopped by the limit, which is right for a refusal and + # wrong for a launch that worked and then died on one. Telling those apart + # needs the stream (a session id, a model, any turns at all), and until it + # does, the second case pays nothing and loses its log. See the triage note. + # + # A session the CLI would not reopen is not a crash either; the process # exits cleanly having done nothing. + # + # Then a process that died outranks whatever it managed to emit, because + # mill has no trustworthy account of what happened either way. def self.classify(attempt) + return :rate_limited if attempt.rate_limited? && !attempt.verdict.valid? return :resume_failed if attempt.resume_failed? return :crashed unless attempt.result.success? return :no_verdict unless attempt.verdict.valid? diff --git a/lib/mill/poller.rb b/lib/mill/poller.rb new file mode 100644 index 0000000..9729807 --- /dev/null +++ b/lib/mill/poller.rb @@ -0,0 +1,268 @@ +require 'json' + +module Mill + # Reconciles the board into runnable work. It asks one idempotent question — + # which items are Ready with no active run — which needs no dedupe key and + # heals itself when mill crashes mid-transition. + # + # The label design that preceded this consumed change *events*, and four bugs + # came from that shape: a relabelled issue deduped permanently, an item that + # was Ready and Running at once, nothing clearing Running when a run was + # killed, and no label change reaching a terminal state. A single-select + # cannot express any of them. + class Poller + # `supervisor` is required and is never built here. There must be exactly + # one supervisor in the process: it is the only thing that knows which + # process groups mill spawned and which runs have a live thread, and a + # second instance believes the answer to both is "none". A reaper holding + # that belief classifies every healthy stage mill just started as a foreign + # process and kills it, about thirty seconds into every run. + def initialize(supervisor:, db: Mill.db, github: nil, board: nil, + preparer: Mill::Repo.method(:prepare), locator: nil) + @db = db + @github = github || Mill::Github.new + @board = board || Mill::Board.new(db: db, github: @github) + @supervisor = supervisor + @preparer = preparer + @locator = locator + end + + MAX_EVENT_ATTEMPTS = 3 + + def tick + @board.redrive + reconcile + sweep + dispatch + end + + def reconcile + return unless @board.configured? + + ready_items.each do |item| + break if @supervisor.at_cap? + + start(item) + end + end + + def ready_items + @board.items.select { |item| item[:status] == 'Ready' && !active?(item) } + end + + # Comments are genuinely events, unlike board state, so these are consumed + # rather than reconciled — which is why they need a cursor and a dedupe key. + def sweep + subjects_of_interest.group_by { |subject| subject[:repo_id] }.each do |repo_id, subjects| + repo = @db[:repos].where(id: repo_id).first + record(repo, subjects.flat_map { |subject| fetch(repo, subject) }) + end + end + + # Bounded deliberately: subjects mill has a live run on, not every issue in + # every repo the board touches. An unbounded sweep on an active repo would + # leave tens of thousands of rows behind with nothing explaining them. + def subjects_of_interest + @db[:runs].where(status: %w[running blocked]) + .select_map(%i[repo_id subject_kind subject_number]).uniq + .map { |repo_id, kind, number| { repo_id: repo_id, kind: kind, number: number } } + end + + # Board Status decides what a comment is. While an item is Blocked, every + # comment on it is an answer and none of them starts a run — otherwise your + # answer tries to start a second run, the uniqueness index refuses it, the + # event retries until it dies, and the blocked run waits for an answer that + # already arrived. + # + # The cap binds here too: a blocked run is not counted as running until its + # own thread says so, so ten answers at once would start ten route walks. + def dispatch + @db[:events].where(kind: 'comment', state: 'pending').order(:id).each do |event| + break if @supervisor.at_cap? + + handle(event) + end + end + + private + + # Marked processed, committed, and only then is the thread spawned — never + # inside the same transaction as it. + # + # A thread started inside an open transaction writes to the same SQLite + # file from another connection while this one holds the write lock, so + # either the thread or the commit fails. If the commit is what fails, the + # event rolls back to pending while the thread it already spawned keeps + # running, and the next dispatch starts a second walker on the same run. + # + # Marking first would drop the answer if `start` then failed, which is what + # the design's same-transaction rule exists to prevent. fail_event is the + # compensation: it puts the event back to pending, so a failed start is + # retried rather than lost. `running?` is what stops a retry becoming a + # second walker. + def handle(event) + payload = JSON.parse(event[:payload_json].to_s, symbolize_names: true) + run = blocked_run_for(event[:repo_id], payload) + + return no_route(event) if run.nil? + return if @supervisor.running?(run[:id]) + + finish_event(event, 'processed') + @supervisor.start(run[:id], answers: [payload[:body].to_s]) + rescue StandardError => e + fail_event(event, e) + end + + def blocked_run_for(repo_id, payload) + @db[:runs].where(repo_id: repo_id, subject_kind: payload[:subject_kind].to_s, + subject_number: payload[:subject_number], status: 'blocked').first + end + + # Only two of the five triggers have a route: the `mill:` marker, review + # comments and red checks all need the iterate route, which does not exist. + # Recorded and logged rather than dropped, so Plan 5 can see what it missed. + def no_route(event) + warn "no route for comment event #{event[:gh_node_id]}" + finish_event(event, 'no_route') + end + + def finish_event(event, state) + @db[:events].where(id: event[:id]).update(state: state, processed_at: Mill.now) + end + + def fail_event(event, error) + attempts = event[:attempts].to_i + 1 + dead = attempts >= MAX_EVENT_ATTEMPTS + @db[:events].where(id: event[:id]).update( + attempts: attempts, state: dead ? 'dead' : 'pending', + last_error: "#{error.class}: #{error.message}"[0, 300], + processed_at: dead ? Mill.now : nil + ) + end + + def fetch(repo, subject) + slug = "#{repo[:owner]}/#{repo[:name]}" + @github.comments(slug, subject[:number], since: repo[:comments_cursor]) + .map { |c| c.merge(subject_kind: subject[:kind], subject_number: subject[:number]) } + end + + # The cursor is advanced inside the same transaction as the inserts. A + # fetch that raises partway therefore writes no cursor, and the comments it + # never saw are picked up next tick rather than skipped forever. + def record(repo, comments) + usable = comments.select { |comment| trigger?(comment, repo) } + latest = comments.filter_map { |comment| comment[:created_at] }.max + + @db.transaction do + usable.each { |comment| insert_event(repo, comment) } + @db[:repos].where(id: repo[:id]).update(comments_cursor: latest) if latest + end + end + + def trigger?(comment, repo) + return false unless Mill::Github.trusted_author?(comment) + return false if Mill::Github.own_comment?(comment[:body]) + + cursor = repo[:comments_cursor] + cursor.nil? || comment[:created_at].to_s > cursor + end + + def insert_event(repo, comment) + @db[:events].insert_conflict.insert( + repo_id: repo[:id], kind: 'comment', gh_node_id: comment[:node_id].to_s, + payload_json: comment.to_json, attempts: 0, state: 'pending', created_at: Mill.now + ) + end + + # Both issues and PRs appear as items, and a PR-entry item is a subject in + # its own right — a Dependabot PR has no issue, so questions need somewhere + # to go. + def subject_kind(item) = item.dig(:content, :type) == 'PullRequest' ? 'pr' : 'issue' + + def active?(item) + repo = repo_row(item) or return false + + @db[:runs].where(repo_id: repo[:id], subject_kind: subject_kind(item), + subject_number: item.dig(:content, :number), status: %w[running blocked]).any? + end + + def repo_row(item) + owner, name = split(item) + return nil if name.nil? + + @db[:repos].where(owner: owner, name: name).first + end + + def split(item) = item.dig(:content, :repository).to_s.split('/', 2) + + def start(item) + owner, name = split(item) + number = item.dig(:content, :number) + return if name.nil? || number.nil? + + prepared = @preparer.call(db: @db, owner: owner, name: name) + return block_item(owner, name, number, prepared) unless prepared.ok? + + repo = @db[:repos].where(owner: owner, name: name).first + located = locate(repo, "#{owner}/#{name}", number) + return no_spec(owner, name, number, located) unless located.found? + + claim(item, repo, number, located) + end + + def claim(item, repo, number, located) + result = @supervisor.claim(repo_row: repo, subject_kind: subject_kind(item), + subject_number: number, route: 'plan', branch: located.branch, + spec_path: located.path, board_item_id: item[:id]) + + case result + when :held then nil + when Mill::Supervisor::Blocked + block_item(repo[:owner], repo[:name], number, result) + else + @supervisor.start(result) + end + end + + def locate(repo, slug, number) + return @locator.call(repo, slug, number) if @locator + + Mill::Spec.locate(github: @github, repo: slug, number: number, + repo_path: repo[:local_path], base: repo[:base_branch], git: Mill::Git) + end + + # :no_branch and :no_spec carry no questions, because there is nothing to + # ask — the answer is a branch or a file, not a decision. Saying "mill + # cannot start this" and then listing nothing reads as a bug in mill rather + # than as a missing spec, so those two are told plainly instead. + def no_spec(owner, name, number, located) + return block_item(owner, name, number, located) if located.blocked? + + body = case located.problem + when :no_branch + 'This item has no linked branch, so there is nothing for mill to adopt. Run ' \ + "`gh issue develop #{number}`, commit a spec on that branch under " \ + '`docs/superpowers/specs/`, and set Status back to `Ready`.' + else + "`#{located.branch}` adds no file under `docs/superpowers/specs/`, so mill has no " \ + 'spec to plan from. Commit one on that branch and set Status back to `Ready`.' + end + comment_on(owner, name, number, body) + end + + # Blocking an item that has no run yet: there is nothing to resume, so it + # re-enters at the top of the graph when you set it Ready again. + def block_item(owner, name, number, result) + body = ["mill cannot start this yet (`#{result.problem}`).", '', + *Array(result.questions).map { |question| "- #{question}" }, '', + 'Fix the cause and set Status back to `Ready`.'].join("\n") + comment_on(owner, name, number, body) + end + + def comment_on(owner, name, number, body) + @github.comment("#{owner}/#{name}", number, body) + rescue Mill::Github::Error => e + warn "could not comment on #{owner}/#{name}##{number}: #{e.message}" + end + end +end diff --git a/lib/mill/repo.rb b/lib/mill/repo.rb new file mode 100644 index 0000000..ec3e2f5 --- /dev/null +++ b/lib/mill/repo.rb @@ -0,0 +1,150 @@ +require 'fileutils' +require 'json' +require 'yaml' + +module Mill + # Finding, or making, the working copy a run happens in. + # + # On a laptop mill uses a clone you already keep, because working against the + # same checkout you use is the point of running it there. A server keeps none, + # so mill clones into its own directory. Those are one code path with a + # different answer to "did anything match". + module Repo + Result = Struct.new(:path, :problem, :questions, keyword_init: true) do + def ok? = problem.nil? + end + + def self.roots + raw = ENV['MILL_CLONES'].to_s + return raw.split(':').reject(&:empty?).map { |path| File.expand_path(path) } unless + raw.empty? + + Mill::Clock::DARWIN ? [File.expand_path('~/code')] : [] + end + + def self.clone_dir = File.join(Mill.home, 'clones') + + def self.default_url(owner, name) = "https://github.com/#{owner}/#{name}.git" + + # owner/name, lowercased, whatever form the remote was written in. + def self.slug(url) + url.to_s.strip.sub(/\.git\z/, '')[%r{[:/]([^/:]+/[^/:]+)\z}, 1]&.downcase + end + + def self.resolve(owner, name, git: Mill::Git, url: nil) + matches = candidates("#{owner}/#{name}".downcase, git) + + return Result.new(path: matches.first) if matches.length == 1 + return ambiguous(owner, name, matches) if matches.length > 1 + + path = File.join(clone_dir, "#{owner}-#{name}") + return Result.new(path: path) if Dir.exist?(File.join(path, '.git')) + + Result.new(path: git.clone(url || default_url(owner, name), path)) + rescue Mill::Git::Error => e + Result.new(problem: :clone_failed, + questions: ["mill could not clone #{owner}/#{name}: #{e.message}"]) + end + + def self.candidates(wanted, git) + roots.flat_map { |root| Dir.glob(File.join(root, '*')) } + .select { |path| Dir.exist?(File.join(path, '.git')) } + .select { |path| slug(git.origin(path)) == wanted } + .sort + end + + # Lazy and per-item: the first time an item from a repo reaches the board. + # Everything here is a read or a local git config write — mill writes + # nothing to the repository itself, which is what lets it use no labels. + # + # Anything missing blocks that one item and names it. Nothing here raises + # at the caller: an exception would wedge the poller loop in a retry cycle + # over one badly configured repo. + def self.prepare(db:, owner:, name:, git: Mill::Git, url: nil) + row = db[:repos].where(owner: owner, name: name).first + return Result.new(path: row[:local_path]) if row && row[:prepared_at] + + resolved = resolve(owner, name, git: git, url: url) + return resolved unless resolved.ok? + + path = resolved.path + git.run!(path, 'config', 'gc.auto', '0') + git.run!(path, 'config', 'maintenance.auto', '0') + + base = base_branch(path, git) + config = read_config(path, base, git) + missing = missing_secrets(owner, name, config) + return missing if missing + + upsert(db, owner, name, path, base, config) + Result.new(path: path) + rescue Mill::Git::Error => e + Result.new(problem: :unprepared, + questions: ["mill could not prepare #{owner}/#{name}: #{e.message}"]) + end + + def self.config(db, repo_id) + raw = db[:repos].where(id: repo_id).get(:config_json) + raw ? JSON.parse(raw, symbolize_names: true) : {} + end + + def self.base_branch(path, git) + result = git.run(path, 'symbolic-ref', 'refs/remotes/origin/HEAD') + ref = result.ok ? result.out.strip.split('/').last : nil + ref.nil? || ref.empty? ? 'main' : ref + end + + # From the base branch, never from a checkout. `git show` reads the + # committed blob, so nothing has to be checked out and no worktree can + # influence what mill reads. + def self.read_config(path, base, git) + git.run(path, 'fetch', 'origin', base) + %W[origin/#{base} #{base}].each do |ref| + result = git.run(path, 'show', "#{ref}:.mill.yml") + next unless result.ok + + parsed = YAML.safe_load(result.out, symbolize_names: true) + return parsed.is_a?(Hash) ? parsed : {} + end + {} + rescue Psych::Exception => e + # Psych::SyntaxError for a malformed file, DisallowedClass for an + # unquoted date. Both are the operator's to fix, and both must block the + # item rather than escaping into the poller loop. + raise Mill::Git::Error, ".mill.yml on #{base} could not be read: #{e.message}" + end + + def self.missing_secrets(owner, name, config) + named = Array(config[:secrets]).map(&:to_s) + return nil if named.empty? + + absent = named - Mill::Secrets.for_repo(owner, name).keys + return nil if absent.empty? + + Result.new(problem: :missing_secrets, questions: [ + "#{Mill::Secrets.path_for(owner, name)} is missing #{absent.join(', ')}, which " \ + "#{owner}/#{name}'s .mill.yml names. A suite without them fails the same way on " \ + 'both attempts, which reads as the stage being wrong. Add them and reply here.' + ]) + end + + def self.upsert(db, owner, name, path, base, config) + db[:repos].insert_conflict(target: %i[owner name]).insert( + owner: owner, name: name, local_path: path, base_branch: base, + config_json: config.to_json, prepared_at: Mill.now, created_at: Mill.now + ) + db[:repos].where(owner: owner, name: name).update( + local_path: path, base_branch: base, config_json: config.to_json, + prepared_at: Mill.now + ) + end + + def self.ambiguous(owner, name, matches) + Result.new(problem: :ambiguous_clone, questions: [ + "#{owner}/#{name} matches more than one working copy: #{matches.join(', ')}. " \ + 'mill will not choose between them, because the choice commits the whole run to ' \ + 'one checkout. Move or remove all but one, then reply here.' + ]) + end + end +end diff --git a/lib/mill/rules.rb b/lib/mill/rules.rb index f7a343d..844222a 100644 --- a/lib/mill/rules.rb +++ b/lib/mill/rules.rb @@ -74,8 +74,8 @@ def self.for_stage(stage) def self.ca_bundle = CA_BUNDLES.find { |path| File.exist?(path) } - # The environment every stage runs with. Plan 3 adds the scoped GH_TOKEN and - # the per-repo secrets here; this is the hook they hang from. + # The environment every stage runs with: the CA bundle, the repo's own + # secrets, and — for the two stages that push — the narrow token. # # SSL_CERT_FILE is set for the benefit of anything a stage runs that reads # a CA file — and **not** for `gh`, which it does not fix. Measured across @@ -86,9 +86,14 @@ def self.ca_bundle = CA_BUNDLES.find { |path| File.exist?(path) } # SSL_CERT_FILE on this platform, and `GODEBUG=x509usefallbackroots=1` was # probed directly and made no difference either. The fix is architectural, # not environmental — see the pr stage. - def self.env_for(_stage) + def self.env_for(stage, owner: nil, name: nil) + env = {} bundle = ca_bundle - bundle ? { 'SSL_CERT_FILE' => bundle } : {} + env['SSL_CERT_FILE'] = bundle if bundle + env.merge!(Mill::Secrets.for_repo(owner, name)) + token = Mill::Secrets.token if Mill::Secrets::PUSHING.include?(stage) + env['GH_TOKEN'] = token if token + env end def self.write!(home: Mill.home) diff --git a/lib/mill/run.rb b/lib/mill/run.rb index f02e078..4d96d23 100644 --- a/lib/mill/run.rb +++ b/lib/mill/run.rb @@ -9,6 +9,11 @@ module Mill class Run attr_reader :run_id, :worktree, :branch, :spec_path, :problem, :questions + # The supervisor sets this to learn which process groups are its own. Set + # per Run rather than globally: a second supervisor believing no group is + # mill's would classify every healthy stage as foreign and kill it. + attr_accessor :on_identity + def initialize(repo:, number:, clone:, db: Mill.db, github: nil, git: Mill::Git, claude: Mill::Claude) @owner, @name = repo.split('/', 2) @@ -55,13 +60,20 @@ def prepared? = @problem.nil? && !@run_id.nil? # Answering a blocked run. Nothing restarts: the blocked stage resumes its # own session with the answers injected, and the route carries on from # there. Plan 3 triggers this from a comment; by hand it is rake mill:answer. - def self.resume(run_id, answers, db: Mill.db, claude: Mill::Claude, &announce) + # Builds a Run from a row that already exists. Nothing is re-resolved, so + # adopting a run cannot pick a different branch than the one it has been + # working on. + def self.adopt(run_id, answers: [], db: Mill.db, claude: Mill::Claude) row = db[:runs].where(id: run_id).first or raise Mill::Error, "no run #{run_id}" repo = db[:repos].where(id: row[:repo_id]).first run = allocate run.send(:initialize_resumed, row, repo, db, claude, Array(answers)) - run.call(&announce) + run + end + + def self.resume(run_id, answers, db: Mill.db, claude: Mill::Claude, &announce) + adopt(run_id, answers: answers, db: db, claude: claude).call(&announce) end def call(launcher: nil, &announce) @@ -76,7 +88,18 @@ def runner(launcher: nil, &announce) launcher: launcher || default_launcher(&announce), context: { issue: issue_body, spec_path: @spec_path, branch: @branch, base: base, answers: @answers }) - @resumed ? r.restore : r + # A blocked run being answered restores, which may spend its sanctioned + # strike reset. A run that already has attempts behind it — one the + # supervisor interrupted and restarted — only reloads: it picks up at + # the stage it was in, with nothing forgiven, because nobody answered + # anything. A run with no attempts starts at the top of its route. + if @resumed + r.restore + elsif @ledger_has_attempts + r.reload + else + r + end end end @@ -109,7 +132,12 @@ def initialize_resumed(row, repo, db, claude, answers) @repo = "#{repo[:owner]}/#{repo[:name]}" @answers = answers @questions = [] - @resumed = true + # A fresh run has nothing to restore; a blocked one has verdicts the + # resumed stage needs handed back to it. + @resumed = row[:status] == 'blocked' + # Anything already attempted means this run is being picked up rather + # than started, whatever its status says. + @ledger_has_attempts = db[:stage_attempts].where(run_id: row[:id]).any? end def fail_with(problem, questions) @@ -146,9 +174,28 @@ def default_launcher(&announce) announce&.call(stage, number, !session_id.nil?) log = File.join(Mill.home, 'logs', @run_id.to_s, "#{Mill::Stages.slug(stage)}-#{number}.jsonl") - @claude.new(stage).run(prompt, number: number, worktree: @worktree, - log_path: log, session_id: session_id, env: Mill::Rules.env_for(stage)) + attempt = @claude.new(stage).run(prompt, number: number, worktree: @worktree, + log_path: log, session_id: session_id, + env: Mill::Rules.env_for(stage, owner: @owner, name: @name), + secrets: Mill::Secrets.values_for(stage, owner: @owner, name: @name), + on_spawn: method(:record_identity)) + forget_identity + attempt end end + + # What the supervisor reaps against, recorded the moment the group exists. + # A run between stages holds no identity, which is a different state from a + # run whose process mill has lost — the supervisor distinguishes them by + # whether it has a thread walking the run, not by these columns. + def record_identity(pid, pgid, started_at, boot_at) + @db[:runs].where(id: @run_id).update(pid: pid, pgid: pgid, pid_started_at: started_at, + host_boot_at: boot_at, heartbeat_at: Mill.now) + @on_identity&.call(pgid) + end + + def forget_identity + @db[:runs].where(id: @run_id).update(pid: nil, pgid: nil, heartbeat_at: Mill.now) + end end end diff --git a/lib/mill/runner.rb b/lib/mill/runner.rb index 87240bc..9765cfa 100644 --- a/lib/mill/runner.rb +++ b/lib/mill/runner.rb @@ -11,12 +11,14 @@ class Runner attr_reader :run_id, :state - def initialize(db:, run_id:, launcher:, github: nil, context: {}) + def initialize(db:, run_id:, launcher:, github: nil, context: {}, pause: method(:sleep)) @db = db @run_id = run_id @launcher = launcher @github = github @context = context + # Injected so a test can assert the wait without taking it. + @pause = pause @ledger = Mill::Ledger.new(db, run_id) @sessions = {} @artifacts = {} @@ -46,6 +48,22 @@ def stage = @stage ||= route_stages.first def restore raise Mill::Error, "run #{@run_id} is not blocked" unless run_row[:status] == 'blocked' + reload + rescue_from_strikes + self + end + + # Picks a run back up where it was. Two callers, and they differ only in what + # they are entitled to do afterwards: answering a blocked run may also spend + # its one sanctioned strike reset, while a run the supervisor interrupted may + # not — nobody answered anything. + # + # Without this, a restarted run began at the first stage of its route and + # re-ran every stage it had already banked: `plan` would write its artifact + # a second time and the ledger would count fresh attempts against stages + # that had already passed. @stage is otherwise `route_stages.first`, because + # that is the right answer only for a run that has never launched anything. + def reload @db[:stage_attempts].where(run_id: @run_id).order(:id).each do |row| verdict = row[:verdict_json] ? JSON.parse(row[:verdict_json], symbolize_names: true) : {} @sessions[row[:stage]] = row[:session_id] @@ -53,7 +71,6 @@ def restore @verdicts << { stage: row[:stage], status: verdict[:status], summary: verdict[:summary] } end @stage = run_row[:current_stage] || route_stages.first - rescue_from_strikes self end @@ -71,6 +88,10 @@ def step return halt(:blocked, "#{@stage} has used both its strikes") if tally.out_of_strikes? return halt(:blocked, "#{@stage} hit its number cap") if tally.out_of_attempts? + # Recorded before the launch, not after it. This is what the supervisor + # reads to know which stage to charge for an interruption, and an + # interruption is by definition something that happens mid-launch. + @db[:runs].where(id: @run_id).update(current_stage: @stage) settle(launch(@stage, tally.next_attempt), tally.next_attempt) end @@ -115,6 +136,8 @@ def settle(attempt, number) @sessions[@stage] = nil @ledger.charge(stage: @stage, outcome: :resume_failed, number: number, attempt: attempt) :rerun + when :rate_limited + wait_out_the_limit(attempt) when :blocked @ledger.charge(stage: @stage, outcome: :blocked, number: number, attempt: attempt) halt(:blocked, "#{@stage} asked a question", questions: attempt.verdict.questions) @@ -127,6 +150,39 @@ def settle(attempt, number) end end + # A launch the subscription refused costs nothing and produced nothing, so + # there is no row to insert and no counter in the database to bound it — + # which is why the cap is held here. Free is not unlimited. + # + # Waiting in this thread is correct rather than lazy: the run is waiting, + # not working, and the supervisor leaves a run alone while its thread is + # alive. Retrying straight away would be a hot loop against a door that + # does not open for hours. + def wait_out_the_limit(attempt) + @rate_limit_waits = @rate_limit_waits.to_i + 1 + if @rate_limit_waits > Mill::Ledger::MAX_RATE_LIMIT_WAITS + return halt(:blocked, "#{@stage} has been rate limited " \ + "#{Mill::Ledger::MAX_RATE_LIMIT_WAITS} times without getting a launch. " \ + 'Nothing was charged against it — the subscription refused the launch, the ' \ + 'stage did not fail. Reply here to try again.') + end + + @ledger.charge(stage: @stage, outcome: :rate_limited) + seconds = self.class.rate_limit_pause(attempt.rate_limit_resets_at) + warn "#{@stage} is rate limited; waiting #{seconds}s for the window" + @pause.call(seconds) + :rerun + end + + # Never shorter than a minute in case the reset has just passed, never + # longer than the cap in case the clocks disagree, and the cap when the CLI + # did not say when the window reopens. + def self.rate_limit_pause(resets_at, now: Mill.now) + return Mill::Ledger::MAX_RATE_LIMIT_PAUSE if resets_at.nil? + + [[resets_at.to_i - now, 60].max, Mill::Ledger::MAX_RATE_LIMIT_PAUSE].min + end + def advance(attempt, number) @ledger.charge(stage: @stage, outcome: reviewer?(@stage) ? :reviewed_clean : :ok, number: number, attempt: attempt) diff --git a/lib/mill/secrets.rb b/lib/mill/secrets.rb new file mode 100644 index 0000000..860f7db --- /dev/null +++ b/lib/mill/secrets.rb @@ -0,0 +1,93 @@ +module Mill + # What a stage runs with beyond its own argv. A fresh worktree holds tracked + # files only, so .env and config/master.key are absent and a repo whose suite + # needs them would fail identically on both attempts — which reads as the stage + # being wrong and is not. + # + # Values from here reach a subprocess environment, so every caller also hands + # them to Spawn's scrubber. Not all of them: see SHORTEST_REDACTABLE. + module Secrets + MODE = 0o600 + + # Pushing is the only thing a stage does that needs a credential of its own. + PUSHING = %w[pr push].freeze + + # A value shorter than this is not redacted, because redacting it does more + # damage than leaking it. The scrubber gsubs literally over every log line, + # and the log is stream-json that mill's own parser reads back: an env file + # carrying RAILS_ENV=test turns every "test" in the transcript into + # [redacted], and DEBUG=true turns "success":true into "success":[redacted], + # which stops being JSON. The stage then reads as having produced no verdict + # and is charged a strike for mill's own scrubber. No real credential is + # this short. + SHORTEST_REDACTABLE = 16 + + def self.dir = File.join(Mill.home, 'secrets') + + def self.path_for(owner, name) = File.join(dir, "#{owner}-#{name}.env") + + def self.for_repo(owner, name) + return {} if owner.nil? || name.nil? + + read_env(path_for(owner, name)) + end + + # The narrow token the pushing stages carry. Setting GH_TOKEN is enough: + # the credential helper the runbook configures asks gh for a credential, and + # gh honours GH_TOKEN over its stored login — so one variable re-points both + # `gh` and `git push` at the scoped token without touching the worktree. + def self.token + path = File.join(dir, 'stage-token') + return nil unless File.exist?(path) + + check_mode!(path) + value = Mill.utf8(File.read(path)).strip + value.empty? ? nil : value + end + + # Exactly the strings that must never appear in a log, and no others. + def self.values_for(stage, owner: nil, name: nil) + values = for_repo(owner, name).values + values += [token].compact if PUSHING.include?(stage) + values.reject { |value| value.to_s.length < SHORTEST_REDACTABLE } + end + + def self.read_env(path) + return {} unless File.exist?(path) + + check_mode!(path) + parse(File.read(path)) + end + + def self.parse(text) + Mill.utf8(text).lines.filter_map do |line| + line = line.strip + next if line.empty? || line.start_with?('#') + + key, value = line.split('=', 2) + next if value.nil? + + key = key.strip + key.empty? ? nil : [key, unquote(value.strip)] + end.to_h + end + + # Matching quotes only. A value that opens with one quote and closes with + # another is not quoted, it is a value containing quotes. + def self.unquote(value) + return value if value.length < 2 + + %w[" '].each do |quote| + return value[1..-2] if value.start_with?(quote) && value.end_with?(quote) + end + value + end + + def self.check_mode!(path) + mode = File.stat(path).mode & 0o777 + return if mode == MODE + + raise Mill::Error, "#{path} is mode #{format('%o', mode)}, expected 600" + end + end +end diff --git a/lib/mill/spawn.rb b/lib/mill/spawn.rb index 9f38bba..4c4dc07 100644 --- a/lib/mill/spawn.rb +++ b/lib/mill/spawn.rb @@ -30,10 +30,11 @@ def success? = error.nil? && !status.nil? && status.success? attr_reader :pid, :pgid, :pid_started_at, :host_boot_at - def initialize(log_path:, chdir:, secrets: [], clock: -> { Mill::Clock.awake }) + def initialize(log_path:, chdir:, secrets: [], on_spawn: nil, clock: -> { Mill::Clock.awake }) @log_path = log_path @chdir = chdir @secrets = expand_secrets(secrets) + @on_spawn = on_spawn @clock = clock end @@ -150,6 +151,11 @@ def pump(log, written, stream, argv, env) # pgid and would otherwise reach kill! with no identity to check. @pid_started_at = Mill::Clock.pid_started_at(@pid) @pgid = safe_pgid(@pid) + # Reported before the first line is read: a caller that waits for the + # result cannot reap a process that is still running. If recording it + # fails — a locked database is the likely way — the group must not + # outlive the failure, because nothing else now knows its identity. + announce_spawn drain = drain_stderr(stderr) stdout.each_line do |raw| @@ -170,6 +176,18 @@ def safe_pgid(pid) nil end + # The callback is how a live process becomes findable by anything other than + # this object. A failure here leaves a running process group that nothing + # has recorded, so it is killed before the exception is allowed out. + def announce_spawn + return if @on_spawn.nil? + + @on_spawn.call(@pid, @pgid, @pid_started_at, @host_boot_at) + rescue StandardError + self.class.reap(@pgid, boot_at: @host_boot_at, started_at: @pid_started_at) + raise + end + # Secrets are injected into the stage environment and must never reach the # log, which mill keeps and the UI tails. A secret travels through the log # as JSON, so its escaped form is a different string from the one in the diff --git a/lib/mill/stream.rb b/lib/mill/stream.rb index 51b6f92..5e907dd 100644 --- a/lib/mill/stream.rb +++ b/lib/mill/stream.rb @@ -14,7 +14,8 @@ class Stream }.freeze attr_reader :session_id, :model, :last_output_at, :pending_tool_at, - :rate_limited_at, :result, :raw_verdict, :structured_verdict, :permission_denials + :rate_limited_at, :rate_limit_resets_at, :result, :raw_verdict, :structured_verdict, + :permission_denials def initialize(clock: -> { Mill::Clock.awake }) @clock = clock @@ -130,11 +131,20 @@ def on_user(msg) # would leaving the stamp in place once the limit lifts, which is why an # "allowed" event clears it. A stage throttled at minute 2 and wedged at # minute 20 must still be reaped. + # `resetsAt` is kept because it is the only thing that says how long to + # wait. Without it a rejected launch is retried immediately, which is a hot + # loop against a door that will not open for hours. def on_rate_limit(msg) status = msg.dig(:rate_limit_info, :status) return if status.nil? - @rate_limited_at = status == 'allowed' ? nil : @clock.call + if status == 'allowed' + @rate_limited_at = nil + @rate_limit_resets_at = nil + else + @rate_limited_at = @clock.call + @rate_limit_resets_at = msg.dig(:rate_limit_info, :resetsAt) + end end # With `--json-schema` the CLI returns the verdict already parsed, in diff --git a/lib/mill/supervisor.rb b/lib/mill/supervisor.rb new file mode 100644 index 0000000..61f9697 --- /dev/null +++ b/lib/mill/supervisor.rb @@ -0,0 +1,337 @@ +require 'fileutils' +require 'set' + +module Mill + # Claims work up to the cap, owns the worktree lifecycle, and reaps process + # groups. Everything here is about the machine rather than the work: what a + # stage decides is the runner's business, and what an item means is the + # poller's. + # + # There is exactly one of these per process. It is the only object that knows + # which process groups mill spawned and which runs have a live thread, so a + # second instance answers "none" to both — and a reaper holding that belief + # kills every healthy stage it finds. + class Supervisor + Blocked = Struct.new(:problem, :questions, keyword_init: true) + + DEFAULT_CAP = 2 + MAX_CAP = 8 + # A lock younger than this may belong to a command running right now — one + # of mill's own stages, or you in a terminal on the same clone. + STALE_LOCK_AFTER = 300 + + attr_reader :own_pgids + + def initialize(db: Mill.db, github: nil, git: Mill::Git, board: nil) + @db = db + @github = github || Mill::Github.new + @git = git + @board = board + @threads = {} + @own_pgids = Set.new + @announced = {} + end + + # Not `.to_i`: MILL_CONCURRENCY=lots would become 0, at_cap? would be true + # forever, and mill would claim nothing while every check stayed green. + def cap = Mill.setting_int('MILL_CONCURRENCY', default: DEFAULT_CAP, min: 1, max: MAX_CAP) + + # Counts running rows only. A blocked run is not working, and there is no + # queued status: a run is inserted as running in the act of claiming it. + def at_cap? = @db[:runs].where(status: 'running').count >= cap + + def claim(repo_row:, subject_kind:, subject_number:, route:, branch:, spec_path:, + board_item_id: nil) + holder = live_holder(repo_row[:id], branch) + return held(repo_row, subject_number, branch, holder) if holder + + clone = repo_row[:local_path] + @git.run(clone, 'worktree', 'prune') + clear_stale_locks(clone, branch) + return checked_out_block(clone, branch) if checked_out?(clone, branch) + + # The row and the worktree go together. A row inserted before a worktree + # that then fails to appear is a running run with no process and no + # thread — nothing reaps it, because there is nothing to identify, and it + # counts against the cap for as long as the database survives. + run_id = nil + begin + @db.transaction do + run_id = insert_run(repo_row, subject_kind, subject_number, route, branch, + spec_path, board_item_id) + attach_worktree(repo_row, run_id, branch) + end + rescue StandardError + discard(repo_row, run_id) + raise + end + + @board&.want(run_id, 'running') + run_id + end + + # One thread per run: a route walk takes tens of minutes, and a supervisor + # that walked it would claim one item and then stop reconciling. + def start(run_id, walker: nil, answers: []) + resumed(run_id) + walk = walker || ->(id) { walk(id, answers: answers) } + @threads[run_id] = Thread.new do + finish(run_id, walk.call(run_id)) + rescue StandardError => e + # A dead runner thread must not leave a run marked running forever. + # That is a concurrency slot nothing else releases. + warn "run #{run_id} thread died: #{e.class}: #{e.message}" + @db[:runs].where(id: run_id).update(status: 'failed', finished_at: Mill.now) + finish(run_id, { stage: nil, status: :failed, reason: e.message, questions: [] }) + ensure + @threads.delete(run_id) + end + end + + def running?(run_id) = @threads[run_id]&.alive? || false + + def finish(run_id, state) + row = @db[:runs].where(id: run_id).first or return + + announce(row, state) + @board&.want(run_id, row[:status]) + teardown(run_id) + end + + # A blocked run keeps its worktree indefinitely: mill needs it to resume, + # and a timer should not destroy the thing you have to answer a question + # about. + def teardown(run_id) + row = @db[:runs].where(id: run_id).first or return + return unless %w[done failed killed].include?(row[:status]) + + repo = @db[:repos].where(id: row[:repo_id]).first + path = row[:worktree_path] + return if path.nil? || !Dir.exist?(path) + + @git.worktree_remove(repo[:local_path], path) + @git.run(repo[:local_path], 'worktree', 'prune') + rescue Mill::Git::Error => e + warn "run #{run_id} worktree not removed: #{e.message}" + end + + # Every run marked running, checked against the live process table. Called + # at boot and on a timer. At boot mill has no live threads, so every group + # it finds is foreign by definition — which is the right answer: mill + # restarted and the stage outlived it. + # + # Interrupting is only half the job. A run interrupted and not restarted + # stays running with no thread forever, holds its slot against the cap, and + # is skipped by the poller because its item has an active run. Two of those + # stop the factory with nothing anywhere reporting a problem. + def reap + @db[:runs].where(status: 'running').select_map(:id).filter_map do |run_id| + row = @db[:runs].where(id: run_id).first + next if row.nil? || row[:status] != 'running' + + case identify(row) + when :ours then next + when :foreign + Mill::Spawn.reap(row[:pgid], boot_at: row[:host_boot_at], + started_at: row[:pid_started_at]) + end + + interrupt(row) + restart(run_id) + run_id + end + end + + # Three branches, in this order. Nothing is signalled on the strength of + # the boot time alone: kern.boottime moves when NTP corrects the clock, + # which it does routinely on waking, so the live process settles it. + # + # `:ours` means a thread is walking this run right now, not merely that no + # process is recorded. pid and pgid are nil for the whole gap between two + # stages, five times over on the plan route, so reading nil as "in hand" + # strands any run mill was restarted during. + def identify(row) + return :ours if running?(row[:id]) + return :gone if row[:pid].nil? || row[:pid_started_at].nil? + + started = Mill::Clock.pid_started_at(row[:pid]) + return :gone if started.nil? + return :gone if (started - row[:pid_started_at]).abs > 2 + + @own_pgids.include?(row[:pgid]) ? :ours : :foreign + end + + private + + # Re-enters the stage the run was in. Costs an attempt and no strike: the + # machine lost the process, the stage did not fail. A run interrupt has + # just blocked, because it hit the interruption cap, is waiting for a + # person, and the next tick would otherwise start it again. + # A run that has been answered is working again, and nothing else says so. + # Left blocked it lies in three ways at once: the board keeps saying Blocked + # for the whole rest of the route, the run does not count against the + # concurrency cap, and `reap` queries running rows only — so if its stage + # died, nothing could ever recover it. + def resumed(run_id) + row = @db[:runs].where(id: run_id).first + return unless row && row[:status] == 'blocked' + + @db[:runs].where(id: run_id).update(status: 'running') + @board&.want(run_id, 'running') + end + + def restart(run_id) + return if at_cap? + return unless @db[:runs].where(id: run_id).get(:status) == 'running' + + start(run_id) + end + + def interrupt(row) + stage = row[:current_stage] or + raise Mill::Error, "run #{row[:id]} is running with no current_stage" + + ledger = Mill::Ledger.new(@db, row[:id]) + ledger.charge(stage: stage, outcome: :interrupted) + @db[:runs].where(id: row[:id]).update(pid: nil, pgid: nil, heartbeat_at: nil) + return unless ledger.out_of_interruptions?(stage) + + @db[:runs].where(id: row[:id]).update(status: 'blocked') + comment(@db[:repos].where(id: row[:repo_id]).first, row[:subject_number], + "Blocked at `#{stage}`: this stage has been interrupted " \ + "#{Mill::Ledger::MAX_INTERRUPTIONS} times without finishing. Nothing was charged " \ + 'against it — each interruption was mill losing the process, not the stage ' \ + 'failing. Reply here to try again.') + @board&.want(row[:id], 'blocked') + end + + # Returns the runner's state — which stage stopped, why, and what it asked — + # not the run row. `finish` announces from this, and the row carries none of + # it: a row-shaped return posted `Blocked at ``: .` to the subject, which is + # the only channel that reaches a person, saying nothing. + def walk(run_id, answers: []) + run = Mill::Run.adopt(run_id, answers: answers, db: @db) + run.on_identity = ->(pgid) { @own_pgids << pgid } + run.call + run.runner.state + end + + def announce(row, state) + repo = @db[:repos].where(id: row[:repo_id]).first + body = case row[:status] + when 'blocked' then blocked_body(state) + when 'done' then "Opened ##{row[:pr_number]}." + else + "This run #{row[:status]}: #{state[:reason]}. Nothing was merged and no further " \ + 'work starts on it. Fix the cause and set Status back to `Ready`.' + end + comment(repo, row[:subject_number], body) + end + + def blocked_body(state) + questions = Array(state[:questions]) + return "Blocked at `#{state[:stage]}`: #{state[:reason]}." if questions.empty? + + ["Blocked at `#{state[:stage]}`: #{state[:reason]}.", '', + 'Answer in a reply and this run continues from where it stopped.', '', + *questions.map { |question| "- #{question}" }].join("\n") + end + + # Running or blocked: a blocked run keeps its worktree, and therefore its + # branch, until it is answered or killed. + def live_holder(repo_id, branch) + @db[:runs].where(repo_id: repo_id, branch: branch, status: %w[running blocked]).first + end + + # A blocked run holds its branch indefinitely by design, so an item waiting + # behind one can wait forever. Said once, or you comment a fix request and + # from your side mill simply ignored you. + def held(repo_row, subject_number, branch, holder) + key = [repo_row[:id], subject_number, branch] + unless @announced[key] + @announced[key] = true + comment(repo_row, subject_number, + "Waiting: run #{holder[:id]} still has `#{branch}` checked out (it is " \ + "#{holder[:status]}). This item starts as soon as that run finishes or is killed.") + end + :held + end + + # No rescue. `git worktree list` failing means mill does not know whether + # the branch is checked out, and answering "it is not" is a rescue that + # turns a failure into a pass — which is how two live checkouts of one + # branch happen. + def checked_out?(clone, branch) = @git.checked_out_branches(clone).include?(branch) + + def checked_out_block(clone, branch) + Blocked.new(problem: :branch_checked_out, questions: [ + "`#{branch}` is checked out in #{clone}. mill will not force a second working " \ + 'copy of one branch, because two live checkouts can diverge the ref without ' \ + 'either side noticing. Switch that clone to your base branch and reply here.' + ]) + end + + # A SIGKILL during git commit leaves an index or ref lock that git never + # cleans, and the next launch fails instantly on it. The branch's own ref + # lock is included whatever the branch is named — scoping this to mill/* + # would miss the plan route entirely, which adopts the branch gh made. + def clear_stale_locks(clone, branch) + dir = @git.run(clone, 'rev-parse', '--git-common-dir') + return unless dir.ok + + common = File.expand_path(dir.out.strip, clone) + (Dir[File.join(common, '*.lock')] + + Dir[File.join(common, 'worktrees', '*', '*.lock')] + + [File.join(common, 'refs', 'heads', "#{branch}.lock")]).uniq.each do |lock| + delete_if_stale(lock) + end + end + + def delete_if_stale(lock) + return unless File.file?(lock) + return if Mill.now - File.stat(lock).mtime.utc.to_i < STALE_LOCK_AFTER + + File.delete(lock) + rescue SystemCallError + nil + end + + def insert_run(repo_row, subject_kind, subject_number, route, branch, spec_path, item_id) + @db[:runs].insert( + repo_id: repo_row[:id], subject_kind: subject_kind, subject_number: subject_number, + route: route, branch: branch, spec_path: spec_path, status: 'running', + board_item_id: item_id, created_at: Mill.now + ) + end + + def attach_worktree(repo_row, run_id, branch) + path = worktree_path(repo_row, run_id) + @git.worktree_add(repo_row[:local_path], path, branch) + @db[:runs].where(id: run_id).update(worktree_path: path) + path + end + + def worktree_path(repo_row, run_id) + File.join(Mill.home, 'worktrees', "#{repo_row[:owner]}-#{repo_row[:name]}", run_id.to_s) + end + + # The transaction rolls the row back; a worktree is not transactional, so a + # directory that did appear before the failure has to go by hand or the next + # claim on this branch trips over it. + def discard(repo_row, run_id) + return if run_id.nil? + + path = worktree_path(repo_row, run_id) + @git.worktree_remove(repo_row[:local_path], path) if Dir.exist?(path) + @git.run(repo_row[:local_path], 'worktree', 'prune') + rescue Mill::Git::Error + nil + end + + def comment(repo_row, number, body) + @github.comment("#{repo_row[:owner]}/#{repo_row[:name]}", number, body) + rescue Mill::Github::Error + nil + end + end +end diff --git a/lib/mill/workers.rb b/lib/mill/workers.rb new file mode 100644 index 0000000..18bc27c --- /dev/null +++ b/lib/mill/workers.rb @@ -0,0 +1,142 @@ +module Mill + # Both loops, inside one process. Each is wrapped in a supervising loop that + # logs the exception and restarts with backoff — a factory whose poller thread + # died in the night and left no trace is worse than one that never started. + # + # Thread.report_on_exception stays at its default of true. + class Workers + # The board is a queue you touch by hand. A minute costs nothing in + # responsiveness and halves what mill spends against the GraphQL budget, + # which is measured in points rather than calls and which one board read + # per tick can consume a real share of. + DEFAULT_INTERVAL = 60 + MAX_BACKOFF = 300 + # A rate-limit window can be most of an hour away. Capped so a clock that + # disagrees with GitHub's cannot park a worker thread indefinitely. + MAX_RATE_LIMIT_WAIT = 3600 + + attr_reader :supervisor, :board + + def initialize(poller: nil, supervisor: nil, interval: nil, db: Mill.db) + @db = db + # One board and one supervisor, both shared. + # + # The supervisor is the only object holding which process groups mill + # spawned and which runs have a live thread; a second instance answers + # "none" to both and reaps healthy stages. + # + # The board has to be handed to the supervisor as well as the poller. + # Built without one, every `@board&.want` in claim, finish and interrupt + # is a silent no-op — mill runs perfectly and never writes a Status, so + # the board sits on Ready while a run works, finishes and opens a pull + # request. Measured on the first real poll, 2026-08-19. + @github = Mill::Github.new + @board = Mill::Board.new(db: db, github: @github) + @supervisor = Mill::Supervisor.new(db: db, github: @github, board: @board) + @poller_tick = poller + @supervisor_tick = supervisor + # Not `.to_f`: an empty or unparseable MILL_POLL_SECONDS would become 0.0 + # and turn the tick into a loop hammering the API as fast as it answers. + @interval = interval || + Mill.setting_float('MILL_POLL_SECONDS', default: DEFAULT_INTERVAL, min: 5, max: 3600) + @beats = {} + @threads = {} + @lock = Mutex.new + @stopping = false + end + + # A stray Ready on the board must not launch a real run against a real repo + # while somebody is editing a template. + def self.enabled? = ENV['MILL_WORKERS'].to_s.downcase != 'off' + + def start + return self unless self.class.enabled? + + @lock.synchronize do + @threads[:supervisor] = loop_thread(:supervisor, supervisor_tick) + @threads[:poller] = loop_thread(:poller, poller_tick) + end + self + end + + def stop + @stopping = true + @lock.synchronize do + @threads.each_value { |thread| thread&.kill } + @threads.clear + end + end + + # Reads a snapshot of both hashes rather than iterating live ones: this runs + # in a Puma thread while two worker threads are writing. + def health + beats = @lock.synchronize { @beats } + threads = @lock.synchronize { @threads.dup } + %i[poller supervisor].to_h do |name| + [name, (beats[name] || {}).merge(alive: threads[name]&.alive? || false)] + end + end + + private + + def poller_tick + @poller_tick || begin + poller = Mill::Poller.new(db: @db, supervisor: @supervisor, board: @board, + github: @github) + -> { poller.tick } + end + end + + def supervisor_tick = @supervisor_tick || -> { @supervisor.reap } + + def loop_thread(name, work) + Thread.new do + failures = 0 + until @stopping + begin + work.call + beat(name, nil) + failures = 0 + sleep @interval + rescue StandardError => e + failures += 1 + beat(name, "#{e.class}: #{e.message}") + warn "#{name} raised: #{e.class}: #{e.message}" + sleep backoff(failures, e) + end + end + end + end + + # The cap is in seconds, and applying it before the multiplier would make + # the real ceiling three seconds rather than five minutes. An expired token + # would then retry twelve hundred times an hour, indefinitely. + def backoff(failures, error = nil) + return rate_limit_wait if error.is_a?(Mill::Github::RateLimited) + + [@interval * (2**failures), MAX_BACKOFF].min + end + + # A rate limit is the one failure that says exactly when to try again, so + # guessing at it is strictly worse. Exponential backoff caps at five + # minutes; a GraphQL window can be forty away, which is eight more attempts + # that fail for a reason already known. The design says a rate-limited + # stage is waiting rather than working — this is the same rule for mill's + # own API access. + # + # Never shorter than one tick, in case the reset has just passed, and never + # longer than an hour, in case the clocks disagree. + def rate_limit_wait + reset = @github.rate_limit_reset or return MAX_BACKOFF + + [[reset - Mill.now, @interval].max, MAX_RATE_LIMIT_WAIT].min + end + + # @beats is written from two worker threads and read from a Puma thread. + # Replacing the hash rather than mutating it means a reader never sees it + # part-written. + def beat(name, error) + @lock.synchronize { @beats = @beats.merge(name => { at: Mill.now, error: error }) } + end + end +end diff --git a/prompts/triage.md b/prompts/triage.md index 1fbabbc..d8b1aa4 100644 --- a/prompts/triage.md +++ b/prompts/triage.md @@ -1,5 +1,7 @@ Decide what this issue is and which route it takes. You are the cheapest stage and the only one with -no reviewer, so **when the answer is not obvious, block.** +no reviewer. You judge exactly the three things below — scope, route, and whether the spec is +buildable at all — and **when any of those three is not obvious, block.** Nothing else here is +yours to judge. ## The issue @@ -24,5 +26,19 @@ no reviewer, so **when the answer is not obvious, block.** bump. One narrow category, no judgment call. - Neither: block with questions. An issue with no spec that is not obviously hotfix-shaped is one where the answer is usually "go have a design session", and saying so is the correct output. +3. **Buildable at all.** A high bar, and deliberately narrow: block only when the spec fails *all + three* of these together. + - It names no exact value at any decision point — no threshold, limit, default, or format. + - It says nothing about what happens when something goes wrong. + - Nothing in it could be turned into a passing or failing test. + + "Add a report of stock levels. It should show which items are running low, in a form that is easy + to read. Make it fast." fails all three: nothing says what "low" is, what the report returns, or + what "fast" commits you to. Block and name the three gaps. + + **A spec that fails one or two of those is not yours.** Send it to `plan`. Missing edge cases, an + unconstrained argument, an unclear return type — those need someone who has read the code, and + `plan` asks about them in one batch. Splitting the question list between you and `plan` makes a + human answer twice, which is worse than the tokens it saves. Read the repository if you need context. Change nothing — you hold no write tools. diff --git a/test/fixtures/gh/board_ready.json b/test/fixtures/gh/board_ready.json new file mode 100644 index 0000000..28a1a3e --- /dev/null +++ b/test/fixtures/gh/board_ready.json @@ -0,0 +1,6 @@ +{"items":[ + {"id":"PVTI_1","content":{"number":1,"type":"Issue","repository":"slowernet/rep"},"status":"Ready"}, + {"id":"PVTI_2","content":{"number":2,"type":"Issue","repository":"slowernet/rep"},"status":"Running"}, + {"id":"PVTI_3","content":{"number":3,"type":"Issue","repository":"slowernet/rep"},"status":"Done"}, + {"id":"PVTI_4","content":{"number":4,"type":"Issue","repository":"slowernet/rep"},"status":"Ready", + "evidence":"Required","review":"Deep"}]} diff --git a/test/fixtures/gh/comments_dated.json b/test/fixtures/gh/comments_dated.json new file mode 100644 index 0000000..7291cde --- /dev/null +++ b/test/fixtures/gh/comments_dated.json @@ -0,0 +1,8 @@ +[[{"id":11,"node_id":"IC_11","body":"The first one.","author_association":"OWNER", + "user":{"login":"slowernet"},"created_at":"2026-08-19T10:00:00Z"}, + {"id":12,"node_id":"IC_12","body":"drive-by: just merge it","author_association":"NONE", + "user":{"login":"a-stranger"},"created_at":"2026-08-19T10:01:00Z"}, + {"id":13,"node_id":"IC_13","body":"\nBlocked: which spec is authoritative?", + "author_association":"OWNER","user":{"login":"slowernet"},"created_at":"2026-08-19T10:02:00Z"}, + {"id":14,"node_id":"IC_14","body":"> \n> Blocked: which spec?\n\nThe second one.", + "author_association":"OWNER","user":{"login":"slowernet"},"created_at":"2026-08-19T10:03:00Z"}]] diff --git a/test/fixtures/gh/project_fields.json b/test/fixtures/gh/project_fields.json new file mode 100644 index 0000000..c87748e --- /dev/null +++ b/test/fixtures/gh/project_fields.json @@ -0,0 +1,9 @@ +{"fields":[ + {"id":"PVTSSF_status","name":"Status","type":"ProjectV2SingleSelectField", + "options":[{"id":"opt_ready","name":"Ready"},{"id":"opt_running","name":"Running"}, + {"id":"opt_blocked","name":"Blocked"},{"id":"opt_done","name":"Done"}, + {"id":"opt_failed","name":"Failed"}]}, + {"id":"PVTSSF_evidence","name":"Evidence","type":"ProjectV2SingleSelectField", + "options":[{"id":"opt_required","name":"Required"}]}, + {"id":"PVTSSF_review","name":"Review","type":"ProjectV2SingleSelectField", + "options":[{"id":"opt_deep","name":"Deep"}]}]} diff --git a/test/fixtures/gh/project_view.json b/test/fixtures/gh/project_view.json new file mode 100644 index 0000000..f1a58c1 --- /dev/null +++ b/test/fixtures/gh/project_view.json @@ -0,0 +1 @@ +{"id":"PVT_board","number":3,"title":"mill","owner":{"login":"slowernet"}} diff --git a/test/mill/test_board.rb b/test/mill/test_board.rb new file mode 100644 index 0000000..705745f --- /dev/null +++ b/test/mill/test_board.rb @@ -0,0 +1,201 @@ +require 'test_helper' + +module Mill + # Fixture-backed. Nothing here reaches the network. + class TestBoard < Mill::TestCase + FIXTURES = File.join(__dir__, '..', 'fixtures', 'gh') + + def fixture(name) = File.read(File.join(FIXTURES, "#{name}.json")) + + # Answers each gh call from a fixture chosen by its subcommand, and records + # every call so the writes can be asserted. + def github(failing: false, fields: nil, &before_edit) + calls = [] + gh = Mill::Github.new(runner: lambda { |args| + calls << args + if args[1] == 'item-edit' + before_edit&.call + raise Mill::Github::Error, 'network is down' if failing + end + + case args[1] + when 'view' then fixture('project_view') + when 'field-list' then fields || fixture('project_fields') + when 'item-list' then fixture('board_items') + else '' + end + }) + [gh, calls] + end + + def board(**opts, &blk) + gh, calls = github(**opts, &blk) + [Mill::Board.new(db: db, github: gh, project: 3, owner: 'slowernet'), calls] + end + + def a_run(status: 'running', item: 'PVTI_1', number: 1) + create_run(repo_id: (@repo_id ||= create_repo), status: status, + subject_number: number, board_item_id: item) + end + + def test_writing_a_status_names_the_option_by_id + run_id = a_run + b, calls = board + + assert b.want(run_id, 'running') + edit = calls.find { |args| args[1] == 'item-edit' } + + assert_includes edit, 'PVTI_1' + assert_includes edit, 'PVTSSF_status' + assert_includes edit, 'opt_running' + assert_includes edit, 'PVT_board' + end + + def test_a_killed_run_reads_as_failed_on_the_board + run_id = a_run(status: 'killed') + b, calls = board + b.want(run_id, 'killed') + + assert_includes calls.find { |args| args[1] == 'item-edit' }, 'opt_failed' + end + + # The whole mechanism: an unconfirmed write is what redrive looks for. + def test_a_failed_write_leaves_the_run_unconfirmed + run_id = a_run + b, = board(failing: true) + + refute b.want(run_id, 'blocked') + row = db[:runs].where(id: run_id).first + + assert_equal 'Blocked', row[:desired_board_status] + assert_nil row[:board_status_at] + end + + def test_redrive_retries_what_was_never_confirmed + run_id = a_run + failing, = board(failing: true) + failing.want(run_id, 'blocked') + + ok, calls = board + ok.redrive + + refute_nil db[:runs].where(id: run_id).get(:board_status_at) + assert_includes calls.find { |args| args[1] == 'item-edit' }, 'opt_blocked' + end + + def test_redrive_leaves_a_confirmed_run_alone + run_id = a_run + b, = board + b.want(run_id, 'running') + + again, calls = board + again.redrive + + assert_nil calls.find { |args| args[1] == 'item-edit' } + end + + def test_redrive_leaves_a_run_that_was_never_asked_for_alone + a_run + b, calls = board + b.redrive + + assert_nil calls.find { |args| args[1] == 'item-edit' } + end + + # redrive runs in the poller thread while run threads decide. A label read + # a moment ago may already be stale, and stamping board_status_at against a + # stale one is exactly what would stop it ever being retried — leaving a + # blocked run behind a board that says Running, where the answer to its + # questions is never read as an answer. + def test_a_decision_that_changed_underneath_a_write_is_not_confirmed + run_id = a_run + db[:runs].where(id: run_id).update(desired_board_status: 'Running', + board_status_at: nil) + + racing, = board do + db[:runs].where(id: run_id).update(desired_board_status: 'Blocked', + board_status_at: nil) + end + + refute racing.send(:confirm, run_id) + assert_nil db[:runs].where(id: run_id).get(:board_status_at) + end + + # A run with no board item was started by hand. It must not raise, and it + # must not read as confirmed either. + def test_a_run_with_no_board_item_is_not_confirmed + run_id = a_run(item: nil) + b, calls = board + + refute b.want(run_id, 'done') + assert_nil calls.find { |args| args[1] == 'item-edit' } + assert_nil db[:runs].where(id: run_id).get(:board_status_at) + end + + # A board missing an option is a configuration error rather than a network + # blip, and must not be swallowed into an endless retry. + def test_a_board_missing_a_status_option_raises + run_id = a_run + b, = board(fields: + '{"fields":[{"id":"F","name":"Status","options":[{"id":"1","name":"Ready"}]}]}') + + error = assert_raises(Mill::Error) { b.want(run_id, 'running') } + + assert_match(/Blocked/, error.message) + end + + def test_a_board_with_no_status_field_raises + run_id = a_run + b, = board(fields: '{"fields":[{"id":"F","name":"Evidence","options":[]}]}') + + assert_raises(Mill::Error) { b.want(run_id, 'running') } + end + + # Board automation writing Status under a live run is what the runbook + # disables. Catching it later is what catches it being re-enabled. + def test_a_status_mill_did_not_write_is_interference + run_id = a_run + b, = board + b.want(run_id, 'running') + row = db[:runs].where(id: run_id).first + + assert b.interference?({ id: 'PVTI_1', status: 'Done' }, row) + refute b.interference?({ id: 'PVTI_1', status: 'Running' }, row) + end + + def test_another_items_status_is_not_this_runs_interference + run_id = a_run + b, = board + b.want(run_id, 'running') + row = db[:runs].where(id: run_id).first + + refute b.interference?({ id: 'PVTI_2', status: 'Done' }, row) + end + + def test_field_and_option_ids_are_resolved_once + run_id = a_run + b, calls = board + b.want(run_id, 'running') + b.want(run_id, 'done') + + assert_equal 1, calls.count { |args| args[1] == 'field-list' } + end + + def test_an_unconfigured_board_writes_nothing + run_id = a_run + gh, calls = github + b = Mill::Board.new(db: db, github: gh, project: nil, owner: nil) + + refute b.configured? + refute b.want(run_id, 'running') + assert_empty calls + end + + def test_an_unknown_run_status_has_no_board_status + run_id = a_run + b, = board + + assert_raises(Mill::Error) { b.want(run_id, 'queued') } + end + end +end diff --git a/test/mill/test_doctor.rb b/test/mill/test_doctor.rb index 5cca965..99483a7 100644 --- a/test/mill/test_doctor.rb +++ b/test/mill/test_doctor.rb @@ -304,5 +304,114 @@ def test_a_red_doctor_is_not_ok refute_predicate doctor(home), :ok? end end + + # --- Plan 3a's preconditions -------------------------------------------- + + def teardown + %w[MILL_CLONES MILL_BIND MILL_ADMIN_EMAILS].each { |name| ENV.delete(name) } + end + + # These values reach a subprocess environment, and the runbook is the only + # thing that ever said to chmod them. + def test_a_world_readable_secrets_file_fails + with_home do |home| + path = File.join(home, 'secrets', 'slowernet-rep.env') + File.write(path, "A=1\n") + FileUtils.chmod(0o644, path) + + assert_match(/slowernet-rep\.env/, check(home, 'secrets files are 0600').detail) + end + end + + def test_correctly_moded_secrets_pass + with_home do |home| + path = File.join(home, 'secrets', 'slowernet-rep.env') + File.write(path, "A=1\n") + FileUtils.chmod(0o600, path) + + assert_predicate check(home, 'secrets files are 0600'), :ok + end + end + + # A root that does not exist silently becomes "clone it myself" for every + # repo, and mill then works in a checkout nobody is looking at. + def test_a_clone_root_that_does_not_exist_is_named + ENV['MILL_CLONES'] = '/no/such/place' + + with_home do |home| + assert_match(%r{/no/such/place}, check(home, 'clone roots exist').detail) + end + end + + def test_no_clone_roots_is_not_a_failure + ENV['MILL_CLONES'] = '' + + with_home do |home| + assert_predicate check(home, 'clone roots exist'), :ok + end + end + + # The write paths are a kill switch and a worktree deleter. On loopback the + # interface is the boundary; anywhere else the allowlist is all there is. + def test_a_public_bind_with_no_admin_list_fails + ENV['MILL_BIND'] = 'tcp://0.0.0.0:9494' + + with_home do |home| + refute_predicate check(home, 'bind is loopback or guarded'), :ok + end + end + + def test_a_public_bind_with_an_admin_list_passes + ENV['MILL_BIND'] = 'tcp://0.0.0.0:9494' + ENV['MILL_ADMIN_EMAILS'] = 'eshepard@slower.net' + + with_home do |home| + assert_predicate check(home, 'bind is loopback or guarded'), :ok + end + end + + def test_the_default_loopback_bind_passes + with_home do |home| + assert_predicate check(home, 'bind is loopback or guarded'), :ok + end + end + + # mill writes five Status values, and a board missing one fails at the + # moment it matters — a run blocking, or finishing — rather than at setup. + def test_a_board_missing_a_status_option_is_named + gh = Mill::Github.new(runner: lambda { |args| + next '{"fields":[{"id":"F","name":"Status","options":[{"id":"1","name":"Ready"}]}]}' if + args[1] == 'field-list' + + '{"data":{"user":{"projectV2":{"workflows":{"nodes":[]}}}}}' + }) + + with_home do |home| + checked = Mill::Doctor.new(home: home, github: gh, project: '3', + project_owner: 'slowernet').run + found = checked.checks.find { |c| c.name == 'board Status has every option mill writes' } + + refute_predicate found, :ok + assert_match(/Blocked/, found.detail) + end + end + + def test_a_complete_board_passes_its_option_check + gh = Mill::Github.new(runner: lambda { |args| + next File.read(File.join(__dir__, '..', 'fixtures', 'gh', 'project_fields.json')) if + args[1] == 'field-list' + + '{"data":{"user":{"projectV2":{"workflows":{"nodes":[]}}}}}' + }) + + with_home do |home| + checked = Mill::Doctor.new(home: home, github: gh, project: '3', + project_owner: 'slowernet').run + + assert_predicate checked.checks.find { |c| + c.name == 'board Status has every option mill writes' + }, :ok + end + end end end diff --git a/test/mill/test_ledger.rb b/test/mill/test_ledger.rb index 955e667..9c3a9ba 100644 --- a/test/mill/test_ledger.rb +++ b/test/mill/test_ledger.rb @@ -12,13 +12,89 @@ def setup end # The smallest thing shaped like a Mill::Claude::Attempt. - def attempt(status: 'ok', valid: true, success: true, resume_failed: false) + def attempt(status: 'ok', valid: true, success: true, resume_failed: false, + rate_limited: false) verdict = Object.new verdict.define_singleton_method(:valid?) { valid } verdict.define_singleton_method(:status) { status } result = Object.new result.define_singleton_method(:success?) { success } - Struct.new(:verdict, :result, :resume_failed?).new(verdict, result, resume_failed) + Struct.new(:verdict, :result, :resume_failed?, :rate_limited?) + .new(verdict, result, resume_failed, rate_limited) + end + + # A launch the subscription refused never ran, so classified after :crashed + # it would take a strike for a door mill could not open — measured live + # 2026-08-20, when a five-hour window closed mid-run. + # + # What marks it is the empty verdict, not the exit status: with no result + # line there is no payload, so Verdict.validate fails it. Nothing here has + # measured what such a launch exits with, which is why these fixtures no + # longer claim one. + def test_a_refused_launch_is_rate_limited_not_crashed + assert_equal :rate_limited, + Mill::Ledger.classify(attempt(success: false, valid: false, rate_limited: true)) + end + + def test_a_rate_limited_launch_costs_neither_an_attempt_nor_a_strike + assert_equal({ attempt: 0, strike: 0 }, Mill::Ledger::COST[:rate_limited]) + end + + # The limit outranks a session that would not reopen: mill never got far + # enough to try the session. Neither refusal produces a verdict. + def test_the_limit_outranks_a_failed_resume + assert_equal :rate_limited, + Mill::Ledger.classify(attempt(success: false, valid: false, rate_limited: true, + resume_failed: true)) + end + + # rate_limited? reports the last rate-limit event the stream saw, not the + # fate of the launch: an "allowed" heartbeat clears it, and a refusal sets + # it again. A stage refused at minute 2, that then got its launch, exited + # 0 and returned a valid verdict, still carries the flag whenever the + # result line arrives before the next heartbeat could clear it. Reading + # the flag as "this launch was refused" throws that finished work away, + # and charges no attempt — so the relaunch reuses the log filename and + # overwrites the log of the run that succeeded. + def test_a_throttled_stage_that_still_finished_keeps_its_work + assert_equal :ok, Mill::Ledger.classify(attempt(success: true, rate_limited: true)) + end + + # PINS A KNOWN-BAD STATE. Delete this and re-price it when the row-insertion + # fix lands; it is here so the behaviour is visible rather than merely + # absent, not because it is right. + # + # A launch that ran, hit the window partway, and handed back nothing looks + # from here exactly like one that was refused outright, so it is priced as + # a refusal: no row, no attempt number, and therefore the relaunch reuses + # the log filename and truncates the log of the twenty minutes that did + # happen. The session goes with it — `reload` rebuilds `@sessions` from + # `stage_attempts`, and there is no row to rebuild from. + # + # Charging it a strike instead is not the answer either: nothing measures + # what a refusal exits with, and charging on an unproven premise is how a + # stage gets blocked for a door mill could not open. The fix is to tell the + # two apart from the stream — a session id, a model, any turns at all — + # and price a launch that happened as an attempt that cost no strike. + # + # Note the wait cap is no comfort here: `@rate_limit_waits` lives on the + # Runner instance, so a restarted or resumed run starts counting again with + # nothing in the database to reconstruct it from. + def test_a_throttled_stage_that_did_work_and_said_nothing_is_priced_as_a_refusal + assert_equal :rate_limited, + Mill::Ledger.classify(attempt(success: true, rate_limited: true, valid: false)) + end + + # The one combination this change re-priced upward, pinned so the decision + # is visible. A readable verdict means the stage produced something, so the + # limit is not what stopped it, and a non-zero exit on top of that is a + # crash like any other. Thin on the ground — it needs a valid result line + # and a bad exit and a still-rejected limit — and if it turns out to be + # reachable in a way that is mill's fault rather than the stage's, this is + # the test that should argue about it. + def test_a_readable_verdict_with_a_bad_exit_is_a_crash_even_under_a_limit + assert_equal :crashed, + Mill::Ledger.classify(attempt(success: false, valid: true, rate_limited: true)) end # --- classification ------------------------------------------------- diff --git a/test/mill/test_poller.rb b/test/mill/test_poller.rb new file mode 100644 index 0000000..5e3a378 --- /dev/null +++ b/test/mill/test_poller.rb @@ -0,0 +1,468 @@ +require 'test_helper' + +module Mill + # Fixture-backed. The supervisor is a stub: claiming is Task 6's business and + # is tested there against real git. + class TestPoller < Mill::TestCase + FIXTURES = File.join(__dir__, '..', 'fixtures', 'gh') + + def fixture(name) = File.read(File.join(FIXTURES, "#{name}.json")) + + # Records what it was asked to claim and answers with an incrementing id. + class FakeSupervisor + attr_reader :claimed, :started, :answers + + def initialize(answer: nil, capped: false) + @claimed = [] + @started = [] + @answers = {} + @answer = answer + @capped = capped + @next = 100 + end + + def at_cap? = @capped + + def running?(_run_id) = false + + def claim(**args) + @claimed << args + return @answer if @answer + + @next += 1 + end + + def start(run_id, **kwargs) + @started << run_id + @answers[run_id] = kwargs[:answers] + end + end + + def located(branch: 'x', path: 'docs/s.md', problem: nil) + Mill::Spec::Located.new(branch: branch, path: path, problem: problem) + end + + def poller(supervisor: FakeSupervisor.new, locator: nil, preparer: nil, calls: []) + gh = Mill::Github.new(runner: lambda { |args| + calls << args + args[1] == 'item-list' ? fixture('board_ready') : '' + }) + board = Mill::Board.new(db: db, github: gh, project: 3, owner: 'slowernet') + Mill::Poller.new(db: db, github: gh, board: board, supervisor: supervisor, + locator: locator || ->(*) { located }, + preparer: preparer || ->(**) { Mill::Repo::Result.new(path: '/tmp/rep') }) + end + + def prepared_repo + create_repo(owner: 'slowernet', name: 'rep', local_path: '/tmp/rep', + base_branch: 'main', prepared_at: Mill.now) + end + + def bodies(calls) + calls.select { |args| args.first(2) == %w[issue comment] }.map { |args| args.join(' ') } + end + + def test_only_ready_items_are_claimed + prepared_repo + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_equal [1, 4], sup.claimed.map { |c| c[:subject_number] }.sort + end + + def test_an_item_with_an_active_run_is_left_alone + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'running') + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_equal [4], sup.claimed.map { |c| c[:subject_number] } + end + + # A blocked run still guards its subject: resume is comment-triggered, so a + # second run would take the branch and the answer would find nothing. + def test_a_blocked_run_still_guards_its_subject + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_equal [4], sup.claimed.map { |c| c[:subject_number] } + end + + def test_a_finished_run_does_not_guard_its_subject + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'done') + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_includes sup.claimed.map { |c| c[:subject_number] }, 1 + end + + def test_nothing_is_claimed_at_the_cap + prepared_repo + sup = FakeSupervisor.new(capped: true) + poller(supervisor: sup).reconcile + + assert_empty sup.claimed + end + + def test_the_board_item_id_reaches_the_run + prepared_repo + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_equal %w[PVTI_1 PVTI_4], sup.claimed.map { |c| c[:board_item_id] }.sort + end + + def test_a_claimed_run_is_started + prepared_repo + sup = FakeSupervisor.new + poller(supervisor: sup).reconcile + + assert_equal sup.claimed.length, sup.started.length + end + + # The item waits rather than failing: the run holding the branch will + # finish or be reaped. + def test_a_held_item_starts_nothing + prepared_repo + sup = FakeSupervisor.new(answer: :held) + poller(supervisor: sup).reconcile + + assert_empty sup.started + end + + def test_a_blocked_claim_is_reported_and_starts_nothing + prepared_repo + calls = [] + blocked = Mill::Supervisor::Blocked.new(problem: :branch_checked_out, + questions: ['switch your clone off it']) + sup = FakeSupervisor.new(answer: blocked) + poller(supervisor: sup, calls: calls).reconcile + + assert_empty sup.started + assert_match(/switch your clone off it/, bodies(calls).first) + end + + # An unprepared repo blocks that one item and names what is missing. + def test_an_unpreparable_repo_blocks_only_its_own_item + calls = [] + sup = FakeSupervisor.new + preparer = ->(**) do + Mill::Repo::Result.new(problem: :missing_secrets, questions: ['API_KEY is missing']) + end + poller(supervisor: sup, preparer: preparer, calls: calls).reconcile + + assert_empty sup.claimed + assert_match(/API_KEY/, bodies(calls).first) + end + + # :no_spec carries no questions, so the generic block comment would post a + # heading over an empty list and read as a bug in mill. + def test_an_item_with_no_spec_is_told_what_to_commit + prepared_repo + calls = [] + poller(locator: ->(*) { located(path: nil, problem: :no_spec) }, calls: calls).reconcile + + body = bodies(calls).first + + assert_match(%r{docs/superpowers/specs/}, body) + refute_match(/^- $/, body) + end + + def test_an_item_with_no_linked_branch_is_told_to_make_one + prepared_repo + calls = [] + poller(locator: ->(*) { located(branch: nil, path: nil, problem: :no_branch) }, + calls: calls).reconcile + + assert_match(/gh issue develop/, bodies(calls).first) + end + + # An ambiguous branch is a real question, and Located already words it. + def test_an_ambiguous_branch_asks_its_own_question + prepared_repo + calls = [] + ambiguous = Mill::Spec::Located.new(problem: :many_branches, detail: 'a, b') + poller(locator: ->(*) { ambiguous }, calls: calls).reconcile + + assert_match(/more than one linked branch/, bodies(calls).first) + end + + # Silence is never success: a board mill could not read is not an empty + # board, and treating it as one would look like there being no work. + def test_an_unreadable_board_raises_rather_than_reading_as_empty + gh = Mill::Github.new(runner: ->(_) { raise Mill::Github::Unauthorized, 'bad token' }) + board = Mill::Board.new(db: db, github: gh, project: 3, owner: 'slowernet') + p = Mill::Poller.new(db: db, github: gh, board: board, supervisor: FakeSupervisor.new) + + assert_raises(Mill::Github::Unauthorized) { p.reconcile } + end + + def test_an_unconfigured_board_is_not_polled + calls = [] + gh = Mill::Github.new(runner: ->(args) { calls << args; '' }) + board = Mill::Board.new(db: db, github: gh, project: nil, owner: nil) + p = Mill::Poller.new(db: db, github: gh, board: board, supervisor: FakeSupervisor.new) + p.reconcile + + assert_empty calls + end + + # There must be exactly one supervisor in the process: it alone knows which + # process groups mill spawned and which runs have a live thread. + def test_a_poller_will_not_invent_its_own_supervisor + assert_raises(ArgumentError) { Mill::Poller.new(db: db) } + end + + # --- the comment sweep --------------------------------------------------- + + def sweeping(sup: FakeSupervisor.new, calls: [], failing: false) + gh = Mill::Github.new(runner: lambda { |args| + calls << args + if args.first == 'api' + raise Mill::Github::Error, 'boom' if failing + + next fixture('comments_dated') + end + args[1] == 'item-list' ? fixture('board_ready') : '' + }) + board = Mill::Board.new(db: db, github: gh, project: 3, owner: 'slowernet') + Mill::Poller.new(db: db, github: gh, board: board, supervisor: sup) + end + + def blocked_subject + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + repo_id + end + + def test_a_trusted_comment_becomes_an_event + blocked_subject + sweeping.sweep + + assert_equal 1, db[:events].where(gh_node_id: 'IC_11').count + end + + # Comment text becomes prompt text, and a subprocess holds real credentials. + def test_a_stranger_starts_nothing + blocked_subject + sweeping.sweep + + assert_equal 0, db[:events].where(gh_node_id: 'IC_12').count + end + + def test_mills_own_comment_is_not_a_trigger + blocked_subject + sweeping.sweep + + assert_equal 0, db[:events].where(gh_node_id: 'IC_13').count + end + + # GitHub's quote-reply copies the source markdown including HTML comments, + # so a whole-body search for the marker would silently discard the only + # channel in the design that reaches a person. + def test_a_quote_reply_carrying_the_marker_is_still_your_answer + blocked_subject + sweeping.sweep + + assert_equal 1, db[:events].where(gh_node_id: 'IC_14').count + end + + def test_the_same_comment_is_never_recorded_twice + blocked_subject + p = sweeping + p.sweep + p.sweep + + assert_equal 1, db[:events].where(gh_node_id: 'IC_11').count + end + + def test_the_cursor_advances_after_a_complete_sweep + repo_id = blocked_subject + sweeping.sweep + + assert_equal '2026-08-19T10:03:00Z', db[:repos].where(id: repo_id).get(:comments_cursor) + end + + # A fetch that stops partway must write no cursor, or the comments it never + # saw are skipped forever. + def test_a_failed_fetch_leaves_the_cursor_alone + repo_id = blocked_subject + p = sweeping(failing: true) + + assert_raises(Mill::Github::Error) { p.sweep } + assert_nil db[:repos].where(id: repo_id).get(:comments_cursor) + end + + # Without `since` a blocked run re-fetches its whole comment history every + # tick, which on a busy issue ends in a rate limit. + def test_the_cursor_is_sent_to_github_rather_than_only_filtering_here + repo_id = blocked_subject + db[:repos].where(id: repo_id).update(comments_cursor: '2026-08-19T09:00:00Z') + calls = [] + sweeping(calls: calls).sweep + + assert(calls.any? { |args| args[1].to_s.include?('since=2026-08-19T09:00:00Z') }) + end + + # Bounded deliberately: subjects mill has a live run on, not every issue in + # every repo the board touches. + def test_only_interesting_subjects_are_swept + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + create_run(repo_id: repo_id, subject_number: 9, status: 'done') + calls = [] + sweeping(calls: calls).sweep + fetched = calls.select { |args| args.first == 'api' }.map { |args| args[1] } + + assert(fetched.any? { |url| url.include?('/issues/1/comments') }) + refute(fetched.any? { |url| url.include?('/issues/9/comments') }) + end + + def test_a_repo_with_nothing_live_is_not_swept + prepared_repo + calls = [] + sweeping(calls: calls).sweep + + assert_empty calls.select { |args| args.first == 'api' } + end + + # --- dispatch ------------------------------------------------------------ + + def pending_event(repo_id, number, body: 'The second one.', node: 'IC_99') + db[:events].insert(repo_id: repo_id, kind: 'comment', gh_node_id: node, + payload_json: { body: body, subject_number: number, subject_kind: 'issue', + author_association: 'OWNER' }.to_json, + attempts: 0, state: 'pending', created_at: Mill.now) + end + + def test_a_comment_on_a_blocked_run_resumes_it + repo_id = prepared_repo + run_id = create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sup = FakeSupervisor.new + sweeping(sup: sup).dispatch + + assert_equal [run_id], sup.started + end + + def test_the_answer_reaches_the_run + repo_id = prepared_repo + run_id = create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1, body: 'Use the second spec.') + sup = FakeSupervisor.new + sweeping(sup: sup).dispatch + + assert_equal ['Use the second spec.'], sup.answers[run_id] + end + + def test_an_acted_event_is_marked_processed + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sweeping.dispatch + row = db[:events].where(gh_node_id: 'IC_99').first + + assert_equal 'processed', row[:state] + refute_nil row[:processed_at] + end + + # Only two of the five triggers have a route. The rest are recorded and + # logged rather than acted on or silently dropped. + def test_a_comment_with_no_route_is_recorded_and_left + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'running') + pending_event(repo_id, 1) + sup = FakeSupervisor.new + sweeping(sup: sup).dispatch + + assert_empty sup.started + assert_equal 'no_route', db[:events].where(gh_node_id: 'IC_99').get(:state) + end + + # A failed start must not swallow the answer: fail_event is the + # compensation for having marked it processed first. + # + # The supervisor is real here, because the bug is in the order it does two + # things. `start` calls `resumed`, which flips the row to running and only + # then tells the board — and a project whose Status field has no matching + # option raises out of that second call. The old fake raised before + # touching the row, so it could never see this. + # + # Both assertions have to be here. The event alone reads pending while the + # answer is already lost; the second sweep alone would pass only if the + # repair happens in the poller. A fix in either place satisfies both. + def test_a_failed_start_leaves_the_answer_to_be_retried + repo_id = prepared_repo + run_id = create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sweeping(sup: supervisor_with_a_broken_board).dispatch + row = db[:events].where(gh_node_id: 'IC_99').first + + assert_equal 'pending', row[:state] + assert_nil row[:processed_at] + assert_equal 'blocked', db[:runs].where(id: run_id).get(:status), + 'a run left running with no thread holds a slot nothing releases' + + retried = FakeSupervisor.new + sweeping(sup: retried).dispatch + + assert_equal [run_id], retried.started, 'the answer was never delivered' + assert_equal ['The second one.'], retried.answers[run_id] + end + + # Mill::Board#want raises Mill::Error when the project has no option for + # the Status it was asked for. Everything else about this supervisor is + # the real one, including the order in which `resumed` writes and calls. + def supervisor_with_a_broken_board + board = Object.new + board.define_singleton_method(:want) do |*| + raise Mill::Error, 'the project\'s Status field has no `In progress` option' + end + Mill::Supervisor.new(db: db, github: Mill::Github.new(runner: ->(_args) { '' }), + board: board) + end + + # One walker per run. A retried event must not become a second thread in + # the same worktree. + def test_a_run_already_walking_is_never_started_twice + repo_id = prepared_repo + run_id = create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sup = FakeSupervisor.new + sup.define_singleton_method(:running?) { |id| id == run_id } + sweeping(sup: sup).dispatch + + assert_empty sup.started + end + + def test_an_event_that_keeps_raising_dies_rather_than_retrying_forever + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sup = FakeSupervisor.new + def sup.start(*) = raise(Mill::Error, 'nope') + p = sweeping(sup: sup) + (Mill::Poller::MAX_EVENT_ATTEMPTS + 1).times { p.dispatch } + row = db[:events].where(gh_node_id: 'IC_99').first + + assert_equal 'dead', row[:state] + assert_match(/nope/, row[:last_error]) + end + + # A blocked run is not counted as running until its own thread says so, so + # ten answers at once would otherwise start ten route walks. + def test_the_cap_binds_on_resumes_too + repo_id = prepared_repo + create_run(repo_id: repo_id, subject_number: 1, status: 'blocked') + pending_event(repo_id, 1) + sup = FakeSupervisor.new(capped: true) + sweeping(sup: sup).dispatch + + assert_empty sup.started + assert_equal 'pending', db[:events].where(gh_node_id: 'IC_99').get(:state) + end + end +end diff --git a/test/mill/test_prompts.rb b/test/mill/test_prompts.rb index fbb41f6..09c482d 100644 --- a/test/mill/test_prompts.rb +++ b/test/mill/test_prompts.rb @@ -10,6 +10,34 @@ def test_every_stage_on_the_plan_route_has_a_prompt end end + # A prompt is wrapped prose, so a phrase worth asserting on will usually + # straddle a line break. Compare against the text with its whitespace + # collapsed rather than writing the assertion around the wrapping, which + # breaks the moment anyone rewraps a paragraph. + def flowed(stage, **context) = prompt(stage, **context).gsub(/\s+/, ' ') + + # triage judges three things and nothing else. An opener telling it to block + # whenever "the answer" is not obvious, followed by a closed list, tells it + # two different things — and the widest reading is the one that costs a + # round trip on a spec only `plan` could have judged. + def test_triage_is_told_exactly_what_it_judges + body = flowed('triage', issue: 'x') + + assert_includes body, 'scope, route, and whether the spec is buildable at all' + assert_includes body, 'Nothing else here is yours to judge' + end + + # The bar is deliberately high. A spec failing one or two of the three + # belongs to `plan`, which reads the code and asks in one batch — splitting + # the question list makes a human answer twice. + def test_triage_blocks_only_on_a_spec_that_is_unbuildable + body = flowed('triage', issue: 'x') + + assert_includes body, 'block only when the spec fails *all three* of these together' + assert_includes body, 'A spec that fails one or two of those is not yours' + assert_includes body, 'makes a human answer twice' + end + # Claude Code never has to guess which skill to load — the quickstart warns # about exactly that guessing. def test_a_stage_prompt_names_its_own_skill diff --git a/test/mill/test_repo.rb b/test/mill/test_repo.rb new file mode 100644 index 0000000..e05102e --- /dev/null +++ b/test/mill/test_repo.rb @@ -0,0 +1,361 @@ +require 'test_helper' +require 'tmpdir' +require 'fileutils' + +module Mill + # Real git in a tmpdir, no network: the "remote" is a bare repository on disk. + class TestRepo < Mill::TestCase + def setup + super + @root = Dir.mktmpdir('mill-repo') + @home = File.join(@root, 'home') + @clones = File.join(@root, 'code') + FileUtils.mkdir_p([@home, @clones]) + Mill.instance_variable_set(:@home, @home) + ENV['MILL_CLONES'] = @clones + disable_invented_identity + @origin = build_origin + end + + def teardown + restore_invented_identity + FileUtils.remove_entry(@root, true) + Mill.instance_variable_set(:@home, nil) + ENV.delete('MILL_CLONES') + super + end + + # A clone does not inherit an identity from the repository it came from, and + # CI runs as an account that has none to fall back on. On a developer's + # machine git invents one from the account name, so a clone that cannot + # commit still looks healthy here and dies on the runner. useConfigOnly turns + # the invention off, which is what the runner effectively has — these tests + # now see what CI sees. + # Config is only half of it: GIT_AUTHOR_NAME and its three companions sit + # above config and useConfigOnly does not touch them, so a machine exporting + # them would satisfy every commit here and still ship a clone the runner + # cannot commit in. They are cleared with it. + GIT_IDENTITY_ENV = %w[GIT_CONFIG_GLOBAL GIT_CONFIG_SYSTEM GIT_AUTHOR_NAME + GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL EMAIL].freeze + + def disable_invented_identity + @ambient_git = ENV.values_at(*GIT_IDENTITY_ENV) + path = File.join(@root, 'gitconfig') + File.write(path, "[user]\n\tuseConfigOnly = true\n") + GIT_IDENTITY_ENV.each { |key| ENV[key] = nil } + ENV['GIT_CONFIG_GLOBAL'] = path + ENV['GIT_CONFIG_NOSYSTEM'] = '1' + end + + # teardown runs even when setup raised, and clearing what was never captured + # would take a developer's real GIT_CONFIG_GLOBAL with it for the rest of the + # process. + def restore_invented_identity + ENV.delete('GIT_CONFIG_NOSYSTEM') + return unless @ambient_git + + GIT_IDENTITY_ENV.each_with_index { |key, i| ENV[key] = @ambient_git[i] } + @ambient_git = nil + end + + # A bare repo standing in for github.com/slowernet/rep. The path has to end + # in owner/name.git, because that is what Repo.slug reads — a bare repo at + # some arbitrary tmpdir path would not resolve to the right slug and the + # test would be exercising nothing. + def build_origin(owner = 'slowernet', name = 'rep') + path = File.join(@root, 'remote', owner, "#{name}.git") + FileUtils.mkdir_p(File.dirname(path)) + Mill::Git.run!(seed, 'clone', '--bare', seed, path) + path + end + + def seed + @seed ||= begin + path = File.join(@root, 'seed') + Mill::Git.clone_init(path) + File.write(File.join(path, 'README.md'), "# seed\n") + Mill::Git.run!(path, 'add', '-A') + Mill::Git.run!(path, 'commit', '-m', 'first') + path + end + end + + # git clone copies no config, so a clone has no identity of its own and mill + # gives it none — what name mill's own commits should carry is a separate + # open question. A test that commits into a clone supplies one itself, the + # same way clone_init does for the repositories the tests build. + # + # Called from both here and commit_to_base: Repo.resolve makes clones too, + # and the first test that commits into one of those would otherwise be green + # here and red on the runner all over again. Re-running git config is free. + def identify(path) + Mill::Git.run!(path, 'config', 'user.email', 'test@example.com') + Mill::Git.run!(path, 'config', 'user.name', 'Test') + path + end + + def place_clone(dir_name, origin_url = @origin) + identify(Mill::Git.clone(origin_url, File.join(@clones, dir_name))) + end + + def test_one_matching_clone_is_used_as_it_stands + expected = place_clone('rep') + + result = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + assert_predicate result, :ok? + assert_equal expected, result.path + end + + # Choosing between two silently means working in a checkout you did not + # pick, and committing to it for the whole run. + def test_two_matching_clones_block_rather_than_choosing + place_clone('rep') + place_clone('rep-again') + + result = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + refute_predicate result, :ok? + assert_equal :ambiguous_clone, result.problem + assert_match(/more than one/, result.questions.first) + assert_match(/rep-again/, result.questions.first) + end + + # The server case: nothing on disk, so mill makes its own. + def test_no_match_clones_into_mills_own_directory + result = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + assert_predicate result, :ok? + assert_equal File.join(@home, 'clones', 'slowernet-rep'), result.path + assert_path_exists File.join(result.path, '.git') + end + + def test_a_clone_mill_already_made_is_reused_rather_than_remade + first = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + marker = File.join(first.path, 'MARKER') + File.write(marker, 'x') + + second = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + assert_equal first.path, second.path + assert_path_exists marker + end + + def test_a_directory_that_is_not_a_repository_is_ignored + FileUtils.mkdir_p(File.join(@clones, 'rep')) + + result = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + assert_predicate result, :ok? + assert_equal File.join(@home, 'clones', 'slowernet-rep'), result.path + end + + # A directory full of clones is the normal laptop case, and only the one + # whose origin matches may be picked. + def test_a_clone_of_a_different_repo_is_not_a_match + place_clone('something-else') + other = build_origin('slowernet', 'other') + + result = Mill::Repo.resolve('slowernet', 'other', url: other) + + assert_equal File.join(@home, 'clones', 'slowernet-other'), result.path + end + + # The same repository is written four ways depending on how it was cloned. + def test_every_origin_form_names_the_same_repository + %w[ + git@github.com:slowernet/rep.git + https://github.com/slowernet/rep.git + https://github.com/slowernet/rep + ssh://git@github.com/slowernet/rep.git + ].each do |url| + assert_equal 'slowernet/rep', Mill::Repo.slug(url), url + end + end + + def test_slug_is_case_insensitive + assert_equal 'slowernet/rep', Mill::Repo.slug('https://github.com/SlowerNet/Rep.git') + end + + def test_roots_default_by_platform + ENV.delete('MILL_CLONES') + + roots = Mill::Repo.roots + + if Mill::Clock::DARWIN + assert_equal [File.expand_path('~/code')], roots + else + assert_empty roots + end + end + + def test_roots_accept_several_directories + ENV['MILL_CLONES'] = "#{@clones}:#{@root}" + + assert_equal [@clones, @root], Mill::Repo.roots + end + + def test_a_root_that_does_not_exist_is_not_an_error + ENV['MILL_CLONES'] = '/no/such/place' + + result = Mill::Repo.resolve('slowernet', 'rep', url: @origin) + + assert_predicate result, :ok? + assert_equal File.join(@home, 'clones', 'slowernet-rep'), result.path + end + + # A clone mill cannot make is a problem to report, not an exception to + # throw at the poller loop. + def test_a_clone_that_fails_reports_rather_than_raising + result = Mill::Repo.resolve('slowernet', 'rep', url: File.join(@root, 'not-a-repo')) + + refute_predicate result, :ok? + assert_equal :clone_failed, result.problem + end + + # --- preparation -------------------------------------------------------- + + def prepare = Mill::Repo.prepare(db: db, owner: 'slowernet', name: 'rep', url: @origin) + + def repo_id = db[:repos].where(owner: 'slowernet', name: 'rep').get(:id) + + def commit_to_base(clone, path, body) + identify(clone) + File.write(File.join(clone, path), body) + Mill::Git.run!(clone, 'add', '-A') + Mill::Git.run!(clone, 'commit', '-m', "add #{path}") + Mill::Git.run!(clone, 'push', 'origin', 'main') + end + + def write_secret_file(name, body) + FileUtils.mkdir_p(File.join(@home, 'secrets')) + path = File.join(@home, 'secrets', name) + File.write(path, body) + FileUtils.chmod(0o600, path) + end + + # A stage's commit must not trigger a gc that rewrites refs while other + # runs are holding them. + def test_preparation_sets_the_config_that_stops_a_stage_triggering_gc + place_clone('rep') + + result = prepare + + assert_predicate result, :ok? + assert_equal '0', Mill::Git.run!(result.path, 'config', 'gc.auto').strip + assert_equal '0', Mill::Git.run!(result.path, 'config', 'maintenance.auto').strip + end + + def test_preparation_caches_the_repo_row + place_clone('rep') + prepare + row = db[:repos].where(id: repo_id).first + + refute_nil row[:prepared_at] + assert_equal 'main', row[:base_branch] + assert_equal File.join(@clones, 'rep'), row[:local_path] + end + + # .mill.yml is read from the base branch, never from a checkout: an agent + # can edit it in its own worktree and that edit must not weaken the next run. + def test_reads_the_config_from_the_base_branch_only + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', + "test_command: bundle exec rake test\nsecrets:\n - API_KEY\n") + Mill::Git.run!(clone, 'switch', '-c', 'feature') + File.write(File.join(clone, '.mill.yml'), "secrets: []\n") + write_secret_file('slowernet-rep.env', "API_KEY=x\n") + + prepare + config = Mill::Repo.config(db, repo_id) + + assert_equal 'bundle exec rake test', config[:test_command] + assert_equal ['API_KEY'], config[:secrets] + end + + def test_a_repo_with_no_config_file_prepares_anyway + place_clone('rep') + + assert_predicate prepare, :ok? + assert_empty Mill::Repo.config(db, repo_id) + end + + # A missing secret fails the suite on both attempts, which reads as the + # stage being wrong. Say so before the run starts instead. + def test_a_named_secret_that_is_absent_blocks_the_item + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', "secrets:\n - API_KEY\n - DATABASE_URL\n") + + result = prepare + + refute_predicate result, :ok? + assert_equal :missing_secrets, result.problem + assert_match(/API_KEY/, result.questions.first) + assert_match(/DATABASE_URL/, result.questions.first) + end + + def test_a_named_secret_that_is_present_does_not_block + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', "secrets:\n - API_KEY\n") + write_secret_file('slowernet-rep.env', "API_KEY=x\n") + + assert_predicate prepare, :ok? + end + + def test_a_prepared_repo_is_not_prepared_twice + place_clone('rep') + prepare + first = db[:repos].where(id: repo_id).get(:prepared_at) + db[:repos].where(id: repo_id).update(local_path: '/gone') + + result = prepare + + assert_equal '/gone', result.path + assert_equal first, db[:repos].where(id: repo_id).get(:prepared_at) + end + + # A config file that does not parse blocks the item. Left to raise it would + # wedge the poller loop in a retry cycle instead. + def test_a_config_file_that_does_not_parse_blocks_the_item + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', "secrets:\n - [unclosed\n") + + result = prepare + + refute_predicate result, :ok? + assert_equal :unprepared, result.problem + assert_match(/\.mill\.yml/, result.questions.first) + end + + # safe_load rejects Date by default, and an unquoted date in YAML is a Date. + # That must block the item, not escape as an unhandled Psych error. + def test_a_config_file_with_a_disallowed_class_blocks_the_item + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', "released: 2026-08-19\n") + + result = prepare + + refute_predicate result, :ok? + assert_equal :unprepared, result.problem + end + + # A YAML file that parses to something other than a mapping is not config. + def test_a_config_file_that_is_not_a_mapping_is_ignored + clone = place_clone('rep') + commit_to_base(clone, '.mill.yml', "- one\n- two\n") + + assert_predicate prepare, :ok? + assert_empty Mill::Repo.config(db, repo_id) + end + + def test_an_unresolvable_clone_reports_rather_than_preparing + place_clone('rep') + place_clone('rep-again') + + result = prepare + + assert_equal :ambiguous_clone, result.problem + assert_nil repo_id + end + end +end diff --git a/test/mill/test_resume.rb b/test/mill/test_resume.rb index 2550705..62f0044 100644 --- a/test/mill/test_resume.rb +++ b/test/mill/test_resume.rb @@ -45,6 +45,9 @@ def scripted(stage, status: 'ok', questions: []) stream = Object.new stream.define_singleton_method(:session_id) { "sess-#{Mill::Stages.slug(stage)}" } stream.define_singleton_method(:resume_failed?) { false } + stream.define_singleton_method(:rate_limited?) { false } + stream.define_singleton_method(:rate_limit_resets_at) { nil } + stream.define_singleton_method(:resume_failed?) { false } stream.define_singleton_method(:tokens) { { tokens_in: 1, tokens_out: 2 } } stream.define_singleton_method(:model) { 'm' } result = Object.new diff --git a/test/mill/test_run.rb b/test/mill/test_run.rb index 6c4363c..1cc79b8 100644 --- a/test/mill/test_run.rb +++ b/test/mill/test_run.rb @@ -182,6 +182,9 @@ def self.scripted_attempt(stage, log_path = '/dev/null') stream = Object.new stream.define_singleton_method(:session_id) { 's' } stream.define_singleton_method(:resume_failed?) { false } + stream.define_singleton_method(:rate_limited?) { false } + stream.define_singleton_method(:rate_limit_resets_at) { nil } + stream.define_singleton_method(:resume_failed?) { false } stream.define_singleton_method(:tokens) { { tokens_in: 1, tokens_out: 2 } } stream.define_singleton_method(:model) { 'm' } result = Object.new diff --git a/test/mill/test_runner.rb b/test/mill/test_runner.rb index d4e9233..3146d24 100644 --- a/test/mill/test_runner.rb +++ b/test/mill/test_runner.rb @@ -22,7 +22,7 @@ def create_pull_request(repo, head:, base:, title:, body:) # The smallest thing shaped like a Mill::Claude::Attempt. def scripted(status: 'ok', valid: true, success: true, objections: [], questions: [], artifact: nil, session: 'sess-1', summary: 'did the thing', title: 'A title', body: 'A body', - resume_failed: false) + resume_failed: false, rate_limited: false, resets_at: nil) verdict = Object.new verdict.define_singleton_method(:valid?) { valid } verdict.define_singleton_method(:status) { status } @@ -38,6 +38,8 @@ def scripted(status: 'ok', valid: true, success: true, objections: [], questions stream = Object.new stream.define_singleton_method(:session_id) { session } stream.define_singleton_method(:resume_failed?) { resume_failed } + stream.define_singleton_method(:rate_limited?) { rate_limited } + stream.define_singleton_method(:rate_limit_resets_at) { resets_at } stream.define_singleton_method(:tokens) { { tokens_in: 1, tokens_out: 2 } } stream.define_singleton_method(:model) { 'claude-opus-5' } @@ -55,8 +57,10 @@ def scripted(status: 'ok', valid: true, success: true, objections: [], questions def runner_for(script, route: 'plan', github: FakeGithub.new) @calls = [] @github = github - run_id = create_run(repo_id: create_repo(local_path: '/tmp/clone', base_branch: 'main'), - route: route, branch: 'a-branch') + run_id = @run_id = create_run( + repo_id: create_repo(local_path: '/tmp/clone', base_branch: 'main'), + route: route, branch: 'a-branch' + ) queue = script.dup launcher = lambda do |stage:, prompt:, number:, session_id:| @calls << { stage: stage, number: number, session_id: session_id, prompt: prompt } @@ -73,6 +77,30 @@ def ok_for(stage) def clean_run = Array.new(6) { ->(stage) { ok_for(stage) } } + # --- what a live run is doing --------------------------------------- + + # The column is what a supervisor reaping a live run reads to know which + # stage to charge. Written only at halt, it is nil for the whole time a + # stage is actually running, and the reaper then silently charges nothing. + def test_the_current_stage_is_recorded_while_the_stage_is_running + seen = [] + runner = runner_for(Array.new(6) do |i| + lambda do |stage| + seen << [stage, db[:runs].where(id: @run_id).get(:current_stage)] + ok_for(stage) + end + end) + runner.call + + assert_equal seen.map(&:first), seen.map(&:last) + end + + def test_a_finished_run_is_in_no_stage + runner_for(clean_run).call + + assert_nil db[:runs].where(id: @run_id).get(:current_stage) + end + # --- the happy path ------------------------------------------------- def test_a_clean_run_walks_the_whole_plan_route @@ -129,6 +157,95 @@ def test_blocking_costs_no_strike assert_equal 1, Mill::Ledger.new(db, runner.run_id).attempts('triage') end + # --- the subscription said no --------------------------------------- + + # A launch the subscription refused never ran, so it hands back no verdict — + # which is what marks it, not the exit status. Without being classified + # first it reads as a crash and takes a strike, charging a stage for a door + # mill could not open. Measured live 2026-08-20. + def test_a_rate_limited_launch_costs_no_strike + waits = [] + runner = runner_with_pause(waits, + [scripted(rate_limited: true, success: false, valid: false)] + clean_run) + runner.call + + assert_equal 0, Mill::Ledger.new(db, runner.run_id).strikes('triage') + end + + # attempt: 0 in the ledger, so it leaves no row: nothing happened. + def test_a_rate_limited_launch_leaves_no_attempt_behind + waits = [] + runner = runner_with_pause(waits, + [scripted(rate_limited: true, success: false, valid: false)] + clean_run) + runner.call + + assert_equal 1, Mill::Ledger.new(db, runner.run_id).attempts('triage') + assert_equal [1, 1], @calls.first(2).map { |c| c[:number] }, + 'the refused launch must not consume an attempt number' + end + + # Retrying straight away is a hot loop against a door that will not open for + # hours, so it waits for the window the CLI named. + def test_it_waits_for_the_window_the_cli_named + waits = [] + resets = Mill.now + 900 + runner = runner_with_pause(waits, + [scripted(rate_limited: true, success: false, valid: false, resets_at: resets)] + clean_run) + runner.call + + assert_equal 1, waits.length + assert_in_delta 900, waits.first, 5 + end + + def test_an_unknown_reset_waits_the_cap + waits = [] + runner = runner_with_pause(waits, + [scripted(rate_limited: true, success: false, valid: false)] + clean_run) + runner.call + + assert_equal Mill::Ledger::MAX_RATE_LIMIT_PAUSE, waits.first + end + + # The shape the ledger change introduced, exercised through the runner + # rather than asserted as a classification. A clean exit with nothing + # readable is still treated as a refusal, so it must wait rather than + # relaunch straight away, and must leave the ledger untouched. + def test_a_clean_exit_with_no_verdict_under_a_limit_waits_rather_than_striking + waits = [] + runner = runner_with_pause(waits, + [scripted(rate_limited: true, success: true, valid: false)] + clean_run) + runner.call + ledger = Mill::Ledger.new(db, runner.run_id) + + assert_equal 1, waits.length, 'a refusal must wait for the window, not relaunch at once' + assert_equal 0, ledger.strikes('triage') + assert_equal 1, ledger.attempts('triage') + end + + def test_a_reset_already_past_still_leaves_a_minute + assert_equal 60, Mill::Runner.rate_limit_pause(Mill.now - 500) + end + + # Free is not unlimited. Every strike-free path has its own cap. + def test_endless_rate_limiting_blocks_rather_than_waiting_forever + waits = [] + refused = Array.new(Mill::Ledger::MAX_RATE_LIMIT_WAITS + 1) do + scripted(rate_limited: true, success: false, valid: false) + end + runner = runner_with_pause(waits, refused) + + assert_equal :blocked, runner.call + assert_match(/rate limited/i, runner.state[:reason]) + assert_equal 0, Mill::Ledger.new(db, runner.run_id).strikes('triage') + end + + # A runner whose pause is recorded rather than taken. + def runner_with_pause(waits, script) + runner = runner_for(script) + runner.instance_variable_set(:@pause, ->(seconds) { waits << seconds }) + runner + end + # --- failure and resume --------------------------------------------- # A relaunch resumes the session, so the agent remembers its own work. diff --git a/test/mill/test_schema.rb b/test/mill/test_schema.rb index 61f44cb..85d08b9 100644 --- a/test/mill/test_schema.rb +++ b/test/mill/test_schema.rb @@ -120,6 +120,19 @@ def test_the_other_three_counts_stay_required end end + # The identity of a live process belongs to the run, not to the attempt row: + # the attempt row does not exist until the attempt is over, so a supervisor + # reaping a running stage would have nothing to check it against. + def test_a_run_carries_the_identity_of_its_live_process + columns = db.schema(:runs).map(&:first) + + assert_includes columns, :pid + assert_includes columns, :pgid + assert_includes columns, :pid_started_at + assert_includes columns, :host_boot_at + assert_includes columns, :board_item_id + end + def test_events_dedupe_on_node_id repo = create_repo db[:events].insert(repo_id: repo, kind: 'comment', gh_node_id: 'IC_1', created_at: Mill.now) diff --git a/test/mill/test_secrets.rb b/test/mill/test_secrets.rb new file mode 100644 index 0000000..a85568a --- /dev/null +++ b/test/mill/test_secrets.rb @@ -0,0 +1,137 @@ +require 'test_helper' +require 'tmpdir' +require 'fileutils' + +module Mill + # No network and no real ~/.mill: MILL_HOME points at a tmpdir throughout. + class TestSecrets < Minitest::Test + def setup + @home = Dir.mktmpdir('mill-secrets') + FileUtils.mkdir_p(File.join(@home, 'secrets')) + Mill.instance_variable_set(:@home, @home) + end + + def teardown + FileUtils.remove_entry(@home, true) + Mill.instance_variable_set(:@home, nil) + end + + def write_secret(name, body, mode: 0o600) + path = File.join(@home, 'secrets', name) + File.write(path, body) + FileUtils.chmod(mode, path) + path + end + + def test_a_repo_with_no_secrets_file_gets_an_empty_environment + assert_empty Mill::Secrets.for_repo('slowernet', 'mill-scratch') + end + + def test_reads_plain_key_value_lines + write_secret('slowernet-rep.env', "DATABASE_URL=postgres://x\nAPI_KEY=abc123\n") + + env = Mill::Secrets.for_repo('slowernet', 'rep') + + assert_equal 'postgres://x', env['DATABASE_URL'] + assert_equal 'abc123', env['API_KEY'] + end + + # A '=' in a value is ordinary in a connection string, and splitting on + # every one of them would truncate it silently. + def test_a_value_may_contain_the_separator + write_secret('slowernet-rep.env', "TOKEN=a=b=c\n") + + assert_equal 'a=b=c', Mill::Secrets.for_repo('slowernet', 'rep')['TOKEN'] + end + + def test_ignores_comments_and_blank_lines + write_secret('slowernet-rep.env', "# a note\n\nA=1\n \n") + + assert_equal({ 'A' => '1' }, Mill::Secrets.for_repo('slowernet', 'rep')) + end + + def test_strips_matching_quotes_only + write_secret('slowernet-rep.env', %(A="one two"\nB='three'\nC="mismatched'\n)) + + env = Mill::Secrets.for_repo('slowernet', 'rep') + + assert_equal 'one two', env['A'] + assert_equal 'three', env['B'] + assert_equal %("mismatched'), env['C'] + end + + # The runbook tells you to chmod this file. A mode drift is otherwise + # silent, and these values reach a subprocess environment. + def test_refuses_a_world_readable_secrets_file + write_secret('slowernet-rep.env', "A=1\n", mode: 0o644) + + error = assert_raises(Mill::Error) { Mill::Secrets.for_repo('slowernet', 'rep') } + + assert_match(/expected 600/, error.message) + end + + # Only the stages that push carry the token. Handing it to `implement` + # would put a credential inside the widest ruleset mill has. + def test_only_the_pushing_stages_carry_the_token + write_secret('stage-token', "ghp_exampleexampleexample\n") + + assert_equal 'ghp_exampleexampleexample', Mill::Rules.env_for('pr')['GH_TOKEN'] + assert_nil Mill::Rules.env_for('implement')['GH_TOKEN'] + end + + def test_the_repo_environment_reaches_a_stage + write_secret('slowernet-rep.env', "API_KEY=abc123\n") + + env = Mill::Rules.env_for('implement', owner: 'slowernet', name: 'rep') + + assert_equal 'abc123', env['API_KEY'] + end + + # A path is not a secret, so SSL_CERT_FILE must not be scrubbed out of + # every log line that happens to mention it. + def test_only_real_secrets_are_offered_to_the_scrubber + write_secret('slowernet-rep.env', "API_KEY=abcdefghijklmnopqrst\n") + write_secret('stage-token', "ghp_exampleexampleexample\n") + + values = Mill::Secrets.values_for('pr', owner: 'slowernet', name: 'rep') + + assert_includes values, 'abcdefghijklmnopqrst' + assert_includes values, 'ghp_exampleexampleexample' + refute(values.any? { |v| v.include?('cert') }) + end + + # The scrubber does a literal gsub on every line of a stream-json log that + # mill parses back. A short value redacts far more than itself: DEBUG=true + # turns "success":true into "success":[redacted], which stops being JSON, + # and the stage is then charged a strike for mill's own scrubber. + def test_a_short_value_is_never_offered_to_the_scrubber + write_secret('slowernet-rep.env', + "RAILS_ENV=test\nDEBUG=true\nAPI_KEY=abcdefghijklmnopqrst\n") + + values = Mill::Secrets.values_for('implement', owner: 'slowernet', name: 'rep') + + assert_equal ['abcdefghijklmnopqrst'], values + end + + # It still reaches the stage. Not redacting it is a decision about the log, + # not about the environment. + def test_a_short_value_still_reaches_the_stage + write_secret('slowernet-rep.env', "RAILS_ENV=test\n") + + assert_equal 'test', + Mill::Rules.env_for('implement', owner: 'slowernet', name: 'rep')['RAILS_ENV'] + end + + def test_a_world_readable_token_is_refused + write_secret('stage-token', "ghp_exampleexampleexample\n", mode: 0o644) + + assert_raises(Mill::Error) { Mill::Secrets.token } + end + + def test_an_empty_token_file_is_no_token + write_secret('stage-token', "\n") + + assert_nil Mill::Secrets.token + end + end +end diff --git a/test/mill/test_settings.rb b/test/mill/test_settings.rb new file mode 100644 index 0000000..b6140de --- /dev/null +++ b/test/mill/test_settings.rb @@ -0,0 +1,66 @@ +require 'test_helper' + +module Mill + # Settings are parsed, never coerced. `'lots'.to_i` is 0, and a concurrency cap + # of 0 stops mill claiming anything while every check stays green. + class TestSettings < Minitest::Test + def teardown + ENV.delete('MILL_TEST_N') + ENV.delete('MILL_TEST_F') + end + + def test_a_typo_falls_back_rather_than_becoming_zero + ENV['MILL_TEST_N'] = 'lots' + + assert_equal 2, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 8) + end + + def test_an_empty_value_falls_back + ENV['MILL_TEST_N'] = ' ' + + assert_equal 2, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 8) + end + + def test_a_value_outside_its_range_falls_back + ENV['MILL_TEST_N'] = '99' + + assert_equal 2, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 8) + end + + def test_zero_is_out_of_range_for_a_cap + ENV['MILL_TEST_N'] = '0' + + assert_equal 2, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 8) + end + + def test_a_valid_value_is_used + ENV['MILL_TEST_N'] = '4' + + assert_equal 4, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 8) + end + + # Integer('010') is 10 in base 10, not 8. Left to Integer's default base a + # leading zero would be read as octal. + def test_a_leading_zero_is_not_octal + ENV['MILL_TEST_N'] = '010' + + assert_equal 10, Mill.setting_int('MILL_TEST_N', default: 2, min: 1, max: 30) + end + + def test_an_unset_value_is_the_default + assert_in_delta 30.0, Mill.setting_float('MILL_TEST_F', default: 30, min: 5, max: 3600) + end + + def test_a_float_is_parsed_and_ranged + ENV['MILL_TEST_F'] = '7.5' + + assert_in_delta 7.5, Mill.setting_float('MILL_TEST_F', default: 30, min: 5, max: 3600) + end + + def test_a_float_typo_falls_back + ENV['MILL_TEST_F'] = 'soon' + + assert_in_delta 30.0, Mill.setting_float('MILL_TEST_F', default: 30, min: 5, max: 3600) + end + end +end diff --git a/test/mill/test_spawn.rb b/test/mill/test_spawn.rb index 8084770..b7ba67b 100644 --- a/test/mill/test_spawn.rb +++ b/test/mill/test_spawn.rb @@ -17,6 +17,88 @@ def fake_stage(fixture) ['ruby', '-e', "print File.read(#{File.join(FIXTURES, "#{fixture}.jsonl").inspect})"] end + # The caller has to be able to record the identity before the process ends, + # because the whole point of recording it is reaping something still alive. + def test_reports_its_identity_as_soon_as_the_process_starts + with_log do |log, dir| + seen = nil + spawn_in(log, dir, on_spawn: ->(*args) { seen = args }) + .run(['ruby', '-e', 'sleep 0.1']) + + refute_nil seen, 'on_spawn was never called' + pid, pgid, started_at, boot_at = seen + + assert_operator pid, :>, 1 + assert_equal pid, pgid + refute_nil started_at + refute_nil boot_at + end + end + + # A callback that raises must not leave a spawned process group with nobody + # holding its identity. The launch fails; the group does not survive it. + def test_a_failing_callback_does_not_orphan_the_process_group + with_log do |log, dir| + pgid = nil + spawn = spawn_in(log, dir, on_spawn: lambda { |_pid, group, *| + pgid = group + raise Mill::Error, 'database is locked' + }) + + assert_raises(Mill::Error) { spawn.run(['ruby', '-e', 'sleep 30']) } + refute_nil pgid + assert_raises(Errno::ESRCH) { Process.kill(0, -pgid) } + end + end + + # The boot-time check exists for a pgid read back from the database, which + # may have crossed a reboot. This group was spawned two lines ago and this + # process still holds its wait_thr — there is no reboot it could have + # crossed, and no stranger it could be. On a host that cannot read its own + # boot time, declining to signal it leaves the group running with nobody + # holding its identity, and parks the raise behind the child it would not + # kill. + # + # The fix belongs in announce_spawn, which can kill the group it just + # created. Spawn.reap's boot gate must NOT be loosened to suit this test: + # Supervisor#reap feeds it pgids straight out of the database, and + # test_hostile_input pins it at :unknown_boot for exactly that reason. + def test_the_group_dies_even_when_the_boot_time_is_unreadable + # Stubbed on this thread, not inside the worker: the worker is the thing + # that may hang, and a restore that hangs with it leaves every later + # test in this process reading a nil boot time. + with_unreadable_boot_time do + with_log do |log, dir| + pgid = nil + finished = nil + spawn = spawn_in(log, dir, on_spawn: lambda { |_pid, group, *| + pgid = group + raise Mill::Error, 'database is locked' + }) + thread = Thread.new do + spawn.run(['ruby', '-e', 'sleep 30']) + :no_error + rescue Mill::Error + :raised + end + + begin + finished = thread.join(20) + + refute_nil pgid, 'the callback never saw a process group' + refute_nil finished, 'run is still blocked on a group it declined to kill' + assert_equal :raised, thread.value + assert_raises(Errno::ESRCH) { Process.kill(0, -pgid) } + ensure + # Only when the fix is absent — killing a pgid that was already + # reaped is the recycling hazard this whole file is about. + Mill::Spawn.signal(pgid, 'KILL') if pgid && finished.nil? + thread.join(5) + end + end + end + end + def test_tees_the_stream_and_parses_it_at_once with_log do |log, dir| result = spawn_in(log, dir).run(fake_stage('plan_ok')) @@ -283,6 +365,23 @@ def test_non_ascii_output_reaches_the_log_intact private + # Minitest 6 ships no stub, so this restores by hand. Every test in the + # suite shares one process and one Mill::Clock, so a restore that does not + # happen is a nil boot time for everything that runs after it. + def with_unreadable_boot_time + raise 'nested stub would restore the stub itself' if + Mill::Clock.singleton_class.method_defined?(:readable_boot_time) + + Mill::Clock.singleton_class.send(:alias_method, :readable_boot_time, :boot_time) + Mill::Clock.define_singleton_method(:boot_time) { nil } + begin + yield + ensure + Mill::Clock.singleton_class.send(:alias_method, :boot_time, :readable_boot_time) + Mill::Clock.singleton_class.send(:remove_method, :readable_boot_time) + end + end + def with_cap(bytes) original = Mill::Spawn::LOG_CAP set_cap(bytes) diff --git a/test/mill/test_supervisor.rb b/test/mill/test_supervisor.rb new file mode 100644 index 0000000..ad28066 --- /dev/null +++ b/test/mill/test_supervisor.rb @@ -0,0 +1,577 @@ +require 'test_helper' +require 'tmpdir' +require 'fileutils' + +module Mill + # Real git in a tmpdir; no network, no claude. + class TestSupervisor < Mill::TestCase + def setup + super + @root = Dir.mktmpdir('mill-supervisor') + @home = File.join(@root, 'home') + FileUtils.mkdir_p(@home) + Mill.instance_variable_set(:@home, @home) + @clone = File.join(@root, 'rep') + Mill::Git.clone_init(@clone) + File.write(File.join(@clone, 'README.md'), "# rep\n") + Mill::Git.run!(@clone, 'add', '-A') + Mill::Git.run!(@clone, 'commit', '-m', 'first') + Mill::Git.run!(@clone, 'branch', '1-a-feature') + @repo_id = create_repo(owner: 'slowernet', name: 'rep', local_path: @clone, + base_branch: 'main', prepared_at: Mill.now) + end + + def teardown + FileUtils.remove_entry(@root, true) + Mill.instance_variable_set(:@home, nil) + ENV.delete('MILL_CONCURRENCY') + super + end + + def repo_row = db[:repos].where(id: @repo_id).first + + def supervisor(comments: [], git: Mill::Git) + gh = Mill::Github.new(runner: ->(args) { comments << args; '' }) + Mill::Supervisor.new(db: db, github: gh, git: git, board: nil) + end + + def claim(sup, branch: '1-a-feature', number: 1) + sup.claim(repo_row: repo_row, subject_kind: 'issue', subject_number: number, + route: 'plan', branch: branch, spec_path: 'docs/spec.md', board_item_id: 'PVTI_1') + end + + # A git double that behaves normally except for the one command named. + def git_failing_at(command) + Class.new do + define_singleton_method(command) { |*| raise Mill::Git::Error, 'no space left on device' } + def self.method_missing(name, *args, &blk) = Mill::Git.send(name, *args, &blk) + def self.respond_to_missing?(*) = true + end + end + + def test_claiming_inserts_a_running_run_and_a_worktree + run_id = claim(supervisor) + row = db[:runs].where(id: run_id).first + + assert_equal 'running', row[:status] + assert_equal '1-a-feature', row[:branch] + assert_equal 'PVTI_1', row[:board_item_id] + assert_equal 'docs/spec.md', row[:spec_path] + assert_path_exists File.join(row[:worktree_path], 'README.md') + end + + def test_the_cap_counts_running_rows_only + ENV['MILL_CONCURRENCY'] = '1' + sup = supervisor + claim(sup) + + assert_predicate sup, :at_cap? + + db[:runs].update(status: 'blocked') + + refute_predicate sup, :at_cap? + end + + def test_the_cap_falls_back_rather_than_reading_a_typo_as_zero + ENV['MILL_CONCURRENCY'] = 'lots' + + assert_equal Mill::Supervisor::DEFAULT_CAP, supervisor.cap + end + + # A blocked run holds its branch by design, so the item behind it waits. + def test_a_branch_another_live_run_holds_is_skipped + sup = supervisor + claim(sup) + + assert_equal :held, claim(sup, number: 2) + end + + def test_a_branch_a_finished_run_held_is_free_again + sup = supervisor + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'done') + Mill::Git.worktree_remove(@clone, db[:runs].where(id: run_id).get(:worktree_path)) + + assert_operator claim(sup, number: 2), :>, 0 + end + + # Silence here means mill appears to ignore you forever. + def test_the_skip_is_announced_once + calls = [] + sup = supervisor(comments: calls) + claim(sup) + claim(sup, number: 2) + claim(sup, number: 2) + comments = calls.select { |args| args.first(2) == %w[issue comment] } + + assert_equal 1, comments.length + assert_match(/1-a-feature/, comments.first.join(' ')) + end + + # git worktree add refuses a branch checked out anywhere, including the + # clone's own HEAD, and the prescribed workflow leaves it that way. + def test_a_branch_checked_out_in_your_clone_blocks_the_item + Mill::Git.run!(@clone, 'switch', '1-a-feature') + + result = claim(supervisor) + + assert_kind_of Mill::Supervisor::Blocked, result + assert_equal :branch_checked_out, result.problem + assert_match(/#{Regexp.escape(@clone)}/, result.questions.first) + assert_match(/1-a-feature/, result.questions.first) + end + + def test_mill_never_forces_a_worktree_onto_a_checked_out_branch + Mill::Git.run!(@clone, 'switch', '1-a-feature') + claim(supervisor) + + assert_equal 0, db[:runs].count + end + + # A SIGKILL during git commit leaves a lock git never cleans, and the next + # launch fails instantly on it. + def test_a_stale_lock_is_cleared_before_claiming + lock = File.join(@clone, '.git', 'index.lock') + File.write(lock, '') + FileUtils.touch(lock, mtime: Time.now - 3600) + + claim(supervisor) + + refute_path_exists lock + end + + # A lock that is minutes old may belong to a command running right now — + # one of mill's own stages, or you in a terminal on the same clone. + def test_a_fresh_lock_is_left_alone + lock = File.join(@clone, '.git', 'index.lock') + File.write(lock, '') + + claim(supervisor) + + assert_path_exists lock + end + + # Scoping this to mill/* would miss the plan route entirely, which adopts + # the branch gh issue develop made and keeps its name. + def test_the_branchs_own_ref_lock_is_cleared_whatever_it_is_named + lock = File.join(@clone, '.git', 'refs', 'heads', '1-a-feature.lock') + FileUtils.mkdir_p(File.dirname(lock)) + File.write(lock, '') + FileUtils.touch(lock, mtime: Time.now - 3600) + + claim(supervisor) + + refute_path_exists lock + end + + # git worktree add refuses a branch whose admin entry survives even after + # its directory is gone. + def test_a_stale_worktree_entry_is_pruned + dead = File.join(@root, 'dead') + Mill::Git.run!(@clone, 'worktree', 'add', dead, '1-a-feature') + FileUtils.remove_entry(dead, true) + + assert_operator claim(supervisor), :>, 0 + end + + # A row inserted before a worktree that never appears is a running run with + # no process, which nothing reaps and which holds a slot forever. Two of + # those stop mill claiming anything again, with every check still green. + def test_a_worktree_that_cannot_be_made_leaves_no_run_behind + sup = supervisor(git: git_failing_at(:worktree_add)) + + assert_raises(Mill::Git::Error) { claim(sup) } + assert_equal 0, db[:runs].count + end + + # Not knowing whether a branch is checked out is not the same as it not + # being checked out, and claiming on that guess is how two live checkouts + # of one branch happen. + def test_a_git_failure_is_never_read_as_a_free_branch + sup = supervisor(git: git_failing_at(:checked_out_branches)) + + assert_raises(Mill::Git::Error) { claim(sup) } + assert_equal 0, db[:runs].count + end + + # --- running, announcing, tearing down ----------------------------------- + + def state(status, questions: [], stage: 'plan') + { stage: stage, status: status, reason: 'scripted', questions: questions } + end + + def bodies(calls) + calls.select { |args| args.first(2) == %w[issue comment] }.map { |args| args.join(' ') } + end + + def test_a_finished_run_is_torn_down_and_its_branch_freed + sup = supervisor + run_id = claim(sup) + worktree = db[:runs].where(id: run_id).get(:worktree_path) + db[:runs].where(id: run_id).update(status: 'done', pr_number: 7) + + sup.finish(run_id, state(:done)) + + refute_path_exists worktree + refute_includes Mill::Git.checked_out_branches(@clone), '1-a-feature' + end + + # mill needs the worktree to resume, and a timer should not destroy the + # thing you have to answer a question about. + def test_a_blocked_run_keeps_its_worktree + sup = supervisor + run_id = claim(sup) + worktree = db[:runs].where(id: run_id).get(:worktree_path) + db[:runs].where(id: run_id).update(status: 'blocked') + + sup.finish(run_id, state(:blocked, questions: ['Which spec is authoritative?'])) + + assert_path_exists worktree + end + + # The questions are the only channel that reaches a person once you have + # walked away. + def test_a_blocked_run_posts_its_questions + calls = [] + sup = supervisor(comments: calls) + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'blocked') + + sup.finish(run_id, state(:blocked, questions: ['Which spec is authoritative?'])) + + assert_match(/Which spec is authoritative\?/, bodies(calls).last) + end + + def test_a_blocked_run_with_no_questions_still_says_why + calls = [] + sup = supervisor(comments: calls) + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'blocked') + + sup.finish(run_id, state(:blocked)) + + assert_match(/Blocked at `plan`/, bodies(calls).last) + end + + def test_a_finished_run_names_its_pull_request + calls = [] + sup = supervisor(comments: calls) + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'done', pr_number: 7) + + sup.finish(run_id, state(:done)) + + assert_match(/#7/, bodies(calls).last) + end + + def test_a_failed_run_says_so + calls = [] + sup = supervisor(comments: calls) + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'failed') + + sup.finish(run_id, state(:failed)) + + assert_match(/failed/i, bodies(calls).last) + end + + # Everything above tests `finish` with a hand-made state hash, which is what + # let the real walker return the wrong shape entirely. This drives the + # actual walker, through a scripted launcher, and asserts what reaches + # GitHub — the only channel that reaches a person once you have walked away. + def test_the_walker_hands_finish_what_a_blocked_stage_asked + calls = [] + sup = supervisor(comments: calls) + run_id = claim(sup) + blocked = Mill::Claude::Attempt.new(stage: 'triage', number: 1, nonce: 'n', + result: fake_result, verdict: fake_verdict) + + sup.start(run_id, walker: lambda { |id| + run = Mill::Run.adopt(id, db: db) + run.runner(launcher: ->(**) { blocked }).call + run.runner.state + }).join + + body = bodies(calls).last + + assert_match(/triage/, body) + assert_match(/Which spec is authoritative\?/, body) + refute_match(/Blocked at ``/, body) + end + + def fake_verdict + v = Object.new + v.define_singleton_method(:valid?) { true } + v.define_singleton_method(:status) { 'blocked' } + v.define_singleton_method(:blocked?) { true } + v.define_singleton_method(:rejects?) { false } + v.define_singleton_method(:questions) { ['Which spec is authoritative?'] } + v.define_singleton_method(:errors) { [] } + v.define_singleton_method(:data) { { summary: 'asked' } } + v + end + + def fake_result + stream = Object.new + stream.define_singleton_method(:session_id) { 'sess-1' } + stream.define_singleton_method(:resume_failed?) { false } + stream.define_singleton_method(:rate_limited?) { false } + stream.define_singleton_method(:rate_limit_resets_at) { nil } + stream.define_singleton_method(:resume_failed?) { false } + stream.define_singleton_method(:tokens) { { tokens_in: 1, tokens_out: 2 } } + stream.define_singleton_method(:model) { 'claude-sonnet-5' } + + r = Object.new + r.define_singleton_method(:success?) { true } + r.define_singleton_method(:error) { nil } + r.define_singleton_method(:log_path) { '/dev/null' } + r.define_singleton_method(:stream) { stream } + r + end + + # An answered run is working again. Left blocked it lies three ways: the + # board keeps saying Blocked, the run does not bind against the cap, and + # reap queries running rows only — so a stage that died could never be + # recovered. + def test_an_answered_run_is_running_again + sup = supervisor + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'blocked') + gate = Queue.new + thread = sup.start(run_id, answers: ['the second one'], + walker: ->(_id) { gate.pop; state(:done) }) + + assert_equal 'running', db[:runs].where(id: run_id).get(:status) + assert_equal 1, db[:runs].where(status: 'running').count + + gate << :go + thread.join + end + + def test_a_resumed_run_is_visible_to_the_reaper + sup = supervisor + run_id = claim(sup) + db[:runs].where(id: run_id).update(status: 'blocked') + gate = Queue.new + thread = sup.start(run_id, answers: ['x'], walker: ->(_id) { gate.pop; state(:done) }) + + assert_includes db[:runs].where(status: 'running').select_map(:id), run_id + + gate << :go + thread.join + end + + def test_a_run_thread_is_tracked_while_it_walks + sup = supervisor + run_id = claim(sup) + gate = Queue.new + thread = sup.start(run_id, walker: ->(_id) { gate.pop; state(:done) }) + + assert sup.running?(run_id) + + gate << :go + thread.join + + refute sup.running?(run_id) + end + + # A dead runner thread must not leave a run marked running forever, which + # is a slot held against the cap that nothing else releases. + def test_a_thread_that_dies_leaves_the_run_failed_rather_than_running + sup = supervisor + run_id = claim(sup) + sup.start(run_id, walker: ->(_id) { raise 'boom' }).join + + assert_equal 'failed', db[:runs].where(id: run_id).get(:status) + refute sup.running?(run_id) + end + + # --- reaping ------------------------------------------------------------- + + def running_run(sup, pid:, started_at:, boot_at: Mill::Clock.boot_time) + run_id = claim(sup) + db[:runs].where(id: run_id).update(pid: pid, pgid: pid, pid_started_at: started_at, + host_boot_at: boot_at, current_stage: 'plan', heartbeat_at: Mill.now) + run_id + end + + # Records what it was asked to restart instead of walking a real route. + def watching_restarts(sup) + started = [] + sup.define_singleton_method(:start) { |id, **| started << id } + started + end + + # No process, so whether the machine rebooted or the process simply died, + # the attempt is over. Signal nothing. + def test_a_run_whose_process_is_gone_is_interrupted + sup = supervisor + watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + + assert_equal [run_id], sup.reap + assert_equal 'interrupted', db[:stage_attempts].where(run_id: run_id).first[:status] + end + + # The machine lost the process; the stage did not fail. + def test_an_interruption_charges_no_strike + sup = supervisor + watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + sup.reap + + refute db[:stage_attempts].where(run_id: run_id).first[:strike_charged] + end + + def test_an_interruption_clears_the_stale_identity + sup = supervisor + watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + sup.reap + row = db[:runs].where(id: run_id).first + + assert_nil row[:pid] + assert_nil row[:pgid] + end + + # A pid that exists but started at a different time is a stranger wearing a + # recycled number. Signalling it would kill something else entirely. + def test_a_recycled_pid_is_never_signalled + sup = supervisor + watching_restarts(sup) + run_id = running_run(sup, pid: Process.pid, started_at: 1) + + assert_equal :gone, sup.identify(db[:runs].where(id: run_id).first) + end + + # mill restarted and the stage kept running. Two agents in one worktree is + # worse than losing partial work. + def test_a_live_group_mill_did_not_spawn_is_foreign + sup = supervisor + run_id = running_run(sup, pid: Process.pid, + started_at: Mill::Clock.pid_started_at(Process.pid)) + + assert_equal :foreign, sup.identify(db[:runs].where(id: run_id).first) + end + + # :ours means a thread is walking this run right now — not merely that no + # process is recorded. pid and pgid are nil for the whole gap between two + # stages, which happens five times on the plan route, so reading nil as + # "mill has this in hand" strands every run mill was restarted during. + def test_a_running_run_with_no_thread_and_no_process_is_gone + sup = supervisor + run_id = claim(sup) + db[:runs].where(id: run_id).update(current_stage: 'plan') + + assert_equal :gone, sup.identify(db[:runs].where(id: run_id).first) + end + + def test_a_run_with_a_live_thread_is_left_alone + sup = supervisor + run_id = claim(sup) + gate = Queue.new + thread = sup.start(run_id, walker: ->(_id) { gate.pop; state(:done) }) + + assert_equal :ours, sup.identify(db[:runs].where(id: run_id).first) + assert_empty sup.reap + + gate << :go + thread.join + end + + # Interrupting without re-entering leaves the run marked running with no + # thread, which nothing else ever picks up. Cap of one, because the + # interrupted run holds its own slot: restarting re-enters that run, it + # does not add another, so a full factory must not stop the restart. + def test_an_interrupted_run_is_started_again + ENV['MILL_CONCURRENCY'] = '1' + sup = supervisor + started = watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + sup.reap + + assert_equal [run_id], started + end + + # A run blocked by the interruption cap is waiting for a person. Starting + # it again would burn its attempts with nobody answering. + def test_a_run_blocked_by_the_cap_is_not_started_again + sup = supervisor + started = watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + Mill::Ledger::MAX_INTERRUPTIONS.times do + db[:runs].where(id: run_id).update(status: 'running', pid: 999_999, pgid: 999_999, + pid_started_at: Mill.now, host_boot_at: Mill::Clock.boot_time) + sup.reap + end + + assert_equal 'blocked', db[:runs].where(id: run_id).get(:status) + assert_equal Mill::Ledger::MAX_INTERRUPTIONS - 1, started.length + end + + def test_hitting_the_interruption_cap_says_it_charged_nothing + calls = [] + sup = supervisor(comments: calls) + watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + Mill::Ledger::MAX_INTERRUPTIONS.times do + db[:runs].where(id: run_id).update(status: 'running', pid: 999_999, pgid: 999_999, + pid_started_at: Mill.now, host_boot_at: Mill::Clock.boot_time) + sup.reap + end + + assert_match(/interrupted/, bodies(calls).last) + end + + # A running row with no current_stage means something above lost track of + # what the run was doing. Charging nothing and moving on hides it. + def test_a_running_run_with_no_stage_is_an_error_rather_than_a_no_op + sup = supervisor + watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + db[:runs].where(id: run_id).update(current_stage: nil) + + assert_raises(Mill::Error) { sup.reap } + end + + # The reaper re-enters the stage the run was in. A restarted run that began + # at the top of its route would re-run every stage it had already banked: + # `plan` writes its artifact a second time, and the ledger counts fresh + # attempts against stages that already passed. + def test_a_restarted_run_picks_up_at_the_stage_it_was_in + sup = supervisor + run_id = claim(sup) + db[:runs].where(id: run_id).update(current_stage: 'implement') + %w[triage plan review:plan].each_with_index do |stage, i| + db[:stage_attempts].insert(run_id: run_id, stage: stage, number: 1, nonce: "n#{i}", + status: 'ok', started_at: Mill.now, verdict_json: '{"status":"ok"}') + end + + run = Mill::Run.adopt(run_id, db: db) + runner = run.runner(launcher: ->(**) {}) + + assert_equal 'implement', runner.stage + end + + # A run with nothing behind it starts at the top, which is the only case + # where that is the right answer. + def test_a_fresh_run_starts_at_the_top_of_its_route + sup = supervisor + run_id = claim(sup) + + runner = Mill::Run.adopt(run_id, db: db).runner(launcher: ->(**) {}) + + assert_equal 'triage', runner.stage + end + + def test_a_finished_run_is_not_reaped + sup = supervisor + started = watching_restarts(sup) + run_id = running_run(sup, pid: 999_999, started_at: Mill.now) + db[:runs].where(id: run_id).update(status: 'done') + + assert_empty sup.reap + assert_empty started + end + end +end diff --git a/test/mill/test_workers.rb b/test/mill/test_workers.rb new file mode 100644 index 0000000..4e85819 --- /dev/null +++ b/test/mill/test_workers.rb @@ -0,0 +1,184 @@ +require 'test_helper' +require 'rack/test' +require_relative '../../app' + +module Mill + class TestWorkers < Minitest::Test + include Rack::Test::Methods + + def app = App.freeze.app + + # GET / counts running runs, so the process-wide connection the app uses + # needs a schema. MILL_DB is ':memory:' for the whole suite. + def setup + Mill::DB.migrate!(Mill.db) + end + + def teardown + ENV.delete('MILL_WORKERS') + @workers&.stop + end + + def workers(**opts) + @workers = Mill::Workers.new(poller: -> {}, supervisor: -> {}, **opts) + end + + # A stray Ready on the board must not launch a real run against a real repo + # while somebody is editing a template. + def test_workers_are_off_when_the_environment_says_so + ENV['MILL_WORKERS'] = 'off' + + refute Mill::Workers.enabled? + end + + def test_off_is_case_insensitive + ENV['MILL_WORKERS'] = 'OFF' + + refute Mill::Workers.enabled? + end + + def test_workers_are_on_by_default + ENV.delete('MILL_WORKERS') + + assert Mill::Workers.enabled? + end + + def test_disabled_workers_start_no_threads + ENV['MILL_WORKERS'] = 'off' + workers(interval: 0.01).start + + refute @workers.health[:poller][:alive] + end + + def test_each_loop_ticks_and_heartbeats + ticks = Queue.new + @workers = Mill::Workers.new(interval: 0.01, supervisor: -> {}, + poller: -> { ticks << :tick }) + @workers.start + ticks.pop + ticks.pop + + refute_nil @workers.health[:poller][:at] + assert @workers.health[:poller][:alive] + end + + # A thread that raises must come back, or the factory silently stops. + def test_a_raising_loop_is_restarted + calls = Queue.new + first = true + @workers = Mill::Workers.new(interval: 0.01, supervisor: -> {}, poller: lambda { + calls << :call + next unless first + + first = false + raise 'boom' + }) + @workers.start + 3.times { calls.pop } + + assert @workers.health[:poller][:alive] + end + + def test_a_raising_loop_records_what_it_raised + raised = Queue.new + @workers = Mill::Workers.new(interval: 0.01, supervisor: -> {}, poller: lambda { + raised << :raised + raise 'boom' + }) + @workers.start + raised.pop + sleep 0.05 + + assert_match(/boom/, @workers.health[:poller][:error].to_s) + end + + # The cap is in seconds. Applied before the multiplier it was three + # seconds, so an expired token retried twelve hundred times an hour. + def test_backoff_grows_to_the_stated_ceiling + w = workers(interval: 30) + + assert_in_delta 60, w.send(:backoff, 1) + assert_in_delta Mill::Workers::MAX_BACKOFF, w.send(:backoff, 20) + assert_operator w.send(:backoff, 20), :>, 60 + end + + # A rate limit is the one failure that says exactly when to try again. + # Exponential backoff caps at five minutes; a GraphQL window can be forty + # away, so backing off means eight more attempts that fail for a reason + # mill already knows. + def test_a_rate_limit_waits_for_the_window_rather_than_backing_off + w = workers(interval: 60) + reset = Mill.now + 1800 + w.instance_variable_set(:@github, stub_github(reset)) + + assert_in_delta 1800, w.send(:backoff, 1, Mill::Github::RateLimited.new('nope')), 5 + end + + def test_a_rate_limit_whose_reset_is_unknown_falls_back + w = workers(interval: 60) + w.instance_variable_set(:@github, stub_github(nil)) + + assert_equal Mill::Workers::MAX_BACKOFF, + w.send(:backoff, 1, Mill::Github::RateLimited.new('nope')) + end + + # A reset that has just passed must still leave a tick between attempts, + # and a clock that disagrees with GitHub's must not park the thread. + def test_the_rate_limit_wait_is_bounded_at_both_ends + w = workers(interval: 60) + + w.instance_variable_set(:@github, stub_github(Mill.now - 500)) + + assert_equal 60, w.send(:backoff, 1, Mill::Github::RateLimited.new('nope')) + + w.instance_variable_set(:@github, stub_github(Mill.now + 999_999)) + + assert_equal Mill::Workers::MAX_RATE_LIMIT_WAIT, + w.send(:backoff, 1, Mill::Github::RateLimited.new('nope')) + end + + def test_any_other_failure_still_backs_off_exponentially + w = workers(interval: 30) + + assert_in_delta 60, w.send(:backoff, 1, Mill::Error.new('boom')) + end + + def stub_github(reset) + Object.new.tap { |g| g.define_singleton_method(:rate_limit_reset) { |*| reset } } + end + + def test_the_default_interval_is_a_minute + assert_equal 60, Mill::Workers::DEFAULT_INTERVAL + end + + def test_the_root_route_reports_worker_health + get '/' + + assert last_response.ok? + assert_match(/poller/, last_response.body) + assert_match(/supervisor/, last_response.body) + end + + # Requiring app.rb must not start polling a real board. + def test_loading_the_app_starts_nothing + refute App.workers.health[:poller][:alive] + end + + # Built without a board, every `@board&.want` in claim, finish and interrupt + # is a silent no-op: mill runs perfectly and never writes a Status, so the + # board sits on Ready while a run works, finishes and opens a pull request. + def test_the_shared_supervisor_can_write_to_the_board + refute_nil workers.supervisor.instance_variable_get(:@board) + end + + # One supervisor per process. It alone knows which process groups mill + # spawned and which runs have a live thread. + def test_the_poller_and_the_reaper_share_one_supervisor + w = Mill::Workers.new + poller = w.send(:poller_tick) + + assert_same w.supervisor, poller.binding.local_variable_get(:poller) + .instance_variable_get(:@supervisor) + end + end +end