# Row 93, a review loop through the bus, round 1 build (Darkwing)

Issue #1587. Brief blob `dff83c09`. Design and Sage's answers (decision
135): `r1-design.md` in this directory.

Base `d240645d`. `build.patch` is `git diff --cached --binary d240645d` of
the 29 paths below. Apply it with `git apply --index` to a detached
worktree of `d240645d`, and `sha256sum -c candidate-manifest.sha256` then
gives 29/29 OK.

## What it does

A coder pushes a branch of its clone to a bare working repository on the
host. It asks a reviewer to look at the commit, the reviewer fetches and
tests it in its own clone and answers with a verdict, and the PM closes the
task citing that verdict. The host never runs git in a session's clone and
never opens a path in a session's workspace. Commits move as git bundles
over a second host socket, `bus-host/git.sock`.

1. `push_working` (verb `working.push`). The runner, inside the sandbox,
   bundles the branch in its own clone against the work ref's tip (or
   `main`) and sends it over the git socket. The host checks the cap and
   the action with the broker, caps the size, writes the bundle under
   `bus-host/`, and lands it in
   `<dataRoot>/working/<business>/<project>.git` as
   `refs/heads/work/vikunja-<project>-<task>`. The broker records the push
   as the `action.allowed` for `git.push.working` on the task, naming the
   commit (see "Event kinds" below).
2. `request_review` (verb `review.request`). The broker checks that the
   commit is the task's latest push and that the named role holds
   `review.verdict`, records a `review.requested` event and sends that role
   a REVIEW-REQUEST message.
3. `fetch_review` (verb `working.fetch`). The host looks the commit up from
   the cited request, bundles it as `refs/mosaic/review`, leaving out the
   commits the reviewer says it has, and sends it back. The runner fetches
   it into `refs/remotes/working/work/vikunja-…` in the reviewer's clone.
4. `review_verdict` (verb `review.verdict`). One verdict per request,
   `approve` or `changes`, sent to the role that asked.
5. `task_close` on a task with a recorded push must cite an
   `approve` verdict on that task for its latest pushed commit. A task with
   no push closes as before.

## Files

| File | Change |
|---|---|
| `packages/seat/src/working.mjs` (new) | the bare working repository: `workRef`, `validBranch`, `push`, `review`, the per-repository lock, the SAFE git environment |
| `packages/cli/src/git-socket.mjs` (new) | the host's git socket: framing, limits, broker checks, bundle files under `bus-host/` |
| `packages/harness/src/git-tools.mjs` (new) | the runner's side: bundle the branch, send it, fetch a review bundle into the clone |
| `packages/bus/src/broker.mjs` | `checkPush`, `recordPush`, `reviewFetch`, `review.request`, `review.verdict`, the `task.close` check, `refuse()` subject |
| `packages/bus/src/process.mjs`, `runtime.mjs` | the new host ops (`checkPush`, `recordPush`, `reviewFetch`) |
| `packages/cli/src/launcher.mjs` | starts the git socket; binds it only for a role with `git.push.working` or `review.verdict`; `session.json` names it |
| `packages/seat/src/sandbox.mjs`, `session.mjs` | the read-only bind of `bus-host/git.sock`; `git-tools.mjs` joins `RUNNER_FILES` |
| `packages/harness/src/tools.mjs`, `runner.mjs`, `bundle.mjs` | the four tools, local routing of the two git verbs, prompt lines |
| `packages/tasks/src/verbs.mjs` | `task.close` passes its `verdict` to the broker's check |
| `roles/coder.md`, `reviewer.md`, `pm.md` | the loop from each role's side |
| `packages/{bus,cli,harness,seat}/README.md` | the git socket, the working repositories, the tools, the limits |
| `packages/bus/tests/review.test.mjs` (new), `packages/cli/tests/git-socket.test.mjs` (new), `packages/seat/tests/working.test.mjs` (new) | see Tests |
| `packages/harness/tests/{bundle,tools}.test.mjs`, `packages/seat/tests/sandbox.test.mjs`, `packages/cli/tests/launcher.test.mjs`, `packages/tasks/tests/verbs.test.mjs` | one test or a few assertions each |
| `packages/bus/tests/broker.test.mjs` | one assertion: its `task-subject-required` check used a `review.verdict` event through `recordEvent`, which is now reserved, so it uses `action.refused` |

