Repository navigation
feat: an unattended pilot runner (ADR-008 to ADR-012) - #3
Merged
sscodeai merged 25 commits intoSep 30, 2026
Merged
Conversation
… bootstrap (ADR-008) Three things an unattended runner needs before it is merely less capable rather than wrong, all of them preconditions rather than features. 1. SLOT LOCK. Every board here declares `atomicClaim: false` — none of GitHub, GitLab, Jira, Notion or Redmine can be told "claim this only if it is free". The docs called exclusion "the caller's job", which was a euphemism: it needs a primitive that survives a crash. An O_EXCL lock file with the owner's identity and TWO independent abandonment rules (the owner's process is provably gone, or nothing refreshed the lock within the stale window) — the second covers the hung owner and the recycled pid that a liveness check alone gets wrong. Node has no flock, so the difference from orbi is real and written down, not hidden behind the same name. The decisive test spawns a second PROCESS: an in-process flag would prove nothing about mutual exclusion. 2. EVENT REGISTRY. The error taxonomy says why something failed; nothing said what happened. A closed vocabulary (`slot.busy`, `deliver.pushed`, `review.findings`, …) where an unregistered kind THROWS, one JSON line per event, run id on every one. Emission is a side channel: a broken sink never changes the outcome of the delivery, but a bug in the emitter surfaces. 3. STATE BOOTSTRAP. A fresh repository has no `takumi-ready` label and a Jira project has no `Fix Needed` status, so the adapter dies before it can claim anything, with a message that teaches the operator nothing. `bootstrapStates()` is now a required port method with a `canBootstrapStates` capability: boards that can create (GitHub, GitLab) create; boards that cannot (Jira, Notion, Redmine) report, and `not-creatable` MUST carry an instruction or the contract suite rejects the report. Idempotent, and a dry run changes nothing — both enforced by the shared suite, so a new adapter cannot ship a bootstrap that mostly works. The loop takes both: a slot turns a racing runner into a `busy` outcome before it touches the board, and the trail records the whole run. Also fixed in this slice: a default sink that ALSO retained each event, so every event landed in the trail twice (found by the first test that read the trail back).
GitHub labels are creatable through the API, so `bootstrapStates` creates the six `takumi-*` labels instead of printing advice, reusing the adapter's own prefix so a custom `labelPrefix` produces the names that instance actually writes. One colour for all six, because the state is in the name and six colours would imply a priority takumi does not mean. The fake board models the same contract in memory, so the reference behaviour the real adapters are measured against is itself testable. Tests assert what the transport RECORDED, not what the adapter returned: a dry run makes no POST at all, the second call duplicates nothing, and the created bodies carry the documented colour.
…st to get right
The demo now drives all three rails end to end: a bootstrap dry run that changes
nothing and an apply that is idempotent, a second runner turned away as `busy`
without touching the board and a slot that is NOT wedged after the first run, and a
ten-event trail printed one JSON line at a time.
The bug ledger gains the two defects this slice produced — an event stored twice
because a default sink and `retain` both appended, and a message escaped twice
because `JSON.stringify` already escapes — with the two new classes they belong to
("two mechanisms, one effect" and "escaping twice"). Both were found by tests that
READ BACK what was written, which is the only kind that finds them.
Both READMEs record the difference between the rails (done) and the pilot itself
(scheduler, graceful shutdown, worktree retention, model-wait/retry, health
metrics), so the gap is visible to a reader rather than buried in a chat log.
GitLab labels are creatable, so `bootstrapStates` creates them through the API, walking every page of the project's label list (the seam exposes no headers, so a short page is the only end-of-list signal — the price of that is documented). One colour for all six, because GitLab requires one and takumi never reads a colour back: a palette would imply a meaning the state model does not have. A label that exists with a different colour is reported `exists` and NOT rewritten. Notion is the opposite case and says so: an option lives in the `select`/`status` property, and `PATCH /v1/databases/:id` replaces the WHOLE options array, so a write would rewrite options a human curated and race with anyone editing that property in the UI with no precondition to detect the loss. `canBootstrapStates` is false, and the report distinguishes a missing OPTION from a missing PROPERTY so the instruction names the right UI step. The Notion test double gained the database retrieve the report reads (one option list, so a test can model a board that is missing one) plus the three report-only cases: a missing option, a missing property, and a dry run that equals the real call because nothing is ever written.
… the exact fix) Both keep their statuses in administration, out of the API's reach, so the honest answer is a report a human can act on — and the instruction has to name the status and the step, not merely say "missing". Jira reads `GET /rest/api/3/project/<key>/statuses` (per-issue-type buckets) and compares against the adapter's OWN `statusMap`, so the report cannot drift from what `transition()` will actually write. With no project key configured, every state is reported at once with that as the instruction. Redmine reads `/issue_statuses.json` — the same cached list its mapping uses — and its instruction carries both halves of the fix: create the status in administration AND pass `statusMap`/`--status-map` so the adapter can resolve it to an id. Verified against an empty board as well as a populated one: a report-only adapter that said "everything exists" would pass a suite trivially while stranding an operator, so an empty instance is exercised and must produce six `not-creatable` entries with non-empty instructions.
The rails are only useful if an operator can reach them. `--check` reports what the board is missing without changing anything; `--bootstrap` creates what the board allows. A dry run goes through the SAME `bootstrapStates` call with `dryRun` set, so what an operator reviews is exactly what the mutating call will do rather than a second implementation that can disagree with it. The report is rendered for a human — state, outcome, the board's own name for it, and the instruction indented underneath — because the instruction IS the product when a state cannot be created. The command exits 1 when any state needs a human, so a scripted check fails loudly instead of printing advice into a log nobody reads.
ADR-008 built the rails; this is the vehicle. `runPilotTick` selects the first ready item nobody else holds, prepares a worktree from the FROZEN base sha, runs the agent with retries on transport failures only, and drives the delivery loop to its end state. One item per tick, so concurrency is the scheduler's business — which is exactly why the slot lock matters. Two decisions worth naming: - reviewMode says WHO is trusted to judge: `checks-only` (green checks are enough) or `label` (a human must approve). Waiting for that human is NOT a defect, so the review hook can return `awaiting-human`: the loop stops with `awaiting_review` without consuming a round, without moving the item to fix_needed, and without asking the agent to rewrite a change nobody rejected. Before this, a missing approval was indistinguishable from a finding. - worktrees have a retention window, and pruning NEVER removes one holding uncommitted work: that is somebody's evidence, and a retention policy is not a licence to destroy it. The report names what was kept and why. Also: `agent.retry` joins the event registry, and the retry wrapper emits ONLY the retry — the loop owns `agent.started`/`agent.finished`, and emitting those in both places would put two events in the trail for one fact (the class of bug the ledger already names "two mechanisms, one effect"; a test caught it here).
A one-shot tick, started by a systemd timer or cron: nothing resident, nothing leaked between ticks, and an accidental overlap is safe because the per-item slot lock says which item is taken. The command reads the `pilot:` section of takumi.yaml and refuses to run without it — which repository, which board, which agent are deployment decisions, and guessing one is how an agent commits into the wrong checkout. The agent is a COMMAND started in the worktree with TAKUMI_ITEM_ID / TAKUMI_RUN_ID / TAKUMI_BRANCH / TAKUMI_ROUND in its environment, so its logs and commits tie back to the run without takumi parsing its output. Its exit code is classification: 0 is done (takumi expects a COMMIT), non-zero is the agent reporting a real failure and BLOCKS the item rather than retrying the same wall, and killed/timed-out/missing-binary are transport failures that retry. SIGTERM/SIGINT kill the child (TERM, then KILL after a grace period) and end the tick as retriable, so the slot is released instead of being held by a process that no longer exists. Exit 0 for idle / delivered / awaiting_review / busy / not_claimed / retriable; exit 1 only for blocked. A scheduler journal that screams "nothing to do" trains people to ignore it, and blocked is the one outcome a human must look at. Tested twice over, because the two levels catch different things: core tests the selection, the approval wait, retry counting, classification and slot release against in-file doubles, while the CLI test drives a REAL bare origin, a real checkout, a real worktree from a frozen sha and a REAL agent child process that commits a file.
…ilt for ADR-009 states the shape and why: a one-shot process driven by an external scheduler (the systemd unit and timer, plus the cron line, are in the ADR), one item per tick, an explicit trust decision for review, an agent whose exit code is classification, a retention window that never deletes uncommitted work, and exit codes a scheduler can act on. It also records the limit honestly: a retriable failure AFTER the claim leaves the item claimed by a run that will not continue, and there is no automatic takeover yet — a stale-claim policy needs a deliberate port extension, and inventing one under time pressure is how a "sometimes steals someone else's item" bug ships. The tick reports it and names the item, so it is visible rather than silent. Both READMEs move the remaining work to what it actually is: health metrics, progress throttling, a stale-claim policy, and epic/milestone scoping (which needs a text-search capability on the board port before it can be uniform).
…d it Two defects with one root cause: the runner selects `ready` items only, so any item left in another state is not "waiting", it is INVISIBLE — and two code paths left one there. 1. PENDING CHECKS WERE ABANDONED. On a pending check the loop returned `retriable` and walked away, leaving the item owned by a run that would never come back. The comment said "another tick will read it again"; no tick ever did. Checks are now waited for INSIDE the tick (`checksWaitSeconds`, default 300, with `checksPollSeconds`), because a slow CI is the ordinary case and it was making the ordinary case fail. If the budget runs out the item BLOCKS with the knob named in the message. 2. A RETRIABLE FAILURE AFTER THE CLAIM did the same thing. The outcome stays `retriable` (the tick itself deserves another go) but the ITEM is blocked with the instruction that resumes it, because handing it to a human is the only state automation can come back from. A failure BEFORE the claim still touches nothing. And the way back for everything already stranded: every tick now SWEEPS the in-flight items. It reports each one (`pilot.in_flight`, with the age and the run that owns it) and, when `blockStaleClaims` is opted in, hands a stale claim back to a human. The evidence is the SLOT it had to take — if it can take the slot, no live runner holds the item — and the action is `blocked`, never a silent takeover of the claim. An open pull request is never swept: that is a human's decision. Three tests encoded the old behaviour and were rewritten with the reason, so the diff shows what changed and why rather than only what the code does now.
…t pace themselves ADR-010. Two things a runner needs before someone leaves it alone for a week. METRICS. `pilot.metricsFile` holds counters as JSON and `<file>.prom` the same numbers as a Prometheus textfile: ticks, every tick outcome, agent retries, seconds spent waiting, and a `last_tick_timestamp_seconds` gauge that is the freshness signal. A file, not a server — a self-hosted box already runs an exporter, so a textfile needs no port, no daemon and no auth, and `node_exporter --collector.textfile.directory` is the whole integration. Written to a temporary file and renamed, so a scraper never reads half a file; a CORRUPT file resets to zero rather than throwing, because losing a counter is cheaper than losing a tick and a metric that lies is worse than one that restarts. PACING. `plan.progressIntervalSeconds` (default 30) suppresses a progress write only when the record's signature — run id, round, pull request — is unchanged and the last write was recent. The first write of any new signature always happens, so the board still reflects the latest round; what disappears is the repeated identical write while a tick waits for checks, which used to be one API call per poll. The two are tested together on purpose: one test asserts that five polls produce two board writes, because that churn is exactly what the throttle exists for. Every counter is a FIELD on the loop's or the tick's result, never a number parsed out of a human-readable step's text — the first draft did the parsing and was replaced before it shipped, since a counter tied to a log line's wording silently becomes zero the day somebody rewords it.
…is slice fixed `pilot.metricsFile` (plus `metricsTextfile`, default `<metricsFile>.prom`) is written by the tick itself: read-modify-write, because a tick is a separate process and the file is the only memory between them. Monotonic counters, so a lost tick is a missed increment rather than a wrong total. The bug ledger gains #8 and #9 with the fix commit that carries them (`e15c62b`), each with the four parts it needs — symptom, root cause, fix, regression test — and two new classes: a comment describing behaviour nobody implements, and a state automation cannot return from. ADR-009's recorded limit is marked fixed, with what replaced it.
`pilot.metricsFile` was parsed out of takumi.yaml and then dropped, so no counters were
ever written; the pacing knobs (checksWaitSeconds, checksPollSeconds,
progressIntervalSeconds, blockStaleClaims, staleClaimSeconds) were in the same state, and
the pilot did not hand those to the loop even when it had them. A configured option that
nothing reads is the "silent drop" class the ledger already names — the operator gets a
file that never appears and no error.
Found by running the real command (`takumi pilot --once` in a throwaway project) and
noticing the metrics file was missing while everything else worked, which is exactly the
kind of gap a test of `runOnce` cannot see: runOnce was fine, the wiring above it was not.
`runPilotCommand` is now exported and tested through a real takumi.yaml, so the config
path itself is covered. The new assertions check the EFFECT, not the wiring: the metrics
file must EXIST after a tick, and a policy budget must show up as the number in the block
message ("still pending after 45s") rather than as an object passed along.
The ledger entry for ada3262, with the class it belongs to: a field parsed from a config file and never used is silence, not a default. Its regression test asserts the EFFECT of a setting — a file that must exist, a number that must appear in a message — because a test that asserts an object was passed along passes whether or not anything reads it.
The pilot could not create work, so a pipeline that stayed red through every fix round produced a blocked item and a comment. That is honest and it is also how a repository rots: the failure has an owner only if somebody reads the comment. `createWork(spec)` is now a required port method with a truthful `canCreateWork` flag. All six adapters implement it (every one of these boards can file an item); an adapter declaring false must fail closed as `unsupported` rather than pretend. IDEMPOTENCY IS THE POINT. A tick is retried, so a filing must not become a second issue. No board here has a native idempotency key, so the key travels as a machine-readable marker (`<!-- takumi:created=<key> -->`, the same HTML-comment trick as the run marker) and every adapter SEARCHES BEFORE CREATING: GitHub through the search API, GitLab through project issue search, Notion through a property `contains` query, Jira through JQL text search, Redmine through `/search.json` followed by a LOCAL marker check (its index can lag, which is stated rather than hidden). The second call returns the FIRST item with `created: false`. A create is a write, so the adapters refuse to guess: Redmine rejects a labels request instead of dropping it and names the available statuses when a state is unmapped; Jira refuses to invent an issue type; GitHub turns a 422 on a missing state label into "run `takumi board --bootstrap`", which finally ties ADR-008's bootstrap to the first write that needs it; Notion puts the marker in the property it already owns. The shared suite now covers this itself: a create must start in the requested state, must not create twice for one key, must appear exactly once in listWork, and must be claimable afterwards. It caught two defects in this slice before they were committed — a missing `case` in `assertBoardCapability` that silently disarmed four capability gates at once, and a test double whose created issue lacked the state custom field, so a fresh item could not be claimed. One commit because a required port method is atomic: it cannot land for core alone while six adapters still lack it.
`fileIssueOnExhaustedChecks`, off by default. When it is on, the loop's "checks stayed red and no fix round is left" path files an item under the key `ci-red:<item>:<pr>`: the title names the item and the failing checks, the body carries the pull request url, the check names and the run id, and a later attempt at the same item and pull request returns the FIRST item with `created: false` instead of filing a copy. Filing is a side channel, so the ADR-008 rule applies: a board that declares `canCreateWork: false` is told so (reported, not silently skipped), and a filing that fails leaves the item blocked with the reason in the step timeline — it never changes the delivery's conclusion. Both cases are tested, along with filing staying off when nobody asked for it. The idempotency key is derived from the item and the delivery, never from the clock or the run id: that is the whole reason a retried tick cannot spam the board.
The lesson from the config-with-no-reader defect, applied immediately: a new policy knob is wired from takumi.yaml through the command into the pilot and into the loop plan, in the same change that introduces it.
…ed (ADR-012) A pilot rarely wants "everything ready": it wants one epic, one milestone, one component. Modelling each board's epic concept would be five concepts behind one name, so the scope is TEXT — `BoardWorkQuery.query` — and each adapter translates it into the search that board already has: GitHub's search API, GitLab's `search=`, Jira's `text ~` inside the SAME JQL as the state filter, Redmine's `/search.json` followed by one read per candidate (its search answers no status, so a hit is not work until it is read — the cost is documented and the test asserts the exact request sequence), Notion's `title.contains` — which searches TITLES ONLY, said plainly in its capability comment rather than hidden behind the word "search". `canTextSearch` is where the rule bites: a board that cannot search must fail closed as `unsupported`. Everywhere else a missing capability means "does less"; here it would mean the runner works on exactly the items the operator excluded, which is the one outcome a scope exists to prevent. The suite enforces both directions — a true board must find an item its text carries and must NOT return it for a term nothing carries. `pilot.policy.scopeQuery` narrows a tick, and the tick asserts the capability BEFORE listing anything: an unhonourable scope is a configuration error, not a reason to process the whole board. The in-flight sweep is deliberately NOT scoped — an item stranded outside today's scope is the one nobody would notice. Two test doubles were case-sensitive where the real search is not (Jira's `text ~`, Redmine's full-text search), which made a capitalised subject invisible to a lowercase term; both now match the boards they stand in for, and a GitHub simulator that gave a created issue every label in the repository — instead of the ones the request carried — is fixed too.
`pilot.policy.scopeQuery` is wired from takumi.yaml through the command into the tick, in the same change that introduces it (the rule from the config-with-no-reader defect). Both READMEs drop the epic/milestone item from the roadmap: it is not a to-do any more, it is a capability that either works on a board or refuses.
The same `scopeQuery` meant three different things depending on which board it pointed at:
GitLab and Notion DROPPED a whitespace-only term (sending no filter, so the caller asked to
narrow and received the whole board while the request looked scoped), Jira refused it, and
GitHub silently STRIPPED a double quote out of the term, searching for words the operator
never wrote. Two of the three were the silent widening ADR-012 exists to forbid, hiding
behind comments that each claimed to be avoiding it.
Found by reading two subagents' reports against my own adapter's behaviour, which is the
only place this class shows up: every implementation looked right in isolation.
`assertScopeQuery` in core now owns the rule — a scope is carried FAITHFULLY or REFUSED,
never quietly turned into a different query — and every adapter (including the fake, which
is the reference) calls it. The contract suite asserts both shapes for every adapter, so
drift cannot come back one adapter at a time:
- a whitespace-only scope must fail `precondition`, and nothing may reach the host;
- a quoted term must either be refused or return ONLY items that carry the word — never a
widened list.
Two tests encoded the old behaviour ("a blank term sends no text filter") and were
rewritten with the reason in them, so the diff records what changed rather than only what
the code does now.
The ledger row for a18b7f5, plus the class it belongs to: when an abstraction has N adapters, any rule left to each adapter drifts. ADR-012 gains the section that records what the first version got wrong and where the rule lives now.
A task prompt for adding OpenHands behind the AgentRuntimeAdapter boundary, archived in English with three additions that only the codebase can supply: the exact files (the port, the contract suite, the registry, the pilot seam, the commit boundary), the fact that the runtime registry and the pilot are TWO different agent seams (proving one does not prove the other), and the repository conventions the work must follow. It also records the gap this task should close: nothing today proves that an agent claiming success without committing is refused — the runtime contract suite knows nothing about git, and its happy path is reachable by a session that never commits.
…entity
Found by running a REAL agent (OpenHands) against a real worktree for the first time: the
agent command received the item's id, run id, branch and round — and nothing else. An agent
that is a command with no board access therefore had no way to know what to do; the only
alternatives were re-reading the board from inside the agent (a second, unauthenticated path
to the same state) or hardcoding the task, which is not an agent.
`TAKUMI_ITEM_TITLE`, `TAKUMI_ITEM_BODY` and `TAKUMI_ITEM_URL` now reach the child, with a
test asserting it does — the earlier test asserted the identity and could not have caught
this, because the identity was never the missing part.
The fake board grows `--items` (a JSON array of {id,title,body,state}) for the same reason:
its built-in demo items say "A ready item (the fake board is a demo)", which is not work any
agent can do. Seeded states are validated against the six declared states and a typo is an
error — a state no runner reads would otherwise park the item and leave the tick looking
idle for no visible reason.
…est limit OpenHands is the agent in a real takumi pilot tick: real repository, real worktree cut by takumi, real commit by the agent (c94c98d), test executed, tree clean, board state written back (OH-1 merged). The interface facts are recorded rather than assumed: the package is `openhands` (CLI, Python ==3.12.*), the non-interactive form is `openhands --headless -t "<task>"`, and LLM_* environment variables are IGNORED unless `--override-with-envs` is passed. The Decision Gate lands on A, so nothing new was abstracted: `examples/openhands-agent.sh` is configuration, the item-text change above is something every real agent needs, and no `runtimes/openhands` package was created. What would justify B later is concrete, not speculative: `--headless --json` streams the events where per-task usage and artifacts live, which the pilot path cannot reach today. Three findings are worth more than the integration itself: the dirty-tree guard is real and the task text is what satisfies it (the first smoke left __pycache__/ untracked and every delivery adapter refuses that — in the pilot run the agent added .gitignore because the task said to); false completion cannot be tested at the runtime seam at all, because that port knows nothing about git, so it must be asserted where the invariant lives; and `cli:<command>` can bridge OpenHands only through a wrapper, since the bridge appends the task as the final argument and OpenHands needs its flags around it. The report states plainly that the DELIVERY in that run was simulated (deliveries/fake — its head is a configured value, which is why the events show the base sha), so no reader can take it as proof that a real host verified the agent's commit. `deliveries/git` is that missing piece. Docker is installed on this host but unusable (the user is not in the docker group), so the agent ran unsandboxed as the host user — also stated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One tick, one item, one worktree - the shape a systemd timer or cron drives.
Full suite at this head: 444 tests, 0 failing.