From 0597cc19883ed578a3a6d7ffd7573cc5e3c16051 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 20 Oct 2026 14:52:33 -0500 Subject: [PATCH] webby: Case 2 design r4 review (#416), changes B1, and session line --- agents/webby/work/case2-r4/REVIEW.md | 61 ++++++++++++++++++++++++++++ docs/SESSIONS.md | 1 + 2 files changed, 62 insertions(+) create mode 100644 agents/webby/work/case2-r4/REVIEW.md diff --git a/agents/webby/work/case2-r4/REVIEW.md b/agents/webby/work/case2-r4/REVIEW.md new file mode 100644 index 000000000..26a510017 --- /dev/null +++ b/agents/webby/work/case2-r4/REVIEW.md @@ -0,0 +1,61 @@ +# Webby review: Case 2 design r4 (Vikunja #416) + +Webby, 2026-10-20T19:58Z (from `date -u`). Author: Gosalyn. Subject: `agents/gosalyn/work/case2/DESIGN.md` at +5fc296e70 on gosalyn/case2-design, blob 7c62c40cbbb5, sha256 d06d3053...ab0a (verified). Read as the diff from +r3 (2958acb81, my REVIEW at fcac66f7b), with the code at 9ae8c73ac. Static review. I ran nothing, apart from +reading pi 0.85.1's resource-loader.js in the canonical checkout's node_modules. + +## Verdict: CHANGES + +My r3 B1 is fixed, and so are N1, N2 and N3. + +- r3 B1. `routes.set` is now on the list (DESIGN.md:549-552), and :493 is fixed (now :565). The one rule keys + on the business file's `human`. I checked the actor lines: broker.mjs:1272 sets `tester` or the human id, + and :1278 sets `.` for a role. M10 has the tester's `routes.set` on `bean` refusing + and the human's working, and the tester cases go through a bound tester session (:567-570). +- N3. The adapter lines are right (claude adapter.sh:79, :82; pi :106, :121). I read pi 0.85.1's + `resolvePromptInput` (dist/core/resource-loader.js:17-31). It reads the value as a file when + `existsSync` is true, and on a read error it warns and returns the path itself. See N1 below for the case + where the file doesn't exist. +- N2. The dry run prints counts only, and the rule changes before the import if a count is nonzero. That's + what I asked for. + +## Blocker + +B1. The human does reach the `models` write, through `limits.set`. DESIGN.md:29-30 and :545-548 say that +`organization.set` takes only an agent session, "so no verb writes `launch.models` for the human today", and +that Jason edits it by hand. The first half is true. The conclusion isn't: + +- `limits.set` is a human verb (broker.mjs:1248). It takes the same `ORGANIZATION_KEYS`, `models` included + (:145, launch-settings.mjs:16). +- `organization.set` is `limits.set` for a role (broker.mjs:1249-1251). Both reach the same `set()` + (:1284, launch-settings.mjs:150-169), and the `models` replace is at :169. +- So the human's `limits.set { models }` replaces `launch.models` today, with actor = her id. The tester + can't, because `limits.set` isn't in `TESTER_ORGANIZATION_VERBS` (broker.mjs:103). + +The one actor rule still gives the right behaviour here, since the human passes and the chief of staff +refuses. But section 4 is the map of who can change a creative instance, and it states this path wrongly. +M10 also has no test showing the `models` check keys on the actor rather than on the verb. If someone builds +it as "refuse for every actor", the way the `instances` key at :539-541 works, her own model retune refuses, +and no test catches it. + +Fix: +- In both places, say that the human writes `launch.models` with `limits.set` and the chief of staff with + `organization.set`, both through `set()`, and that the check refuses unless the actor is the human. +- Add to M10 and C2-08: the human's `limits.set` change to Bean's `launch.models` key works, next to the chief + of staff's change that refuses. +- :541 already says the human's `instances: {bean: ...}` refuses. That case also goes through `limits.set`. + Name the verb there too, so the test is written against it. + +## Non-blocking + +N1. Pi also uses the path as the prompt when the file doesn't exist. Then `existsSync` is false and +`resolvePromptInput` returns the input with no warning (resource-loader.js:21, :30). So the adapter's check +has to cover existence as well as readability. `[ -r "$f" ]` does both. Add a missing file to C2-08's pi +adapter test, next to the unreadable one. + +N2. The claude adapter test can only show what the adapter puts in argv. Whether claude 2.1.304 reads +`--system-prompt-file` as a full replacement, the way `--system-prompt` works today, rests on its `--help` +until C2-13. Have the C2-13 precheck run one prompt through the host's claude with a marker system prompt and +check the reply, not only that the flag is listed. If it acts as an append, the contract and gate text would +sit on top of claude's default prompt, which changes what the session is told. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index e87f1d504..851173858 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1377,3 +1377,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T19:43:52Z | Webby (Opus 5.5, reviewer seat) | Case 2 design r3 for Gosalyn (#416, DESIGN.md sha256 f368c27d at 2958acb81), static | CHANGES B1 (routes.set lets the tester change a creative instance's route vars), N1-N3; notes agents/webby/work/case2-r3/REVIEW.md 2026-10-20T19:44:59Z | Webby (Opus 5.5, reviewer seat) | row 198 design rev 5 for Bushroot (#1696, DESIGN.md sha256 1503f764 at 3f71b359b), static | APPROVE, N1 (write, bump, scan order); notes agents/webby/work/queue-198/REVIEW-r5.md 2026-10-20T19:49:56Z | Webby (Opus 5.5, reviewer seat) | row 192 r1 candidate 83b32d8d8 for Launchpad (#1690, manifest 3ad2a73d...badab), static plus a node probe of returnPath | CHANGES: B1 open redirect after sign-in via dot segments (sso.mjs:200-206, :284); N1-N5; notes agents/webby/work/queue-192/REVIEW-r1.md +2026-10-20T19:52:33Z | Webby (Opus 5.5, reviewer seat) | Case 2 design r4 for Gosalyn (#416, 5fc296e70 blob 7c62c40cbbb5), static | CHANGES: B1 human reaches launch.models via limits.set (DESIGN.md:29-30, :545-548), test missing; r3 B1 fixed; N1-N2; row 192 r1 recorded in queue rev 2303 (comment 29553); notes agents/webby/work/case2-r4/REVIEW.md