## Event kinds: no schema change

The first gate run failed because `packages/bus/schema.sql` limits
`events.kind` to a fixed list, and the build wrote two kinds not on it,
`git.push.working` and `review.request`. Adding them would be the bus's
first schema change. `store.mjs` pins the DDL's hash and compares a live
database's schema with it, so the running bus would refuse its own
database (`schema-mismatch`) until a migration rebuilt `events`, an
append-only table, and replaced the `schema_digest` row in `meta`, which
is append-only too. That is a breaking data change the row doesn't need,
so the build uses kinds the schema already has:

- The request is a `review.requested` event. The schema has listed that
  kind since row 37, and the Console already shows it as "Review
  requested". The verb and the action stay `review.request`.
- The push is the `action.allowed` for `git.push.working`, written by
  `recordPush` once the ref moved, with `{project, branch, ref, commit,
  previous}` in its body next to the usual `{action, class, target}`.
  The git socket's first broker call, `checkPush`, now checks the
  authority without recording anything (a refusal is still filed). So
  each landed push has exactly one `action.allowed`, and a refused one
  has none. Decision 135's "a task with any `git.push.working` event"
  reads as "a task with an `action.allowed` for `git.push.working` that
  names a commit".
- `recordEvent` refuses `review.requested`, `review.verdict`, and an
  `action.allowed` whose action is `git.push.working`, so a trusted
  adapter can't forge a push or a verdict. Mutant `push-forgeable` drops
  the last check.

If Sage wants a dedicated `git.push.working` kind instead, that is a
schema migration and a separate ruling.

## The `refuse()` subject, a bus behaviour change

Sage asked for this as its own item (ruling 3).

Before: `broker.refuse(cap, code)` filed every host-decided refusal with
no context, so its `action.refused` event had a null subject. After:
`refuse(cap, code, subject = null)` files it under
`{ args: { task_ref: subject } }`. The refusal's subject is then the task
ref when it is one, and null otherwise, by the same rule every other
refusal already uses. The git socket passes the request's `task_ref`
(`process.mjs` forwards `m.subject`), so a `non-fast-forward` refusal
shows in that task's `trail`. The launcher's three existing `refuse` calls
pass no subject and record what they did before. `runtime.mjs`'s
`authorizeLaunch` became a `refusing()` wrapper that the git ops share;
launch refusals still pass no subject.

Test: review.test "a refusal the host decided is filed on the task it
names, and only a task ref counts". `TASK` files on `TASK`; `'gitea:1'`
(not a vikunja ref) and no subject both file null. Mutant
`refuse-subject-null` sets the subject back to null.

`broker.mjs` is a bus landing, so decision 109 notices go to Mos before
and after it lands. Sage sends them; the bus restart stays with Mos.

## Decision 135, item by item

1. **The close needs an approve verdict for the latest push.**
   `authorize()` runs `#closeVerdict` in the same transaction as the
   authority it consumes, so a refusal rolls the `action.allowed` back.
   Codes: `verdict-not-found` (no citation, or one that isn't a verdict on
   this task), `verdict-not-approve`, `verdict-stale`. Tests: review.test
   "task.close on a pushed task needs …", and tasks verbs.test "a task
   with pushed work closes only citing …", which also checks that no
   refused close reached the tracker.
2. **Descends from main.** The bundle's branch is fetched into a temp ref
   `refs/mosaic/incoming/<hex>`. `merge-base <commit> main` must succeed
   before the work ref moves, else `unrelated-history`. The temp ref is
   deleted on every path. The check runs on every push, not only the
   first. A later push that fast-forwards a descendant passes it anyway,
   and running it every time costs one git call. Test: working.test
   "a branch with no history in common with main refuses
   unrelated-history".
3. **Re-seed main under the lock.** Each push and each review fetches
   `main` from the project root into the bare repository first, under the
   per-repository lock. A bundle whose prerequisites the repository lacks
   refuses `missing-prerequisite`, read from `git bundle verify`'s message
   (not `-q`, which drops it in 2.55). Tests: working.test "main is
   fetched from the root again before each push …", "a bundle that needs
   commits … missing-prerequisite", "a root moved back after the clone …".
