superseded: exported option validation consolidated in #126 - #114
superseded: exported option validation consolidated in #126#114seonghobae wants to merge 4 commits into
Conversation
Added strict input validation for `conf.level` in `icci()` and `nested`, `adj` in `vuongtest()` to fail securely and prevent information leakage via raw R errors on invalid types or bounds.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes입력 검증 강화
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change adds localized validation for invalid function inputs and prevents raw internal errors from being exposed; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.) ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!is.character(adj) || length(adj) != 1 || is.na(adj) || !(adj %in% c("none", "aic", "bic"))) { | ||
| stop("adj must be 'none', 'aic', or 'bic'", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
🟡 Ignored nested adjustment now fails
Nested comparisons reject unknown adj values even though the public contract says this argument is ignored. Existing callers can now fail before evaluation.
Prompt for agents
Restore the documented nested-comparison contract in R/vuongtest.R. When nested is TRUE, adj must have no effect and must not be rejected. Gate adj validation to non-nested calls, and ensure the later AIC/BIC parameter-count and likelihood-ratio adjustment block is also skipped for nested calls. Add regression coverage comparing nested results for default, supported, and otherwise invalid adj values.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!is.numeric(conf.level) || length(conf.level) != 1 || is.na(conf.level) || conf.level <= 0 || conf.level >= 1) { | ||
| stop("conf.level must be a single numeric value between 0 and 1", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
🔍 Public validation lacks regression coverage
Three public argument contracts changed without tests for invalid values, clean errors, or boundaries. Add coverage before merging to prevent validation regressions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!is.logical(nested) || length(nested) != 1 || is.na(nested)) { | ||
| stop("nested must be a single logical value", call. = FALSE) | ||
| } | ||
| if (!is.character(adj) || length(adj) != 1 || is.na(adj) || !(adj %in% c("none", "aic", "bic"))) { | ||
| stop("adj must be 'none', 'aic', or 'bic'", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
🔍 Validation change missing from NEWS
The repository requires user-visible changes in NEWS. These exported functions now reject inputs differently, but the release notes remain unchanged.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!is.numeric(conf.level) || length(conf.level) != 1 || is.na(conf.level) || conf.level <= 0 || conf.level >= 1) { | ||
| stop("conf.level must be a single numeric value between 0 and 1", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
📝 Info: Generated signatures remain current
The function signatures and parameter text did not change. Existing generated usage documentation therefore remains current without regeneration.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #' @export | ||
| icci <- function(object1, object2, conf.level=.95, ll1=llcont, ll2=llcont) { | ||
|
|
||
| if (!is.numeric(conf.level) || length(conf.level) != 1 || is.na(conf.level) || conf.level <= 0 || conf.level >= 1) { |
There was a problem hiding this comment.
🟨 Complex input bypasses sanitized validation
A complex conf.level passes is.numeric, then range checking emits a raw base error. Malformed input can still expose internal execution details.
Was this helpful? React with 👍 or 👎 to provide feedback.
Added strict input validation for `conf.level` in `icci()` and `nested`, `adj` in `vuongtest()` to fail securely and prevent information leakage via raw R errors on invalid types or bounds.
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR introduces strict input validation to prevent information leakage, but it creates a behavioral regression in vuongtest. According to the function's documentation, when nested = TRUE, the adj argument should be ignored. However, the new validation at lines 104-106 of R/vuongtest.R unconditionally rejects invalid adj values regardless of the value of nested, breaking the existing public contract and potentially failing existing callers.
Reviewed changed lines
R/vuongtest.R:104 (RIGHT): Unconditional validation of 'adj' violates the contract that 'adj' is ignored when 'nested=TRUE'.R/icci.R:68 (RIGHT): Correctly implements type and bounds checking for conf.level with call. = FALSE..jules/sentinel.md:15 (RIGHT): Correctly documents the vulnerability and prevention strategy for input validation.
Adversarial validation
R/vuongtest.R:104 (RIGHT)confirmed: Calling vuongtest(obj1, obj2, nested=TRUE, adj='invalid') should succeed because adj is ignored when nested=TRUE. — The code at line 104 executesif (!is.character(adj) || ... || !(adj %in% c('none', 'aic', 'bic')))before checking the value ofnested, resulting in astop()call.- Residual risk: The function now throws an error for invalid 'adj' values even when 'nested=TRUE', which is a regression in behavior.
Findings
-
[medium] R/vuongtest.R:104 (RIGHT): Behavioral regression: 'adj' validation is applied even when 'nested=TRUE', contradicting the function documentation which states 'adj' is ignored in nested comparisons.
-
Result: REQUEST_CHANGES
-
Head SHA:
c058a1bfcbf09c63974b9fb0c5e2808a1b1b03fc -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
Added strict input validation for `conf.level` in `icci()` and `nested`, `adj` in `vuongtest()` to fail securely and prevent information leakage via raw R errors on invalid types or bounds.
Added strict input validation for `conf.level` in `icci()` and `nested`, `adj` in `vuongtest()` to fail securely and prevent information leakage via raw R errors on invalid types or bounds.
seonghobae
changed the title
1 participant
Superseded by #126
Fresh exact-head audit at
7a4eb8614c30b2d987c64a8835172fa02148e4f6confirms this branch carries the same exportednested/adj/conf.levelvalidation semantics now owned by canonical Draft #126@4af198147f33a83cb62d1e66f6d95eaf2b06d838.#126 has a stronger permanent production-path regression covering NA/NULL/wrong type/vector/bounds, unsupported case, empty and whitespace-padded adjustment values, stable package-owned messages, and
conditionCall(err) == NULL. No unique statistical behavior, test, fixture, or release evidence remains only on this branch.The generated repository-wide Sentinel rule and MEDIUM/information-leak severity narrative are intentionally not inherited because available evidence establishes bounded API validation/error behavior rather than a demonstrated deployment exploit.
Every valid semantic/evidence delta is fully represented by #126. This closure transfers neither predecessor GREEN nor merge authority.