feat(lua): lighten keep_origin provenance#131
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR reworks ChangesThreshold-based lightweight provenance for origin tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
tests/lua/origin_materialize_spec.lua (1)
37-66: ⚡ Quick winAdd exact-boundary coverage for the
>24/>64rules.These tests cover well-below and well-above the thresholds, but the implementation relies on strict
>comparisons. A regression to>=would currently slip through. One string token at exactly 24 bytes and one child container at exactly 64 bytes would lock down the contract nicely.🤖 Prompt for 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. In `@tests/lua/origin_materialize_spec.lua` around lines 37 - 66, Add two tests that hit the exact thresholds to lock the >24 and >64 semantics: (1) add an "exact 24-byte escaped string" test that builds a JSON with a string token whose escaped/raw length is exactly 24 bytes and verifies materialize/encode behaves as it does for values below the large-string threshold (i.e. not treated as "above 24"); reference the existing pattern in origin_materialize_spec.lua (use qjson.decode, qjson.materialize, qjson.encode and modify/inspect fields as in the other string tests). (2) add an "exact 64-byte child container" test that constructs a parent object whose single child container's encoded size is exactly 64 bytes and verifies materialize/encode behaves as it does for values below the large-container threshold (i.e. not treated as "above 64"); follow the existing container tests' style and assertions so a regression from > to >= would fail.
🤖 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 `@lua/qjson/table.lua`:
- Around line 561-579: origin_child_record currently forces callers to
materialize raw_token (allocating Lua strings) before checking thresholds;
change it to accept byte-span metadata (bs, be or a span table) instead of
raw_token and only call source:sub(bs, be) inside the branches that actually
keep provenance. Update origin_child_record signature and body to use bs/be for
length checks against ORIGIN_STRING_MIN_RAW and ORIGIN_TABLE_MIN_RAW and to
build the returned record (string raw or table origin) after materializing the
substring; also update callers that currently call cursor_raw_token(...) to
instead fetch the bs/be span (without slicing) and pass that span into
origin_child_record, and ensure TABLE_ORIGIN and child_origin checks remain
unchanged.
---
Nitpick comments:
In `@tests/lua/origin_materialize_spec.lua`:
- Around line 37-66: Add two tests that hit the exact thresholds to lock the >24
and >64 semantics: (1) add an "exact 24-byte escaped string" test that builds a
JSON with a string token whose escaped/raw length is exactly 24 bytes and
verifies materialize/encode behaves as it does for values below the large-string
threshold (i.e. not treated as "above 24"); reference the existing pattern in
origin_materialize_spec.lua (use qjson.decode, qjson.materialize, qjson.encode
and modify/inspect fields as in the other string tests). (2) add an "exact
64-byte child container" test that constructs a parent object whose single child
container's encoded size is exactly 64 bytes and verifies materialize/encode
behaves as it does for values below the large-container threshold (i.e. not
treated as "above 64"); follow the existing container tests' style and
assertions so a regression from > to >= would fail.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 943cd06c-4685-4f6a-8eb9-95d0ebe0c801
📒 Files selected for processing (3)
README.mdlua/qjson/table.luatests/lua/origin_materialize_spec.lua
1 participant
Summary
keep_originprovenance withorigin.completegating whole-slice reuseFixes #130
Test Plan
PATH="$HOME/.luarocks/bin:$PATH" make testPATH="$HOME/.luarocks/bin:$PATH" make lintcargo test --release --no-default-featurescargo test --features test-panic --releaseSummary by CodeRabbit
Documentation
qjson.materialize()provenance tracking, covering how key order preservation and token reuse are optimized.Tests