diff --git a/agents/webby/work/queue-252/REVIEW-design-r1.md b/agents/webby/work/queue-252/REVIEW-design-r1.md new file mode 100644 index 000000000..3ab7a9773 --- /dev/null +++ b/agents/webby/work/queue-252/REVIEW-design-r1.md @@ -0,0 +1,75 @@ +# Webby review: row 252 (#1753, #435 J2) design, revision 1 + +Webby, 2026-10-21T18:59Z (from `date -u`). Author: Gander (gander-0033). Gate owner Sage (sage-2053), +reviewer by mos-1193. Brief: 640aa03f2:docs/plans/2026-10-21_console-inbox-approve.md, items 1-6 of +"What the design must settle". Candidate: agents/gander/work/queue-252/DESIGN.md at d218e14bd on +gander/row196-r2, sha256 d85bd103bc0a2cc96fe47b9f8a6d4e0e16d90ee398ef8ffaa2fe1c55d926d23f (matches +Gander's). The branch tip is now ab1f0e576. d218e14bd..ab1f0e576 doesn't touch queue-252/. + +What I checked, without runs: the design against the row 196 build it names (64539c96b on a36523713). +I read #approvedGrant, the decision.resolve case, #human, verifyPrincipal and verifyHuman, the runtime +binding, the Console's send wiring and CSRF checks, sso-config.mjs, the decision_events DDL, and every +reader of a resolution's `via`. Line numbers below are at 64539c96b. + +## Verdict: APPROVE + +1. Scope. Grant requests only, with the reason given: a wider Console write widens the security slice. + Eligibility matches what #approvedGrant (broker.mjs:686-693) already requires: credential.mint, + route_to human, and a grant binding on the original raise. It adds exactly two option keys, with + approvalChoice naming one. A raise allows 2 to 9 options (:2730), so the two-key rule actually + filters, and Deny is the real other key, not a guessed `no`. +2. Identity. (provider, iss, sub) from the live row 192 session. Session.name, headers and body fields + are refused. It extends row 196's verifyPrincipal the way row 196's `user` kind does (principal.mjs: + 20-39): an exact same-tree `cli.mjs serve` parent, reread after the walk, with its own nonce + namespace. Agent, seat, user and tester callers are refused. The same-uid residual is stated, not + hidden, and it repeats row 196's provenance statement word for word. +3. Write path. A restricted Console cap and its own resolve op, sharing the decision.resolve + transaction. #human (:470-473) and bindHuman are left alone. decision_events.via has no CHECK + (migrations/3b.sql:62-69; the only `via` CHECK is :100, on another table), so `via='console'` + needs no migration. That backs the design's "no new schema column". The subject goes in the + human.input body in the same transaction. #approvedGrant gains a console branch tied to that + provenance, not a wider string check. +4. Guards. Origin and Sec-Fetch-Site follow the existing send boundary (serve.mjs:177-178). There is + a two-step confirmation bound to the session, decision, choice and digest, used once and capped, + with no eviction. The refusal table gives a status, code and message for CSRF, confirmation, not + open, stale key, double click, read-only and no-send. Its codes match webui bus.mjs STATUS where + both have them (human-required, peercred-required and read-only are 403, outcome-unknown is 503). + It never retries automatically after an uncertain transport. +5. Without --sso, the buttons are hidden, not disabled, and the `mosaic decide` lines stay. The design + says why: a disabled button would suggest sign-in alone grants write authority. +6. Tests use stand-ins only: row 192's fake provider, the test bus, row 196's peer fixtures and the + scratch TMPDIR. There are 17 mutants, each tied to one boundary. + +## Non-blocking + +N1. Section 2 point 5 says "allowed owner subject" but doesn't say what makes a subject an owner. +console.sso subjects are {sub, name} only (sso-config.mjs:66), and the key check refuses any other +field. Row 196's design (:124-125) says any subject in console.sso.subjects reaches owner bus calls. +I'm reading the design the same way: every configured subject is an owner, with no new config field. +The build should say so. That means every configured subject can approve a credential mint. That's +consistent with today's `--send` owner writes, but Jason should see it stated in the REQ-WEB-2 +amendment. + +N2. Order inside the shared transaction. Today decision.resolve calls #approvedGrant (broker.mjs:2884) +before it writes the human.input event (:2889). If the console branch requires the matching human.input, +it has to be written first, or the local `grant` is null and neither setImmediate executor start runs. +The approval would then record but never act. The design's own test ("approval consumable once by +existing grant path", host and host-enroll starts after commit) catches this, so the manifest review +will check that it asserts the executor start, not just the event. + +N3. Other readers of `via` the design doesn't list. webui public/bus.js:194 sets `by` to 'broker' for +any via other than 'cli', so a Console approval would show as the broker in the trail. That breaks +"Trail output shows this provenance". The close notice's from_run is 'human-cli' for any human +(broker.mjs:2920). git-socket.mjs:274 RESOLVED_VIA falls back to `via console`, which is fine. +landing.mjs:107 and #protectedEnd (:1427) keep requiring 'cli'. That is correct, since neither a merge +decision nor a role.revoke decision is eligible. The build should name each of these. + +N4. Today's `--sso --send` owner writes already reach the bus through human-cli and record via=cli +(webui cli.mjs:80, bus.mjs:32, runtime.mjs:226). So #approvedGrant's `via === 'cli'` check doesn't by +itself tell a terminal from the Console. That's acceptable, because the Console wires only a fixed verb +list. The build should keep decision.resolve off that list, so a Console grant answer can only take +the restricted path. A test or mutant that checks the list would hold that. + +N5. readProcess keeps only an allowlist of environment keys (human.mjs:34). The Console nonce and the +provider, iss and sub keys need adding there. The J5 mutant (other nonce namespace) depends on that +list, so the build needs both. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 007e5dd14..a3159aa3e 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1423,3 +1423,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-21T12:17Z | Webby (Opus 5.5, reviewer seat) | row 197 queue round 6 (Herb r8 c45ddeb6b, manifest 80dc1468), merge scope | APPROVE, N1 doc; notes agents/webby/work/queue-197/REVIEW-q6r8.md 2026-10-21T13:15Z | Webby (Opus 5.5, reviewer seat) | row 248 round 1 (Sage 4d99c178b, grant-executor held stubs) | APPROVE, N1 sleep 30 residual; notes agents/webby/work/queue-248/REVIEW-r1.md 2026-10-21T15:20Z | Webby (Opus 5.5, reviewer seat) | row 197 proof driver a5b636750 (Herb, proof 4 watcher change), herb-0090 | APPROVE, N1 probe cleanup retry, N2 pin is Sage's; notes agents/webby/work/queue-197/REVIEW-proof-driver-a5b636750.md +2026-10-21T19:00Z | Webby (Opus 5.5, reviewer seat) | row 252 design r1 (Gander d218e14bd, DESIGN.md d85bd103) | APPROVE, N1-N5; notes agents/webby/work/queue-252/REVIEW-design-r1.md