Rename prohibitAllSharing to containsRestrictedData

[Maximo-Guk]

Also rely on observer verification when viewing restricted data

WIP

[github-actions]

Preview: restricte-74e0a59d

https://restricte-74e0a59d-os-public-router.gadgets-staging.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

The test mock uses prohibitAllSharing as the storage key, which is correct and unchanged.

My review is complete. This PR is a clean, well-executed rename. Let me summarize the findings.

Review Summary

This PR renames the sharing-policy flag from prohibitAllSharing to containsRestrictedData across the public API surface (ObservationDescription.containsRestrictedData, GadgetMetadata.containsRestrictedData), all callers (gatekeeper-google's BigQuery session), the frontend ShareModal, and the docs. It is not a behavior change — despite the earlier bot comment describing a large observer-verification policy overhaul, the actual diff is purely nominal.

What's correct:

• The typed-storage key is deliberately NOT renamed (overseer.ts:820), with an explicit comment (overseer.ts:817) explaining that the property name is the storage key, so renaming it would silently unlatch every workspace that already observed restricted data. This is the right call and the single most important decision in the PR.
• The rename is complete and consistent. No sharingProhibited / share-prohibited stragglers remain; all 46 containsRestrictedData references line up across shared types, backend, frontend, gatekeeper, and docs. The test mock (overseer-hooks.test.ts:130) correctly keeps prohibitAllSharing as the storage key.
• Kernel doc-comment bar met. Both renamed public API members (ObservationDescription.containsRestrictedData, GadgetMetadata.containsRestrictedData) retain full doc comments.

Non-blocking observations (advisory):

1. Semantic drift between name and behavior. The new name containsRestrictedData describes what the data is, but the flag still means what the policy does: prohibit all sharing + latch lockdown (no actions, no web fetch). The doc comment at gatekeeper.ts:1068-1082 still describes the full prohibit-all-sharing/lockdown behavior, so a reader who sees only the field name may under-appreciate that setting it permanently locks the gadget down. The name reads as a milder, descriptive flag than it is. Worth a one-line reinforcement at the field, or reconsidering whether the rename improves clarity given the behavior is unchanged.

2. Metadata field vs. storage field now diverge. GadgetMetadata.containsRestrictedData is populated from storage.prohibitAllSharing (overseer.ts:7351, :7370). This is unavoidable given the storage key can't be renamed, and the comment explains why, but it's a small readability wart that future maintainers will trip over. Fine as-is; noting for awareness.

3. Title/description undersell. The PR is marked WIP and titled only "Rename". Since workshop-backend/workshop-shared are kernel-bar, a reviewer skimming would expect a mechanical rename — which this is — so the title is accurate here (unlike what the stale bot comment implied). No action needed beyond dropping WIP before merge.

No actionable defects found. The rename is careful and the storage-key preservation is exactly right.

github run

[ask-bonk]

The .dup() pattern in startSession matches the real gatekeepers. The added try/catch disposal on error is a nice touch (the real ones don't have it, but it's more careful, not wrong).

I've completed a thorough review. This is a substantive kernel-level policy change (the two conflicting earlier bot comments each got half the story — commit 1 renames, commit 2 changes the policy). My analysis:

Correct and well-designed:

• Storage key deliberately not renamed (overseer.ts:818-821) — the single most important decision, correctly documented.
• Restricted-mode latch preserved (submitAction, getWebFetchEnv).
• Coverage guard #assertSensitiveObservationCoverage handles the live-session gap correctly and errs safe (blocks, never leaks) for unverifiable gatekeepers.
• Rename is complete; no stragglers except intentional storage-key/test-mock ones.
• Tests are comprehensive; docs updated consistently.

I found one real availability concern worth an inline comment, plus advisory notes. The concern (bot observation #1) is confirmed by the code: for "use"-role collaborators, #inScopeGatekeepers only includes gatekeepers bound to a gadget, so accountChoices will never contain an unbound gatekeeper (e.g. an ambient singleton read directly in executeCode) — yet the coverage guard checks gatekeeperId in observer.accountChoices for all collaborators unconditionally. Such an observation is permanently blocked while any "use" collaborator has access. It errs safe, so it's not a security defect, but it's a surprising availability cliff with no code comment. Since the working tree is read-only I'll report it in the final response rather than as an inline review.

Given the PR is explicitly marked WIP, these are advisory.

Review Summary

This PR is not just a rename (despite the title). It has two commits: (1) rename prohibitAllSharing → containsRestrictedData across the public API, and (2) "Govern sharing of restricted data by observer verification" — a real policy change replacing the all-or-nothing "sensitive data ⇒ workspace unshareable" lockdown with per-collaborator, gatekeeper-mediated observer verification. This touches the kernel (workshop-backend/workshop-shared), so it's held to the higher review bar.

What's correct:

• Storage key deliberately not renamed (overseer.ts:818-821) with a clear comment — the typed-storage key is the property name, so renaming would silently unlatch every workspace that already observed restricted data. Right call.
• Restricted-mode latch intact — submitAction (overseer.ts:2998) and getWebFetchEnv (overseer.ts:2949) still block on the flag, so the no-actions/no-web-fetch leak protection is preserved.
• Coverage guard is sound — #assertSensitiveObservationCoverage (overseer.ts:2880) closes the live-session gap using listCollaborators() (current collaborators only), and unverifiable gatekeepers (legacy / no vendor account) block rather than leak. The removed open() and sharing-RPC short-circuits are correctly superseded by ensureObserver.
• Rename is complete — no stragglers beyond the intentional storage-key references and the test mock (overseer-hooks.test.ts:130). Both renamed public API members keep full doc comments (kernel bar met).
• Tests are comprehensive — latch, unredeemed link, unverified-collaborator block, verified pass, late-added gatekeeper, open-after-latch, and gatekeeper refusal. The fixture TestSession drives the real ApprovalQueue funnel and follows the .dup() pattern the shipping gatekeepers use.

Findings (advisory; PR is WIP, working tree read-only so no inline review submitted):

1. Availability cliff for "use"-role collaborators + unbound gatekeepers. #assertSensitiveObservationCoverage checks gatekeeperId in observer.accountChoices for every collaborator, but accountChoices is populated by ensureObserver via #inScopeGatekeepers(role), which for "use" collaborators only includes gatekeepers bound to a gadget (overseer.ts:6038-6047). A gatekeeper that reads restricted data but is bound to no gadget (e.g. an ambient singleton the agent reads directly in executeCode) will never appear in a "use" collaborator's accountChoices, so such an observation is permanently blocked while that collaborator has access — even though they're legitimately verified against everything the UI can invoke. It errs safe (blocks, never leaks), so not a security problem, but it's a surprising availability edge worth a code comment or a follow-up.

2. Semantic drift between the new name and the behavior. containsRestrictedData describes what the data is, but the flag still drives what the policy does (restricted-mode latch: no actions, no web fetches, permanently). The gatekeeper.ts doc comment covers this well; just flagging that the field name reads milder than its effect.

3. Metadata field populated from the old storage field — containsRestrictedData in getMetadata/subscribe reads storage.prohibitAllSharing (overseer.ts:7373, :7392, :7424, :7430). Unavoidable given the key can't be renamed, and the comment explains it; noting for future-maintainer awareness.

4. Pre-existing typo "Disallwing" at overseer.ts:2950 — untouched by this PR, mentioned only because you're editing nearby.

5. Title/description undersell the change. For a kernel-bar diff, the title should reflect the policy change (the second commit message already does). Drop WIP before merge.

No actionable defects. The coverage-guard reasoning and storage-key preservation are exactly right, and the test matrix is thorough.

github run

[ask-bonk]

Confirmed: listCollaborators() returns all reachable collaborators regardless of role, and CollaboratorInfo includes their role. The coverage guard ignores the role and doesn't account for the fact that a "use" collaborator's accountChoices legitimately excludes unbound gatekeepers.

I've completed a thorough review. Let me summarize the findings. Since the working tree is read-only, I'll report findings in the final response. My findings are advisory design notes (the PR is explicitly WIP) — no defects that would warrant an inline review.

Review Summary

This PR has two commits and is not just a rename despite the title: (1) rename prohibitAllSharing → containsRestrictedData across the public API, and (2) "Govern sharing of restricted data by observer verification" — a real kernel-level policy change replacing the all-or-nothing "sensitive data ⇒ workspace unshareable" lockdown with per-collaborator, gatekeeper-mediated observer verification. Since it touches workshop-backend/workshop-shared, it's held to the higher review bar.

What's correct:

• Storage key deliberately not renamed (overseer.ts:818-821) with a clear comment — the typed-storage key is the property name, so renaming would silently unlatch every workspace that already observed restricted data. This is the single most important decision, and it's right. I verified the only remaining prohibitAllSharing references are this storage key and the test mock; no sharingProhibited stragglers.
• Restricted-mode latch intact — submitAction (overseer.ts:2998) and getWebFetchEnv (overseer.ts:2949) still block on the flag, so the no-actions/no-web-fetch leak protection is preserved.
• Coverage guard is sound — #assertSensitiveObservationCoverage (overseer.ts:2880) closes the live-session gap using listCollaborators() (current collaborators only), and unverifiable gatekeepers (legacy no-creationSpec throw → caught; null vendorId → falsy) block rather than leak. The removed open() / sharing-RPC short-circuits are correctly superseded by ensureObserver.
• Frontend consistent — ShareModal replaces the full lockout with a non-blocking warning banner and re-enables all sharing controls, matching the new "sharing stays available, per-recipient verification at open" model.
• Tests are comprehensive — latch, unredeemed link, unverified-collaborator block, verified pass, late-added gatekeeper, open-after-latch, and gatekeeper refusal (surfacing the gatekeeper's own reason). The fixture TestSession now drives the real ApprovalQueue funnel and follows the .dup() pattern shipping gatekeepers use (with a nice try/catch disposal on error).

Findings (advisory; PR is WIP, working tree read-only, no inline review submitted):

1. Availability cliff for "use"-role collaborators + unbound gatekeepers. #assertSensitiveObservationCoverage checks gatekeeperId in observer.accountChoices for every collaborator regardless of role, but accountChoices is populated by ensureObserver via #inScopeGatekeepers(role), which for "use" collaborators only includes gatekeepers bound to a visible gadget (overseer.ts:6038-6047). A gatekeeper that reads restricted data but is bound to no gadget (e.g. an ambient singleton the agent reads directly in executeCode) will never appear in a "use" collaborator's accountChoices, so such an observation is permanently blocked while that collaborator has access — even though they're legitimately verified against everything the UI can invoke. It errs safe (blocks, never leaks), so not a security defect, but it's a surprising availability edge worth a code comment or a follow-up.

2. Semantic drift between the new name and the behavior. containsRestrictedData describes what the data is, but the flag still drives what the policy does (restricted-mode latch: no actions, no web fetches, permanently). The gatekeeper.ts:1068-1084 doc comment covers this well; flagging only that the field name reads milder than its effect.

3. Metadata field populated from the old storage field — containsRestrictedData in getMetadata/subscribe reads storage.prohibitAllSharing (overseer.ts:7373, :7392, :7424, :7430). Unavoidable given the key can't be renamed, and the comment explains it; noting for future-maintainer awareness.

4. hasAnyShares() now has no production caller. Its only use was the removed short-circuit in authorizeObservation; it now survives only in sharing.test.ts. Its doc comment was correctly de-referenced from the old policy, but consider whether the method should stay (dead in production) or be removed with its test — minor.

5. Pre-existing typo "Disallwing" at overseer.ts:2950 — untouched by this PR, mentioned only because you're editing nearby.

6. Title/description undersell the change. For a kernel-bar diff, the title should reflect the policy change (the second commit message already does: "govern sharing by observer verification"). Drop WIP before merge.

No actionable defects found. The coverage-guard reasoning, storage-key preservation, and the test matrix are exactly right.

github run