[maxwellpeterson]

This PR deprecates the workspace-wide "use" sharing role and replaces it with gadget-specific sharing grants.

Existing users with the "use" role will retain their current access (subject to the new delegation rules discussed below), and continue to automatically gain access to new gadgets added to the workspace. Adding new users with the "use" role will no longer be possible, and existing share links with the "use" role will stop granting new users access. In the language of the sharing graph, “use” edges continue to exist, but new “use” edges cannot be created except via keepUsers during existing "use" role revocation.

In place of the "use" role, every gadget in a workspace will become individually sharable. This is a fairly simple extension of the existing sharing graph model. New, gadget-scoped “use:<gadget_id>” edges are created by direct shares and gadget-scoped share links.

When a gadget is shared with someone, that user is verified against observations attributed to that gadget, as well as any workspace-wide observations from any connection the gadget has. Once all gatekeepers migrate to the API introduced in #881, verification will be narrowed to just gadget-scoped observations (with a few small caveats) in new workspaces going forward.

This PR splits the current sharing modal into separate sharing modals for workspace collaborators (the "build" role) and individual gadget sharing. The modal itself doesn't change much beyond simplifying away "build" vs. "use" indicators for workspaces without legacy "use" roles.

This PR also changes how grants are conferred, requiring that a user hold the "build" role in order to confer grants to others. Consider the following scenario: Alice owns a workspace. Alice invites Bob with the "build" role, and Bob invites Carol with the "build" role. Alice revokes Bob's access and declines to keep Carol. Both Bob and Carol lose all access. Alice then shares gadget A with Bob.

Under the old model, Carol regains access to gadget A through the old "build" grant from Bob. Carol's access being resurrected in a different form is unintuitive to me, but the bigger problem is how to present Carol in the UI. Carol only has access to gadget A, but she doesn't have a grant to gadget A. If Alice later shared gadget B with Bob, Carol would also gain access to gadget B. It doesn't make sense to show Carol in the shared user list for gadget A, since her access and revocation logic are different from everyone else with a direct grant to gadget A. It also doesn't make sense to show Carol in the workspace collaborator list, since she only has use access to gadget A. Carol effectively has the deprecated "use" role, predicated on gadgets that Bob has access to instead of all gadgets in the workspace.

