Repository navigation
test(docs): narrow field-and-enum coverage guard for schema and docs (#247) - #365
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
Verdict: Request changes. One blocking finding. Everything below it is a note, including one I would have blocked on and should not. I am not approving and not merging.
Reviewed at d087d998, base cd92e16. Read first: the 2 September comment that added item 5 to #247, your reply the same day, the 11 September ruling, and #364 and #249, which the body cites. #364 touches spec/trace-v0.2.md, CHANGELOG.md and docs/glossary.md; the third is inside this guard's corpus and only adds tokens, so nothing here moves.
The guard is the narrow one that was ruled, both gaps it reports finding are real, the counterfactual is run in both directions rather than asserted, and $defs names are correctly kept out of the field set.
A doc entry can be removed without the guard going red, for 26 of the 60 field names
undocumented() tests f[1] not in tokens, where tokens is one bare-name set, built from all 28 corpus files for the corpus-wide test and from docs/schema.md alone for the page-level one. A member counts as documented while its name appears inside backticks anywhere: another table, a sentence, a heading, a different object, a different file.
Swept. For each of the 60 distinct field names I deleted every row of docs/schema.md whose first cell is that name and ran the whole suite. The guard stays green for 26 of them. For five of those, builder, id, references, rel and resolver, another test catches it, so the whole suite stays at 1665 passed, 1 skipped for 21:
appraisal, build_provenance, cnf, crv, data_class, delegation, e, iat, jwk, kty, model, n, origin, parent_record_hash, policy, runtime, signature, tool_transcript, transparency, x, y
The sharpest case is this PR's own new table. Delete six of the seven rows of the cnf.jwk member table, kty, crv, x, y, n, e, and the suite is 1665 passed, 1 skipped; only kid reds. All six tokens come from one sentence, docs/schema.md:195, which this PR adds directly above the table: "kty is required and decides which key-material members are: OKP keys carry crv and x, EC keys crv, x and y, RSA keys n and e." `kty` occurs in exactly two places in the whole corpus, that sentence and the row it makes redundant, and the other five appear in no corpus file outside docs/schema.md. So the paragraph you wrote to explain the table is what stops the table being guarded.
The enum surface is better than the field surface, and I should say so rather than leave the impression I first gave. Swept all 27 pairs, stripping each value from the description cell of its own field's row: 7 stay green, the three depth values on both build_provenance.provenance_depth and appraisal.provenance_depth_verified, and origin.kind: self. Six of those seven survive because the value really is documented elsewhere: docs/verification.md:114 has a table row keyed on `builder` as a depth, :152 and :154 are the depth-floor table, and docs/build-provenance-depth.md:7 names all three. self comes from docs/schema.md:125 in the same section. That is the corpus doing what it should. The page-level test is fooled for a narrower reason, and I checked it rather than assuming: docs/schema.md:159 and :173 each enumerate all three depth values in their own description cell, so stripping one row's leaves the other's. The unrelated build_provenance.builder row at :156 is a third copy and is not the mechanism; deleting it as well leaves all six green.
One shape the name sweep structurally cannot show, because it deletes every row for a name at once. version has a row at docs/schema.md:46 for model and one at :74 for policy. Deleting either one alone leaves the suite at 1665 passed, 1 skipped; only deleting both reds. So a field can lose its documentation entirely and stay covered by an unrelated field two sections away that happens to share its name.
Whose sentence this is, and how many surfaces carry it. The claim I am measuring against is "removing a doc entry reds the guard". The body's first line carries it unqualified and is a verbatim quotation of the 11 September ruling, so on that surface it is @imran-siddique's sentence before it is yours. Two more carry it unqualified and both are inside this diff: the module docstring, "The counterfactual that matters is: remove a doc entry, or add a schema field, and this file goes red", and the test name test_the_guard_fails_when_a_doc_entry_is_removed. A sixth is yours and earlier than the ruling: your #247 comment of 7 September, the one the ruling adopted, says "remove a doc entry for a field or an enum value, the guard reds", which is where the ruling's wording came from. Two qualify it: your CHANGELOG.md entry writes the domain in (a doc entry removed for kid or declared), and your 16 September announcement on #247 does not make the claim. So four of six are unqualified, and the origin is yours rather than the ruling's. The docstring is the sentence a future reader will cite, so it is the one that most needs the domain.
The next PR is the demonstration, and it is not one I constructed. #364's schema follow-up will add 15 field names and 4 enum values, derived from its §3.1.4 tables since the schema is not written yet. Three of the fifteen are input_closure's entry members, id, digest and resolver, because the section reuses §3.1.2's shape. All three are already inline-code tokens in docs/schema.md itself, at :138, :139 and :141 in the references table and :157 for build_provenance.digest. So both tests stay green for every member of the closure the moment the schema lands, with nothing written about the closure at all. method and reason are already corpus tokens too, on two crosswalk pages, as a CHAP JSON-RPC method and an ACTA receipt field. And #364's glossary hunk makes reproducibility, transcript_digest, reproduced, diverged and not-attempted corpus tokens the day it merges. Ten of the nineteen pass the corpus-wide test on arrival; the page-level test reds for sixteen and never for the three closure members. The position-keyed version below catches all nineteen.
The remedy is constructible, it is in this repository twice, and your own failure message already states it. Of the 39 names that red, five red only on tests/test_references_block.py and tests/test_build_provenance_depth_doc.py. Those two parse a named table and ask whether the row exists under the right heading: they key on position. And _report already tells a failing contributor to "Document each one where a producer will look for it (a table row in docs/schema.md for a field, the field's row for an enum value)", which is the position rule stated by the test itself and not enforced by its assertion.
Keying on the dotted path does not work, and I checked before proposing anything: of the 46 nested fields, 21 appear in the corpus as a dotted token and 25 do not. Keying on position does. I built it to be sure rather than to be right: map each path to its section in docs/schema.md, require the field's name as the first cell of a row in that section, and require each enum value as an inline-code span in the description cell of its own field's row. On d087d998 that gives 0 fields and 0 enum values missing, and on cd92e16 it gives exactly the seven cnf.jwk fields and the two enforcement_mode values the PR reports finding. It is a change to the test and to nothing else. One thing it needs that the current guard does not: the path-to-section map is hand-maintained, so a new nested object with no section mapped wants its own floor.
Notes
The traversal the walker implements is nine tenths redundant, and I nearly blocked on it, which would have been wrong. Measured, by disabling each keyword in turn: dropping items loses 5 fields, and dropping any of additionalProperties, if, then, else, not, allOf, anyOf, oneOf or $defs loses nothing. Reduced to _SUBSCHEMA_KEYS = ("items",) and _SUBSCHEMA_LISTS = (), the walker still reaches 62 fields and 27 enum values and the suite gives 1665 passed, 1 skipped.
Redundant rather than dead, and the distinction decides what you write in the comment. else and oneOf have no site in the schema at all. anyOf, not, $defs and additionalProperties have sites and reach nothing. allOf, if and then do traverse live structure: the top-level allOf[0] conditions on origin and re-declares runtime.platform, and cnf.jwk carries three more. They lose nothing when dropped because everything they reach is reached through properties as well. (additionalProperties is in the code and not in the body's list of nine.)
What makes this a note rather than a blocker is that the floors do catch the regression that would cost something: break the items descent and fields drop to 57, below the floor of 60. The ("references[]", "rel") pin would fail too, though the floor assertion aborts the test four lines earlier, so it is never reached. The nine dead keywords are forward-looking scaffolding that costs nothing today. The one thing that is actually false is the inline comment at lines 185 to 186, which says the five pinned assertions cover "a top-level field, a nested object member, an array item member, a conditional branch, and a nested enum". No assertion pins a conditional branch, because on this schema nothing is reachable only through one: every cnf.jwk member is declared under properties, and the if/then blocks only re-declare kty and list required. The assertion the comment lines up with the conditional branch is ("policy.enforcement_mode", "declared") in enums, which is a nested enum. One sentence in that comment saying the conditional traversal reaches nothing properties does not already reach, and it is closed. Not "reaches nothing": it reaches origin, origin.kind, runtime, runtime.platform and two origin.kind values, and I would rather not send you a wording your own schema contradicts.
advisory is attributed to a section that does not define it, and the nearer source does. The row ends "Section 4.3 of the spec defines the four". Section 4.3 names enforce and silent in its bullet and declared in its own paragraph, opening it with "The three modes above" and presupposing a third it never states. advisory occurs twice in spec/trace-v0.2.md, at :411 and :503, in 3.3.2 and 3.3.4, neither as an enforcement mode. The wording you used is close to schema/trace-claim.json's own description of policy.enforcement_mode, which does gloss all four, and I nearly told you to attribute it there. That would have been wrong, and wrong against the ruling this PR implements: "schema/trace-claim.json is authoritative for what validates, and for nothing else ... It does not define meaning, and a schema description is not a normative statement." So the row should attribute the four to nothing: say that §4.3 states enforce, silent and declared, and name the schema description as where the wording came from if you want it, as provenance rather than authority. Three surfaces carry the §4.3 attribution: the row, CHANGELOG.md, and the PR body, which needs an exact find-and-replace from you since I cannot edit it. The underlying gap, a value the schema accepts and no normative surface states, is #247's item 2 and I will file it there rather than add it to your list.
"A key with no material is refused" is narrower in the schema than in the sentence. Validated with examples/amd-sev-snp.json as the carrier and only cnf replaced: {"kty": "OKP"} is refused with 'crv' is a required property, and {"kty": "oct"} and {"kty": "FOO"} are both accepted. The $comment on that object says why, and it is deliberate.
Not blocking, for a reason worth stating: the schema's own description, on main, is "Keys must carry actual key material: OKP keys require crv and x; EC keys require crv, x, and y; RSA keys require n and e." Same universal sentence, same enumeration under it. You paraphrased the authoritative surface. models.py:330 agrees with the schema (required_by_kty.get(self.kty, ())), and sign.py:422 refuses any kty that is not OKP outright, which is the $comment's "refused later by the verifier". The sentence is operationally true end to end, and a clause naming where the refusal happens would make the page exact.
Four const values are outside the guard entirely. _walk collects enum and not const. OKP, EC and RSA are const in the three if branches of cnf.jwk, and eat_profile's profile URI is a const. None is in the set the guard checks; software-only is, because it is also an enum elsewhere. Moving a value from enum to const removes it from coverage silently.
The enum floor cannot distinguish the two things it is for. 62 fields against a floor of 60 has two of slack. 27 enum values against a floor of 27 has none, so a legitimate removal fails with only 26 enum values found, under a comment saying a drop below the floor "is a bug in the walker, not in the schema".
The corpus-wide test has no independent failure mode, and both counterfactual cases are page-level gaps. documented_tokens(docs/schema.md) is a subset of documented_tokens(corpus), 119 tokens against 384, necessarily, because the page is in the corpus. So test_every_schema_field_and_enum_value_is_documented can never be the only failure. Related, and measured on the base commit: the corpus-wide check found 6 missing fields there, the six cnf.jwk members, while the page-level check found 7 fields and 2 enum values. kid was already an inline-code token four times across three files, docs/crosswalks/acta-decision-receipts.md:40 and :85, docs/verification.md:67 and docs/integration/pic-trace-bridge-v1.md:12, and declared in docs/verification-outcome-statements.md, so both members the parametrize strips were page-level gaps rather than corpus-level ones. The parametrize itself strips from _corpus(), across all 28 files, which is a heavier mutation than "a doc entry is removed". None of that contradicts the body, which scopes both gaps to docs/schema.md in its own lead-in and is right there.
_FENCE matches backtick fences only, and docs/integration/pic-trace-bridge-v1.md:70 opens a ~~~json block. Measured rather than assumed: stripping ~~~ blocks as well changes no member in either corpus today, because a JSON example carries no backtick spans. I also checked the loud failure, a fence opened in one file and closed in the next once the corpus is concatenated: all 28 files have an even number of ``` lines.
The CHANGELOG entry does not close with "Informative only: no schema, wire-format or normative change.", which the two entries directly below it carry and which matches this PR's own type-of-change box. Two entries is not a convention, so take it or leave it.
Checked and clean
Both schema copies are byte-identical, so reading one and claiming both is sound. The walker reaches 62 fields and 27 enum values and the five pinned paths all resolve. canonicalizableValue is correctly absent from the name set, and that assertion has a positive control: making _walk add $defs keys to fields fires it. The docs/rfcs/ exclusion matches what those three files say about themselves. The inline-code rule does the work claimed for it: replacing every `none` span reds the guard on appraisal.status: none while the English word stays in the prose. Both gaps are real on cd92e16. ruff, mypy --strict on the new file and tools/check_dashes.py are clean, and pytest gives 1665 passed with exactly 10 new node ids. The one red check is gate: I read .github/workflows/require-maintainer-approval.yml and it fails unless a maintainer's latest non-commented review is an approval at the current head sha, so it is waiting rather than reporting a defect.
Not approving, on the first section. The guard is worth having and I would like it to land. What I do not want is for it to be cited later as evidence that a doc entry cannot be removed silently, when for 26 of the 60 field names it can, and the position-keyed version passes today with no further writing.
… bare name (agentrust-io#247) A field is documented by a row whose first cell is its name in the section of docs/schema.md for its containing object; an enum or const value by an inline-code span in that row's description cell. The bare-name rule let 26 of 62 rows be deleted while the name survived in a sentence, a heading or a same-named field elsewhere on the page. - const values (cnf.jwk.kty OKP/EC/RSA, eat_profile) are in scope - the path-to-section map is hand-maintained and has its own floor - both fence styles are stripped before tables are parsed - floors have slack; counterfactuals delete a single row (kid, kty, each of the two version rows, appraisal) or strip a value from its own row while it survives elsewhere on the page - the corpus-wide check is dropped: its tokens are a superset of the page's, so it could never be the only failure - docs/schema.md: advisory is attributed to the schema description, section 4.3 defines enforce, silent and declared; a listed key type with no material is refused by the schema, other kty values by the verifier Review: agentrust-io#365 Signed-off-by: chernistry <sanderchernitsky@gmail.com>
|
Agreed on the blocking finding, and thank you for verifying the remedy before proposing it. The design. Each schema path maps to its Doc-side counterfactuals are now single-row deletions, each leaving the name elsewhere on the page where it can: The notes, one by one.
Full suite 1672 passed, ruff and |
|
One more commit, |
lywinged
left a comment
There was a problem hiding this comment.
Re-measured on f47c92c. The blocking finding is closed, and the same sweep on values, which the finding did not reach, is closed with it.
| before | now | |
|---|---|---|
| delete every row for a field name, guard stays green | 26 of 60 | 0 of 60 |
| delete a field's own row, guard stays green | 30 of 62 | 0 of 62 |
| strip a value from its own field's row, guard stays green | 7 of 27 | 0 of 31 |
The design holds up under the questions I would ask of any hand-maintained map. Twelve paths, twelve distinct headings, so no two objects share a table. An object with no section is reported as undocumented rather than skipped, which I checked by adding one: it names the object and its member. A row moved to the wrong section is reported. The floors have slack now, 55 and 26 against 62 and 31, so the enum floor no longer fails on a legitimate removal. On main's page the guard reports exactly the seven cnf.jwk fields and five values, which is the gap the guard found on arrival plus the three kty consts, now that const is walked.
Two things beyond the finding. Bringing const into scope, which was a note, closes the hole where moving a value from enum to const would drop it silently, and eat_profile with the three kty consts are the four values between 27 and 31. And _rows splitting on unescaped pipes only, which was yours, is hardening on a row that does not exist yet, with a unit test that says so. I checked the direction: on b9d8313 the split kept only the last cell, so v1 in one of `v1` \| `v2` would have been reported as undocumented and the guard would have gone red, a false failure rather than a vacuous pass, which agrees with your "red rather than vacuous".
One thing left, two digits in the body, and not a reason to hold this. The body says 1672 passed, 1 skipped (17 in this file). On f47c92c it is 1673 and 18, the escaped-pipe test being the eighteenth.
That closes it from my side.
…gentrust-io#247) Every property name and every enum value in schema/trace-claim.json must appear as an inline-code token under docs/ (rfcs excluded) and in docs/schema.md itself. Checked directly against the two hand-maintained surfaces; nothing is generated from the schema, since generated text cannot drift from its source and a test over it would exercise the generator rather than the agreement between two surfaces. The guard is shown to fail in both directions: a field or enum value added to the schema without a doc entry, and a doc entry removed for `kid` or `declared`, the two members agentrust-io#247 found missing. On arrival it found docs/schema.md listing `cnf` with no member table, so `kty`, `crv`, `x`, `y`, `n`, `e` and `kid` appeared only inside a fenced example, and describing `policy.enforcement_mode` as "`enforce` or `silent` (advisory)", naming neither `advisory` as a value nor `declared`. Both corrected. Narrow by design: presence is mechanically decidable, description agreement is not, and the precedence rule is separate normative text. Refs agentrust-io#247. Signed-off-by: chernistry <sanderchernitsky@gmail.com>
… bare name (agentrust-io#247) A field is documented by a row whose first cell is its name in the section of docs/schema.md for its containing object; an enum or const value by an inline-code span in that row's description cell. The bare-name rule let 26 of 62 rows be deleted while the name survived in a sentence, a heading or a same-named field elsewhere on the page. - const values (cnf.jwk.kty OKP/EC/RSA, eat_profile) are in scope - the path-to-section map is hand-maintained and has its own floor - both fence styles are stripped before tables are parsed - floors have slack; counterfactuals delete a single row (kid, kty, each of the two version rows, appraisal) or strip a value from its own row while it survives elsewhere on the page - the corpus-wide check is dropped: its tokens are a superset of the page's, so it could never be the only failure - docs/schema.md: advisory is attributed to the schema description, section 4.3 defines enforce, silent and declared; a listed key type with no material is refused by the schema, other kty values by the verifier Review: agentrust-io#365 Signed-off-by: chernistry <sanderchernitsky@gmail.com>
A description cell that lists alternatives as `a` \| `b` is one cell. No such row exists today; the guard would have gone red, not vacuous, on the first one. Signed-off-by: chernistry <sanderchernitsky@gmail.com>
|
Rebased onto On the rebased branch: 1673 passed, |
f47c92c to
e3a32d9
Compare
Signed-off-by: Imran Siddique <imran.siddique@opaque.co> # Conflicts: # CHANGELOG.md
imran-siddique
left a comment
There was a problem hiding this comment.
Approving at the merge commit I just pushed. The guard is the narrow one ruled on #247 and it holds: 18 guard tests pass, and deleting the iat row from docs/schema.md fails 11 of them. @lywinged's sweep closed at 0 of 60, 0 of 62 and 0 of 31.
The second CHANGELOG.md conflict was ours, not yours: merging #347 added an entry at the same point after your rebase. I resolved it in the branch keeping both entries, so you are not paying for it twice.
What this changes
The narrow guard ruled on #247 (11 September): every field and every fixed value in the schema must be documented on the schema reference page, checked directly, and removing the entry that documents it reds the guard. No generation.
tests/test_schema_doc_coverage.pywalksschema/trace-claim.json(everypropertieskey, everyenumandconstvalue, throughitemsand theallOf/if/thenbranches) and holds each againstdocs/schema.mdby position:constvalue is documented by an inline-code span in the description cell of its own field's row.Fenced examples, in either fence style, are stripped before tables are parsed, so a pasted record does not stand in for a row. The bare name is not enough:
versionundermodeland underpolicyare two rows, and the sevencnf.jwkmembers named in the sentence above their table still each need their row.The guard found these gaps on arrival, all in
docs/schema.md, all fixed here:cnfwas a one-row table with no member table under it, sokty,crv,x,y,n,eandkidappeared only inside a fenced example. Acnf.jwkmember table is added, with what eachktyrequires (the schema refuses a listed type with no material; aktyoutside the three passes the schema and is refused by the verifier, which acceptsOKPonly) and the private-key refusal from 0.10.0.policy.enforcement_moderead "enforceorsilent(advisory)", naming neitheradvisoryas a value nordeclaredat all. The row now lists the four values. Section 4.3 definesenforce,silentanddeclared;advisoryis in the schema's closed set with a one-line description and no spec text, and the row says so rather than attributing it to 4.3.The counterfactual, run rather than asserted
Per the standing requirement from #247 (item 5): a drift test arrives with evidence that it fails when the drift is introduced.
nonexistent_audit_tokenadded underappraisal;nonexistent-outcomeappended toappraisal.status;data_classturned into aconstnobody documented; a new nested object with no section mapped. Each reds the guard naming exactly that member.kid;kty(its name survives in the sentence above the table, and the row's threeconstvalues go missing with it);model.versionalone, thenpolicy.versionalone; the top-levelappraisalrow while theappraisalheading and section stay. One value stripped from its own row:declared;builderfromprovenance_depth_verifiedwhile the other depth row still names it. Each reds the guard naming exactly that member, and in each case exceptkidanddeclaredthe name is still on the page, which is what a bare-name rule would have accepted.constdeclared only inside a conditional branch.Not in scope, deliberately
Whether a description agrees with the spec, or whether an example is correct. Neither is mechanically decidable, and a guard that reaches past what it can decide produces failures nobody can act on. The wider
docs/corpus is not consulted: a mention on a tutorial page is not documentation of a field, and a corpus-wide token check is a strict subset of the page check, so it could never be the only failure. The precedence rule is separate normative text, which the ruling assigns to the maintainer.Checks
pytest: 1672 passed, 1 skipped (17 in this file)ruff check src tests: cleanmypy --strict tests/test_schema_doc_coverage.py: cleanpython tools/check_dashes.py: cleanType of change
Refs #247. Counts as one of the three items from the 13 September check-in on #249; the spec PR is #364.