chore(spec): scan platform-object definitions for permissive empty states (#3896 follow-up) - #3966
Merged
Merged
Conversation
…ates (#3896) #3945 added a gate requiring any "empty = permissive" statement in the spec to be classified on purpose. It scanned packages/spec/src/**/*.zod.ts — and that scope had a hole big enough to miss the bug it was built for. The sentence that shipped #3896, "leave empty to share every record", was the `description` of `sys_sharing_rule.criteria_json`, which lives in plugin-sharing. The gate could not see its own crime scene. It now scans **/*.object.ts anywhere under packages/ — plugins, platform-objects, metadata-core, and the create-objectstack templates, since a starter file is the highest-leverage thing a model copies from. 214 → 290 files. It immediately found a real one. sys_user_permission_set.organization_id declares "NULL = applies in every org context": a user↔permission-set grant with no org scope applies everywhere. Deliberate and load-bearing rather than an oversight — ADR-0095 D3 / ADR-0068 D2 derive the platform_admin posture from an UNSCOPED admin_full_access grant specifically, and an org-scoped grant of the same set must not confer it. The empty state is not merely wider; it is the distinguishing input to the highest privilege in the system. Registered `open` with that rationale and both enforcement sites cited — the point being that the answer now lives somewhere other than a maintainer's memory. Three fixes the new surface forced, each the gate being wrong in a way that mattered: - Repudiated prose no longer fires. #3929's own comment reads Deliberately NOT "leave empty to share everything" — the gate flagged the sentence recording why the gate exists. Negation now covers the imperative form as well as the token form (deny-all), and the escape is narrow on purpose: the negator must be attached to the phrase, not merely present in the line, because a false negative here is a missed over-share. - The owning property is found by nesting, not a name list. A field's prose sits in a nested key, so the first attempt answered `description` for every platform-object hit; skipping doc slots then answered `required`, the sibling above it. What separates a field from its own config is indentation. - The property-search window is per-surface. A platform-object description: can sit 15+ lines below its field name, while a .zod.ts statement sits beside its property. Widening globally would let .zod.ts narrative be mis-attributed to a distant property — a correct note becoming a wrong failure. Evidence resolution is now honest too: entries are parsed with the liveness ledger's own checkEvidence, so prose works, several paths can be cited, and another repo's path is recorded without being resolved here. The README already promised evidence resolved "like the ledger's"; a raw-path existsSync quietly did not. 6 new unit tests (32 total). spec: 263 files / 6862 tests green, tsc clean, both gates green. No runtime behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 105 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 30, 2026 00:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3945 (merged). That PR built a gate requiring any "empty = permissive" statement in the spec to be classified on purpose. Its scan scope had a hole big enough to miss the bug it was built for.
The gate could not see its own crime scene
The sentence that shipped #3896 — "leave empty to share every record" — was the
descriptionofsys_sharing_rule.criteria_json. That field lives inplugin-sharing, not inpackages/spec. #3945 scannedpackages/spec/src/**/*.zod.tsonly.So the ratchet was real but it was pointed slightly away from where the class actually lives: platform-object field descriptions are the surface authors and models read most directly, and they sit in plugins.
Now scans
**/*.object.tsanywhere underpackages/— plugins,platform-objects,metadata-core, and thecreate-objectstacktemplates (a starter file is the highest-leverage thing a model copies from). 214 → 290 files.It immediately found a real one
sys_user_permission_set.organization_id:A user↔permission-set grant with no org scope applies everywhere. That is deliberate and load-bearing rather than an oversight, and the reason is worth stating precisely: ADR-0095 D3 / ADR-0068 D2 derive the
platform_adminposture from an unscopedadmin_full_accessuser grant specifically, and an org-scoped grant of the same set must not confer it.So the empty state here is not merely wider — it is the distinguishing input to the highest privilege in the system. Registered
openwith that rationale and both enforcement sites cited (resolveAuthzContext.hasPlatformAdminGrantas the single source of truth;explain-engine.ts, which replicates the identical predicate on purpose so the explain panel cannot sit higher than enforcement). The point is that the answer now lives somewhere other than a maintainer's memory.No behaviour is changed here — this is a classification, and the classification is that the current behaviour is intended.
Three fixes the new surface forced
Each was the gate being wrong in a way that mattered, and each was found by running it rather than by reasoning about it.
1. Repudiated prose no longer fires. #3929's own comment on
criteria_jsonreads:The gate flagged the sentence recording why the gate exists. Negation now covers the imperative form as well as the token form (
deny-all, which had the same bug in #3945 and was fixed there). The escape is deliberately narrow — the negator must be attached to the phrase, not merely present in the line, so"If not set, leave empty to share every record"is still caught. A false negative here is a missed over-share, which is strictly worse than a false positive; there's a test pinning both directions.2. The owning property is found by nesting, not by a list of key names. A platform-object field is a nested call, so the first attempt answered
descriptionfor every hit — a name that is not a property and would key every entry in a file to the same string. Adding a doc-slot skip list then answeredrequired: the sibling config key above it. What actually separates a field from its own config is indentation, so the resolver takes the nearest key at a shallower indent. Direction matters too — a JSDoc block sits at the same indent as the property below it, so forward search allows equality where backward search must not.3. The property-search window is per-surface. A platform-object
description:can sit 15+ lines below its field name; a.zod.tsstatement sits on or beside its property. Widening globally would let.zod.tsnarrative be mis-attributed to a distant property — turning a correct non-failing note into a wrong failure — so.object.tsgets a wider window and the schema surface keeps its tight one.Also: evidence resolution is now honest
Entries are parsed with the liveness ledger's own
checkEvidence, so prose around the paths works, several paths can be cited, and another repo's path (objectui: …) is recorded without being resolved here. #3945's README already promised evidence resolved "like the ledger's" — a single raw-pathexistsSyncquietly did not, and would have rejected exactly the prose-plus-paths form the ledger documents.Verification
packages/spec: 263 files, 6862 tests, all passing.tsc --noEmitclean.check:empty-statereports 20 statements across 290 files, all classified (1 closed, 2 open, 4 output, 9 scope);check:livenessunaffected.Still not in scope
translations/*.objects.generated.ts), so a permissive-empty claim could survive in a non-English bundle after the source is fixed. Those files are generated, so the right fix is at the generator, not the gate — a separate change.required(write-time) fromNOT NULL(storage), still the outstanding ergonomics item behind fix(sharing)!: a rule with no criteria shares NOTHING, not every record (#3896) #3929's imperative write guards.Generated by Claude Code