Repository navigation
feat: deterministic review, resume, and a delivery with no review surface (ADR-013 to ADR-016) - #4
Merged
sscodeai merged 23 commits intoSep 30, 2026
Conversation
… 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.
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.
A green pipeline can be bought, so the reviewer reads the change set instead.
Full suite at this head: 496 tests, 0 failing.