4. **Host git environment.** Every host git call runs with
   `GIT_CONFIG_NOSYSTEM=1`, `GIT_CONFIG_GLOBAL=/dev/null`,
   `core.hooksPath=/dev/null` and the SAFE flags, and every bundle fetch
   with `transfer.fsckObjects=true`. Test: working.test "a commit with a
   broken object refuses invalid-bundle, and only because fsck is on"
   lands the same bundle with `fsck: []`, so the refusal is fsck's. Mutant
   `fsck-off` empties the real constant.
5. **The bundle file is deleted on every path.** The git socket removes it
   in a `finally`, and the runner removes its own copy in the run's
   `/tmp`. After every case, refusals included, the git-socket tests check
   that no `.bundle` file is left in `bus-host/` or in either session's
   `/tmp`. Mutant `bundle-kept` drops the host's delete.

## Tests

**New files.**

- `packages/seat/tests/working.test.mjs`, 15 tests: workRef and
  validBranch; a push and a fast-forward; non-fast-forward; unrelated
  history; missing prerequisite; re-seed; root moved back; broken object
  and fsck; only a v2 bundle with the named branch; a branch named `main`
  or for another task moves only this task's ref; a record that throws
  moves the ref back; a repository path that isn't a directory; the review
  bundle and `have`; review with no repository; the runner's limits match
  the host's.
