Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
Review rate limit: 0/5 reviews remaining, refill in 1 minute and 15 seconds. Comment |
There was a problem hiding this comment.
Pull request overview
Release prep for v0.9.13, capturing the new skip_validation hook feature (from #103), its tests, and packaging metadata for LuaRocks.
Changes:
- Add
skip_validationoption support to generated validators (early-return hook per value). - Add test cases covering bypass behavior across types/enums/defaults/nesting/arrays and the no-hook baseline.
- Add
jsonschema-0.9.13-0.rockspecfor the release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/jsonschema.lua |
Wires skip_validation through codegen context and runtime custom lib table; emits early-return check in generated validators. |
t/default.lua |
Adds test cases 10–15 validating skip_validation behavior and ensuring normal validation without the hook. |
rockspec/jsonschema-0.9.13-0.rockspec |
Adds LuaRocks spec for the v0.9.13 tag/package version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
t/default.lua (1)
347-359:⚠️ Potential issue | 🟡 MinorAdd a boolean-
falseregression case.The new tests don’t cover the one schema branch that still preempts the bypass hook. A small regression here would protect the new behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/default.lua` around lines 347 - 359, Add a regression test that explicitly sets skip_validation = false to exercise the schema branch that preempts the bypass hook: copy the existing test (rule, validator, and the call validator({ port = "$secret://vault/port" })) but include skip_validation = false in the test setup and assert that validation still fails (ok is false) and emits the same failure message; reference the symbols rule and validator so the new case mirrors the existing test but with skip_validation = false.lib/jsonschema.lua (1)
614-629:⚠️ Potential issue | 🟠 MajorMove
skip_validationahead of the boolean-schema fast paths.Right now
schema == falsereturns before the new hook runs, so secret placeholders still fail on a validfalseschema. That undercuts the bypass behavior for one JSON Schema form.🛠️ Suggested fix
- if schema == true then - ctx:stmt('do return true end') - return ctx - elseif schema == false then - ctx:stmt('do return false, "expect false always" end') - return ctx - end - -- skip_validation hook: if the caller provided a predicate via -- custom.skip_validation, check it before any constraint. When it -- returns true the value is accepted as-is (useful for placeholder -- strings like secret references that will be resolved at runtime). if ctx._root._skip_validation then ctx:stmt(sformat('if %s(%s) then return true end', ctx:libfunc('custom.skip_validation'), ctx:param(1))) end + + if schema == true then + ctx:stmt('do return true end') + return ctx + elseif schema == false then + ctx:stmt('do return false, "expect false always" end') + return ctx + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/jsonschema.lua` around lines 614 - 629, The skip_validation hook check must run before the boolean-schema fast paths so placeholders can bypass validation even when schema is false; move the block that checks ctx._root._skip_validation (the ctx:stmt using ctx:libfunc('custom.skip_validation') and ctx:param(1)) to come before the `if schema == true` / `elseif schema == false` checks, ensuring the custom.skip_validation predicate is evaluated first and can return true to short-circuit validation for both true and false schemas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/jsonschema.lua`:
- Around line 614-629: The skip_validation hook check must run before the
boolean-schema fast paths so placeholders can bypass validation even when schema
is false; move the block that checks ctx._root._skip_validation (the ctx:stmt
using ctx:libfunc('custom.skip_validation') and ctx:param(1)) to come before the
`if schema == true` / `elseif schema == false` checks, ensuring the
custom.skip_validation predicate is evaluated first and can return true to
short-circuit validation for both true and false schemas.
In `@t/default.lua`:
- Around line 347-359: Add a regression test that explicitly sets
skip_validation = false to exercise the schema branch that preempts the bypass
hook: copy the existing test (rule, validator, and the call validator({ port =
"$secret://vault/port" })) but include skip_validation = false in the test setup
and assert that validation still fails (ok is false) and emits the same failure
message; reference the symbols rule and validator so the new case mirrors the
existing test but with skip_validation = false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9e7f235-a8c7-4932-a236-c9ffe02f6ec1
📒 Files selected for processing (3)
lib/jsonschema.luarockspec/jsonschema-0.9.13-0.rockspect/default.lua
jarvis9443
force-pushed
the
release/v0.9.13
branch
2 times, most recently
from
3d246a3 to
017bb9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/jsonschema.lua`:
- Around line 622-633: The skip_validation bypass block (checking
ctx._root._skip_validation and emitting the custom.skip_validation call using
ctx:uservalue(schema) and ctx:param(1)) must be moved so it runs before any
boolean-schema handling (i.e. before the code path that treats schema == false);
relocate the entire if ctx._root._skip_validation ... ctx:stmt(...) block
earlier in the validation generation so the custom predicate can short-circuit
even when schema is the literal false, ensuring the bypass executes prior to any
checks that branch on schema boolean values.
🪄 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: ba3edc70-cb6b-4580-929a-301e7a1f6f0f
📒 Files selected for processing (3)
lib/jsonschema.luarockspec/jsonschema-0.9.13-0.rockspect/default.lua
✅ Files skipped from review due to trivial changes (1)
- rockspec/jsonschema-0.9.13-0.rockspec
🚧 Files skipped from review as they are similar to previous changes (1)
- t/default.lua
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jarvis9443
force-pushed
the
release/v0.9.13
branch
from
017bb9b to
ef178b8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/jsonschema.lua (1)
1223-1224: Consider validatingcustom.skip_validationtype up front.A non-function truthy value will fail later at runtime when invoked from generated code; fail-fast here would provide a clearer error.
♻️ Suggested hardening
generate_validator = function(schema, custom) + if custom and custom.skip_validation ~= nil and type(custom.skip_validation) ~= "function" then + error("custom.skip_validation must be a function") + end local customlib = { null = custom and custom.null or default_null, match_pattern = custom and custom.match_pattern or match_pattern, parse_ipv4 = custom and custom.parse_ipv4 or parse_ipv4, parse_ipv6 = custom and custom.parse_ipv6 or parse_ipv6, skip_validation = custom and custom.skip_validation or nil, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/jsonschema.lua` around lines 1223 - 1224, Validate custom.skip_validation when building the options table: if custom and custom.skip_validation is present, assert its type is "function" and raise a clear error (e.g., error("custom.skip_validation must be a function")) instead of silently assigning non-functions; then set skip_validation = custom.skip_validation or nil. This change should be applied where parse_ipv6 and skip_validation are assembled (the code handling the options/custom table) so generated code never receives a non-callable skip_validation.t/default.lua (1)
331-430: Add one regression test to lock theschema == falsenon-bypass rule.Current additions cover positive paths well, but a focused negative case for forbidden fields would prevent future regressions in hook placement/semantics.
🧪 Suggested test addition
+----------------------------------------------------- test case 18 +-- skip_validation must NOT bypass forbidden fields (`schema == false`) +rule = { + type = "object", + additionalProperties = false, + properties = { + host = { type = "string" }, + }, +} +validator = jsonschema.generate_validator(rule, { skip_validation = always_skip_secret }) +ok, err = validator({ host = "ok", forbidden = "$secret://vault/forbidden" }) +assert(not ok, "fail: skip_validation must not bypass additionalProperties=false") +ngx.say("passed: forbidden fields are still rejected with skip_validation")Based on learnings: In
lib/jsonschema.lua,skip_validationintentionally does not bypassschema == falsebecause forbidden-field positions must remain invalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/default.lua` around lines 331 - 430, Add a regression test that asserts skip_validation hooks do NOT bypass forbidden fields where the schema is explicitly false: generate_validator(rule, { skip_validation = <hook> }) and call the validator with an object containing a property whose schema is false (e.g., properties = { bad = false }) and a secret-like string value to confirm the validator returns not ok; reference generate_validator, skip_validation, and the schema == false case to locate where to add the test so future changes don't allow hooks to bypass forbidden-field validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/jsonschema.lua`:
- Around line 1223-1224: Validate custom.skip_validation when building the
options table: if custom and custom.skip_validation is present, assert its type
is "function" and raise a clear error (e.g., error("custom.skip_validation must
be a function")) instead of silently assigning non-functions; then set
skip_validation = custom.skip_validation or nil. This change should be applied
where parse_ipv6 and skip_validation are assembled (the code handling the
options/custom table) so generated code never receives a non-callable
skip_validation.
In `@t/default.lua`:
- Around line 331-430: Add a regression test that asserts skip_validation hooks
do NOT bypass forbidden fields where the schema is explicitly false:
generate_validator(rule, { skip_validation = <hook> }) and call the validator
with an object containing a property whose schema is false (e.g., properties = {
bad = false }) and a secret-like string value to confirm the validator returns
not ok; reference generate_validator, skip_validation, and the schema == false
case to locate where to add the test so future changes don't allow hooks to
bypass forbidden-field validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bef54432-acea-44ec-a010-a7f5e3e8719d
📒 Files selected for processing (3)
lib/jsonschema.luarockspec/jsonschema-0.9.13-0.rockspect/default.lua
✅ Files skipped from review due to trivial changes (1)
- rockspec/jsonschema-0.9.13-0.rockspec
jarvis9443
force-pushed
the
release/v0.9.13
branch
from
ef178b8 to
0d1880d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes since v0.9.12: - feat: add skip_validation custom hook for value-level bypass
jarvis9443
force-pushed
the
release/v0.9.13
branch
from
0d1880d to
64adfae
Compare
Changes since v0.9.12
skip_validationcustom hook for value-level bypass (feat: add skip_validation custom hook for value-level bypass #103)Release checklist
v0.9.13after mergeSummary by CodeRabbit
New Features
Tests
Packaging