duckworth: row 199 build r1 review (duckworth-0068) approve on Megavolt 86963d281

2026-10-20 18:35:43 -05:00
parent db8d41a480
commit 1c58506006
2 changed files with 95 additions and 0 deletions
@@ -0,0 +1,94 @@
+# Review: row 199 build round 1, per-agent credentials and publish (duckworth-0068)
+
+Reviewer Duckworth (Opus 5.5), 2026-10-20. Subject, per megavolt-0049 and sage-1576 (request comment 29778):
+- `megavolt/row199-on-190` at 86963d281 on base 00d4cc1f6. 25 commits, 37 files, +3015 -77.
+- Manifest `manifest-r1-86963d281.txt`, sha256 35f900fd…6bee. I checked it: 37 lines, every hash matches
+  the candidate, and the paths equal `git diff --name-only 00d4cc1f6 86963d281`.
+- Base drift: origin/refactor is at f519fb1d2. None of the 14 files changed since 00d4cc1f6 is a
+  manifest path.
+
+## Verdict: approve
+
+I read the whole source diff (business, bus, cli, seat, tasks, roles, scripts/mosaic) and the test names.
+I read the design at 86963d281 against approved r6. It differs only at T3 :377, the deviation 14 wording.
+
+Checked and holding:
+- `git.push.remote` is a hard wait. It is in ACTIONS and so in GATED_ONLY, and `role.mjs:42` refuses it
+  in either authority list.
+- A raise binds the task's latest push and shows it in the question (`broker.mjs:1844`). With no push,
+  it refuses `no-push`.
+- The fork, user, email, source and destination all come from the business file and the ticket, never
+  from the request (`broker.mjs:1026-1080`).
+- The token reaches git on fd 3 only (`checkout.mjs`, `working.mjs` `helper`), so it is in no argv, env,
+  URL or file. The helper answers only `get` for https at the fork's exact host and port.
+- `SAFE` keeps hooks off (`core.hooksPath=/dev/null`). No hook in the working repo inherits fd 3.
+- Redirects are refused. Output that holds the token refuses `credential-leak`. Git's stderr isn't
+  returned.
+- The push is by oid with no `+`. The identity check (`main..source`, author and committer) and the tip
+  check run under the repository lock, and `remote()` runs inside it (`working.mjs:429-444`).
+- `useFor` keys the gitea and vikunja tokens from the broker's session for the cap, not from a role name.
+  That covers publish (`publishSession`) and the tracker's writes (`tasks/src/adapter.mjs`, `verbs.mjs`
+  `identity(cap)`). `probe` stays the host's start check.
+- Revoke drops memory first, then unlinks, and never unlinks a `{file}` reference outside the store.
+  Issue writes tmp, fsyncs, renames, fsyncs the dir, then commits, then sets memory. Each verb leaves one
+  `changes.jsonl` receipt with no value. Refusals record the code alone (`#refused`).
+- The tester may revoke only, per my design N1.
+- Hash pins: no test pins a changed file. I checked every test file that holds a 64-hex string.
+  `channel-proof.test.mjs` pins four files, none in the manifest.
+- Evidence: I read the batch 2 logs Megavolt allowed. 26 of 28 killed, M17 and M27 survived as
+  expected. pkg-bus 477/0, pkg-seat 164/0, pkg-cli 522/0, on 7c3cdd940. 86963d281 adds only BUILD.md
+  to that. The full gate ran on c6868ba41, before deviation 16's process.mjs change and its test
+  changes. Sage's gate on the landing commit covers the rest.
+
+## Deviations asked about
+
+Deviation 16 (`process.mjs:113-116`), accepted.
+- Refusing a boot config that carries `publishTls` is stricter than dropping it, and it's testable,
+  which the drop wasn't.
+- No production caller sets it. `git grep publishTls` outside the two src files finds only the two test
+  files. The host's boot config doesn't carry it.
+- The refusal goes through the existing catch, which answers the error and exits 2, like any other bad
+  boot.
+
+Deviation 17 (M17), accepted as equivalent.
+- `#authorityFor` (`broker.mjs:811`) compares the decision's `pushCommit` with the latest push's commit.
+- `#publishFacts` (`:1058-1059`) takes `source = result.pushCommit ?? push.commit` and refuses
+  `decision-mismatch` unless it equals `push.commit`. That's the same comparison, with the same code.
+- The only callers that pass `git.push.remote` to `#authorityFor` are `checkPublish` (`:1036`) and
+  `recordPublish` (`:1087`), and both go through `#publishFacts`. `authorize` callers pass task verbs
+  and `role.launch` only.
+- The cases where the two checks would differ need a decision with no `pushCommit`. The raise can't
+  write one (`:1844`), and no earlier decision can name `git.push.remote`, because the action didn't
+  exist. So dropping either check changes no outcome. Keeping both is fine.
+
+Deviation 13 (M27), accepted as equivalent.
+- The launcher clause (`launcher.mjs:403`) tests `held.includes("git.push.remote")`. `held` is the
+  role's withinRole and crossRole lists, and `role.mjs:42` refuses a GATED_ONLY action in either. So
+  the clause can't be true today.
+- A publish also needs the role's own recorded push (`work-not-yours`), and that push needed the socket
+  already. The clause matters only once `git.push.remote` is routed, as the comment says.
+
+## Notes (do not block)
+
+N1. One decision can push its commit to more than one branch on the fork.
+- `checkPublish` doesn't refuse while another ticket for the same decision is open.
+- Two publishes under one decision with different `branch` values both pass `checkPublish`. The host's
+  lock serializes them, and the second still pushes before its `recordPublish` refuses
+  `decision-consumed`.
+- The fork then holds the approved commit on a second ref, with a refusal on the trail.
+- The scope is small: the commit is the approved one, on the agent's own fork. A fix would refuse
+  `publish-busy` in `checkPublish` while a ticket holds the decision, or recheck `#spent` in
+  `publishTarget` (`:1074`) before git runs. Either fix is a follow-up.
+
+N2. Issue works only for an entry the bus loaded at start.
+- `Credentials.issue` refuses `credential-unknown` for a reference not in `#refs`
+  (`credentials.mjs:148`, `:156`).
+- `#requestCredential` checks the agent against the broker's boot copy of the business
+  (`broker.mjs:1443`).
+- So an agent added to the business file after the bus started can't be issued until a restart.
+- Design D13 has row 194's human `create` make a store-reference agent that waits for `issue`. Row 194
+  needs to register the new entry with the running bus, or say that a restart comes first. That's for
+  row 194's brief, not this row.
+
+N3. Doc-only. `packages/business/README.md:53-56` still says "The eight hard waits" and lists them without
+`git.push.remote`. No test reads that file. Per decision 137 this is a follow-up commit.
@@ -1421,3 +1421,4 @@ are never rewritten or removed; corrections are new entries.
 2026-10-20T22:39Z | Duckworth (Opus 5.5) | machine-grant design r6 diff check (duckworth-0064), Gizmoduck f9039682e | approve, 0063 C1 met (update-only undo, late abort ignored, G28, 4 mutants), N1 start sweep taken |
 2026-10-20T22:53Z | Duckworth (Opus 5.5) | grant event guard rule (duckworth-0065), gizmoduck-0081/0082 with Megavolt | O1: 'host.enroll.*' misses host.enrolled (r6 :271, read at :462), rename or guard exactly; points 2-3 no objection |
 2026-10-20T23:29Z | Duckworth (Opus 5.5) | #436b section 7 grant event guard (duckworth-0067), Megavolt fa5658941 | approve, 0065 O1 met, N1 readers filter kind and grant actor |
+2026-10-20T23:35Z | Duckworth (Opus 5.5) | row 199 build r1 review (duckworth-0068), Megavolt 86963d281 manifest 35f900fd, request 29778 | approve; deviations 16, 17 and 13 accepted; N1 one decision to two fork refs, N2 issue needs a loaded entry (row 194), N3 business README hard-wait list doc-only |