Rename prohibitAllSharing -> containsRestrictedData - #381
Conversation
github-actions
Bot
added
workshop/frontend
Preview:
|
|
@Maximo-Guk Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
325daa6 to
dbddb79
Compare
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 2 actionable inline findings. |
dbddb79 to
7b44daa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 1 actionable inline finding. |
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.
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>
7b44daa to
a8442aa
Compare
|
We are ok with this risk
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 : |
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Upstream sync plus two fork fix commits. `pnpm fork:sync --verify` is clean on the merge. ## Upstream (foundation/main → 5af356e, 11 commits) - **cloudflare#464 / cloudflare#473 — connect handoff**: adopted wholesale, with the fork's initiator guards kept in front. Completion tickets are bound to the initiating browser and redeemed from the popup; BroadcastChannel transport dropped. - **cloudflare#381 / cloudflare#382 / cloudflare#308 — restricted data**: observer-verification model adopted; the fork's owner-only tier is retained alongside it. Share modal stays usable for restricted workspaces. - **cloudflare#460 — gatekeeper-kit overhaul**: replayable credential runs, declared action fences, conformance consumer. Reviewed against gatekeeper-ai-executor: no wiring needed (it uses zero kit modules; no provider credentials, single action kind, strategy-A observers). - **cloudflare#450 — multi-tab Google Docs**; **cloudflare#433 — xlsx export** in workspace-sheets; **cloudflare#478** integration test; **cloudflare#470** vitest bump; **cloudflare#454** sidebar CSS. ## Fork fixes (config fallout from ae28b29) - `b23db94d`: manifest-lib reads and drops `limits`/`placement` (no v1-contract field; golden regenerated); staging strips dev-only `browser.remote`; integration harness drops backend assets for test boots (inline configs resolve `assets.directory` against the harness root). - `a8815836`: removes `browser.remote` (verified dev-only in wrangler's own source; prod Browser Rendering comes from the binding, now pinned in the deploy-contract test); run-dev-server drops backend assets in normal dev mode (run-local unchanged). ## Verification - scripts 378/378, backend 881, integration 82/82, frontend 474, ai-executor 57+6 — all green, no workarounds. - `types:scripts`, package `tsc`, lint (0 errors), `fork:sync --verify` clean. ## Open (not in this PR) - Context `artifacts` prod namespace flows verbatim into preview configs — needs a product call (cut vs per-preview namespace). - Customer manifests now ship frontend assets on the backend too (consequence of the fork's backend-assets decision).
2 participants
Rename
prohibitAllSharingtocontainsRestrictedData, 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