stack/agents/darkwing/work/queue-93/r1-design.md

Row 93 (#1587), review loop: design proposal before building (Darkwing)

Read-only prep. Nothing built or run. Brief blob dff83c09, "A review loop through the bus".

The problem with the brief's wording

The brief says that for push_working "the host ... pushes one branch of the caller's clone", and that for fetch_review "the host fetches the requested commit into the caller's clone". Both mean the host running git inside a clone whose session is live. Row 91's update() is safe because it runs only when no session of the instance runs, so nothing changes the clone between the unsafe() checks and the git call. Here the session is running and can write its clone at any moment. That includes the window between a check and the git call, where it can add a config key, an attribute filter, a gitlink, a hook or a symlinked .git piece. Row 91's round 2 escape went through a single git child the checks didn't cover. A live clone gives the session a race against every check.

What I propose

The host never runs git in a session's clone, and never opens a path inside a session's workspace. Commits move as git bundles over a host socket.

push_working.

1. The runner (inside the sandbox, as the session) runs git bundle create - <branch> --not <the working ref's tip, or main> in its own clone and streams the bytes to the host.
2. The host caps the size and writes the bytes to a file under bus-host/ (0700).
3. In the bare working repository it runs git fetch <bundle> refs/heads/<branch>:refs/heads/work/<task ref> with transfer.fsckObjects=true, the SAFE -c flags and no +. Git then refuses anything but a fast-forward of that one ref.
4. The host builds the ref name from a task ref it has validated. The branch name only selects a ref inside the bundle.
5. The host records the new tip in a git.push.working event on the task.

Git on the host reads only a pack and the bundle header, as in a fetch from an untrusted remote, which git already hardens with fsck. No session config, attribute, hook or filter is in play.

fetch_review.

1. The host looks up the commit from the cited review.request event.
2. It runs git bundle create - work/<task ref> --not main in the working repository and streams the bundle back.
3. The runner writes it to /tmp and runs git fetch <file> work/<ref>:refs/remotes/working/<ref> in the reviewer's clone, inside the sandbox, under that session's own config.

The reviewer ends up with the same remote-tracking ref the brief asks for. The difference is that the session's own git writes it, not the host's.

The working repository. <dataRoot>/working/<business>/<project>.git, bare, 0700. The host creates it and seeds it with main fetched from the project root, which the host trusts. No sandbox binds it. One lock per repository serializes pushes and fetches.

Transport. This follows the launch socket's pattern (launcher.mjs fromSession). The runner sends its cap, and the host resolves the business, role and run with host.op identity. It finds the workspace from its own launch records, never from the request. Authorization goes through the broker, as authorizeLaunch does today. The bundles need a length-prefixed body: the launch socket reads one JSON line under LINE_MAX, and the bus client caps a request at 64 KiB. So either a second socket, bus-host/git.sock, or an op field on the launch socket with a framed body. I lean toward a second socket bound only for roles that hold git.push.working or review.verdict. The launch socket today goes only to the role that may launch.

request_review. The broker checks the commit against the task's recorded git.push.working events. It needs no git and no host call. A commit no push on that task named refuses as unknown-commit.

review_verdict and task_close. The verdict cites a review.request event on the same task. My proposal for the citation question the brief leaves open: when task_close cites a verdict, the broker checks that it is a review.verdict on the same task with approve, and that its request named the task's latest pushed commit. A close with no code needs no verdict, so the broker doesn't require one on every close. A stale approval, given before a later push, refuses.

What changes against the brief

• checkout.mjs: no host-side push or fetch is needed there. The git calls live in a new host module next to the launcher, for example packages/seat/src/working.mjs or the launcher's file. I'd ask to own one new file in place of edits to checkout.mjs.
• fetch_review runs git inside the reviewer's sandbox, not on the host.
• sandbox.mjs binds the new socket. The brief doesn't list it.

The outcome the brief sets is unchanged: the coder pushes, the reviewer fetches and tests in its own workspace, the PM closes citing a verdict, and the root stays unchanged.

Tests I'd add beyond the brief's list

• A clone rigged during the push (a filter, a hook, a gitlink, a symlinked .git/config) leaves no marker on the host. That holds because the host never looks in the clone.
• A bundle with a broken object or a missing prerequisite, and an oversized bundle, each refuse with no ref moved.
• Every case leaves both the working repository's other refs and the project root unchanged.
• A mutant per guard: no fsck, + in the refspec, the ref name taken from the request, and the size cap off.

Open points for Sage

1. Is the deviation from "the host fetches into the caller's clone" acceptable? If not, I don't know how to make the host's git safe against a live session, and I'd rather say that now than after a round.
2. A second socket, or the launch socket with an op field?
3. The new host file in place of checkout.mjs edits, which changes the row's file list.

Sage's answers (decision 135, f3b175e1, 2026-10-13)

1. The deviation is accepted. The host never runs git in a live clone and never opens a path in a session workspace.
2. Use a second socket, bus-host/git.sock, bound only for roles that hold git.push.working or review.verdict. The launch socket stays bound to the launching role alone.
3. A new host file, not checkout.mjs edits. Name it in the packet's file list with the sandbox.mjs bind. Dewey reviews the bind under decision 130.

Additions to build and test:

• task_close: if the task has any git.push.working event, the close must cite an approve verdict on that task whose request named the latest pushed commit. Only a task with no push closes without a verdict. Tests: a pushed task closed with no citation refuses, and one closed citing a stale approval refuses.
• The first push must descend from the working repo's main. Check it in the bare repo after the fetch: either roll the ref back, or fetch into a temp ref and move it only after the check. Test an unrelated-history bundle.
• Re-seed the working repo's main from the project root, under the lock, before each push. A missing prerequisite refuses with a named reason. Test it.
• Host git in the bare repo runs with no system or global config (GIT_CONFIG_NOSYSTEM=1, GIT_CONFIG_GLOBAL=/dev/null) and core.hooksPath=/dev/null, plus the SAFE flags. A test must fail if fsck isn't really applied to bundle fetches; the broken-object case does that if it runs against the actual flag. Git is 2.55.0.
• Delete the bundle file on every path, refusals included. Test that bus-host/ holds no leftover bundle after each case.

The base is HEAD once row 91 lands, not 2b2a9e14. Don't build until Sage switches the row. The design file rides in the round 1 packet; don't commit it separately.