Repository navigation
feat: an unattended pilot runner (ADR-008 to ADR-012) - #7
Merged
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.
… bought
The review conveyor was complete and empty. `deps.review` — the one place a real reviewer
could stand — was never supplied in production, so every delivery was reviewed `clean`, and
the only automated gate was CI. Two consequences followed, both real: a repository with NO
pipeline merged unverified (an empty check list is indistinguishable from a checked one), and
a pipeline could be satisfied by WEAKENING THE TESTS IT RUNS — an agent that deletes the
failing test, adds a skip, or drops an assertion turns CI green while making the repository
worse, and no CI can catch that, because CI is what is being gamed.
`runReviewRules` is a pure function — change set in, findings out — so every rule is
exhaustible by tests and the reviewer's failure mode is a fact about the diff instead of a
mood. A model reviewer is a second opinion whose failure mode is "it said it was fine"; it
belongs behind the same seam later, never first.
THE RULES THAT CARRY THE WEIGHT:
- `test-weakening/deleted` — a test file present at the base is gone at the head;
- `test-weakening/skipped` — skip markers increased (`.skip`, `xit(`, `@Ignore`,
`pytest.mark.skip`, `t.Skip(`, `SkipTest`, `xfail`, …);
- `test-weakening/assertions-removed` — the assertion count dropped between two sides of a
file that exists in both. A deliberate heuristic, and an ANSWERABLE finding rather than a
final verdict, because the agent that weakened the test is the one who can restore it;
- `test-only-change` — nothing outside the tests changed. The ambiguous case: legitimate when
the item IS the tests, the signature of a bought pipeline when it is not, so it is a note
next to the item title and the operator decides if it should block. Suppressed when a
weakening finding already exists — noise on a defect is not a second opinion;
- `protected-path` — CI configuration, dependency manifests, the container build, takumi's
own configuration. Severity `human`, not `block`: telling the agent to "fix" a legitimate
dependency bump would mean reverting real work, so the item waits for a person and burns no
round.
FAIL CLOSED is the whole point: a reviewer that cannot run must never be read as clean.
`collectReviewInput` throws `ProviderError('transport')` when git will not answer, when a
revision is unreadable, or when the change set is larger than any rule should judge; the loop
does not catch it, the tick ends `retriable`, and nothing merges. Whether `git show` failing
means "the file was absent" is decided by the RECORDED STATUS and never by the exit code —
reading an exit code as absence would silently weaken every rule that reads content.
`ReviewContext` gained `worktree` and `baseSha` (a reviewer handed file names can say "the CI
config changed", not "this test lost three assertions", and reconstructing the base elsewhere
would give it a second source of truth), and a `clean` verdict may now carry a note, so an
observation reaches the board comment without pretending to be a defect.
`reviewMode: rules` and a real reviewer are built together, so an operator who asks for the mode gets the gate. A mode that promises a review and ships a rubber stamp is the same class of defect as a configuration field nothing reads (the ledger's #10) — and that was exactly the state of `deps.review` in production before this: the seam existed, nothing stood in it. The CLI test proves the wiring the only way that means anything: a REAL repository, a REAL agent process that deletes an assertion and commits, and the CLI's own tick — which must end `blocked`, exit 1, and merge nothing. The honest variant of the same change still merges. And a git that refuses to answer (a stand-in for a shallow clone or a vanished object) ends `retriable` with the reason named, instead of delivering — the fail-closed direction, tested through the seam a deployment actually uses. The test needed a delivery that reports the head REALLY on the branch, because the fake delivery hands back a configured sha: coherent for the loop's own tests, useless for a reviewer that reads the change (a canned head makes it fail closed — correctly, and untestably). `RealHeadDelivery` is that smallest thing, and it asserts the standing rule while it is there: only the reviewed head may merge.
The claim "none of which need a port change" was an assumption made when the loop did not yet exist in this shape. Reading the code before building `deliveries/git` shows the loop guards `pr === undefined` on every failure path but assumes a pull request on the happy one: the delivered/review events name it, `checks(pr)` is called with it, and `merge()` takes a PullRequestRef. A bare-remote provider therefore needs the port to carry a `DeliveryRef` (branch + head + optional review surface) with PR events and the checks phase capability-gated. Recorded rather than quietly worked around: the next person reading ADR-007 would otherwise plan a slice that cannot land.
…everted)
Making DeliveryOutcome.pr optional was tried: the port change is one line, and it broke the
shared delivery contract suite in more than twenty places, because every PR-shaped assertion
("the same call twice does not open two", "the merged head is the reviewed one") assumes a pull
request exists. That suite is the safety net for three adapters, so it must learn
capability-aware expectations before a no-PR adapter can inherit it.
Recorded because the experiment was cheap and the discovery is not: the next attempt should
start at the suite, not at the adapter, and should not be planned as an afternoon of code. The
work stopped at a green tree rather than half-way through a port change.
Found by running it for real: `takumi pilot` against gitlab.com failed immediately with
`spawn examples/openhands-agent.sh EACCES`, because the file was committed mode 100644. Every
documented use of it — the pilot's `agent.command` — starts the file directly, so the shipped
example could never have worked, and nothing in the suite would notice: the tests build their
own agent commands.
Takumi's own behaviour in that failure was exactly right, which is how the defect was found
rather than hidden: the EACCES was classified as a transport failure (retryable, twice), the
item was then blocked with an honest reason ("no runner can resume a held claim — move back to
ready"), the state record was written as a real GitLab note, and the `takumi-blocked` label was
applied. The transport retry path and the board's state-record write were both exercised against
a real instance for the first time by this accident.
The executable bit is now committed as well as set (git tracks the mode, so a plain `chmod`
would have left the same defect for the next clone).
…ommit
Measured against gitlab.com, on a real merge request:
GET /projects/:id/repository/commits/<sha> -> 200 (the commit exists)
GET /projects/:id/merge_requests/1/pipelines -> 200 [] (no CI configured)
GET /projects/:id/commits/<sha>/statuses -> 404 (nothing published)
The adapter read that 404 as `not_found` and blocked the item, leaving a MERGEABLE merge
request with a real commit on a real branch unmerged. The two cases are different facts and
the fix separates them instead of swallowing both: when the statuses endpoint answers 404,
ask the commit itself — if the commit exists, "no statuses were published" is the honest
answer (an empty check list, which the loop already treats as "no checks reported"); if the
commit is unknown too, the original error stands, because a commit the host cannot see is a
real problem that must not be mistaken for a quiet project.
Only a real instance showed this: every offline double answers the statuses endpoint with an
array, so the fallback had never once been exercised against the 404 the host actually sends.
Every adapter read the state record FIRST and refused unconditionally when it named another run
— ahead of the check on the board's own state. On the first real run against a real instance
that made an item UNRECOVERABLE: a run claimed the item, died of a transport failure, and
blocked it. The operator did exactly what takumi's own block message told them to do, moved it
back to `ready`, and every later run was refused, because the dead run's record named itself.
The only way past it was deleting takumi's own note through the host's API — a workaround for a
defect, not a recovery path.
THE RULE NOW LIVES IN CORE, ONCE (`decideClaim`):
1. The board's STATE is the authority for whether an item is held: a run that holds an item
never leaves it at `ready`, so an item the board reports as `ready` is available no matter
what a previous run's record says.
2. The record is EVIDENCE ABOUT A RUN, not a lock — it answers "who worked this last" and
drives the tracing a human reads.
3. A refused claim still says why (unchanged), and a same-run repeat is named for what it is
(a repeat of its own claim) rather than reported as somebody else's.
All six implementations call it — the five adapters and the fake, which is the reference the
others are measured against; a fake that kept its claim in a private field was measuring a
different contract from the one the adapters are held to. The takeover is not silent: the claim
reports `takeoverFrom`, and the record it writes says whose claim it took over, because a human
reading the trail later is the audience for exactly that fact.
The shared contract suite now asserts the recovery path for EVERY board: it writes a record
naming a run that died, requires the item to still read as `ready`, then requires a claim to
succeed, report the takeover, and take the item. A board that keeps no machine-readable state
reports NOT_RUN instead of passing — the defect cannot exist without records, and a green tick
for a check that never ran is the kind of lie this suite exists to prevent.
Two GitLab tests had encoded the old mechanism and now assert the new behaviour with the reason
written down: an unguarded provider (no trustedAuthors ⇒ no trust boundary) still refuses, but
the refusal comes from the claim-race verdict instead of the record read — the board says
`ready`, so a note whose provenance this provider cannot judge must not work as a lock, and the
refusal stays fail-closed (nothing is delivered).
… blocked visibly
Found on a real instance, right after the claim fix let the delivery through: GitLab computes
mergeability ASYNCHRONOUSLY, so a merge request that was just opened answers `mergeable: null`
for a moment. The loop read that answer as a verdict — honestly refusing to merge ("an unknown
is not a yes") — and returned `retriable`.
Two things were wrong with what happened next:
1. The moment was given no chance to pass. A host that is still computing is not a host that
said no, and one read is not an answer to a question the host is still answering.
2. `retriable` left the item in `pr_open`, where NOTHING looks at it again: the pilot selects
`ready` work. So a transient hiccup at merge time stranded a finished, reviewed delivery —
the same silent stall the pending-checks budget already refuses to create ("nothing ever
selects an item in review again, so leaving it there was a silent stall"). Two paths, one
principle, two behaviours; now one.
So: the unknown is re-read inside a BOUNDED window (default 5 reads, 3s apart — `mergeabilityReads`
/ `mergeabilityReadSeconds`), with the head re-verified on EVERY attempt, because a head that
moves is a decision rather than a transient. Each wait is visible in the trail
(`merge.mergeability_waited`, registered like every event name). If the window ends and the host
still has not answered, the item is BLOCKED — visible to the watchdog and to a human — and the
comment carries the way back ("move the item back to `ready`"), which the claim fix now actually
honours instead of refusing forever.
The tests drive a delivery whose status is unknown twice and then an answer (merges, two waits,
three reads), and one that never answers (blocked, waits bounded by the window, merge never
called, the way back in the comment) — with an injected sleep, so neither waits in real time.
…evidence for it Written from two real runs on the same afternoon: a delivery reached pr_open with a mergeable merge request and then hit a network failure inside git fetch, and another reached pr_open where the host answered "mergeability not computed yet". Both were handled correctly by the loop, and both left a finished delivery in a state no tick selects again — so the only way forward is a human, and that path re-runs the agent and pushes a SECOND branch and merge request for work that was already reviewed. Proposed rather than accepted: three questions are genuinely open (a PR-shaped DeliveryRef vs a bare-remote delivery, a human who parked the item on purpose, and where a resumed review reads its diff from when the worktree is pruned), and the first of those shares a seam with deliveries/git. Recorded now so the next session starts from the design and the evidence instead of re-deriving them.
…edoing it The pilot selected `ready` work only, and everything after the claim lives in states no tick selects again. Two real runs on gitlab.com showed what that costs: a delivery reached `pr_open` with a mergeable merge request and then hit a network failure inside `git fetch`, and another reached `pr_open` where the host answered "mergeability not computed yet". Both were handled correctly by the loop, and both left a finished, reviewed delivery stranded — the sweep reported it, and the only way forward was a human resetting the item, which RE-RAN THE AGENT and pushed a SECOND branch and a second merge request for work that was already reviewed. This is the shape orbi runs: its tick scans the open-PR states (`ai-pr-opened`, `ai-fix-needed`) as well as `ai-ready`, and resumes the same branch, worktree and pull request. Now takumi does too, with the same one-item-per-tick discipline: - **The record remembers the branch** (`BoardStateRecord.branch`, written beside `deliveryRef`). Without it there is nothing to resume, and a record that lacks it is SKIPPED rather than guessed at: a wrong branch means a wrong delivery. - **The selection holds two kinds of work**: fresh items first, and — when there are none, because fresh work is the operator's new request — items in `pr_open`/`fix_needed` whose record names a delivery. The scan is bounded (ten record reads) and announced (`pilot.resumed`), not a silent mode change. - **The worktree is checked out ON that branch**, fetched from the remote, and the frozen base becomes the MERGE BASE with it — so the review's diff is exactly what the delivery adds, whatever the base branch has done since. - **The agent does not run.** Its work is already committed and pushed; the loop's contract (a commit exists, the tree is clean) is what the resumed branch already satisfies, and if it does not, the delivery refuses exactly as it would for a fresh run. - **`decideClaim` gained its third rule**: `pr_open`/`fix_needed` are IN FLIGHT, not held, so they are claimable when the record names another run — finishing a delivery is work too. `merged`/`blocked` stay unclaimable (a human decides those), and `claimed` means somebody is working it right now. On gitlab.com this is what makes a transient failure recoverable without duplicating anything. The tests cover the whole path: a tick with nothing ready resumes and asks for the recorded branch; an in-flight item whose record names no branch is left alone; and — through the real CLI, a real repository, a real bare remote — a tick RESUME delivers and merges while the agent command is `/nonexistent-agent-command`, which is how the test proves the agent never ran, and the remote ends with exactly ONE branch, which is how it proves no duplicate was pushed. ADR-014 (proposed) is accepted for the part that landed; the PR-less delivery seam it also touches stays open, because generalizing the delivery reference is `deliveries/git`'s slice.
…could not merge Pre-existing, and found while building the resume path. The loop moved the item to `pr_open` after delivering only when the delivery CREATED the pull request (`round === 0 && created`) or when the item sat in `fix_needed`. A delivery that REUSES an existing pull request in round 0 — exactly what resuming is, and what any item gets when an earlier run already pushed its branch — therefore stayed in `claimed`, and the merge that followed was refused as an illegal transition (`claimed → merged`): the item blocked on the state machine rather than on anything real. The state after delivering is not a function of who opened the pull request. The item IS delivered and under review, so the rule is now read off the state itself, and a delivery already in `pr_open` is left where it is. Found by a test that could not have existed before this slice: the resume test in the CLI drives a real repository and asserts the tick finishes an interrupted delivery. It failed with `illegal board transition claimed → merged`, which is the defect speaking.
…surface
ADR-007 said a `deliveries/git` (a bare remote: a pushed branch and nothing to review) would need
no port change. Measured against the code, it did: `DeliveryOutcome.pr` was required, and the
steps that follow a delivery — status, checks, merge — all took a pull request reference. The only
ways to fit a bare remote in were to invent a fake pull request (the events and the board state
would then claim one that does not exist) or to weaken the contract suite. Both are lies about
what happened, so the port changed instead.
- **`DeliveryOutcome.pr` is optional.** A provider that reports `canOpenPullRequest: false`
reports no reference, and `deliveryRefFor(outcome, baseSha)` is what the loop uses to drive the
delivery steps: the pull request when there is one, the pushed branch when there is not. The
provider is never asked to invent a reference at the call site — it receives the branch it just
pushed, which is exactly what it can answer about.
- **The loop threads one reference through the round** (`thisRef`, narrowed once after the
delivery), so `status`/`checks`/`merge`/`waitForChecks` and the reviewer all work against the
same thing regardless of shape.
- **Nothing claims a pull request that does not exist.** The `deliver.pr_opened`/`pr_reused`
events are emitted only when there IS a pull request; the per-event `pr:` field is spread
conditionally; and the trail, the record and the closing comment name the branch instead
("branch takumi/7-abc pushed for review", "merged onto the base branch: branch …, commit …").
- **The shared contract suite now enforces the honesty in both directions** (ADR-007's pattern of
a capability-gated gate): a provider declaring `canOpenPullRequest: false` that reports a pull
request FAILS, one declaring `true` that reports none FAILS, and the PR-shaped assertions
(one pull request per delivery, reused on the second call, the reference points at the pushed
head) apply exactly when the capability is there. Everything that holds regardless — a plain
push, the pushed branch being the task branch, the head being reported, stale-head and
conflicting merges refused — is asserted unconditionally.
- **The delivery adapters' own tests say what they are:** they are about providers that DO open a
review surface, so each narrows the outcome once (`prRefOf`) rather than asserting at every call
site. The fake, github and gitlab suites are unchanged in what they prove.
Why this ordering (the suite first, then the implementation) is in the ledger: doing it the other
way round is how this slice got reverted twice, and the second attempt started from the suite.
…ACK to prove it
The first delivery target with no review surface: branches, and nothing else. It is what a
self-hosted git server without an API adapter looks like, and leaving it out meant takumi's only
honest use was against a forge.
- **The push is verified, not assumed.** `git push`'s exit code says the command ran; it does not
say the remote holds the commit. The head this provider reports is read back with `ls-remote`
after the push, so "delivered" is a fact about the remote. That is the difference between a
delivery and a hopeful log line.
- **It claims nothing it does not have.** `canOpenPullRequest: false` and `canRunChecks: false`,
and the loop then names the branch instead of a pull request ("branch takumi/7-abc pushed for
review"; "no checks reported") — which is why the honesty had to live in the port (previous
commit) before this could exist.
- **Merge is a fast-forward of the base branch to exactly the reviewed head**: a plain push of
`HEAD:refs/heads/<base>`. A non-fast-forward is refused by git itself, so a base that moved on
is a `precondition` handed to a human — never a force push, never rewritten history. `squash`
and `rebase` are refused as `unsupported` rather than silently becoming something else.
- **`status` answers from the refs**: `merged` once the base contains the head, `open` otherwise,
and `mergeable` is the honest question "can the base move forward to this head" — a tri-state
that is never a yes when it is unknown.
Tests: the shared Delivery Contract Suite with its capability-gated parts REPORTED (not skipped),
against a real bare remote — plus the cases this provider adds: the remote head is read back, a
head that moved is refused, a diverged base is refused without touching the base, squash is
refused, a dirty worktree pushes nothing, and HEAD equal to the frozen base is "no commit".
And through the real CLI, end to end: an item is claimed, a real agent commits in a real worktree,
the branch is pushed and verified, the base branch on the REMOTE moves to the reviewed head, and
the agent's file is asserted to be THERE (fetched and read) — with the trail asserted to contain no
pull request and no green pipeline, because a bare remote has neither.
…age its own books record
Decision B said the reason to promote OpenHands from a CLI seam to a runtime was per-task usage and
artifacts from its `--json` stream. Measuring a real run showed the stream carries NEITHER: it
emits ActionEvent/ObservationEvent/MessageEvent and no token or cost accounting at all — a
`getUsage` built on it would have had to invent numbers.
The numbers do exist, in the conversation's own state
(`<home>/conversations/<id>/base_state.json` → `stats.usage_to_metrics.<id>`): model, accumulated
cost, accumulated prompt/completion/cache_read/reasoning tokens, and a per-call breakdown. One real
task: 70347 prompt + 890 completion (58752 of them cache reads), cost 0.0 as recorded,
execution_status finished.
- events from the stream (mapped, with unknown kinds passed through rather than dropped), usage
from the accounting, artifacts = the verbatim transcript under the runtime's own home + the files
git reports as changed (never written into the worktree: the delivery refuses a dirty tree and
would be right to).
- **An unavailable accounting is not a zero**: the token numbers are zeros WITH the reason in
`extra.usageUnavailable`, and cost travels with a caveat — a "0" that could be read as "this task
was free" is the failure mode this avoids.
- **No `test.completed` from a heuristic**: the stream does not distinguish a test run from any
other command, so claiming one would be a guess dressed as an observation.
- The prose the CLI interleaves ("Initializing agent...", "Agent finished", the summary) is skipped
and COUNTED, and the count travels in the completion event.
- `extraArgs` are passed before the CLI flags — found by a failing test, because a wrapper binary
must be told before its arguments are parsed as the CLI's.
Passes the shared runtime contract suite with a REAL run (not `skipRealRun`), against a stub whose
fixtures are copied from a real capture, and verified once against the real CLI: status completed,
71237 tokens over a real conversation, a 16KB transcript, and the changed files read from git.
Not yet wired into the pilot (that is the next slice, and the reason this exists): a tick will emit
the usage from `getUsage` so a run reports what it cost instead of only that it happened.
…that cost `pilot.agent.runtime: openhands` runs the agent through `@takumi/runtime-openhands` instead of a subprocess, and the tick then emits `runtime.usage` — tokens in and out, the model, the duration, the cost as recorded, and how many artifacts the runtime collected. Before this, a tick could say that work happened and never what it cost; a subprocess has nothing to ask, which is exactly why the runtime port exists. - **An unknown runtime name is an `unsupported` error, never a silent fall back to `command`.** A tick that quietly ran a different agent than the one configured would be lying about its result. - **Usage is reported as the runtime reports it.** When the accounting is unavailable the trail says `usage NOT AVAILABLE (reason)` — a zero that could be read as "this task was free" is the failure mode being avoided, and the runtime adapter carries the distinction. - **The runtime prompt carries the pilot's contract** (do the work here, verify it, COMMIT it, leave the tree clean), the same rules the example wrapper states. This was found by running a real agent through the adapter: it edited files and did not commit, and the delivery refused it — correctly, and because the instruction had never been given. Verified end to end in a test: a tick with the agent on the runtime delivers, and the events file contains `runtime.usage` with the numbers the runtime reported (4500 tokens, the model, artifacts counted) plus the human line in the trail.
feat: deterministic review, resume, and a delivery with no review surface (ADR-013 to ADR-016)
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.
Second half of the stacked chain: the first PR of this block was opened against the previous branch, so this one carries the block into dev. Same commits, no content change.