- `packages/cli/tests/git-socket.test.mjs`, 6 tests: the whole loop
  between a fake coder and a fake reviewer through a real bus, ending in
  the PM's close and the task's `trail`; refusals (no action, non-fast-
  forward, unknown commit or request, another task's branch); the push
  size cap; the fetch size cap; no capability, an unknown one, a bad
  request line; a rigged clone (hooks, fsmonitor, a filter, a gitlink, a
  symlinked `.git/config`) leaves no marker, and a branch whose
  `.gitmodules` carries an `update = !cmd` line refuses `invalid-bundle`
  at the host's fsck with the work ref unmoved. Every case checks that the
  project root's refs and the working repository's other refs did not
  move.
- `packages/bus/tests/review.test.mjs`, 7 tests: `checkPush` records
  nothing, push recording, and the reserved kinds and forged push; review.request; its refusals (`stale-commit`,
  `unknown-commit`, `unknown-role`, `self-review`, `not-a-reviewer`, and
  `decision-required` for a role that holds it only cross-role);
  reviewFetch; one verdict per request; the
  close check; the refuse subject.

**Additions.** harness bundle.test (the prompt lines per role), tools.test
(which role gets which tools), seat sandbox.test (the git socket bind is
read-only and only when given), cli launcher.test (the PM's `gitSocket` is
null, the socket is 0600, the coder is bound and the PM is not), tasks
verbs.test (the verdict reaches the broker).

## Gate

Run 2, 15:27:52Z to 15:31:00Z, on the 29-path candidate over `d240645d`,
in a clean worktree with `node_modules` linked, under the busy check.
Runner: `../gate.sh`.

| Suite | Result |
|---|---|
| seat (`node --test tests/`) | 105 pass, 0 fail |
| cli | 137 pass, 0 fail |
| harness | 69 pass, 0 fail |
| bus | 122 pass, 0 fail |
| tasks | 90 pass, 0 fail |
| webui (`node --test tests/`) | 104 pass, 0 fail |
| test-auth | 15 passed, 0 failed |
| test-conductor | 17 passed, 0 failed |
| test-config | 24 passed, 0 failed |
| test-discord | 66 passed, 0 failed |
| test-extension-package | 18 passed, 0 failed |
| test-foundation | 44 passed, 0 failed |
| test-queue | 27 passed, 0 failed |
| test-release | 14 passed, 0 failed |
| test-task (`DOCKER_HOST=unix:///nonexistent`) | 26 passed, 0 failed |

Run 1 (15:19Z) failed 14 node tests. Most came from the event kinds
above. The rest were three test faults: a sandbox assertion compared a
relative path with `RUNNER_FILES`' absolute ones, the junk-bundle case
sent bytes git read as a bundle with no such branch, and the rigged
clone's `update = !cmd` line was refused by the host's fsck before any
clone could run it. That refusal is right, so it became its own case.

## Mutants

`../mutants.sh` with `../mut.mjs`, 15:31Z, each mutant against the named
test files. All 15 killed; every file restored.

| Mutant | Site | Killed by |
|---|---|---|
| fsck-off | `FSCK = []` in working.mjs | git-socket rigged clone, working broken object |
| plus-refspec | `+` before the refspec | working non-fast-forward |
| ref-from-request | ref name from the request's branch | 6 tests |
| cap-off-push | push size check off | git-socket loop and size cap |
| cap-off-fetch | fetch size check off | git-socket fetch over the cap |
| refuse-subject-null | refuse() subject back to null | review.test refuse subject |
| no-close-check | task.close skips `#closeVerdict` | review.test close check |
| no-verdict-stale | stale verdict accepted | review.test close check |
| verbs-drop-verdict | task.close drops `verdict` | tasks verbs.test |
| push-forgeable | recordEvent takes a git.push.working action.allowed | review.test push record |
| no-request-commit-check | review.request takes any commit | review.test refusals |
| no-merge-base | no shared-history check | working unrelated history |
| no-reseed-on-push | main not fetched before a push | 9 tests |
| bundle-kept | host keeps the bundle file | 5 tests |
| git-socket-always | git socket bound for every role | launcher.test |

The runner-side `transfer.fsckObjects` in `git-tools.mjs` has no mutant.
The host repository fscks every bundle it takes, so a runner without it
still can't land a broken object on the host. What it guards is the
reviewer's own clone.

## Deviations from the brief

- The owner is Darkwing, not Filbert (Sage's assignment).
- The host-side git lives in `packages/seat/src/working.mjs` and
  `packages/cli/src/git-socket.mjs`, not in `checkout.mjs` edits
  (decision 135, answer 3).
- `fetch_review` runs git in the reviewer's sandbox, not on the host
  (answer 1).
- Files outside the brief's list: `packages/tasks/src/verbs.mjs` (the
  close passes its verdict), `packages/tasks/tests/verbs.test.mjs`, and
  the cli README.
- The seat README said the runner files were "ten" at base, while
  `RUNNER_FILES` held 11. With `git-tools.mjs` it says twelve.
- The `refuse()` task-ref change in `broker.mjs` (and its `runtime.mjs`
  and `process.mjs` plumbing) is beyond the design. It has its own
  section above, a test, and mutant `refuse-subject-null`.
- `packages/bus/tests/broker.test.mjs` L199: its `task-subject-required`
  case wrote a `review.verdict` through `recordEvent`, which this build
  reserves, so it now writes `action.refused`. The case checks the same
  rule.
- The event kinds differ from the design's: `review.requested` and an
  `action.allowed` for `git.push.working`, not two new kinds. See "Event
  kinds: no schema change".

## Limits and follow-ups

- **Timeouts disagree.** `callTool` gives up at 60 s and the tool socket
  at 150 s, while git-tools allows 300 s for git and its socket. A push
  slower than 60 s looks failed to the model while the runner still
  finishes it. A repeat then refuses `nothing-to-push`, and `trail` shows
  the push's `action.allowed`. The 32 MiB cap keeps this rare. Documented
  in the harness README's Limits.
- **No assignment check.** Any role holding `git.push.working` can push
  to any task's work ref, and the broker doesn't check that the task is
  assigned to it. That matches how the other task verbs treat authority
  today. A follow-up row could add it.
- **`--no-auto-maintenance`** on every host fetch, so no `gc` runs inside
  the lock.
- **Row 80 overlap.** Row 80 also edits `packages/bus/src/broker.mjs`;
  its diff adds a `STRANDED` const near L47, a stranded query in
  `#agentState` (about L1047 to L1052), and `stranded` in that return.
  None of it touches lines this build changes. The nearest hunk here
  starts at base L1055, about a dozen lines below `#agentState`'s return,
  so a rebase should apply without a conflict. It is still the same file. Sage's ruling:
  row 93 lands first, and row 80 rebases onto it and gates again. If row
  80 lands first, I rebase and gate again.
- **Base drift.** At 15:28Z, `d240645d..11bf4cb0` touched none of the 29
  paths. It did change `packages/tasks/src/fake.mjs` (row 71) and some
  webui tests, which the tasks and webui suites run, so the landing gate
  on the new base covers them. Rechecked at 15:33Z on `11bf4cb00`:
  still none of the 29 paths.
- Out of scope, as the brief says: landing a work ref, `git.push.protected`,
  a forge, credentials.