Repository navigation
feat: evidence bound to its inputs, gates that fire, and a second reviewer (ADR-017 to ADR-022) - #5
Merged
sscodeai merged 21 commits intoSep 30, 2026
Conversation
…or the people looking Notion and Redmine as VIEWERS while GitHub or GitLab does the work and the pull requests: the two are not alternatives, they are an extension. This is the port-level shape for that, with zero changes to the six board adapters, the pilot, the delivery loop or any delivery adapter — the port boundary is the seam, which is what having one was for. - **Exactly one authority.** Reads, claims, the state record, bootstrap and every decision go to the primary. A mirror is never read for control flow: someone editing the Notion page must not change what takumi does. - **A mirror failure never fails the work, and is never silent.** Projection runs after the primary recorded the fact; a failure emits `mirror.failed` with the mirror's own reason. Failing the tick would let a decorative board block real work; swallowing it would leave a board people trust going stale. - **Projections are rebuildable.** resync() re-derives each mirror item from the primary, so a mirror that drifted or was unreachable comes back without touching the primary. The id map is a cache: losing it costs API calls, never correctness. - **Identity by marker, not hope**: `<!-- takumi:mirror:<primaryId> -->` plus an idempotency key derived from the primary item, so a re-run or a post-restart resync cannot duplicate the item. - **What is mirrored is what a PERSON reads** (state, comments). The state RECORD is deliberately not mirrored: it is the control-flow surface, and copying it into a human-facing board is the confusion rule 1 exists to prevent. - **Outbound only**, as agreed: nothing in a mirror creates work or drives state. An inbound path is a new ingress with its own trust decision, and is not part of this. Tests: control flow never reaches a mirror (a recording double proves it), transitions and comments are projected, a failing mirror leaves the primary's transition intact AND emits mirror.failed with the reason, a mirror that cannot create work is reported rather than silently absent, resync rebuilds a lost projection without duplicating it, and a composite with no mirror is refused. Core: 218 tests / 215 pass / 0 fail. Not yet reachable from the CLI (ADR-017 names the wiring as the next slice, so this does not become a capability with no reader).
… a digest, and refuse a stale one
The third delivery rule already refuses to merge anything but the reviewed HEAD. It does not close
this: a tick reviews cleanly and is interrupted before the merge; the operator edits the policy
(`reviewMode: rules` -> `checks-only`, the protected-path list, the rule set); the next tick RESUMES
the delivery — which is what ADR-014 added, and what resuming is for — and merges the OLD review
under the NEW policy. Nothing anywhere said so. The head was pinned; the JUDGEMENT was not.
- **bind**: a clean review is bound to `reviewDigest({head, base, policy, ruleset, reviewer})`, where
`policy` hashes only the knobs that decide whether a change may merge (a digest that moved for a
poll interval would refuse merges for no reason).
- **record**: the digest travels in the versioned state record (`reviewed{...}`), written FORCIBLY —
progress writes are throttled, evidence is not.
- **re-check**: at the merge, against what the board said BEFORE this run started. The read is taken
at the top of the loop on purpose: this run overwrites the record as it goes, so a digest compared
against our own fresh write would always agree and would check nothing.
- **refuse, by name**: a delivery on record with NO digest is unknown provenance, not "probably
fine"; a digest that no longer matches is stale. Both refuse, emit `review.stale` with both digests
and which input moved, and move the item to `blocked` with the reason. Re-reviewing is cheap.
- **approval becomes a record**: `approval{by, at, digest, ref}` instead of "the label exists". `by`
is null when the board gave no identity — a label says somebody approved, not who, and an unknown
actor is never written as a known one.
- a malformed `reviewed`/`approval` block touches `BoardStateRecordError`, like every other
present-but-corrupt block: a block we cannot read is not the same as no block.
The refusal is fail-closed in the direction this project keeps choosing: **deliveries created before
this change can no longer be finished by a merge** (their records carry no digest). That is why the
resume tests now assert a refusal rather than a merge — intended, not a regression: those deliveries
need one re-review, and from then on their records carry a digest. The same discipline applies next
to the rule set itself (when the rules become data) and to a model reviewer (builder family !=
reviewer family).
Core 224/221/0, CLI 40/40, 0 TS errors. ADR-018 records what is still unverified offline (a resumed
delivery whose digest MATCHES merging — the head of a real delivery is unknowable in a fixture, so
that one is verified on the live instance).
… adapter does not own
Found by verifying ADR-018 on a real GitLab instance. The digest WAS written, and then the board's
own record was reduced to a handful of fields the moment the item moved state:
{"schema":1,"runId":"b531b400","item":"4","reviewRound":0,"baseBranch":"main",
"deliveryRef":"#5","note":"merged 30bf1e10"}
No `branch`, no `reviewed`. Every adapter's `transition()` rebuilds the record with its own evidence,
and five of them did it from a fixed list of fields they knew about — so anything a run put there
that the adapter did not recognise was dropped. Invisible until two things depended on surviving a
transition: a resumed delivery could no longer find the branch to resume (the pilot refused to guess,
correctly), and the review digest never reached the next tick — which left the "policy changed after
the review" hole wide open behind a check that looked right. The same root cause explains both.
- `carryStateRecordForward` in core: a writer sets the fields it OWNS (`runId`, `note`, `updatedAt`,
the item id) and carries every other field forward. An adapter that does not understand a field
must not delete it.
- gitlab and redmine delegate through it; github, jira and notion carry `...existing` first and then
overwrite what they own. (The fake board never rewrote the record, so it never had the bug.)
- **The shared board contract suite now asserts it for every board**: write a record containing
`branch` and a `reviewed` digest, move the item through a transition, read it back — both must
survive. A class of bug that was invisible in five adapters is now impossible to reintroduce
without a red suite.
Board suite: gitlab 27/27, github 24/24, jira 27/27, notion 27/27, redmine 33/33, fake 9/9.
Core 224/221/0, 0 TS errors.
…of fetching into a branch it holds
Second finding of the ADR-018 live verification, in ADR-014's resume path:
retriable 2: git fetch --force origin takumi/2-c0d47800:refs/heads/takumi/2-c0d47800 exited 128:
fatal: refusing to fetch into branch 'refs/heads/takumi/2-c0d47800' checked out at
'/tmp/takumi-gl-pilot/worktrees/2-c0d47800'
The resume fetched the branch into a same-named local branch — but the interrupted run's worktree
still holds that branch, and git refuses. Every retry hit the same worktree, so the delivery could
never finish: `retriable` forever.
Resuming MEANS continuing in that worktree, so it is reused (`git worktree list --porcelain` finds
it, the frozen base becomes the merge base with it, and the delivery proceeds from there). A dirty
leftover is refused BY NAME — a human must decide what happens to uncommitted work, and cleaning it
would be takumi committing for the agent, which the delivery contract forbids.
Tested through a real resume with the leftover present: the worktree is reused, no fetch-into-branch
attempt happens, and the run reaches its verdict. CLI 40/40, core 0 TS errors.
…says only what it can prove Two findings from finishing the ADR-018 live verification, both in the resume path. 1. **The run ignored the base the worktree was created from.** The pilot resolved the base branch's head at the start and put THAT in the plan, while the worktree handle — the authoritative answer to "what is this worktree based on" — already carried the right value (the merge base, for a resume). A delivery that had been sitting in `pr_open` while the base branch moved on is not "based on" today's head, so the delivery's descent check refused it: correctly, but for a branch nobody had rewritten, and the resume could never proceed. The plan now uses `worktree.baseSha`. 2. **The refusal claimed more than it knew.** "the branch was rebased or rewritten" — the head not being descended from the base is all that is provable; whether the base moved past the branch's fork point or the branch was rewritten is not knowable from there, and the old wording pointed a real operator at the wrong cause (the base had simply moved on). The message now states the fact and both possibilities, and says a human decides which. Core 224/221/0, 0 TS errors, six boards 147 green, CLI 40/40.
… previous tick is not grounds to refuse Finishing the ADR-018 live verification exposed the check's scope as wrong, and the wrongness would have been worse than the hole it closed: "a delivery on record with no review digest must be refused" fires on an ordinary fix round (tick 1: findings, `fix_needed`, no digest written; tick 2: the agent fixes it, the review is clean, the prior record has no digest -> refused). That refusal would hit the common path far more often than the rare one it targets. The reasoning it forced out: **every merge is justified by the review THIS run performed, under the policy in force now** — the loop always reviews before it merges — so a digest on record cannot justify a refusal in a machine-gated delivery at all. What a digest really binds is the **human's approval**, given in an earlier run and able to name a head or a policy the human never saw. - the check is approval-centric: it only applies when `reviewMode: label` (a human gate is in force), and it refuses when the record cannot show what was presented for approval, or when the presented digest differs from the one about to be merged. - the version being presented is recorded WHEN the delivery parks for a human (`awaiting-human`), by the same `reviewInputsFor` function the merge uses — two computations of "what was reviewed" is how a check passes while being wrong. - the two refusal tests now set the human gate explicitly; the resume/fix-round tests assert the opposite (they must FINISH), which is what caught this. Core 224/221/0, CLI 40/40, 0 TS errors.
…can point at a line The rules caught an agent WEAKENING its own tests. Nothing caught an agent committing a credential — the other way a delivery can be worse than no delivery at all: a committed secret is leaked the moment it is pushed, and removing it in the next commit does not un-leak it. (Found while reading the semgrep/reviewdog designs: this is the rule their tooling would have contributed, and it is pure determinism, so it belongs in the engine rather than in a new dependency.) - `secret/committed` — BLOCK, on narrow, provable shapes only (GitLab/GitHub/OpenAI/AWS/Slack prefixes, private-key headers, JWTs). The message says ROTATE, not just "remove": the failure mode this rule exists for is a leaked credential, and deleting the line does not fix it. - `secret/suspected` — **human**, for a long high-entropy run. A heuristic never blocks: the false positive that matters is a hex digest in a lockfile, which has no upper/lower/digit MIX and is therefore never reported. Tested in both directions. - A secret already present at the BASE is not this change's doing, and is not re-reported every round — the reviewer's subject is the change, and noise buries the finding that is new. - `ReviewFinding.line` — findings can point at a line now, which is what a person needs to act and what any diff-anchored reporting would need. - `REVIEW_RULESET_ID` goes to `rules@2`: adding rules changes what a review MEANS, so digests computed under `rules@1` are now correctly stale for an approval. Core 229/226/0, 0 TS errors.
…the guard FIRE? The shape of a benchmark that can actually catch a broken guard: every row is a fixture that would fail loudly if the guard stopped working, the rows call PRODUCTION predicates (the real rules engine, mapped by `verdictFromFindings`, now exported from `review.ts` so anything reasoning about a review reaches the same verdict the live reviewer does), and the vocabulary is fixed — Block / Allow / Pass / Fail / Partial / n/a / TBD, with nothing left TBD because every fixture here runs offline and deterministically. Rows that need a live forge or a real git binary are marked n/a WITH the suite that covers them, rather than guessed at. weakened-tests Block review: test-weakening/deleted committed-secret Block review: secret/committed protected-path Block review: protected-path -> human (waits for a person) dirty-worktree Block delivery: agent boundary = commit head-moved Block merge: only the reviewed head (stale merge refused, re-reviewed) mergeability-unknown Block merge: unknown is not yes reviewer-unavailable Block fail-closed: no review, no merge (retriable) force-push-attempt n/a covered by the delivery adapter suites stale-approval Block covered by the ADR-018 tests Two honest notes from writing it: - The first draft of the head-moved row asserted "nothing merges", and it FAILED — because that is not the guard. The guard is "never merge code the review did not see": the stale merge is refused, the item returns for review, and the run then merges a head that was reviewed. The loop was right; the assertion was wrong, and it is now precise (it counts the re-review). - The protected-path row documents the difference between a gate and a wall: a human-owned change is not blocked, it WAITS, which is why it does not consume a fix round. Core 237/234/0, 0 TS errors.
…, and the ADR-018 correction
Three documents the last two slices owed, plus one that had quietly become wrong.
- ADR-019: why a committed credential blocks a delivery, why the rule lives in the engine rather than in
semgrep, why the entropy heuristic may only ever reach a person, what the base is NOT the subject of,
what a line number on a finding is for, and what `rules@2` means for digests already stored.
- ADR-020: what separates a guard's evidence from a test of its implementation, why every row calls a
production predicate instead of re-deriving the answer, why the vocabulary carries an `n/a` and a
`TBD`, and the bench's own first failure (its head-moved assertion was wrong, not the loop). It also
says what the bench does NOT measure: guards, not ability.
- ADR-018 correction: decision 4 ("a delivery on record with no digest cannot merge") was too broad and
fires on an ordinary fix round. The live run is what showed it. The refusal is now approval-centric —
it applies under a human gate, where a digest binds a person's approval rather than a previous tick's
machine review. The original text is kept visible and the correction appended, the same way ADR-007
was corrected twice, because the cost of the wrong rule is part of the record.
Found on the live label-mode verification. Tick 2 was finishing a delivery an earlier tick had opened; the branch history held exactly two commits -- the earlier run's, and the deliberate head move -- and NO commit from tick 2. The event trail said `agent.started`, `round 1 starting`, `agent.finished`. The hook already knew: run-command.ts returns immediately when the run is resuming, because the work is already committed (ADR-014). The LOOP did not know, and it narrates the agent step unconditionally -- so it described a STEP instead of a FACT, in the artefact whose whole purpose is to be the record. - the loop plan gains `resumed?: boolean`; the pilot sets it from the resume decision it already made - a resumed round 0 runs no agent and reports `agent.skipped`, naming where the work already is - a LATER round of the same run still runs the agent: the fix round after findings is work that does not exist yet, and skipping it would leave the finding unanswered - `agent.skipped` joins the closed event registry (an unregistered name still fails loudly) Core 239/236/0 (+2), CLI 40/40, 0 TS errors.
…ongs to The fix is 1ba710b; this is the record it owes -- the symptom as it was SEEN (a live tick whose event trail named an agent start its branch history could not support), the root cause (the hook knew and the loop did not, so the narration described a step instead of a fact), and the class worth remembering whenever a layer narrates a stage it does not own.
…runs
Found by reading the record the live label-mode verification produced:
"reviewed": { ..., "ruleset": "29eefc93...", "reviewer": "reviewer:rules@1" }
`ruleset` is the hash of `rules@2`. `reviewer` was a SECOND hand-maintained constant, and the rules@2
bump never touched it. The digest exists to bind an approval to what did the reviewing, so a field
claiming version 1 beside a rule set at version 2 is the record saying two things about one fact --
and it is exactly the field a person reads to answer "which reviewer saw this?".
- `REVIEWER_DETERMINISTIC_RULES` is DERIVED (`reviewer:${REVIEW_RULESET_ID}`) so the two cannot drift;
the test asserts the equality rather than the string
- `REVIEW_RULESET_ID` is exported from the package: which rule set a build runs is a public fact
- the two fixtures carrying a copy of the old literal now say `reviewer:fixture`, so the next bump
cannot leave a stale copy behind inside the contract suite
Stored digests are untouched (they keep the reviewer identity they were computed with); the next
review of each item records `reviewer:rules@2`.
Core 240/237/0 (+1), 0 TS errors.
Fix is 5407f05. The symptom is worth keeping exactly as it appeared: a live record whose `ruleset` hash came from `rules@2` and whose `reviewer` still said `rules@1`, which is the record disagreeing with itself where a person would look to trust it.
…udged The loop hardcoded `REVIEWER_DETERMINISTIC_RULES` into every digest, so a record claimed the deterministic rules reviewer for a change judged by an INJECTED or sidecar reviewer -- the record naming something that did not happen. The same class as the version-label drift just fixed beside it, one level deeper: this field looked authoritative precisely because it was a constant. - the loop plan takes `reviewer?: string` (default: the rules reviewer), and the pilot forwards `deps.reviewerId` when the caller wired a different judge - the reviewer is part of the digest, so an approval does not silently transfer between reviewers - prerequisite for the semgrep sidecar: without it, wiring one would have written a false identity into every record it produced Core 241/238/0 (+1), 0 TS errors.
Fix is c00fa3b. Found while starting the semgrep sidecar: the first thing a second reviewer needs is for the record to be able to say it was the second reviewer.
…es nothing `ReviewFinding.line` was added when the committed-credential rule landed, and `describeFindings` -- the function whose output travels to the board comment -- never rendered it. The field was recorded, typed, tested at the object level, and dropped at the only point a person reads it: the same class as a version label nobody checks, with the added irony that the line exists so a finding can be LOCATED. - `describeFindings` renders `path:line` when the line is known, `path` when it is not - the fixture that pinned the old rendering now allows the line, and a new assertion pins the rendering itself (`semgrep:fixture (calc.py:42): a credential`) Core 241/238/0, 0 TS errors.
Fix is 6e5144f. Found while mapping semgrep's `start.line` into our findings and asking which surface would show it: no surface did.
…the identity to name it semgrep is a rule LANGUAGE plus a corpus we do not have to maintain, which is the one thing the deterministic rules cannot offer: every new rule there is code we write. It costs ~333MB, its own release cadence, and a rule corpus that changes under whoever consumes it -- so the interesting decision was not "run semgrep" but WHAT KIND OF THING it is here. - a PROCESS behind the existing review seam, never a git submodule and never a dependency: the judge stays replaceable, auditable, and ABSENT, which the build has to be able to represent - absence is a refusal: a missing binary, a non-zero exit, output that is not JSON, a body without `results`, a non-empty `errors` array (semgrep's own per-target failures = a partial scan), or more findings than the cap each throw and nothing merges. There is no path where this reviewer answers "clean" without having run. - no `--config auto`: the rule set is pinned IN THE REPOSITORY IT JUDGES, its content hash is part of the reviewer's identity, and a rule file edited while the reviewer is alive is refused rather than applied. `expectedVersion` pins the engine that interprets it. - the severity translation is ONE table, documented; an unknown severity from a newer tool is a judgement (`human`), never a gate -- a tool must not be able to start blocking through our translation by inventing a level. - the deterministic rules still run (`composeDeterministic`, on by default): semgrep knows nothing about a weakened test, and trading one reviewer's coverage for another's is a downgrade wearing an upgrade's clothes. - `plan.reviewer` (bug #19) carries the identity into the review digest, so an approval binds to the judge that actually ran. Measured on this host, not assumed: the scan with `--baseline-commit` takes ~1.7s on a two-file change; the exit code is 0 WITH OR WITHOUT findings (a missing rule set exits 7), so the JSON is the signal; the shipped rule set finds a committed `AKIA...` key with its line. Tests: 8 offline (every refusal path, the mapping, the argv invariant that `auto` never appears, the rule-set-changed-under-us refusal), one REAL-BINARY test that skips rather than passing silently when semgrep is absent, and a CLI end-to-end (`runOnce (REAL semgrep)`) where an agent commits a credential and the delivery is blocked with the finding naming `semgrep:...aws-access-key`. ADR-021. Full suite 0/0/0 (EXIT=0), 0 TS errors.
… through one process reviewdog owns the host-specific half of "where a finding is shown" (GitHub review comments, GitLab MR discussions, a Check, SARIF), and that vocabulary changes on someone else's schedule -- so its natural boundary is a process boundary, like the semgrep reviewer's. The seam is a format: we translate OUR findings into `rdjsonl`, and reviewdog owns the host. - `toRdjsonl` is the pure half (exported, so an audit reaches the same bytes we post): severity back to the host vocabulary (block->ERROR, human->WARNING, note->INFO), the line carried through, the rule id as the diagnostic code, `path:line` so the comment lands on the right row. - `-diff` is a COMMAND STRING that reviewdog executes, so the shas are validated as hex before they are interpolated: a "commit" that is `; rm -rf /` is not a commit, and a reporter is not a shell. (Tested: the refusal happens BEFORE anything runs.) - NOT_RUN is visible, never silent: a missing binary or a non-zero exit throws. An absent report is not a clean review, and the caller decides what to do with the fact -- which is why it gets the fact. - A finding with no path cannot be an inline comment, so it comes back as `unlocated` instead of being dropped: reviewdog would refuse it, and dropping it ourselves is the same silence, shorter. - host credentials come from the ENVIRONMENT only; a test asserts no `token`-shaped string ever reaches argv. Verified with the REAL binary (0.21.1): a finding on a line the change did not touch is filtered out, and a finding on an added line is reported -- i.e. the diff filter is exercised, not assumed. Also corrects the previous commit's message, which said "Full suite 0/0/0": a bug in the snippet that generated it. The real numbers for that run were 519 tests / 516 pass / 0 fail (EXIT=0) -- 8 packages plus this new one.
…not become a gate The reviewer could find things (ADR-019) and every finding lived in the event trail plus a one-line board comment. Both are ours; the person reading the merge request saw nothing. That is the mirror-board problem one level down (ADR-017), answered the same way: one authority, N write-only projections for the people looking. - a HOOK, not a port: a report surface owns no state, so it must not sit in the delivery's critical path where a comment nobody needed could hold up a merge - a report is a BYPASS: a reporter that throws is recorded as `report.failed` and the delivery outcome is EXACTLY what it would have been. Asserted in core by running the same scenario with a working and a broken reporter and requiring the same outcome. - the findings travel WITH the verdict (`ReviewOutcome.findings`), so a reporter never re-derives what was found: a second computation of "what was found" is how a comment and a decision come to disagree - `reporters/reviewdog` translates to `rdjsonl` (one pure, exported function), validates the shas as hex before they go into reviewdog's `-diff` COMMAND string, keeps the line, and returns unpostable findings instead of dropping them — the CLI prints those, because a finding no comment can carry still has to reach somebody - `reporter: reviewdog` in takumi.yaml, with `-reporter=local` verified against the real binary 0.21.1: a finding on an untouched line is filtered OUT and one on an added line is REPORTED The slice's own bug, caught before it landed and left here on purpose: the first wiring put `report` into the loop PLAN, where the loop never looks. Every unit test passed; the CLI test failed, because a configuration that reaches nothing is silent -- the ledger's own class, in miniature, and exactly why the wiring's test asserts the EFFECT (a `report.failed` in the trail) rather than the plumbing. ADR-022. Core 242/239/0, CLI 44/44, 0 TS errors. Not done and named: the live host reporter needs reviewdog's `-conf` (project + MR ids) plus a token, and ADR-017's mirror-board wiring is still open.
The repo had no `.env` in its .gitignore, and the next step needs one on this host (a Notion token). A token pasted into a chat is already leaked; the only remaining question is whether it also reaches a commit, and the answer should not depend on someone remembering not to `git add -A`. Nothing was tracked, so nothing leaked: this is the guard added while it is free.
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.
What a review is evidence ABOUT becomes explicit.
Full suite at this head: 544 tests, 0 failing.