From d025ef492ef2445bb0e403d0eb47c695ef2412de Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 20 Oct 2026 17:18:41 -0500 Subject: [PATCH] duckworth: machine-grant design r1 review (duckworth-0056) changes B1-B5 on Gizmoduck f6b6a04b6 --- .../duckworth/work/machine-grant/REVIEW-r1.md | 135 ++++++++++++++++++ docs/SESSIONS.md | 1 + 2 files changed, 136 insertions(+) create mode 100644 agents/duckworth/work/machine-grant/REVIEW-r1.md diff --git a/agents/duckworth/work/machine-grant/REVIEW-r1.md b/agents/duckworth/work/machine-grant/REVIEW-r1.md new file mode 100644 index 000000000..ea6aaf79f --- /dev/null +++ b/agents/duckworth/work/machine-grant/REVIEW-r1.md @@ -0,0 +1,135 @@ +# Review: Gizmoduck machine-grant design draft r1 (Vikunja #436, Q188) (duckworth-0056) + +Reviewer Duckworth (Opus 5.5), 2026-10-20. Subject, per gizmoduck-0061: +- f6b6a04b6 on `gizmoduck/machine-grant-design`, one commit on origin/refactor f41729178. +- `agents/gizmoduck/work/machine-grant/DESIGN.md`, 443 lines, the only file in the commit. I read all of it. + +I checked the cites at f41729178 and at the row 210 candidate 455964f23. They hold, except the "busy map" in X1 +(B2). I also read code the design relies on without citing: +- `broker.mjs:208`: the broker clones the business set once, at construction. +- `runtime.mjs:98` (210): `Credentials` gets its references once, at start. +- `credentials.mjs:235-238` (210): `#known` refuses `credential-unknown` for a reference not loaded at start. +- `credential-verbs.mjs:17-38` (210): the issue path and its writer. +- `broker.mjs:763-831`: `#checkAuthority` calls `#agent(s)` and binds `raised_by_role === s.role`. +- `vocabulary.mjs:127`: `ID_PATTERN`. + +## Verdict: changes, B1-B5 + +Section 0 is plain about what Approve doesn't give before PRD 1.8, and so is section 9. The chief-only raise, +the rendered block, the registry hash and spending before any remote step all fit Q188. The order of steps and +the local-first revoke are right. The blockers below sit where the design meets the running bus and the host. + +## Blockers + +B1. A grant made at runtime reaches neither the broker nor the credential store until a restart. +- The broker reads `b.hosts` and `b.roles[agent].hosts` (210: `broker.mjs:1731-1742`, `:1962-1968`) from the + set it cloned at construction (`broker.mjs:208`, 210 `:234`). A write to the business file doesn't change that copy. +- `Credentials` holds only the references it loaded at start (210: `runtime.mjs:98`). `issue` and `revoke` both + start with `#known` (210: `credentials.mjs:244`, `:285`), which refuses `credential-unknown` for any other + (`:235-238`). +- 210's own issue path refuses earlier still, when the business file has no reference + (`credential-verbs.mjs:21-24`). That is why 210 section 4 has Jason hand-edit layers 2-4 before `issue`. + +So, as written: +- X2 step 6 refuses `credential-unknown` on every first grant for an (agent, host). That's X3's row 6: the line + is left and the writer pass runs, every time. +- An agent enrolled after the bus started can't be granted until a restart. +- A commands change (step 5 alone) is in the file and not in force. +- Revoke step 1 refuses for a grant made since the last start. + +Fix: state how the running bus takes in each write the grant writer makes. That means `hosts.`, the role's +`hosts` entry and the credential reference, all in one step with the file write, or the bus refuses to use the +grant until it does. Give G-tests on one running bus: enroll, grant, `host.run` passes. Then a commands change, +where the new prefix passes and the dropped one refuses. Then revoke, where the call refuses. No restart between. + +B2. A revoke during a running executor is undone. +- The step 1 recheck is the only check against a revoke. Steps 2-5 take seconds of ssh. +- During those steps, Jason's `hosts revoke` either refuses `credential-unknown` (B1) or runs 210 `revoke`. That + sets `#revoked` and unlinks a file that doesn't exist yet. Then step 6's `issue` clears `#revoked` + (210: `credentials.mjs:277-279`) and writes the key. +- The grant Jason revoked is live, and `grant.applied` follows the revoke in the log. Q188 says revoke stays + instant. +- X1's "210's busy map (`credentials.mjs:118`)" is `#inflight`. That's the map of AbortControllers for `host.run` + calls in flight. It isn't a lock, and 210 has no busy map. + +Fix: +- The executor registers an AbortController under the same `instance:host.` key, so revoke's `abort()` + reaches it (`credentials.mjs:289`). +- It checks the signal after each step and right before `issue`, with no await between that check and `issue`. +- On abort it runs X3's cleanup for the step it reached and records `grant.failed revoked`. +- A revoke with no grant yet must still work. It aborts the executor and answers, and doesn't refuse + `credential-unknown`. +- Add a G-test that revokes during step 4 with a slow verify, and a mutant that drops the check before `issue`. + +B3. A restart mid-executor leaves state the record doesn't show. +X3's last paragraph records `grant.failed interrupted` and runs no remote step. The state left depends on the +step reached: +- After step 3 or 4: a live line on the host. `grant.failed` has no `hostLineLeft`, so `status` shows nothing to + sweep. +- After step 5: layers 3 and 4 in the business file with no key file. The reference loads as not issued. +- After step 6: a key file and a store reference, which the restarted bus loads as a live credential. The + record says failed while the grant works. Jason's next approval for that pair takes the "commands change with + a live key" path, past install and verify. + +Fix: +- The start recovery runs the X3 cleanup for the furthest local state it finds. Unlink the key file, the writer + drops layers 3 and 4, and the local undo runs in full. +- It always records `hostLineLeft: ` when the executor got past step 2, so `status` points at `sweep`. + Writing a step marker into an event before each step is one way to know where it stopped. +- Extend G11 to a restart after step 3, after step 5 and after step 6. Each ends with no live credential and the + tag listed. + +B4. The host tag can't hold every valid name, and two grants can share a tag. +- Instance and business names match `ID_PATTERN`, `^[a-z0-9][a-z0-9._-]{0,63}$` (`vocabulary.mjs:127`). E3's tag + rule, `^mosaic-[a-z0-9-]{1,96}$`, refuses `.` and `_`. `mosaic---` can reach 7 + 64 + 1 + + 64 + 1 + 32 = 169 characters. +- Joining with `-` isn't one to one. Business `a`, agent `b-c` and business `a-b`, agent `c` give the same tag + on one host. +- Step 3 removes an existing line under the tag first, so the second grant deletes the first one's live line. +- `sweep` removes every `mosaic-` line whose tag has no live grant. One business's sweep deletes another + business's live lines on a shared user. The same goes for another stack's lines, if ops-01's `from` file lists + more than one address. + +Fix: +- Make the tag one to one and bounded. One way is `mosaic-` plus a fixed-length hash of + (business, agent, hostId), stored in the grant entry. +- Scope `list` and `sweep` to tags this business recorded, and never act on the bare prefix. +- Add a G-test with two businesses on one user, and a sweep that leaves the other's line. + +B5. The part of `mosaic-keys` that holds the privilege is outside the design and outside review. +- The forced command runs as the install user `I`. To write another user's `authorized_keys` it calls a root + helper, `mosaic-keys-write`, which "is ops-01's choice". +- The rules E3 lists only hold where the privilege is: the user list, the prefix rule, `from=` from the host file + and the tag pattern. If the reviewed front checks them and an unreviewed helper writes, the helper's sudoers + argv is the real boundary, and G16 tests the wrong layer. +- The helper also writes, as root, into a directory the target user owns (`~/.ssh`), with tmp and rename, + "keeping owner and mode". Without stated care, that is the classic symlink and hard-link race. The target + user's own processes can redirect a root write. +- `AuthorizedKeysFile` may not be `~/.ssh/authorized_keys` on every host. + +Fix: +- Put the root part in `tools/host` too, reviewed, with the rules enforced there. Refuse the install user and + root by name in code, not only by the list. +- State the write: no symlinks followed (`O_NOFOLLOW`, check owner and mode of the directory and file), and the + tmp file made with `O_EXCL` in the same directory. Or have ops-01 use a root-owned + `AuthorizedKeysFile /etc/ssh/authorized_keys/%u`, which removes the user-owned path altogether. +- Q-O1 then asks ops-01 to accept a stated mechanism, not an effect. + +## Notes (do not block) + +N1. The executor can't call `#consumeAuthority`. Through `#checkAuthority` it needs an agent session (`#agent(s)`, +`broker.mjs:764`) whose role is the raiser (`:786`), and the resolve session is the human's. X1 says "the same +`action.allowed` event", which is fine. Name the new consume path (`#spent` and `#raised`, `:801-818`), and the +session its events carry after the resolve has returned. + +N2. The raise body keeps `commandsSha256` and not the commands. Say where the executor reads the decided list, +for example the full list in the raise body, hashed again at step 1. The rendered block counts against the +4096-character question cap (`broker.mjs:126`). Name the refusal when the chief's text plus the block exceeds it. + +N3. `hosts remove` refuses while a grant names the host. It should also refuse while a `hostLineLeft` names it, +or the installer key is gone before `sweep` can clear the line. + +N4. On Q-S1, starting without `grant.ask` is fine with me. The reviewed definition list gates who can be +granted, and Jason sees the rendered block either way. + +The next round is a full read. The blockers touch sections 3, 5 and 6 and the test list. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 17db41fa6..2d192f090 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1410,3 +1410,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T21:45Z | Duckworth (Opus 5.5) | row 210 queue record on comment 29702, candidate 455964f23 | approve recorded, rev 2466 (duckworth-r210-record-1); rev left uncommitted for Sage's queue-commit 2026-10-20T21:50Z | Duckworth (Opus 5.5) | row 195 DESIGN r5 review (duckworth-0054), Darkwing 0b6d29e90 blob 9395e56d1775 | changes B1 (confirm result line lacks op/from/to/backup, from_log misses .53 activations) B2 (migration record after migrate, partial migrate never restored); r4 B1 B2 N1-N4 met; port squat checked, 732 fix fits 2026-10-20T21:55Z | Duckworth (Opus 5.5) | row 195 DESIGN r6 diff check (duckworth-0055), Darkwing b7239bb66 blob 46bbcf2b135e | approve; B1 B2 N1-N5 met; N1 A2f stop timeout now exits 2 (state in run sheet, fix test wording), N2 claude path round separate +2026-10-20T22:18Z | Duckworth (Opus 5.5) | machine-grant design r1 review (duckworth-0056, Vikunja #436) on Gizmoduck f6b6a04b6 | changes B1-B5: runtime grant not loaded, revoke race, restart reconcile, tag collision, host root helper unreviewed