test(schema): probe every pattern constraint, and hold the mirroring models.py claims - #270
Conversation
…models.py claims The matrix in this file answers whether the two validators disagree about a list of values. It cannot answer whether each constraint is discriminated at all, and two gaps followed from that, neither of which produced a failing test. Half the pattern constraints were unreachable. BASE carries five of the schema's ten; model.weights_digest, delegation.parent_record_hash, references[].retention, references[].digest and signature are optional, absent, and so never mutated. A sweep cannot disagree about a field it never sets. The mirroring was claimed and not held. models.py says its pattern constants are 'mirrored verbatim in schema/trace-claim.json and its copy, and held there by tests/test_the_schema_and_the_models_agree.py'. Two of the ten were pinned, in test_references_block.py; the other eight were maintained by hand under a comment naming this file. The generator reads the ten patterns out of the schema rather than listing them, probes each at its own boundary with values derived from the pattern and from a valid instance, and demands one of three outcomes per constraint: the two artifacts hold the same pattern string, so no string can split them and that is a proof rather than an observation; or a splitting value exists and is declared in DECLARED_DIVERGENCES; or it is neither, which fails. UNMIRRORED_AND_UNSPLIT is the two-sided declaration for the third case and is empty. Counterfactuals, each verified to fail: dropping signature from the fixture; changing _DIGEST_RE so it no longer mirrors the schema; a stale UNMIRRORED_AND_UNSPLIT entry; and restoring the pre-agentrust-io#244 prefix pattern on subject, which the generator splits on 'spiffe://' -- a value nobody wrote into this file, and the class the matrix could not reach because every value in it fails a prefix test as well.
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
Approve. This is the generator from #247, and it does what was asked: it turns the subject finding into a property every pattern constraint is held to, rather than a second fixed matrix.
I ran it against the two regressions it claims to catch, on both sides of the split, since a discrimination test is only worth its own counterfactual.
- Put the pre-#244 prefix pattern back on
subjectin the schema:test_every_pattern_is_mirrored_or_split_or_declaredfails and namesspiffe://anddid:. The matrix carries those two literals today, but only because they were hand-added as the guard after #244 exposed the miss; the generator derives them from the pattern, so the same prefix shape on a field nobody has patched is caught without anyone thinking to list it. - Drop the
signaturepattern from the model, a field the old sweep never touched because it is optional and absent fromBASE: the same test fails and names the splitting probes. So the mirroring check has teeth on the five optional pattern fieldsFULLreaches thatBASEdid not (weights_digest,parent_record_hash,references[].digest,references[].retention,signature), which is the coverage that did not exist before this.
Two things I checked that hold: the ten patterns are read out of the schema, so a new one cannot be added without this file failing until it is accounted for; and the three-indirection walk in _model_pattern_strings is load-bearing, signature's pattern is found only by the full walk and drops out of a shallow one, which is the miss the docstring describes.
Two non-blocking notes, neither a reason to hold it:
- The mirror match at
if pattern in model_patternsis by string against a flat set, not by field. It is sound today because every field carrying the sharedsha...digest pattern is genuinely constrained by it in the model, which I checked by injecting a malformed digest at each. It would stop being sound if a later field took that pattern in the schema and was left unconstrained in the model: the string would still be in the set and the field would read as mirrored. Matching the pattern to the field's own model constraint would close that, whenever it is convenient rather than now. - A pattern on a field absent from
FULLmakes_splittersraiseKeyErrorfrom_get(FULL, path)rather than the clean messagetest_the_fixture_reaches_every_pattern_in_the_schemais written to give. The reachability test goes red in the same run, so nothing slips, but the message a maintainer sees is the raw exception rather than the field name.
Suite is green here and the change is one file over the current head.
|
Thanks for running both counterfactuals rather than reading the diff — the |
The mirror check read every pattern the model carries into one flat set and asked whether a schema pattern's string was a member of it. Two fields sharing a pattern string is not the same fact as one field's schema constraint mirroring that same field's model constraint, and the flat set could not tell the two apart: a pattern reassigned onto an unrelated field would still read as mirrored as long as some other field carried the same string. _model_field_patterns keys each pattern to the specific field it constrains, and test_every_pattern_is_mirrored_or_split_or_declared now compares a schema path against that same path's own model pattern instead of set membership. test_the_mirror_check_is_by_field_not_flat_membership covers the case a flat set would miss: a pattern string genuinely used elsewhere in the model, reassigned onto a field it does not constrain. Signed-off-by: chernistry <sanderchernitsky@gmail.com>
A pattern constraint on a field FULL omits made _splitters raise a bare KeyError out of _get, since it read the path unguarded. test_the_fixture_reaches_every_pattern_in_the_schema already catches the same reachability gap cleanly and by field name via _reachable; this was the other place reading an arbitrary schema path out of FULL, and it failed worse when it hit the same gap. _splitters now checks reachability first and raises LookupError naming the field, and test_every_pattern_is_mirrored_or_split_or_declared catches that and reports it alongside every other unaccounted pattern instead of aborting the run. test_a_missing_path_fails_with_the_field_name_not_a_keyerror covers it directly. Signed-off-by: chernistry <sanderchernitsky@gmail.com>
|
Both notes are folded in.
Each commit adds a test that fails for the stated reason on the prior commit and passes after it; full suite, ruff, and mypy are clean on the branch. |
imran-siddique
left a comment
There was a problem hiding this comment.
Merging. This is the sharper half of #247 and it is not the half I filed.
I opened #247 on the premise that four surfaces state the rules and nothing says which wins. @lywinged's #248 then audited all ten schema patterns against the model and found one divergence, which I recorded as narrowing the issue. You have found the reason that audit could not have said what I took it to say:
A sweep cannot disagree about a field it never sets, and a green run says the same thing either way.
Five of the ten patterns are on optional fields absent from BASE, so they were never mutated and never reached their own boundary. The test answered "do these two validators disagree about the values I tried" and I read it as answering "are these ten constraints consistent". Those are different questions and only one of them was being asked.
That is the same defect class the test exists to catch, one level up: something that looks like coverage, reports green, and is not discriminating. A test that cannot fail is worse than an absent one, because the absent one does not stop anyone looking.
Probing each constraint's own boundary is the right fix, and holding models.py's mirroring claims rather than trusting the comment that says they mirror is the other half. A constant asserting it is "mirrored verbatim" is a claim, and until something compares the two it is an unverified one.
Merging, and I am updating #247 with what this changes about the issue.
|
Thanks for the writeup, and for the extra scrutiny on this one — the sweep-vs-audit distinction (a test that can't disagree on a field it never sets vs. one that probes each field's own boundary) is a good general lesson to keep in mind past this specific fix. |
Closes the generator half of #247.
The matrix in
test_the_schema_and_the_models_agree.pyanswers "do the two validators disagree about any of these values". It cannot answer "is each constraint discriminated at all" — whether a value reaches the constraint's own boundary, or fails so early that both validators reject for an unrelated reason and agree by accident. Two gaps followed from that, and neither produced a failing test.Half the pattern constraints were unreachable.
BASEcarries five of the schema's ten.model.weights_digest,delegation.parent_record_hash,references[].retention,references[].digestandsignatureare optional, absent from the fixture, and therefore never mutated. A sweep cannot disagree about a field it never sets, and a green run says the same thing either way.The mirroring was claimed and not held.
models.pysays its pattern constants are "mirrored verbatim inschema/trace-claim.jsonand its copy, and held there bytests/test_the_schema_and_the_models_agree.py". They were not held here. Two of the ten are pinned intest_references_block.py; the other eight were maintained by hand, under a comment naming this file.What it does
_pattern_constraints()reads the ten patterns out of the schema rather than listing them, so a pattern added to the published artifact cannot be added without appearing here.FULLextendsBASEso every one of those ten paths exists._probes(pattern, valid)derives boundary values from the pattern text and from a valid instance of it — structural prefixes of the valid value, literal runs taken out of the pattern, case flip, one character shorter and longer, one character outside the class, and the empty/space/tab values a producer sends for an unset field.Then, per constraint, one of three outcomes and nothing silent:
DECLARED_DIVERGENCESwith its reason, not into a second exemption list.models.pyalready claims is impossible, and the one that produced schema: the subject pattern accepts identities the reference model refuses #244. It fails unless a human writes down why, inUNMIRRORED_AND_UNSPLIT, which is two-sided the same way: an entry that becomes mirrored has to leave the set.On the three steers
DECLARED_DIVERGENCESis reused, not duplicated. No new entries: the generator finds no split against the current artifacts.UNMIRRORED_AND_UNSPLITships empty and exists so the third case cannot pass in silence. I read the computed_URI_FORMAT_ENFORCEDexemption before touching the assertions it feeds —format: uriis not apattern, so it is outside the ten and the new tests do not reach it.tests/test_validate.py:142already pins the two schema copies against each other, so that part of Four surfaces state the rules and nothing says which wins: schema, reference model, normative spec and docs disagree #247's third bullet is covered. What is not covered is a producer's claim against a published version, which is the precedence text rather than a test.Verification
1027 passed, 1 skipped;ruff check src tests scriptsclean;mypyclean on the file.Each new test was verified to fail when the thing it guards is broken, rather than only to pass today:
signaturefromFULL_DIGEST_REchanged so it no longer mirrors the schemaUNMIRRORED_AND_UNSPLITentrysubjectrestored to the pre-#244 prefix pattern^(spiffe://|did:)spiffe://That last row is the one worth reading.
spiffe://is not written anywhere in this file — it comes out of the pattern's own literal runs — and it is the class the hand-written matrix could not reach, because every value in it fails a prefix test as well, so both validators rejected and agreed.The preimage bytes for #245 are posted on that issue.