Under the new model, Carol's access doesn't change when Alice shares gadget A with Bob (note that Carol would still regain "build" access if Alice restored Bob's "build" access). Any workspace collaborator can add Carol as a peer collaborator or grant her access to a specific gadget. There is no in between state. The only "in-betweeners" are users with the legacy "use" role, which show up in the workspace collaborator list under their own section.

Maybe this logic change should be its own PR, but it has a significant impact on how the new UI works and I feel confident in it so I decided to include it here. The main risk of the new model is a future where we allow single-gadget users to reshare those gadgets. Even in this world, I don't think we would bring back the old model.

Instead, we could adopt this model: A grant confers access only while the sharer’s effective access covers the grant’s entire scope. Otherwise it confers nothing. When computing effective access, each fully supported grant contributes its entire scope to the recipient, and unsupported grants contribute nothing rather than being narrowed. This preserves the existing cheap revoke/restore mechanism while preventing a revoked "build" role from coming back to life as a gadget grant later on (the Carol situation described above). Spooky 👻

[ask-bonk]

Review: 3 findings.

Submitted 3 actionable inline comments in a single review.

github run

[ask-bonk]

Since last review: 2 resolved, 1 still open, 0 new.

The scope-widening and hook-attribution findings are fixed in the current code. The existing browser observer-configuration finding still applies.

No duplicate inline comments were posted.

github run

[github-actions]

Preview: pr894-mpeterson-sin-d8c81e9e

https://pr894-mpeterson-sin-d8c81e9e-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

Since last review: 1 resolved, 0 still open, 2 new.

Submitted 2 actionable inline comments in one review: one P1 and one P2. All three earlier findings are addressed in the current code.

github run

[ask-bonk]

Review: 4 findings.

Submitted 1 new P2 inline comment. Three previously published findings still apply:

• Hook attribution
• Workspace-context producers omitted from admission
• Narrowed grants unreachable for revocation

Verified these with isolated runtime reproductions; no duplicate inline comments were posted.

Applied the backend package’s kernel bar and the frontend guide’s rule to test observable contracts and failure paths (packages/workshop-{backend,frontend}/AGENTS.md).

github run

[ask-bonk]

Review: 1 findings.

The existing gadget-admission timeout finding still applies. No new findings or duplicate inline comments.

Verified by source inspection that d2357424 fixes the narrowed-grant revocation case.

Applied the backend kernel bar and the frontend guide’s requirement to “Test observable contracts, product rules, state transitions, accessibility behavior, race handling, and failure paths” (packages/workshop-frontend/AGENTS.md:241–244).

github run

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[clanky-the-clanker]

Superseded by the review of 1818815d.

This review, of 20b5b919

💬 Approved with comments — No blocking issues. There is 1 WARNING about the exclusion check using a narrower scope than admission, and 1 WARNING about observer docs that still describe the old verification timing, plus INFO notes.

Findings: 2 warnings · 3 info (1 is an inline comment on the diff)

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #894.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (2)

### [Code Quality] `#inAccessVerificationScope` ignores `gadgetProducers`, which `#inScopeGatekeepers` includes. A "use" observer verified against a producer-only connection is treated as out of scope on exclusion: the observation is admitted and their registration is dropped, then re-added on the next open.
- **File:** packages/workshop-backend/src/overseer.ts:5893

### [Documentation] `docs/observers.md` still says every collaborator is re-verified at every workspace `open()`. "Use" collaborators are now verified per gadget at `getGadget()`, and their workspace shell is not verified.
- **File:** docs/observers.md:715

## INFO (3)

### [Performance] `#gadgetReaches()` is recomputed for every excluded observer inside the `#enforceExcludeObservers` loop. Compute it once and pass it in.
- **File:** packages/workshop-backend/src/overseer.ts:5893

### [Documentation] Ungrammatical "contexts what is opened reaches" and "requirements what is opened reaches". Suggest "reached by what is opened".
- **File:** docs/observers.md:965

### [Documentation] `plans/restricted-data-sharing.md` still references `#useScopeGatekeeperIds` and `computeEffectiveRoles`. Update those references or mark the passages as historical.
- **File:** plans/restricted-data-sharing.md:10

---

📝 Documentation: ⚠️ 1 warning · 2 info

Observer docs still describe verification as happening at workspace open for every collaborator. There is also a grammar slip and a stale plan doc.

Recommendation: 📝 Minor Comments

• [WARNING] docs/observers.md still says every collaborator is re-verified at every workspace open(). "Use" collaborators are now verified per gadget at getGadget(), and their workspace shell is not verified. (docs/observers.md:715)
• [INFO] Ungrammatical "contexts what is opened reaches" and "requirements what is opened reaches". Suggest "reached by what is opened". (docs/observers.md:965)
• [INFO] plans/restricted-data-sharing.md still references #useScopeGatekeeperIds and computeEffectiveRoles. Update those references or mark the passages as historical. (plans/restricted-data-sharing.md:10)

View full review

• Verification timing: docs/observers.md:150-153 and 714-717 say every collaborator is re-verified against every in-scope gatekeeper at every open(), and that nobody can be in the workspace without passing. With this change, "build" collaborators are verified at workspace open. "Use" collaborators open the workspace shell unverified and are verified per gadget at getGadget().
• Grammar: docs/observers.md:965 and 981 contain "contexts what is opened reaches" and "requirements what is opened reaches".
• Stale plan doc: plans/restricted-data-sharing.md still refers to #useScopeGatekeeperIds and computeEffectiveRoles, which this change replaces.

---

📋 Code Quality: ⚠️ 1 warning

One scope mismatch between the exclude-observers check and admission. The rest is coherent.

Recommendation: 📝 Minor Comments

• [WARNING] #inAccessVerificationScope ignores gadgetProducers, which #inScopeGatekeepers includes. A "use" observer verified against a producer-only connection is treated as out of scope on exclusion: the observation is admitted and their registration is dropped, then re-added on the next open. (packages/workshop-backend/src/overseer.ts:5893)

View full review

Verification-scope mismatch (overseer.ts:5890 vs 9908)

#inScopeGatekeepers builds a gadget grant's scope from two sources:

• reach.gatekeeperIds
• observerLedger.gadgetProducers(reach.gadgetIds), the connections through which a reached gadget recorded requirements, even if it no longer binds them.

#inAccessVerificationScope is documented as "whether they could have been verified against it", but it only consults #gadgetReaches(). A "use" collaborator verified against a producer-only connection is therefore treated as out of scope when named in that connection's excludeObservers. The observation is admitted, and their registration on that connection is dropped. Their next gadget open then re-verifies them and registers them again, because admission still counts the producer.

What this means in practice:

• The two functions now disagree on what "in scope" means.
• New data from the connection probably cannot reach a gadget the collaborator holds if no included gadget reaches it.
• The exclusion path still loses its fail-closed property for producer connections.

Either derive both functions from one helper, or state in the comment that exclusion only concerns forward reach and add a test for it.

---

⚡ Performance: 💬 1 info

One low-priority note about reach recomputation on the observation path.

Recommendation: 💬 Optional Nits

• [INFO] #gadgetReaches() is recomputed for every excluded observer inside the #enforceExcludeObservers loop. Compute it once and pass it in. (packages/workshop-backend/src/overseer.ts:5893)

View full review

#enforceExcludeObservers calls #inAccessVerificationScope once per named observer. Each call runs #gadgetReaches(), a full transitive walk over every permanent gadget with SQLite reads. Computing it once before the loop would avoid N identical walks per observation. Optional.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

Checked and found sound:

• Sharer-must-hold-build conferral in computeEffectiveAccess, including share links through their creator.
• #requireCanShare, which blocks sharing by "use"-only holders.
• Legacy "all gadgets" links, which add no new access.
• Grants that name provisional or missing gadgets confer nothing.
• Restarts on any narrowing in removeCollaborator and revokeShareLink.
• Per-gadget admission in getGadget/authorizeGadget: access is re-read live, and a refused gadget gets the same "No such gadget" error as a missing one.
• Per-gadget session counting.
• Filtered listings for "use" collaborators.
• Widening detection with spawner quarantine.

On the frontend:

• Share URLs are built with URL/searchParams, and the key stays in the fragment.
• UI gating is only a hint; the server enforces.

---

📖 AGENTS.md: ✅ LGTM

No agent-guide changes needed.

---

_{Reviewed commit: 20b5b919}

[clanky-the-clanker]

⚠️ WARNING · Code Quality

#inAccessVerificationScope ignores gadgetProducers, which #inScopeGatekeepers includes. A "use" observer verified against a producer-only connection is treated as out of scope on exclusion: the observation is admitted and their registration is dropped, then re-added on the next open.

Admission (#inScopeGatekeepers, line 9912) puts every connection in a gadget grant's scope through which a reached gadget recorded requirements (gadgetProducers). This check only looks at #gadgetReaches(). A "use" collaborator who was verified against such a producer connection and is named in its excludeObservers is therefore classified out of scope: the observation is admitted and their registration is removed, and their next open re-registers them. Either derive both scopes from one helper, or document that exclusion intentionally considers only forward reach and add a test.

[ask-bonk]

LGTM!

github run

[clanky-the-clanker]

💬 Approved with comments — No blocking or warning-level issues. The earlier warnings on the exclusion scope and on the observer docs are fixed; two INFO notes remain, one about a missing test and one about the cost of the reach computation.

Findings: 2 info

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

📋 Code Quality: 💬 1 info

The earlier warning on the exclusion scope is fixed. One INFO note about a missing test.

Recommendation: 💬 Optional Nits

• [INFO] Exclusion looks only at what a gadget reaches now, while admission also counts historical producers. The difference is documented in a comment but has no test for a connection that is only a historical producer. (packages/workshop-backend/src/overseer.ts:5835)

View full review

Earlier warning, fixed. #inAccessVerificationScope no longer exists. #enforceExcludeObservers (packages/workshop-backend/src/overseer.ts:5835-5882) works out which gadgets reach a connection from #gadgetReaches(). Its comment explains why it differs from admission (#inScopeGatekeepers): a historical producer (a connection a gadget recorded requirements through but no longer binds) does not deliver new observations, so it does not block an exclusion. observer-ledger.test.ts adds a test where a historical producer's new read is let through and only that registration is dropped.

Still open (INFO). observer-exclude-scope.test.ts does not cover a connection that is only a historical producer of a gadget the observer holds. One test there would pin this deliberate difference, so a later change to #inScopeGatekeepers cannot quietly change how exclusion behaves.

Checked and found sound:

• The computeEffectiveAccess fixed point, the rule that a sharer must hold build, and #reRootKeptUsers.
• Per-gadget session counting.
• Widening detection with spawner quarantine.
• sameObserverScope now requires an exact match.
• The observerRequirementPrefix key now ends in :, so gadget ids like 1 and 12 cannot collide.
• The frontend admission flow, which disposes stubs and aborts superseded prompts.

---

⚡ Performance: 💬 1 info

The earlier note about recomputing reach per observer is fixed. One INFO note about the reach computation spanning the whole workspace.

Recommendation: 💬 Optional Nits

• [INFO] #gadgetReaches() walks every gadget's reach in the workspace on the observation path and twice per bind, hook enable or merge. Fine at current sizes; consider caching if workspaces grow. (packages/workshop-backend/src/overseer.ts)

View full review

Earlier note, fixed. #enforceExcludeObservers now fills reachingGadgets once, lazily, with ??=.

INFO. #gadgetReaches() walks the reach of every permanent gadget. Each node in that walk costs a storage read. It runs:

• through gadgetsReaching() on an observation that has a "use" observer;
• twice in #restartIfGadgetScopesWidened on every bind, hook enable and merge.

gadgetProducers adds O(connections × reached gadgets) listKeys probes on each verification pass. This is fine at typical workspace sizes. If large workspaces become common, consider caching the reach map per synchronous turn, or computing reach only for gadgets that have a live session.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

Checked and found sound:

• Grant conferral: a grant, including a share link, confers access only while its sharer or creator holds build. computeEffectiveAccess is a least fixed point, so a cycle of grants that the owner does not reach confers nothing.
• Who can share: #requireCanShare guards addCollaborator, createShareLink and newShareLinkKey. Grants that name a missing or provisional gadget confer nothing.
• Gadget admission: authorizeGadget and getGadget re-read access live. They refuse an excluded gadget with the same "No such gadget" error as a missing one.
• Spawner queue: it checks assertGatekeeperUsable, so a spawner held until a restart starts nothing.
• Observation audience: ObserverLedger.#audience fails closed for "use" observers on chat and gadget contexts.
• Frontend: share URLs keep the key in the URL fragment, and UI gating is only a hint because the server enforces access.

---

📖 AGENTS.md: ✅ LGTM

The agent guides need no changes for this pull request.

---

📝 Documentation: ✅ LGTM

Fixed since the last review:

• Verification timing: docs/observers.md (around lines 362 and 717) now says that "build" collaborators are re-verified at workspace open() and "use" collaborators at getGadget().
• Grammar: the "what is opened reaches" phrasing is gone.
• Plan: plans/restricted-data-sharing.md now opens with a "Historical plan" notice, and its anchors section is renamed to say it is historical, so the stale #useScopeGatekeeperIds and computeEffectiveRoles references are framed correctly.

---

_{Reviewed commit: 1818815d}

[github-actions]

Eval results

Verdict: ⚪ Unchanged. No task moved beyond what these runs can tell apart from noise.

Task	Score	Δ score	Fisher test	Cache hits	Avg min	Avg steps
change-calendar (gpt-6-luna)	100% → 80%	−20 pp	p = 0.47	95% 0 pp	3.7 → 3.5	28.1 → 24.2
chess (gpt-6-luna)	100% → 60%	−40 pp	p = 0.09	97% 0 pp	8.6 → 6.7	54.5 → 41.7
compaction (claude-sonnet-5-5)	100%	0 pp	p = 1.00	90% 0 pp	1.8	43.0
compaction (gpt-6-luna)	100%	0 pp	p = 1.00	93% 0 pp	1.7 → 1.6	43.0
incident-desk (gpt-6-luna)	100%	0 pp	p = 1.00	96% → 95% −1 pp	4.1 → 4.4	29.8 → 30.0
ticket-triage-agent (gpt-6-luna)	100%	0 pp	p = 1.00	96% → 97% +1 pp	0.1	1.7 → 2.0
worker-logs (gpt-6-luna)	90%	0 pp	p = 1.00	95% 0 pp	3.4 → 4.3	21.5 → 26.1

Failed checks

Task	Check	Failed
change-calendar (gpt-6-luna)	t1 schedules-and-lists-windows	0/10 → 1/10
change-calendar (gpt-6-luna)	t1 rejects-invalid-windows-without-changing-anything	0/10 → 1/10
change-calendar (gpt-6-luna)	t1 overlap-is-per-service-and-touching-is-allowed	0/10 → 1/10
change-calendar (gpt-6-luna)	t3 existing-windows-survive-the-rule-change	0/10 → 1/9
change-calendar (gpt-6-luna)	t3 same-service-windows-must-be-a-day-apart	0/10 → 1/9
change-calendar (gpt-6-luna)	t3 billing-is-capped-at-four-hours-others-are-not	0/10 → 1/9
chess (gpt-6-luna)	t1 agrees-with-the-oracle-on-the-hard-positions	0/10 → 4/10
chess (gpt-6-luna)	t1 agrees-with-the-oracle-on-perft-positions	0/10 → 4/10
chess (gpt-6-luna)	t1 agrees-with-the-oracle-through-random-games	0/10 → 3/10
chess (gpt-6-luna)	t1 the-game-is-shared-across-connections	0/10 → 2/10
worker-logs (gpt-6-luna)	t1 hourly-buckets-cover-every-hour-including-empty-ones	1/10
worker-logs (gpt-6-luna)	t1 ranges-are-half-open-and-filter-by-worker	1/10

Run · trajectories and raw results

[ask-bonk]

🔬 Eval runs review

Performance

Against main, change-calendar fell from 100% to 80% and chess from 100% to 60%, but neither exceeds noise (p=0.47 and p=0.087); all other pass rates stayed unchanged. Chess lost the most runs, and its lower mean cost ($0.050 → $0.039) largely reflects four failures stopping at turn 1; worker-logs instead increased from 21.5 to 26.1 steps, $0.018 to $0.023 per run, and 3.4 to 4.3 minutes, with more editing retries. No cache-hit change is significant, and worker-logs’ significant reduction in cache breaks does not show fewer misses.

⚪ VERDICT: NO REGRESSION FROM THIS PR

The sharing and observer-admission changes do not explain the generated-code mistakes, and the unchanged comparison verdict plus mixed costs and step counts establish no PR-attributable improvement or regression.

Triage

Failure modes

• Incorrect chess rules and FEN transitions · chess 4/10 · model error ·
  this PR: no — Turn 1 writeFile(server.js) restricts rook/bishop attack detection to adjacent squares in trials 1 and 4, leaves the original rook behind after castling in trial 2, and reverses en passant validation in trials 4 and 5. The latter makes valid saved positions fall back to the starting position, also explaining the apparent cross-connection failures. All four finish without an executeCode check; main passes 10/10, including demonstrated castling and en passant probes.
• Broken client syntax after a rule update · change-calendar 1/10 · model error ·
  this PR: no — Trial 7, turn 3 editFile(client.js) adds TOO_CLOSE without a trailing comma before UNKNOWN_SERVICE. The resulting syntax error prevents Worker startup and blocks all three checks; the final reply claims completion after reads/searches, without executing the updated gadget.
• Date validation broken after successful verification · change-calendar 1/10 · model error ·
  this PR: no — Trial 8, turn 1 successfully exercises scheduling through executeCode, then its last editFile(server.js) introduces an overescaped ISO regex containing \\d. Valid timestamps subsequently return INVALID_RANGE; the agent does not repeat its probe after that edit.
• Hourly aggregation loses populated buckets · worker-logs 1/10 · model error ·
  this PR: no — Trial 4, turn 1 writeFile(server.js) makes hourly() read row.ts_ms, but its shared selectRows() omits that column. Every emitted bucket is therefore zero despite successful ingestion and summaries; main also fails 1/10 on hourly aggregation, and this PR changes neither the task nor the file tools.

Tool errors

• editFile: No matching text was found in the file. · change-calendar 1 → 2, chess 14 → 5, incident-desk 2 → 0, worker-logs 2 → 6 · model error — Searches use incorrect escaping or text that no longer matches. For example, chess trial 4 searches for doubly escaped regex text, then succeeds after grep reveals the literal source.
• editFile: Multiple matches were found. The text to match must be unique. · chess 4 → 2, incident-desk 4 → 2, worker-logs 0 → 8 · model error — Agents target repeated fragments instead of unique surrounding context. Candidate worker-logs trial 9 repeats ambiguous SQL replacements even after the error, then recovers with larger snippets.
• editFile: Validation failed for tool "editFile": · change-calendar 3 → 4, chess 0 → 1, incident-desk 2 → 2, worker-logs 1 → 2 · model error — Calls omit required filename or replacement fields; the transcripts show successful retries after supplying them.
• readFile: File does not exist. · change-calendar 2 → 0, worker-logs 0 → 2 · model error — Agents read server.js and client.js immediately after creating an empty, non-blueprint gadget, despite the prompt explaining that it has no files yet.
• executeCode: This step already made code changes… · worker-logs 1 → 1 · tool design — Both sides batch an edit with execution during turn 3. The description omits this step-boundary restriction; the error explains recovery, and both agents successfully execute in the next step.

Prompt cache

• worker-logs · cache hits 94.66% → 95.12% · breaks 0.01274% → 0.01092% · this PR: no — Break-rate p=0.0147, but no step stands out as eliminating a miss. At trial 1’s turn-2 transition, after gadget creation and file additions, both sides retain the previous prefix with only three uncached tokens; the candidate’s extra cached editing/retry steps dilute the break rate rather than demonstrate better caching.

What to do

• Optional follow-up in packages/workshop-backend/src/agent.ts: add guidance to smoke-test the requested behavior through executeCode after the final source edit, not just before subsequent “cleanup” edits.
• Optional follow-up in the same file’s EXECUTE_CODE_TOOL_DESCRIPTION: state that file edits and execution must occur in separate model steps, preventing the avoidable retry seen on both sides.

github run