Count nesting depth over open containers, and record suiteRevision 5 at 149/149 - #3
Merged
Conversation
📝 WalkthroughWalkthroughThe JSON parser now counts only open containers toward a depth limit of 128. Suite revision 5 evidence, parity metadata, CI pins, reproduction instructions, and revision notes are updated for a 149/149 conformance run. ChangesConformance revision 5
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/json.rs`:
- Around line 582-589: Update the scalars_do_not_consume_depth test to wrap the
wide object in 127 additional containers before parsing, then assert parsing
succeeds. Keep the 2,000 scalar members and ensure the resulting object is
evaluated at open-container depth 128.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef57b7ec-81bc-4ae0-a7ce-60dc476d4d07
📒 Files selected for processing (6)
.github/workflows/conformance.ymlNOTES.mdREADME.mdreports/INDEX.jsonreports/suite-revision-5.jsonsrc/json.rs
The spec pins both halves of the depth rule at v0.6+: a statement or record payload deeper than 128 is malformed, and depth is the number of arrays and objects open at a point, with the outermost brace at depth 1 and scalars not counting. This parser had neither. It incremented in parse_value, so every value including a scalar leaf moved the counter, and the bound was 256. The constant was the visible half and the counting rule was the load-bearing one. Measured on the raw bytes of every document in suiteRevision 5, which needs no parser and so covers the deliberately ill-formed vectors too, this parser read exactly one level deeper than the spec rule on all 149 statements and on every record payload inside them, because every deepest path in the corpus ends in a scalar. Changing only the constant to 128 therefore rejects a statement at depth 128, which the spec calls valid. The corpus cannot see the difference: both the constant-only edit and this one score 149/149 on it. Not because it is shallow -- bad-741's payload sits at depth 130 -- but because nothing in it sits at 128, the one depth where the two readings disagree. A scalar leaf inside 128 open containers reads as 129 to a per-value counter and 128 to a per-container one; at 129 both reject, and at 128 with an empty-container leaf both accept. The tests carry that boundary instead. Refs in-toto/attestation#570. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unchanged revision-3 checker scored 148/149 against revision 5. The record for the fixed build is added, revision 3 is retired from continuous verification, and the workflow moves with it: suite pin, spec pin, the report the fresh run is compared against, and the parity string the corpus step asserts. That last one is a grep, so leaving it on 105/105 would have failed the step against the newly pinned suite rather than failing quietly. INDEX.json's entry for revision 3 gains its checkerCommit at the same time. It was the one record whose provenance nothing could check, and 0cf46c1 hashes to exactly the digest that entry already recorded, so the field becomes a verifiable binding rather than a label. It names a source tree rather than a commit, so 47dbaf1 satisfies it equally; that is the digest working as intended, not a weaker claim than it looks. Revision 4 is listed in NOTES.md as not run here rather than left as a silent gap in the sequence. Refs in-toto/attestation#570. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rul1an
force-pushed
the
depth-counting-rule
branch
from
July 28, 2026 09:01
5ad04a3 to
8953588
Compare
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.
Revision 5 of the conformance corpus pins the two things the v0.6 text had left open, encoding and nesting depth. The unchanged revision-3 build scored 148/149 against it, missing only
bad-741.The bound was the visible half of that miss. The revision also states the counting rule, and this parser was incrementing in
parse_value, so every value including a scalar leaf moved the counter. Measured on the raw bytes of every document in the corpus, which needs no parser and so covers the deliberately ill-formed vectors too, that read exactly one level deeper than the spec rule on all 149 statements and on every record payload inside them, because every deepest path in the corpus ends in a scalar.So changing only the constant to 128 would reject a statement at depth 128, which the spec calls valid. The corpus cannot see the difference: the constant-only edit scores 149/149 as well. Not because the corpus is shallow, since
bad-741's payload sits at depth 130, but because nothing in it sits at 128, the one depth where the two readings disagree. A scalar leaf inside 128 open containers reads as 129 to a per-value counter and 128 to a per-container one; at 129 both reject, and at 128 with an empty-container leaf both accept.depth_bound_is_counted_over_open_containerscarries that boundary instead.The encoding half needed no change. This checker rejected ill-formed UTF-8, CESU-8, overlong forms and unpaired surrogate escapes from the first build.
Also here
reports/suite-revision-5.json, and revision 3 retired from continuous verification.grepinsideset -euo pipefail, so leaving it on105/105would have failed the step rather than failing quietly.INDEX.json's revision-3 entry gains itscheckerCommit. It was the one record whose provenance nothing could check, and0cf46c1hashes to exactly the digest that entry already recorded.NOTES.mdas not run here, rather than left as a silent gap in the sequence.Verification
astrogilda/aee-conformance@ea25a1e, whose vendored spec is byte-identical to the head blob of Add Adversarial Execution Evidence predicate (v0.7) in-toto/attestation#570.bad-741, comparing all per-vector fields.reports/suite-revision-3.jsonexactly with the fixed build.Refs in-toto/attestation#570.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Conformance
Documentation