fix(gate-54): a relation nested in an array-of-objects is still a property - #241
Closed
rubenvdlinde wants to merge 1 commit into
Closed
fix(gate-54): a relation nested in an array-of-objects is still a property#241rubenvdlinde wants to merge 1 commit into
rubenvdlinde wants to merge 1 commit into
Conversation
…perty
Check (c) reported a CORRECTLY shaped relation as "placed off a property"
whenever it sat inside an array of objects. Observed on larpingapp:
character.skillOverrides.items.properties.skill
{ "type": "string", "format": "uuid", "$ref": "skill",
"x-relation-filter": { "setting": "@object.setting" } }
That is byte-for-byte the shape the gate accepts one level up on
character.skills. The only difference is nesting depth.
Cause: check (c) works by identity. _raw_walk() flags every x-relation-filter
whose containing dict is not in property_ids, and property_ids was built from
schema["properties"] in a single non-recursive pass. A nested property could
therefore never be in the set, so the finding was unconditional — and
unfixable in the app: moving the filter off the property IS the rule-6
violation, flattening the array is a schema redesign to satisfy a linter, and
deleting the filter loses the setting-scoped picker.
_collect_nested_property_ids() registers property dicts below a top-level
property, through items.properties and inline properties, bounded at depth 8
(a register file is JSON so it cannot cycle; the hazard is pathological depth,
and past the bound the walk over-reports rather than under-reports).
test_check_relation_dialect.py is new — the helper had no suite, so
run-helper-suites.sh will now execute one. Every accepted case is paired with
an arm that must still be REPORTED, so the suite cannot pass against a checker
that returns nothing:
1 top-level relation accepted (harness positive control)
2 relation in items.properties accepted (the regression)
3 filter on a non-property items node STILL REPORTED
4 filter in a schema-level x-* block STILL REPORTED
5 relation two array levels deep accepted
6 pathological depth terminates and over-reports
Can-fail proof: removing the _collect_nested_property_ids() call turns cases 2
and 5 red with the exact 'placed off a property' text; restoring it returns
the suite to 6 passed, 0 failed.
Measured on real trees — larpingapp full-tree gate-54 drops from 4 findings to
3, and its own lib/Settings/larpingapp_register.json goes clean; the remaining
findings are genuine ones in register.d/ fragments.
Closes #231
Contributor
Author
|
Superseded by #255. This branch fixed only the false-positive half: it added nested property ids to #255 uses one recursive collector that feeds all the checks, keeps this branch's depth-cap fail-safe assertion (carried over as a test), and ships 24 mutation-checked tests. |
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
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.
Closes #231.
gate-54 check (c) reported a correctly shaped relation as "placed off a property" whenever it sat inside an array of objects. The finding was unfixable in the app — every way to clear it damages correct schema — so it sat as permanent red on any repo with that (common) shape.
The finding
ConductionNL/larpingapp@development,character.skillOverrides.items.properties.skill:{ "type": "string", "format": "uuid", "$ref": "skill", "x-relation-filter": { "setting": "@object.setting" }, "title": "Skill" }Byte-for-byte the shape the gate accepts twenty lines above on
character.skills. The only difference is nesting depth.Cause
Check (c) works by identity.
_raw_walk()flags everyx-relation-filterwhose containing dict is not inproperty_ids, andproperty_idswas built in a single non-recursive pass:Nothing descended into
properties.<p>.items.properties.*, so a nested property could never be in the set and the finding was unconditional.Fix
_collect_nested_property_ids()registers property dicts below a top-level property, throughitems.propertiesand inlineproperties, bounded at depth 8. A register file is JSON so it cannot contain a cycle; the realistic hazard is pathological depth, and past the bound the walk over-reports rather than under-reports — the safe direction.New test suite
scripts/lib/test_check_relation_dialect.py. The helper had no suite, sotests/run-helper-suites.sh(which globsscripts/lib/test_*) will now execute one.Every accepted case is paired with an arm that must still be reported, so the suite cannot pass against a checker that returns nothing — the failure mode a checker test most easily falls into:
items.propertiesitemsnodex-*blockCan-fail proof. Removing the
_collect_nested_property_ids()call:Restoring it returns 6 passed, 0 failed.
Measured on real trees, not just fixtures
larpingapp full-tree gate-54: 4 findings → 3, and its own
lib/Settings/larpingapp_register.jsongoes clean. The three that remain are genuine rule-7 findings inregister.d/fragments, two of which are fixed in ConductionNL/larpingapp#289.One thing worth knowing about rule 7 while you are here
Applying rule 7's remediation (add the canonical
$ref) tocharacter.ownerRefin larpingapp broke the character create dialog:character.ocNamealready declares$ref: "player", and with a second player-targeted relation on the same schema the dialog stopped rendering a player selector entirely. Reproduced on two independent CI runs (detail-forms-admin.spec.ts:570, 1 failed / 170 passed) and green ondevelopment. Not addressed here — flagging it because rule 7 currently reads as unconditionally safe and is not.Also noted while measuring, filed separately: #230 (gate-58 matches comment lines), #232 (gate-38 scopes
ISettingsfragments), #233 (gate-60 reports 43 confident FAILs whennode_modulesis absent), #238 (gate-53 never joinssrc/registry.js).