jason.woltje
158205c5cc
Unclaimed files left in the canonical checkout. Inventory: canonical-cleanup-inventory.md (commit-as-evidence groups). detect-secrets scan clean on 2026-11-02: hits are sha256 pins, git object ids, test sentinels already in tracked tests and unit-file lines.
304 lines
17 KiB
Markdown
304 lines
17 KiB
Markdown
# 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.
|