Skip to content

Rename prohibitAllSharing -> containsRestrictedData - #381

Open
Maximo-Guk wants to merge 2 commits into
mainfrom
restricted-data-flag-rename
Open

Rename prohibitAllSharing -> containsRestrictedData#381
Maximo-Guk wants to merge 2 commits into
mainfrom
restricted-data-flag-rename

Conversation

@Maximo-Guk

@Maximo-Guk Maximo-Guk commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Rename prohibitAllSharing to containsRestrictedData, with a typed-storage option ( as @kentonv suggested in this comment #340 (comment) ) so the rename does not need to migrate anything. Once #382 lands, reads off persisted records go through a helper that accepts both names.

I'm not planning on landing this PR until #382 is ready for review

@github-actions github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel gatekeeper Changes to a gatekeeper integration workshop/shared Changes to shared Workshop APIs labels Aug 28, 2026
@Maximo-Guk Maximo-Guk mentioned this pull request Aug 28, 2026
Closed
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Preview: pr381-restricted-da-5e9ec049

https://pr381-restricted-da-5e9ec049-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@ask-bonk

ask-bonk Bot commented Aug 28, 2026

Copy link
Copy Markdown

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@Maximo-Guk Maximo-Guk mentioned this pull request Aug 28, 2026
Merged
Base automatically changed from observer-session-restart to main September 3, 2026 23:04
@Maximo-Guk
Maximo-Guk force-pushed the restricted-data-flag-rename branch 2 times, most recently from 325daa6 to dbddb79 Compare September 10, 2026 21:35
@ask-bonk

ask-bonk Bot commented Sep 10, 2026

Copy link
Copy Markdown

  • P1 Security packages/workshop-shared/src/gatekeeper.ts:1228: The wire rename is unsafe during version skew. Old gatekeepers send prohibitAllSharing, which the new backend ignores, allowing restricted observations without lockdown. Preserve a deprecated alias, OR both fields server-side, and dual-write during rollout.

  • P2 packages/typed-storage/src/index.ts:243: SingletonSchema is structurally typed but detected using instanceof. A bare default shaped like { defaultValue, options } is incorrectly typed as wrapped while runtime treats it as the value. Add a nominal/private brand.

  • P2 packages/typed-storage/src/index.ts:221: T extends StorageValue prevents singleton(null, ...) and singleton(undefined, ...), although bare singleton defaults support both and existing schemas use them. Remove this constraint for singleton schemas.

github run

ask-bonk[bot]

This comment was marked as resolved.

Sign in to view

@ask-bonk

ask-bonk Bot commented Sep 10, 2026

Copy link
Copy Markdown

Submitted 2 actionable inline findings.

github run

@Maximo-Guk
Maximo-Guk force-pushed the restricted-data-flag-rename branch from dbddb79 to 7b44daa Compare September 10, 2026 22:13
ask-bonk[bot]

This comment was marked as resolved.

Sign in to view

@ask-bonk

ask-bonk Bot commented Sep 10, 2026

Copy link
Copy Markdown

Submitted 1 actionable inline finding.

github run

Maximo-Guk and others added 2 commits September 10, 2026 17:30
@Maximo-Guk
typed-storage: Let a schema declare the storage key it lives under.
ca5db8f 
A schema property name is also the KV key it maps to, so renaming a property
in code is a storage migration. Give a singleton slot somewhere to say
otherwise: `singleton(defaultValue, {storageKey})` declares the key on disk
explicitly, and a bare default value stays the shorthand for the common case
and behaves exactly as before. Collections get the same option as
`storageName`, which prefixes the records and every index alike.

This is the schema-level version of what would otherwise be a special case at
each call site, and it keeps the old name on disk with no migration.
@Maximo-Guk @claude
Refactor: Rename prohibitAllSharing -> containsRestrictedData.
a8442aa 
The flag's real meaning is "this observation contains restricted data". What
the platform does about that is policy, which shouldn't be baked into the name
-- the next commits replace the all-or-nothing lockdown with per-collaborator
observer verification.

ObservationDescription.prohibitAllSharing and GadgetMetadata.sharingProhibited
both become containsRestrictedData. No alias: this is a hard rename, so the
gatekeeper call sites move in the same commit.

The overseer's durable singleton is renamed too, and declares its old name as
its `storageKey` so nothing on disk moves. Without that, every workspace that
has already observed restricted data would silently unlatch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Maximo-Guk
Maximo-Guk force-pushed the restricted-data-flag-rename branch from 7b44daa to a8442aa Compare September 10, 2026 22:31
@ask-bonk

ask-bonk Bot commented Sep 10, 2026

Copy link
Copy Markdown

  • P2 packages/workshop-shared/src/api.ts:1313: Metadata rename breaks already-loaded frontends during backend rollout/reconnect. Preserve and emit deprecated sharingProhibited temporarily, and accept either field client-side.

  • P2 packages/typed-storage/src/index.ts:714: Rejecting . and : prevents aliasing previously valid legacy keys such as "old:key". Detect actual namespace collisions instead of rejecting delimiters universally.

github run

@Maximo-Guk

Maximo-Guk commented Sep 11, 2026

Copy link
Copy Markdown
Member Author
  • P2 packages/workshop-shared/src/api.ts:1313: Metadata rename breaks already-loaded frontends during backend rollout/reconnect. Preserve and emit deprecated sharingProhibited temporarily, and accept either field client-side.

We are ok with this risk

  • P2 packages/typed-storage/src/index.ts:714: Rejecting . and : prevents aliasing previously valid legacy keys such as "old:key". Detect actual namespace collisions instead of rejecting delimiters universally.

github run

storageName and storageKey exist only to alias keys that already exist, and no existing schema in the repo has/had a property name containing . or :

@Maximo-Guk
Maximo-Guk marked this pull request as ready for review September 11, 2026 00:17
@Maximo-Guk

Maximo-Guk commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

devin-ai-integration[bot]
devin-ai-integration Bot reviewed Sep 11, 2026
View reviewed changes

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@Maximo-Guk Maximo-Guk mentioned this pull request Sep 11, 2026
Open
@Maximo-Guk Maximo-Guk mentioned this pull request Sep 11, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments
@ask-bonk ask-bonk[bot] ask-bonk[bot] left review comments
@kentonv kentonv kentonv approved these changes

Assignees

No one assigned

Labels

gatekeeper Changes to a gatekeeper integration kernel Changes to the Workshop kernel workshop/frontend Changes to the Workshop frontend workshop/shared Changes to shared Workshop APIs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Maximo-Guk @kentonv