diff --git a/agents/webby/work/case2-r1/REVIEW.md b/agents/webby/work/case2-r1/REVIEW.md new file mode 100644 index 000000000..a9cbb5818 --- /dev/null +++ b/agents/webby/work/case2-r1/REVIEW.md @@ -0,0 +1,173 @@ +# Webby review: Case 2 design r1 (#416) + +Webby, 2026-10-20 (from `date -u`). Independent reviewer, replacing Dewey (Mos, mos-0980). +Subject: `agents/gosalyn/work/case2/DESIGN.md`, blob fba567f71dc1, at e997f3b76 and on +origin/gosalyn/case2-design 88222f3f3 (same blob). Author: Gosalyn. Static review only. I ran no +tests and changed no product code. + +Code base: `git archive 5d019d876`. `git diff --stat 5d019d876 9ae8c73ac -- packages adapters roles scripts` +is empty, so every line below also holds at origin/refactor 9ae8c73ac. Paths are relative to `packages/` +unless they start with `adapters/` or `roles/`. + +## Verdict: CHANGES + +Twelve blockers. Section 6's no-schema claim holds. Section 5's write-side guardrails hold. The +message-side guardrails have two channels the design misses (B3, B4), and the smallest slice can't +run as written (B1, B2). + +## Citations checked + +These hold as cited: git.mjs:8 (`protocol.allow=never` in SAFE), git.mjs:44 (any `remote.*` key +refuses), git.mjs:55-56; project index.mjs:12, :31-84, :97, :195-234, :209, :248-261, :256-258; +paths.mjs:7, :14-15, :34-48 (`absolute(project.root)` at :41 throws on a missing root); +business.mjs:186-187; gate.mjs:130-167 (typed by name at :134, built-ins at :137-138); +claude-gate.mjs:13; tools.mjs:6-7, :95-102, :186, :211-220, :271-280; vocabulary.mjs:92-97, :131; +role.mjs:19-28, :55-66; broker.mjs:290-291, :642-650, :1378-1430, :1434-1435, :1867, :2331-2347, +:2374-2399; schema.sql:240-250, :296-306, :310; runner.mjs:446-456; seat session.mjs:59-64; +adapters/claude/adapter.sh:47-53; launcher.mjs:135, :236-238, :353; webui bus.js:885, :919. + +One miss: the `ls-tree` and `cat-file` reads that 1.3 cites at read.mjs:67-86 are in `blob()` at +read.mjs:19-26 (N1). + +## Section 6: "no schema or trigger change" + +It holds. `threads` has no definition column or check (schema.sql:240-250). +`message_threads_match` (:296-299) and `delivery_same_thread` (:300-306) check one thread's +instance, and a room is one thread per instance, so each insert in `room.send` passes as written. +`thread_one_per_run` allows one held thread per run, which matches one session per agent. The only +constraint the design must name is `UNIQUE (project, name)` (:249). `room.open` names each thread +after its instance, so an existing thread with that name refuses (N9). + +The single-instance rule is code, not schema: broker.mjs:291, :2332-2334, launcher.mjs:135 and +:236-237. Changing it is C2-09's job, which is why B1 matters. + +## Jason's three guardrails + +| Guardrail | Enforced in code? | +|---|---| +| Agents never write prose for the user | Into the library: yes. Gate (gate.mjs:137-138), typed tools (no `write_record` without `record.write`, tools.mjs:211) and human-only verbs make an agent write impossible. In messages: only the form is bounded (M5), and two channels skip M5 (B3, B4). The 400-character limit is stated in 5.1, but the stated residual is too small (B5) | +| Agents always refer to the source material | Structural once B6 and B7 are fixed. The quote-in-lines check at a reachable commit is real verification | +| Tool use is guarded | Yes: gate, typed tool list, broker re-check (tools.mjs:6-7) and `#checkMessaging` (broker.mjs:1867). N6 closes a role-file gap | + +## Blockers + +B1. The smallest slice gives Melanie no way to chat with a creative agent. `#threaded` is true only +for definition `assistant` (broker.mjs:291). `thread.create` refuses `thread-instance-ambiguous` +with more than one (:2334). The launcher refuses `thread-not-allowed` for any other definition +(launcher.mjs:135, :237). C2-08 adds the `THREAD_DEFINITIONS` constant, but C2-09 is the row that +makes the broker and launcher read it and adds `instance` to `thread.create`, and C2-09 is outside +the slice (DESIGN 9). The Console's thread creation must pass `instance` too. Fix: put C2-09's +broker, launcher and `thread.create` parts in the slice. The Console chat must pick the instance. +`room.*` can stay out. + +B2. In the slice, Melanie reads raw JSON and can't see sync failures. Replies are JSON bodies (5.2), +and only C2-10 renders them. 7.2 step 2 says "The Console shows them with file and line", but the +library pages are C2-11b, outside the slice. A bad Obsidian push silently stops agents seeing new +work. Fix: the slice needs a minimal cited-item renderer in the existing chat view and a sync status +line (last valid commit, problems, `unpushed`). + +B3. `raise_decision` is a second prose channel to the human. Every agent gets it with free-text +`question` and option `text` (tools.mjs:221, comment at :5-6). An uncited raise goes to the human. +M3's "no write tool exists" is true, but M5 covers only `message.send`. Fix: either refuse +`decision.raise` from creative definitions in the broker and drop the tool from their typed set, or +put the raise text through the same guard. Test both paths. + +B4. The Console conversation view shows the model's raw text, including text M5 refused. +webui serve.mjs:559-569 proxies `/api/conversation` to the control board, and the reader emits +assistant text blocks (conversation/src/events.mjs:58, :82). A refused prose answer stays in the +session file and is readable. Fix: leave creative sessions out of the catalogue on a library +instance, or show stored messages only. Test that a creative session isn't listed. + +B5. The stated limit understates what M5 lets through. A reply holds 20 items of 400 characters, +which is 8,000 characters. An `idea` item's text is free, and any real quote of any line satisfies +the cite. That is a full scene per reply, not "a one-sentence suggestion" (5.1). Jason wrote +"NEVER", and Q176 says the system enforces it. Fix: add a total-characters cap per reply and a lower +cap for `idea` items (for example 200 characters and at most 5 items). State the residual with +those numbers. Mos takes the residual to Jason or Melanie for acceptance, because it is their rule. + +B6. A `correction` isn't bound to the source. M5 limits the edit distance between `from` and `to`, +but nothing requires `from` to be text in the cited lines. An agent can invent both. Fix: `from` +must equal one cite's `quote`, after the same whitespace normalization, inside that cite's lines. +Test that an invented `from` is refused. + +B7. `citation-unfetched` has nothing to check against. The web trail stores the URL +(broker.mjs:1521) and a summary of status, bytes and redirects (bus/src/web.mjs:46), not the page +text. "The quote occurs in the fetched text" needs that text. Also, Claude's built-in WebSearch and +WebFetch are on when `policy.webBuiltin` is set (gate.mjs:28-32), and those fetches never reach the +broker. Fix: say where fetched text is kept (bounded, per run, or a new table, and if a table then +say so). M1 must refuse the built-in web source for creative definitions, so `research` uses only +the broker's tools. + +B8. Remote content is a new untrusted input, and the S-rules don't check tree entries. Today only +the bus writes record repos. Under 7.2, Obsidian or anyone with push to the Gitea repo writes too. +A fast-forward checks out whatever the commit holds. No rule refuses symlinks (120000), gitlinks +(160000), executables or nested `.gitattributes` and `.gitmodules`. Separately, S1 refuses the +`.gitignore` that C2-12 commits, and Obsidian puts new notes and `.trash/` at the vault root by +default. Fix: add a rule that every entry is a 100644 blob or a tree, and refuse those names at any +depth. Allow `.gitignore` at the top. Commit Obsidian's new-file location and trash settings in +`app.json`. Test each case. + +B9. The fail-closed state in 7.2 strands Melanie's writes. If remote main is ahead and invalid, the +bus stays on the old commit (step 2). The next bus write commits on that base. The push can't +fast-forward, so the commit is `unpushed`, then `library-diverged`, with no merge until C2-17. +S5 makes this likely: Obsidian users create `[[stub]]` links to notes that don't exist yet, so one +dangling link in the playground blocks the whole sync. Fix: refuse library writes while remote main +is ahead and invalid (`library-remote-invalid`). Apply S5 to canon only, or allow unresolved links +in `playground/`. Test both. + +B10. The 7.2 gate can't catch the obvious mutant. Tests use the file protocol through a test-only +argument, so the https path, `GIT_ASKPASS` and the protocol override are never run. A mutant that +sets `protocol.allow=always` passes them all. `git()` passes only `gitEnv()` plus the identity +argument (git.mjs:10-12, seat checkout.mjs:79-89), so sync needs its own env entry for the helper. +`GIT_TERMINAL_PROMPT=0` (checkout.mjs:85) doesn't stop askpass, which is fine. Fix: one function +builds the production fetch and push argv and env. A test asserts that the argv holds SAFE, then +exactly `-c protocol.https.allow=always`, and no other `protocol.*` key. Another asserts that +`GIT_ASKPASS` names a helper outside the repo that reads the slot file. Keep the file-protocol +switch out of any path that a business file or a bus verb can reach. + +B11. An optional `root` breaks more than the overlap loop. `ensureCheckouts` clones every entry's +root (seat checkout.mjs:310, :319). With no root it calls `clone(project, undefined, ...)`, so "a +project with no root gives agents no clone" (2.1) is false today. `projectFilePath(entry.root)` +(business/src/project.mjs:9-10) throws on undefined, and it is called at business/src/cli.mjs:72, +:78, :118, resolve.mjs:44 and instance-settings.mjs:142. Fix: C2-04 lists these consumers, each +skips a library project explicitly, and each has a test. + +B12. Scope and crossovers can also come through front-matter fields, and in the slice a hybrid +rests on convention. S2 and S3 speak of links only. `extends:`, `sources`, and `exclude` and `pin` +paths are references too, and a canon book entity with `extends: playground/...` would bring +playground text into canon lookups labelled canon. `library_manuscript` has no `scope`, so it must +label or refuse a playground book. In the slice there is no crossover dialog (C2-11b), so a hybrid +is a hand-written `universe.json` pushed from Obsidian, and S6 doesn't require `sanctioned`. Fix: +put every path field under S2 and S3. Make S6 require `sanctioned`. State that in the slice a hybrid +is Melanie's hand edit, and that only her account and the bus account can write to the Gitea repo. + +## Non-blocking + +N1. Cite read.mjs:19-26 for the blob reads, not :67-86. +N2. `END_PROTECTED` is `[THREAD_DEFINITION]` (vocabulary.mjs:135). Say whether creative +definitions become end-protected when `THREAD_DEFINITIONS` replaces it. +N3. The runner sends `(the session gave no answer)` and `The turn failed: ...` (runner.mjs:449). +The guard will refuse both, so Melanie never learns a turn failed. Add a fixed turn-failed notice, +and test that any notice other than the fixed bodies is refused. +N4. Promotion moves several files. `commitFile` commits one path (index.mjs:219). C2-02 needs a +multi-path commit. +N5. Ids allow capitals (paths.mjs:7). Two universes that differ only by case collide on a +case-insensitive file system. Enforce lowercase in `checkLibrary`. +N6. A role with no `messaging` key may message anyone (role.mjs:50). M1 must require `messaging` +for creative definitions. +N7. C2-05b's sync-before-read needs C2-03, which its row doesn't list. C2-03 reads the remote URL +that C2-04 adds to the business file, so either pass the URL in or order C2-04 first. +N8. C2-06 must run the guard through both the runner's RESULT path and `send_message`, with a +non-creative sender in a library business. Otherwise a mutant keyed on `CREATIVE_DEFINITIONS` +passes, against 5.2's "role choice doesn't matter". +N9. Name the refusal when `room.open` meets an existing thread with an instance's name +(schema.sql:249). + +## Rows and the date + +The rows are small and most gates name the right refusal. B6, B10 and N8 are the gates that miss +their obvious mutant. The listed slice (C2-01 to C2-08, C2-12, C2-13) does not let Melanie start. +She can write in Obsidian, but she can't reach an agent (B1) or read its answers (B2). With +C2-09's broker part and a minimal renderer added, it can be done by 10-27 only if T1 row 1 (row +190) lands by about 10-24, as section 9 says. Reaching the Console from her machine also needs SSO +(Q175). diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 9f20adc24..0ae0d281b 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1361,3 +1361,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T18:34:53Z | Filbert (Opus 5.5, 048f3c63) | Q175 SSO design review r2 (DESIGN.md blob 3eb718b5e131 at 1a268249e), static | changes: C1 config.json console key fails old loaders (mosaic-config.mjs:113 rejects unknown keys; key add inside phase 2 window, rollback removes it, loader test), C2 app.js signs out only on opaqueredirect so 401 never shows sign-in; notes on tests 15/7/9a, Q-B 200-HTML classification, scratch Console flags; Q-E keep iss/sub escaped and capped; verdict sent to Sage, no Gitea/queue record 2026-10-20T18:40:20Z | Filbert (Opus 5.5, 048f3c63) | Q175 SSO design review r3 (DESIGN.md blob a152fd58e18e at 817e8f51b), static | approve on condition D1 (slot file checks stay out of shared loader, shape-only there) as a line edit; D2 notes: slot root from configDir, scratch dataRoot, name discovery GETs for Mos; C1/C2 and notes verified; Mos C1 sequence confirm still open; no Gitea/queue record 2026-10-20T18:41:20Z | Filbert (Opus 5.5, 048f3c63) | Q175 SSO design D1/D2 check (DESIGN.md blob eba861f5d2bb at d55d86bb0), static git diff 817e8f51b..d55d86bb0 | ack: D1 and D2 match; design approved; Mos C1 sequence confirm still open; no Gitea/queue record +2026-10-20T18:57:45Z | Webby (Opus 5.5, reviewer seat) | Case 2 design r1 review for Gosalyn (#416, DESIGN.md blob fba567f71dc1 at e997f3b76 / 88222f3f3), static | changes: B1-B12, N1-N9 in agents/webby/work/case2-r1/REVIEW.md; section 6 no-schema claim holds