soa_config: reject unknown keys; correct the classid docs (codex P1-2 + operator correction) - #909
Merged
Merged
Conversation
…2 on #908) Two P1s from Codex, both real, both now enforced invariants rather than comments — this field had ALREADY been corrected once in #908 (for naming the wrong domain) and still shipped wrong in WIDTH, so a doc-comment warning has demonstrably not been sufficient. P1-1 — concept id written where a u32 classid belongs. CLASSID_ORDER is CanonHigh, so a classid is compose_classid(canon, custom): canon (the minted concept 0xDDCC — domain in the MOST-SIGNIFICANT byte, which is what makes classids sort/prefix-search hierarchically) in the HIGH half, the app render prefix in the LOW half. The example config carried the bare concept "0x0F01", which lands it in the LOW half — classid_canon() then returns 0 and every reader routes the bake as Reserved/default, the exact "total class collapse" that function's own doc warns about. Verified from source, not taken on the reviewer's word. Fix: parse() now parses the classid as a real u32 and rejects a zero canon half with ConceptIdAsClassid { found, suggestion }, where the suggestion is the composed form (0x0F01 -> "0x0F010000") so the operator is told what to write, not merely that they were wrong. New BakeEntry::classid_u32() so callers stop re-parsing the string and re-deciding what a malformed one means. Example config and every test fixture moved to composed 8-hex classids. P1-2 — unknown keys silently ignored. A typo'd `slab_digset` or `hydarte` was dropped by serde and the field defaulted: digest pin gone, hydration silently off, in a config whose entire purpose is failing loudly at boot. Fix: #[serde(deny_unknown_fields)] on BakeEntry and SoaConfig. Both falsifiers verified by the disable-the-fix run: with the canon check stubbed to `if false` and both deny_unknown_fields removed, exactly the two new tests go red (16 passed / 2 failed) and no others — so neither is passing for an unrelated reason. Each is two-sided: the bad shape is rejected AND the good shape is accepted with its value intact, so the rules cannot pass by rejecting everything. Note the cross-check earned its keep: the_shipped_example_config_parses_ through_this_parser stayed green only because the example was corrected alongside the parser — fixing one without the other would have gone red. 18/18 soa_config, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_68f904c8-e3e5-4d1c-a0e1-572e8142343e) |
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…n an inference Operator correction: "It's cleaner to read the left Bytes and don't enforce something that you didn't understand." Correct, and the check I added was wrong on two counts I could have verified before writing it: - A ZERO CANON IS LEGAL, not a typo signature. The zero-fallback ladder (CLAUDE.md, locked 2026-06-13) states a zero tier means "not consulted" — dormant, reserved, explicitly never an error. My rule conflated an operator typo with a legitimately dormant/bootstrap class and would have refused valid configs. - PRE-FLIP STORED FORMS EXIST. `classid_canon_compat` ships precisely to serve both stored orders; a legacy-order classid it reads correctly would have been rejected outright by my strict check. I generalised a policy from one example being wrong. One wrong example is evidence about that example. What the parser validates now: ONLY that the classid is 0x-prefixed hex fitting the u32 the canonical node key reserves at bytes 0..4. That is structural — the width is the key layout, not an inference about intent. It no longer polices either half. Docs corrected in the same pass, because they were wrong in the same direction: the LOW half is not padding and not "0x0000 when none". It carries the app/render half — ClassView + WideFieldMask ergonomics and slot-schema switching — and is a slot a CONSUMER FILLS (operator's example: a session writing an ontology routing value into it). A config carrying ...0000 declares that slot UNSET, not meaningless. The routing guidance is now what it should have been from the start: read the LEFT bytes — high half is the minted concept 0xDDCC with the domain in the top byte, which is what makes classids sort and prefix-search hierarchically. KEPT from the prior commit, because neither needs a semantic guess: - #[serde(deny_unknown_fields)] on both structs (codex P1-2) — a typo'd `slab_digset`/`hydarte` silently defaulting a field is unambiguously wrong in a config whose purpose is failing loudly at boot. Its two-sided test stays. - BakeEntry::classid_u32() — an accessor, no policy. - Composed classids in the example/fixtures — correct as documentation of the common case, now without the false claim that other shapes are rejected. 17/17 soa_config (the removed rule's test went with it), clippy clean, fmt clean. The shipped-example-parses cross-check still green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
AdaWorldAPI
changed the title
…, with the C2 caveat
Operator: the low half also serves ClassView/WideFieldMask — "meaning you
address a screen region akin to CSS."
Verified before writing it, rather than taking the analogy on trust:
- `class_view.rs:70` FieldMask(u64) — bit N = the Nth field in the class's
ordered field list, stable and append-only.
- `class_view.rs:221` WideFieldMask(WideRepr) — the same logical positions
for classes past 64 fields, promoted losslessly from FieldMask.
- a2ui-rs is the shipped consumer of exactly this: `NodeDelta { key,
mask_words, values }` — "don't push pixels, address the screen."
So the analogy holds for ADDRESSING: the low half selects the ClassView
(per-app template/skin) the way a CSS selector picks an element, and the
field mask selects which of that view's fields are in play the way
declarations pick properties.
AND THE ONE PLACE IT MUST NOT BE FOLLOWED, which is why this is a doc
change worth making rather than a nice line in a chat: a field mask is
**presence, never semantics** (`class_view.rs` C2, stated there in those
words). `has(n)` answers "is field n populated here"; it must never gate
"field n means something different here." CSS's cascade genuinely does
change which rule wins — so an unqualified CSS analogy invites precisely
the violation C2 forbids. Both halves are now recorded together, because
the useful analogy and its failure mode travel as a pair or not at all.
Intra-doc links to FieldMask/WideFieldMask verified to resolve (the
remaining `cargo doc` link warnings in this crate are pre-existing and
unrelated: r:KNOWS, SpoRecord, relation, get_associated, find_path).
17/17 soa_config, fmt clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
AdaWorldAPI
marked this pull request as ready for review
August 7, 2026 22:14
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 7, 2026
…op a stale citation The doc comment landed in #909 overgeneralized in two ways an operator correction caught after merge: - It cited "the zero-fallback ladder (CLAUDE.md, locked 2026-06-13)" as live justification. This repo's dev cycle is two weeks; a doc locked a month-plus back, spanning the 2026-07-02 classid canon-high flip, is not something to cite as current authority. Replaced with a plain statement of what this parser actually does (nothing — it neither enforces nor relies on any reading of a zero half). - It presented "low half + FieldMask = CSS-style screen-region addressing" as classid's general meaning. That is one rendering pattern's reading — MedCare-rs's Redmine-style QueryColumn fieldview (crates/medcare-server/src/views/fieldview.rs) — not a property of classids or ClassView in general. Classes are not only for display; routing, storage, RBAC, and action dispatch are classid consumers too, each free to read the low half differently. Made the domain-dependence concrete with a same-bit-pattern example: 0xFFFF is a defined sentinel (Unassigned) in the ontology/concept domain per ogar_codebook::canonical_concept_domain, and carries no such meaning outside it. The parser's own behavior is unchanged — still purely structural (0x-prefixed hex fitting u32) — only the doc comment's claims about what it does or doesn't imply are corrected. Also corrected PR #909's merged description, which repeated the same two claims. Gate: 17/17 soa_config (including the shipped-example cross-check against the edited YAML) · clippy clean · fmt clean.
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.
Started as two Codex P1s from #908. One landed; the other was backed out after an operator correction, and the backout is the more useful half of this PR.
Shipped — unknown keys rejected (codex P1-2)
A typo'd
slab_digsetorhydartewas dropped by serde and the field defaulted: digest pin gone, hydration silently off — in a config whose entire purpose is failing loudly at boot. Fixed with#[serde(deny_unknown_fields)]on both structs.Two-sided test: the typo is rejected and the correct spelling is accepted with its value intact, plus an unknown top-level key. Verified real by the disable-the-fix run — removing the attribute turns exactly this test red.
Backed out — the classid half-policing
I had added a rule rejecting any classid whose canon half is zero, on the theory that it signals a bare u16 concept id written where a u32 classid belongs. Operator correction: "It's cleaner to read the left Bytes and don't enforce something that you didn't understand."
That was right, and the rule was wrong on two counts I could have checked before writing it:
classid_canon_compatships precisely to serve both stored orders; a legacy-order classid it reads correctly would have been rejected outright.I generalised a policy from one example being wrong. One wrong example is evidence about that example.
The parser now validates only that the classid is
0x-prefixed hex fitting theu32the canonical node key reserves at bytes0..4— structural, since the width is the key layout. It polices neither half, and makes no claim about what either half means to any downstream consumer.Also kept
BakeEntry::classid_u32()(an accessor, no policy), and composed classids in the example/fixtures as documentation of the common case — now without the false claim that other shapes are rejected.Gate: 17/17
soa_config· clippy clean · fmt clean · the shipped-example-parses cross-check still green.🤖 Generated with Claude Code
https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw