๐ก๏ธ Sentinel: [CRITICAL] Fix integer coercion vulnerabilities in interactive prompts - #198
๐ก๏ธ Sentinel: [CRITICAL] Fix integer coercion vulnerabilities in interactive prompts#198seonghobae wants to merge 2 commits into
Conversation
|
๐ 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. |
๐ WalkthroughWalkthrough
Changesํจํค์ง API ๋ฐ ์ถ์ ํ๋ฆ
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: ๐ฅ Pre-merge checks | โ 5โ Passed checks (5 passed)
โจ Finishing Touches๐ Generate docstrings
๐งช Generate unit tests (beta)
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/testthat/test-regex-security.R (1)
1-16: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win์ ๊ท ์์ BILOG-MG ๊ฒฝ๋ก์ ํ๊ท ํ ์คํธ๋ฅผ ์ถ๊ฐํ์ธ์.
ํ์ฌ
tests/testthat/test-regex-security.R๋R/aFIPC.RLine 144์ ๊ณตํต ๋ฌธํญ ๊ฒฝ๋ก์ Line 174์ oldform ๊ฒฝ๋ก๋ง ์คํํฉ๋๋ค. Line 393์checknewformBILOGprior()๋ ์คํ๋์ง ์์ต๋๋ค.newformBILOGprior = NULL์ธ ์ ๊ท ์์ ๊ฒฝ๋ก์์"12"์ ๋ ฅ์ด ์ธ ๋ฒ ๊ฑฐ๋ถ๋๊ณToo many invalid newform BILOG prior attempts๊ฐ ๋ฐ์ํ๋์ง ํ์ธํ์ธ์. oldform ๋จ๊ณ๋ fixture ๋๋ stub์ผ๋ก ํต๊ณผ์ํค์ธ์.Also applies to: 18-36
๐ค 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/testthat/test-regex-security.R` around lines 1 - 16, Add a regression test alongside the existing regex-security tests that exercises autoFIPC with newformBILOGprior set to NULL and reaches checknewformBILOGprior. Stub or fixture the oldform confirmation so it succeeds, provide "12" for the newform BILOG prior prompt, and assert that three rejected attempts raise "Too many invalid newform BILOG prior attempts".
๐ค 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 `@tests/testthat/test-regex-security.R`:
- Around line 4-5: Declare mockery as a test dependency by adding it to the
Suggests field in DESCRIPTION, so the mockery::stub calls in the regex security
tests are available during CI.
---
Nitpick comments:
In `@tests/testthat/test-regex-security.R`:
- Around line 1-16: Add a regression test alongside the existing regex-security
tests that exercises autoFIPC with newformBILOGprior set to NULL and reaches
checknewformBILOGprior. Stub or fixture the oldform confirmation so it succeeds,
provide "12" for the newform BILOG prior prompt, and assert that three rejected
attempts raise "Too many invalid newform BILOG prior attempts".
๐ช 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 Plus
Run ID: e6a55407-ed27-405f-a031-3f5a37a54002
๐ Files selected for processing (3)
.jules/sentinel.mdR/aFIPC.Rtests/testthat/test-regex-security.R
| mockery::stub(aFIPC::autoFIPC, 'interactive', my_interactive) | ||
| mockery::stub(aFIPC::autoFIPC, 'readline', my_readline) |
There was a problem hiding this comment.
๐ฉบ Stability & Availability | ๐ Major | โก Quick win
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
set -euo pipefail
description_file="$(fd -a '^DESCRIPTION$' . | head -n 1)"
test -n "$description_file"
rg -n -C 2 '^(Suggests|Imports):|mockery' "$description_file"Repository: ContextualWisdomLab/aFIPC
Length of output: 375
๐ Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mockery usages and test setup ---'
rg -n -C 3 'mockery|testthat|devtools::test|R CMD check|rcmdcheck|remotes::install' \
.github tests DESCRIPTION 2>/dev/null || true
printf '%s\n' '--- candidate CI and dependency files ---'
fd -a -t f '(^DESCRIPTION$|^NAMESPACE$|^.*\.(ya?ml|yaml|R|Rmd|md)$)' .github tests . 2>/dev/null \
| sed -n '1,160p'Repository: ContextualWisdomLab/aFIPC
Length of output: 16855
mockery๋ฅผ ํ
์คํธ ์์กด์ฑ์ผ๋ก ์ ์ธํ์ธ์.
DESCRIPTION์ Suggests์ mockery๊ฐ ์์ผ๋ฏ๋ก CI๊ฐ mockery::stub๋ฅผ ๋ก๋ํ์ง ๋ชปํ๊ณ ํ
์คํธ๋ฅผ ์คํจ์ํต๋๋ค. mockery๋ฅผ Suggests์ ์ถ๊ฐํ์ธ์.
๐ค 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/testthat/test-regex-security.R` around lines 4 - 5, Declare mockery as
a test dependency by adding it to the Suggests field in DESCRIPTION, so the
mockery::stub calls in the regex security tests are available during CI.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
aFIPC.Rcheck/tests/testthat.Rout (1)
1-288: ๐ Maintainability & Code Quality | ๐ Major | โก Quick win
R CMD check์ฐ์ถ๋ฌผ ์ ์ฒด๋ฅผ ๋ฒ์ ๊ด๋ฆฌ์์ ์ ๊ฑฐํ์ญ์์ค.aFIPC.Rcheck/๋๋ ํฐ๋ฆฌ๋R CMD check์คํ์ด ์๋์ผ๋ก ์์ฑํ๋ ์ฐ์ถ๋ฌผ์ ๋๋ค. ์์ค ์ค๋ ์ท, ์ปดํ์ผ๋ ๋์๋ง ๋ฐ์ด๋๋ฆฌ, ํ ์คํธ ๋ฌ๋ ์ฌ๋ณธ, ์คํ ๋ก๊ทธ๋ฅผ ๋ชจ๋ ํฌํจํ๋ฉฐ, ์ด ์ ์ฒด๊ฐ ์ ์ฅ์์ ์ปค๋ฐ๋์์ต๋๋ค. ๊ทผ๋ณธ ์์ธ์ ํ๋์ ๋๋ค: ์์ฑ๋ ๋น๋/๊ฒ์ฌ ์ฐ์ถ๋ฌผ์ด ์์ค ํธ๋ฆฌ์ ํจ๊ป ์ถ์ ๋๊ณ ์๋ค๋ ์ ์ ๋๋ค.
aFIPC.Rcheck/tests/testthat.Rout#L1-L288: ํ๊ฒฝ๋ณ ์คํ ๋ก๊ทธ(R ๋ฒ์ , ํ๋ซํผ, ์์ ์๊ฐ ํฌํจ)๋ฅผ ์ ์ฅ์์์ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/00_pkg_src/aFIPC/test_dummy.R#L1-L3:R CMD check์ด ์์ฑํ ์์ค ์ค๋ ์ท ๋ด๋ถ ์คํฌ๋ฆฝํธ๋ฅผ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/00_pkg_src/aFIPC/test_validation.R#L1-L3: ๊ฐ์ ์ค๋ ์ท ๋ด๋ถ์ ๊ตฌ๋ฌธ ํ์ธ ์คํฌ๋ฆฝํธ๋ฅผ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat.R#L1-L4: ์ค๋ ์ท ๋ด๋ถ์ testthat ๋ฌ๋ ์ฌ๋ณธ์ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/aFIPC/help/aFIPC.rdb#L1-L14: ์ปดํ์ผ๋ ๋์๋ง ๋ฐ์ดํฐ๋ฒ ์ด์ค ๋ฐ์ด๋๋ฆฌ๋ฅผ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/aFIPC/help/aFIPC.rdx#L1-L1: ์ปดํ์ผ๋ ๋์๋ง ์์ธ ๋ฐ์ด๋๋ฆฌ๋ฅผ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/aFIPC/help/aliases.rds#L1-L1: ์์ฑ๋ ๋ณ์นญ RDS ํ์ผ์ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/aFIPC/help/paths.rds#L1-L1: ์์ฑ๋ ๊ฒฝ๋ก RDS ํ์ผ์ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/tests/startup.Rs#L1-L3: ์์ฑ๋ ๊ฒ์ฌ์ฉ startup ํ์ผ์ ์ญ์ ํ์ญ์์ค.aFIPC.Rcheck/tests/testthat.R#L1-L4: ๊ฒ์ฌ ์ถ๋ ฅ ๋๋ ํฐ๋ฆฌ ๋ด testthat ๋ฌ๋ ์ฌ๋ณธ์ ์ญ์ ํ๊ณ ,.gitignore์*.Rcheck/ํญ๋ชฉ์ ์ถ๊ฐํด ์ฌ๋ฐ์ ๋ง์ผ์ญ์์ค.๐ค 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 `@aFIPC.Rcheck/tests/testthat.Rout` around lines 1 - 288, Remove all generated R CMD check artifacts from version control: delete the listed files in aFIPC.Rcheck/tests/testthat.Rout (lines 1-288), aFIPC.Rcheck/00_pkg_src/aFIPC/test_dummy.R (1-3), aFIPC.Rcheck/00_pkg_src/aFIPC/test_validation.R (1-3), aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat.R (1-4), aFIPC.Rcheck/aFIPC/help/aFIPC.rdb (1-14), aFIPC.Rcheck/aFIPC/help/aFIPC.rdx (1-1), aFIPC.Rcheck/aFIPC/help/aliases.rds (1-1), aFIPC.Rcheck/aFIPC/help/paths.rds (1-1), aFIPC.Rcheck/tests/startup.Rs (1-3), and aFIPC.Rcheck/tests/testthat.R (1-4), then add *.Rcheck/ to .gitignore to prevent these build and check outputs from being tracked again.
๐งน Nitpick comments (6)
aFIPC.Rcheck/aFIPC-Ex.R (1)
1-44: ๐ Maintainability & Code Quality | ๐ต Trivial | โก Quick win
aFIPC.Rcheck/์ ์ฒด๋ฅผ ์ ๊ฑฐํ๊ณ.gitignore์ ์ถ๊ฐํ์ธ์.ํ์ฌ
aFIPC.Rcheck/์๋ 53๊ฐ ํ์ผ์ด ์ถ์ ๋๊ณ ์์ต๋๋ค. ์ด ๋๋ ํฐ๋ฆฌ๋R CMD check๊ฐ ์์ฑํ ์์ค ๋ณต์ฌ๋ณธ, ์ค์น ๊ฒฐ๊ณผ, ๋ก๊ทธ, lazy-load ๋ฐ์ดํฐ๋ฒ ์ด์ค ๋ฐ ๋ฉํ๋ฐ์ดํฐ๋ฅผ ํฌํจํฉ๋๋ค. CI๋ ๊นจ๋ํ ์์ ๋๋ ํฐ๋ฆฌ์์ ๊ฒ์ฌ๋ฅผ ์คํํด์ผ ํฉ๋๋ค.๐ค 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 `@aFIPC.Rcheck/aFIPC-Ex.R` around lines 1 - 44, Remove the entire tracked aFIPC.Rcheck/ directory, including all listed generated files, and add aFIPC.Rcheck/ to .gitignore so future R CMD check artifacts are not committed.aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R (1)
1-52: ๐ Performance & Scalability | ๐ต Trivial | โก Quick win๊ธด ์คํ ์๊ฐ์ ๋ํ ๋ณดํธ ์ฅ์น๋ฅผ ์ถ๊ฐํ์ญ์์ค.
์ด ํ ์คํธ๋
N = 1600์ผ๋ก ๋ ๊ฐ์ ๋ชจํ์SE = TRUE์ ์ต๋ 500 ์ฌ์ดํด๋ก ์ถ์ ํฉ๋๋ค. ๊ทธ ๋ค์autoFIPC()๊ฐ ์ฐ๊ฒฐ ๋ชจํ์ ์ถ๊ฐ๋ก ์ถ์ ํฉ๋๋ค.R CMD check์คํ ์๊ฐ์ด CRAN ํ๋๋ฅผ ์ด๊ณผํ ์ ์์ต๋๋ค.skip_on_cran()์ ์ถ๊ฐํ๊ฑฐ๋ ํ๋ณธ ํฌ๊ธฐ๋ฅผ ์ค์ด์ญ์์ค.๐ง ์ ์ ์์
test_that("autoFIPC fixes common-item parameters on the old-form scale", { skip_if_not_installed("mirt") + skip_on_cran()๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R` around lines 1 - 52, Update the test beginning with the autoFIPC calibration setup to add a CRAN runtime safeguard, preferably by calling skip_on_cran() before the expensive mirt model fitting; alternatively reduce the simulated sample size while preserving the testโs calibration coverage.aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.R (1)
37-50: ๐ Maintainability & Code Quality | ๐ต Trivial | ๐๏ธ Heavy lift์ด ํ ์คํธ๋ ๊ตฌํ ์ฝ๋๋ฅผ ํธ์ถํ์ง ์์ต๋๋ค.
Line 37-46๊ณผ Line 64-77์ ํํ์์ ํ ์คํธ ํ์ผ ์์์ ๋ค์ ์์ฑํฉ๋๋ค. ๊ทธ๋ฌ๋ฏ๋ก
R/aFIPC.R์ ์ค์ ์ฝ๋๊ฐ ๋ณ๊ฒฝ๋์ด๋ ์ด ํ ์คํธ๋ ์คํจํ์ง ์์ต๋๋ค. ํ๊ท ๋ฐฉ์ง ํจ๊ณผ๋ฅผ ์ป์ผ๋ ค๋ฉด ํด๋น ๋ก์ง์ ๋ด๋ถ ํฌํผ ํจ์๋ก ์ถ์ถํ๊ณ , ํ ์คํธ์์ ๊ทธ ํฌํผ๋ฅผ ํธ์ถํ์ญ์์ค.๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.R` around lines 37 - 50, Refactor the duplicated optimization logic in R/aFIPC.R into an internal helper function, then update test-optimization-equivalence.R to call that helper instead of recreating the expressions locally. Keep the testโs expected-value and legacy-equivalence assertions, ensuring changes to the production implementation can cause the test to fail.aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R (1)
48-57: ๐ Maintainability & Code Quality | ๐ต Trivial | ๐ค Low value์ด ํ ์คํธ๋ ํผ๋์ค๋ฌ์ด ์ค๋ฅ ๋ฉ์์ง๋ฅผ ๊ณ ์ ํฉ๋๋ค.
nItems๊ฐ 1์ด๋ฏ๋ก ๋ฉ์์ง๊ฐ "length 1 or length 1"๋ก ์ถ๋ ฅ๋ฉ๋๋ค.R/aFIPC.RLine 91์itemtype๊ฒ์ฆ์ ์ ๋ฆฌํ ๋ค ์ด ๊ธฐ๋๊ฐ๋ ํจ๊ป ์์ ํ์ญ์์ค.๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R` around lines 48 - 57, Update the itemtype validation in aFIPC so that its error message is not redundant when nItems equals 1, then revise the corresponding expect_error assertion in test-autoFIPC.R to match the corrected message. Preserve the validation behavior for invalid itemtype lengths and anchor the changes to the aFIPC function and its itemtype test.aFIPC.Rcheck/00_pkg_src/aFIPC/NAMESPACE (1)
3-5: ๐ Maintainability & Code Quality | ๐ต Trivial | ๐ค Low value
import(mirt)์ ์ฒด ์ํฌํธ๋ฅผ ์ขํ์ญ์์ค.
R/aFIPC.R๊ณผR/surveyFA.R์ ๋ชจ๋ ํธ์ถ์mirt::์ ๋์ฌ๋ฅผ ์ฌ์ฉํฉ๋๋ค. ๊ทธ๋ฌ๋ฏ๋ก ์ ์ฒด ๋ค์์คํ์ด์ค ์ํฌํธ๋ ํ์ํ์ง ์์ต๋๋ค.R/aFIPC.RLine 3์@import mirt๋ฅผ ์ ๊ฑฐํ๊ณDESCRIPTION์Imports๋ง ์ ์งํ๋ฉด ์ด๋ฆ ์ถฉ๋ ์ํ์ด ์ค์ด๋ญ๋๋ค.๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/NAMESPACE` around lines 3 - 5, Remove the broad mirt namespace import from the package metadata, specifically the NAMESPACE import generated from the `@import` mirt directive, while retaining mirt in DESCRIPTIONโs Imports and preserving the existing explicit mirt:: calls in aFIPC and surveyFA.aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R (1)
235-291: ๐ Maintainability & Code Quality | ๐ต Trivial | ๐ค Low value์ค๋ณต๋
!exists()๊ฒ์ฌ๋ฅผ ์ ๋ฆฌํ์ญ์์ค.Line 235-237์
oldFormModel์ด ์์ผ๋ฉด ์ด๋ฏธ ์ค๋จํฉ๋๋ค. ๊ทธ๋ฌ๋ฏ๋ก Line 241๊ณผ Line 265์ ์ฒซ ๋ฒ์งธ!exists()์ ์ ๊ทธ ์์ ์ ํญ์FALSE์ ๋๋ค. ๋ค๋ง Line 248์rm()์ดํ์๋ ์ ํจํฉ๋๋ค. ์กฐ๊ฑด ์๋๋ฅผ ์ฃผ์์ผ๋ก ๋ช ์ํ๊ฑฐ๋, ํฌํผ ํจ์ ํ๋๋ก "๋ชจํ์ด ์๊ฑฐ๋ 2์ฐจ ์กฐ๊ฑด์ ํต๊ณผํ์ง ๋ชปํจ"์ ํํํ์ญ์์ค. ๊ฐ์ ํจํด์ด ์ ํผ ๊ฒฝ๋ก(Line 453-508)์๋ ๋ฐ๋ณต๋ฉ๋๋ค.๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R` around lines 235 - 291, Clarify the redundant model-validity checks in the oldFormModel estimation flow: after the initial guard, the first !exists() terms in the QMCEM and MHRM conditions are always false until rm() runs. Remove those redundant terms or reuse a helper expressing โmodel missing or second-order test failed,โ preserving the post-rm checks. Apply the same cleanup to the corresponding new-form estimation path around its model fallback logic.
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/DESCRIPTION`:
- Line 13: Add mockery to the DESCRIPTION fileโs Suggests dependency list
alongside testthat, so tests/testthat/test-regex-security.R can use
mockery::stub() when only declared dependencies are installed.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R`:
- Around line 763-807: The common-item parameter cache uses input strings
instead of the validated model column names, allowing NULL indices and false
successful-link logs. In the loop over oldformCommonItemNames, update the
newScaleParmsItemIdxCache and oldScaleParmsItemIdxCache lookups to use
newFormItemName and oldFormItemName, and warn when either resolved index is
empty before attempting the parameter assignments.
- Around line 639-647: Update the IPDData construction around IPDItemCount to
handle zero matching items safely: check for no matches before creating or
naming the data frame, and use seq_len(IPDItemCount) instead of 1:IPDItemCount
when generating column names. Preserve the existing assignment flow for cases
where common items are found.
- Around line 87-91: Update the itemtype validation near the initial itemtype
checks to reject vectors longer than one, including when nItems remains NA for
mirt inputs, so all later scalar comparisons such as the itemtype checks in the
affected branches remain valid. Preserve the existing character-type validation
and security-error style.
- Around line 293-366: Wrap each fallback surveyFA() invocation in the shown
retry blocks, including the corresponding fallback calls in the new-form path,
so a stop() from one attempt does not abort autoFIPC(). Preserve the existing
exists("oldFormModel", inherits = FALSE) and OptimInfo$secondordertest checks so
execution proceeds to the next fallback whenever the attempt fails.
- Around line 853-879: Update the forceNormalZeroOne branch to target the
single-factor parameter name "MEAN_1" instead of "MEAN_11", setting
NewScaleParms["MEAN_1", ] est to FALSE and value to 0; ensure the corresponding
OldScaleParms mean parameter is also identified consistently, while preserving
the existing COV_11 handling.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/surveyFA.R`:
- Around line 242-251: Update the candidate-selection logic around the
pThreshold filtering and variance fallback so that when a valid p-value column
exists but no item is below pThreshold, it returns NA_character_ immediately
instead of selecting the minimum-variance item. Preserve the variance-based
fallback only for cases where p-values are unavailable or invalid, using the
surrounding surveyFA logic to distinguish those cases.
- Around line 83-92: Update the response_data column subset in surveyFA to use
drop = FALSE, ensuring a single retained column remains a data frame so the
nrow() and ncol() validation works without a length-zero condition.
- Line 86: Add stats to the package DESCRIPTION Imports alongside the existing
mirt and methods declarations, covering the stats::na.omit, stats::var, and
stats::vcov usages in surveyFA.R and aFIPC.R.
In
`@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R`:
- Around line 63-88: Extend the test around the autoFIPC call and
linked$LinkedModel to assert the latent distribution group parameters produced
with forceNormalZeroOne = TRUE, including the expected fixed mean values. Use
the existing mirt model-value inspection pattern and verify the relevant
group-parameter estimates so a regression in the mean-fixing behavior is
detected, while preserving the current covariance and structural-parameter
checks.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-surveyFA.R`:
- Around line 61-70: Set a deterministic seed before the rbinom calls in the
surveyFA exhaustion test so the generated response data is reproducible across
runs. Follow the seed-setting convention used by the surrounding tests and leave
the test data generation otherwise unchanged.
In `@aFIPC.Rcheck/aFIPC/html/R.css`:
- Around line 126-129: Update the font-family declaration in span.rlang to quote
the spaced font name Courier New, while preserving the existing fallback Courier
and color styling.
In `@aFIPC.Rcheck/aFIPC/NAMESPACE`:
- Around line 3-5: Remove the generated aFIPC.Rcheck directory contents from
version control, including aFIPC.Rcheck/aFIPC/NAMESPACE and the duplicate
test-fixed-parameter-calibration.R under aFIPC.Rcheck/tests/testthat; retain the
test only in the repositoryโs root tests/testthat directory. Add *.Rcheck/ to
.gitignore so future R CMD check output is ignored.
In `@aFIPC.Rcheck/tests/testthat/test-autoFIPC.R`:
- Around line 1-12: Update the test identified by โautoFIPC raises error in
non-interactive session for inputsโ to skip when base R interactive() is TRUE,
before invoking autoFIPC. Keep the existing non-interactive error assertion
unchanged.
---
Outside diff comments:
In `@aFIPC.Rcheck/tests/testthat.Rout`:
- Around line 1-288: Remove all generated R CMD check artifacts from version
control: delete the listed files in aFIPC.Rcheck/tests/testthat.Rout (lines
1-288), aFIPC.Rcheck/00_pkg_src/aFIPC/test_dummy.R (1-3),
aFIPC.Rcheck/00_pkg_src/aFIPC/test_validation.R (1-3),
aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat.R (1-4),
aFIPC.Rcheck/aFIPC/help/aFIPC.rdb (1-14), aFIPC.Rcheck/aFIPC/help/aFIPC.rdx
(1-1), aFIPC.Rcheck/aFIPC/help/aliases.rds (1-1),
aFIPC.Rcheck/aFIPC/help/paths.rds (1-1), aFIPC.Rcheck/tests/startup.Rs (1-3),
and aFIPC.Rcheck/tests/testthat.R (1-4), then add *.Rcheck/ to .gitignore to
prevent these build and check outputs from being tracked again.
---
Nitpick comments:
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/NAMESPACE`:
- Around line 3-5: Remove the broad mirt namespace import from the package
metadata, specifically the NAMESPACE import generated from the `@import` mirt
directive, while retaining mirt in DESCRIPTIONโs Imports and preserving the
existing explicit mirt:: calls in aFIPC and surveyFA.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R`:
- Around line 235-291: Clarify the redundant model-validity checks in the
oldFormModel estimation flow: after the initial guard, the first !exists() terms
in the QMCEM and MHRM conditions are always false until rm() runs. Remove those
redundant terms or reuse a helper expressing โmodel missing or second-order test
failed,โ preserving the post-rm checks. Apply the same cleanup to the
corresponding new-form estimation path around its model fallback logic.
In `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.R`:
- Around line 48-57: Update the itemtype validation in aFIPC so that its error
message is not redundant when nItems equals 1, then revise the corresponding
expect_error assertion in test-autoFIPC.R to match the corrected message.
Preserve the validation behavior for invalid itemtype lengths and anchor the
changes to the aFIPC function and its itemtype test.
In
`@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R`:
- Around line 1-52: Update the test beginning with the autoFIPC calibration
setup to add a CRAN runtime safeguard, preferably by calling skip_on_cran()
before the expensive mirt model fitting; alternatively reduce the simulated
sample size while preserving the testโs calibration coverage.
In
`@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.R`:
- Around line 37-50: Refactor the duplicated optimization logic in R/aFIPC.R
into an internal helper function, then update test-optimization-equivalence.R to
call that helper instead of recreating the expressions locally. Keep the testโs
expected-value and legacy-equivalence assertions, ensuring changes to the
production implementation can cause the test to fail.
In `@aFIPC.Rcheck/aFIPC-Ex.R`:
- Around line 1-44: Remove the entire tracked aFIPC.Rcheck/ directory, including
all listed generated files, and add aFIPC.Rcheck/ to .gitignore so future R CMD
check artifacts are not committed.
๐ช 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 Plus
Run ID: 80bc81c6-ede3-4a98-99a7-072099affefd
โ Files ignored due to path filters (3)
aFIPC.Rcheck/00check.logis excluded by!**/*.logaFIPC.Rcheck/00install.outis excluded by!**/*.outaFIPC.Rcheck/aFIPC-Ex.pdfis excluded by!**/*.pdf
๐ Files selected for processing (51)
.RbuildignoreaFIPC.Rcheck/00_pkg_src/aFIPC/DESCRIPTIONaFIPC.Rcheck/00_pkg_src/aFIPC/LICENSEaFIPC.Rcheck/00_pkg_src/aFIPC/NAMESPACEaFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.RaFIPC.Rcheck/00_pkg_src/aFIPC/R/surveyFA.RaFIPC.Rcheck/00_pkg_src/aFIPC/README.mdaFIPC.Rcheck/00_pkg_src/aFIPC/man/autoFIPC.RdaFIPC.Rcheck/00_pkg_src/aFIPC/man/surveyFA.RdaFIPC.Rcheck/00_pkg_src/aFIPC/test_dummy.RaFIPC.Rcheck/00_pkg_src/aFIPC/test_validation.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-autoFIPC.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-optimization-equivalence.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-package-api.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-regex-security.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-sentinel-validation.RaFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-surveyFA.RaFIPC.Rcheck/aFIPC-Ex.RaFIPC.Rcheck/aFIPC-Ex.RoutaFIPC.Rcheck/aFIPC/DESCRIPTIONaFIPC.Rcheck/aFIPC/INDEXaFIPC.Rcheck/aFIPC/LICENSEaFIPC.Rcheck/aFIPC/Meta/Rd.rdsaFIPC.Rcheck/aFIPC/Meta/features.rdsaFIPC.Rcheck/aFIPC/Meta/hsearch.rdsaFIPC.Rcheck/aFIPC/Meta/links.rdsaFIPC.Rcheck/aFIPC/Meta/nsInfo.rdsaFIPC.Rcheck/aFIPC/Meta/package.rdsaFIPC.Rcheck/aFIPC/NAMESPACEaFIPC.Rcheck/aFIPC/R/aFIPCaFIPC.Rcheck/aFIPC/R/aFIPC.rdbaFIPC.Rcheck/aFIPC/R/aFIPC.rdxaFIPC.Rcheck/aFIPC/help/AnIndexaFIPC.Rcheck/aFIPC/help/aFIPC.rdbaFIPC.Rcheck/aFIPC/help/aFIPC.rdxaFIPC.Rcheck/aFIPC/help/aliases.rdsaFIPC.Rcheck/aFIPC/help/paths.rdsaFIPC.Rcheck/aFIPC/html/00Index.htmlaFIPC.Rcheck/aFIPC/html/R.cssaFIPC.Rcheck/tests/startup.RsaFIPC.Rcheck/tests/testthat.RaFIPC.Rcheck/tests/testthat.RoutaFIPC.Rcheck/tests/testthat/test-autoFIPC.RaFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.RaFIPC.Rcheck/tests/testthat/test-optimization-equivalence.RaFIPC.Rcheck/tests/testthat/test-package-api.RaFIPC.Rcheck/tests/testthat/test-regex-security.RaFIPC.Rcheck/tests/testthat/test-sentinel-validation.RaFIPC.Rcheck/tests/testthat/test-surveyFA.R
| the item response theory paradigm using mirt package estimates. | ||
| License: GPL-3 | file LICENSE | ||
| Imports: mirt, methods | ||
| Suggests: testthat (>= 3.0.0) |
There was a problem hiding this comment.
๐ Maintainability & Code Quality | ๐ Major | โก Quick win
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(DESCRIPTION|test-regex-security\.R)$' || true
printf '%s\n' '--- DESCRIPTION ---'
desc="$(git ls-files | rg '(^|/)DESCRIPTION$' | head -n 1)"
[ -n "$desc" ] && cat -n "$desc"
printf '%s\n' '--- mockery references ---'
rg -n -C 3 'mockery|stub\s*\(' --glob '*.R' --glob 'DESCRIPTION' . || trueRepository: ContextualWisdomLab/aFIPC
Length of output: 8996
๐ Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
description = Path("DESCRIPTION").read_text(encoding="utf-8")
suggests = re.search(r"(?ms)^Suggests:\s*(.*?)(?=^[A-Za-z][A-Za-z0-9.-]*:|\Z)", description)
suggested = set()
if suggests:
suggested = set(re.findall(r"([A-Za-z][A-Za-z0-9.]*)\s*(?:\([^)]*\))?", suggests.group(1)))
test_files = [Path("tests/testthat/test-regex-security.R")]
refs = set()
for path in test_files:
text = path.read_text(encoding="utf-8")
refs.update(re.findall(r"\b([A-Za-z][A-Za-z0-9.]*)::", text))
print("DESCRIPTION:", Path("DESCRIPTION"))
print("Suggests:", sorted(suggested))
print("Test namespace references:", sorted(refs))
print("mockery referenced:", "mockery" in refs)
print("mockery declared in Suggests:", "mockery" in suggested)
print("mockery declared anywhere in DESCRIPTION:", bool(re.search(r"(?im)^\s*mockery\b", description)))
PY
printf '%s\n' '--- tracked mockery/package metadata ---'
git ls-files | rg -i '(^|/)(mockery|DESCRIPTION|packrat/[^/]*(lock|init)|renv\.lock|dependencies)' | head -n 200 || true
printf '%s\n' '--- package test helper files ---'
git ls-files tests | sortRepository: ContextualWisdomLab/aFIPC
Length of output: 3631
mockery๋ฅผ Suggests์ ์ถ๊ฐํ์ญ์์ค.
tests/testthat/test-regex-security.R๋ mockery::stub()๋ฅผ ์ฌ์ฉํ์ง๋ง DESCRIPTION์๋ testthat๋ง ์ ์ธ๋์ด ์์ต๋๋ค. ์ ์ธ๋ ์์กด์ฑ๋ง ์ค์นํ ํ๊ฒฝ์์๋ ํ
์คํธ๊ฐ ์คํจํฉ๋๋ค.
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/DESCRIPTION` at line 13, Add mockery to the
DESCRIPTION fileโs Suggests dependency list alongside testthat, so
tests/testthat/test-regex-security.R can use mockery::stub() when only declared
dependencies are installed.
| if (!is.character(itemtype)) stop('Security Error: itemtype must be a character vector') | ||
| nItems <- NA_integer_ | ||
| if (is.data.frame(newformXData) || is.matrix(newformXData)) nItems <- ncol(as.data.frame(newformXData)) | ||
| else if (is.data.frame(oldformYData) || is.matrix(oldformYData)) nItems <- ncol(as.data.frame(oldformYData)) | ||
| if (!is.na(nItems) && !(length(itemtype) == 1 || length(itemtype) == nItems)) stop(sprintf('Security Error: itemtype must be length 1 or length %d (number of items).', nItems)) |
There was a problem hiding this comment.
๐ฏ Functional Correctness | ๐ Major | ๐๏ธ Heavy lift
itemtype ๊ธธ์ด ๊ฒ์ฆ๊ณผ ์ดํ if (itemtype == '3PL') ๋น๊ต๊ฐ ์ถฉ๋ํฉ๋๋ค.
Line 91์ length(itemtype) == nItems๋ฅผ ํ์ฉํฉ๋๋ค. ๋ ์
๋ ฅ์ด ๋ชจ๋ mirt ๋ชจํ์ด๋ฉด nItems๊ฐ NA๋ก ๋จ์ ๊ธธ์ด ๊ฒ์ฆ ์์ฒด๊ฐ ์๋ต๋ฉ๋๋ค. ๊ทธ ๋ค์ Line 166, 189, 385, 408, 607, 657, 891์ itemtype == '3PL'์ ์ค์นผ๋ผ ์กฐ๊ฑด์ผ๋ก ์ฌ์ฉํฉ๋๋ค. itemtype ๊ธธ์ด๊ฐ 1๋ณด๋ค ํฌ๋ฉด R 4.2 ์ด์์์ if ์กฐ๊ฑด ๊ธธ์ด ์ค๋ฅ๊ฐ ๋ฐ์ํฉ๋๋ค.
๋ฌธํญ๋ณ itemtype ๋ฒกํฐ๋ฅผ ์ค์ ๋ก ์ง์ํ๋ ค๋ฉด ๋ชจ๋ ๋ถ๊ธฐ ์กฐ๊ฑด์ identical(itemtype, '3PL') ๋๋ all(itemtype == '3PL') ํํ๋ก ๋ฐ๊พธ์ญ์์ค. ์ง์ํ์ง ์๋๋ค๋ฉด ๊ธธ์ด 1๋ง ํ์ฉํ์ญ์์ค.
๐ง ๊ธธ์ด 1๋ง ํ์ฉํ๋ ๋ฐฉ์
- if (!is.na(nItems) && !(length(itemtype) == 1 || length(itemtype) == nItems)) stop(sprintf('Security Error: itemtype must be length 1 or length %d (number of items).', nItems))
+ if (length(itemtype) != 1 || is.na(itemtype)) stop('Security Error: itemtype must be a single non-NA character value.')๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!is.character(itemtype)) stop('Security Error: itemtype must be a character vector') | |
| nItems <- NA_integer_ | |
| if (is.data.frame(newformXData) || is.matrix(newformXData)) nItems <- ncol(as.data.frame(newformXData)) | |
| else if (is.data.frame(oldformYData) || is.matrix(oldformYData)) nItems <- ncol(as.data.frame(oldformYData)) | |
| if (!is.na(nItems) && !(length(itemtype) == 1 || length(itemtype) == nItems)) stop(sprintf('Security Error: itemtype must be length 1 or length %d (number of items).', nItems)) | |
| if (!is.character(itemtype)) stop('Security Error: itemtype must be a character vector') | |
| nItems <- NA_integer_ | |
| if (is.data.frame(newformXData) || is.matrix(newformXData)) nItems <- ncol(as.data.frame(newformXData)) | |
| else if (is.data.frame(oldformYData) || is.matrix(oldformYData)) nItems <- ncol(as.data.frame(oldformYData)) | |
| if (length(itemtype) != 1 || is.na(itemtype)) stop('Security Error: itemtype must be a single non-NA character value.') |
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R` around lines 87 - 91, Update the
itemtype validation near the initial itemtype checks to reject vectors longer
than one, including when nItems remains NA for mirt inputs, so all later scalar
comparisons such as the itemtype checks in the affected branches remain valid.
Preserve the existing character-type validation and security-error style.
| if ( | ||
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | ||
| itemtype != 'ideal') | ||
| ) { | ||
| message( | ||
| 'Estimation failed. trying to remove weird items by itemfit statistics' | ||
| ) | ||
| try(rm(oldFormModel)) | ||
|
|
||
| oldFormModel <- | ||
| surveyFA( | ||
| oldformYData, | ||
| autofix = F, | ||
| SE = T, | ||
| forceUIRT = T | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | ||
| itemtype != 'ideal') | ||
| ) { | ||
| message( | ||
| 'Estimation failed. trying to remove weird items by itemfit statistics by normal MMLE/EM' | ||
| ) | ||
| try(rm(oldFormModel)) | ||
|
|
||
| oldFormModel <- | ||
| surveyFA( | ||
| oldformYData, | ||
| autofix = F, | ||
| SE = T, | ||
| forceUIRT = T, | ||
| forceNormalEM = T | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | ||
| itemtype != 'ideal') | ||
| ) { | ||
| message( | ||
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/QMCEM' | ||
| ) | ||
| try(rm(oldFormModel)) | ||
|
|
||
| oldFormModel <- | ||
| surveyFA( | ||
| oldformYData, | ||
| autofix = F, | ||
| SE = T, | ||
| forceUIRT = T, | ||
| unstable = T | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | ||
| itemtype != 'ideal') | ||
| ) { | ||
| message( | ||
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/MHRM' | ||
| ) | ||
| try(rm(oldFormModel)) | ||
|
|
||
| oldFormModel <- | ||
| surveyFA( | ||
| oldformYData, | ||
| autofix = F, | ||
| SE = T, | ||
| forceUIRT = T, | ||
| forceMHRM = T | ||
| ) | ||
| } |
There was a problem hiding this comment.
๐ฉบ Stability & Availability | ๐ Major | โก Quick win
surveyFA() ๋์ฒด ๊ฒฝ๋ก๊ฐ ์ค๋ฅ ์ ๋ค์ ๋จ๊ณ๋ก ์งํํ์ง ์์ต๋๋ค.
surveyFA()๋ ์คํจํ ๋ stop()์ ํธ์ถํฉ๋๋ค (R/surveyFA.R Line 285-290). Line 302, 320, 339, 358์ ํธ์ถ์ try()๋ก ๊ฐ์ธ์ง ์์์ต๋๋ค. ์ฒซ ๋ฒ์งธ surveyFA() ํธ์ถ์ด ์คํจํ๋ฉด autoFIPC()๊ฐ ์ฆ์ ์ค๋จ๋ฉ๋๋ค. ๋ฐ๋ผ์ Line 311-366์ ๋๋จธ์ง ๋ค ๊ฐ ๋์ฒด ๊ฒฝ๋ก๋ ์คํ๋์ง ์์ต๋๋ค. ์ ํผ ๊ฒฝ๋ก(Line 511-584)์๋ ๊ฐ์ ๋ฌธ์ ๊ฐ ์์ต๋๋ค.
๊ฐ ๋์ฒด ํธ์ถ์ try()๋ก ๊ฐ์ธ๊ณ exists() ๊ฒ์ฌ๋ก ๋ค์ ๋จ๊ณ๋ฅผ ์งํํ์ญ์์ค.
๐ง ์ฒซ ๋ฒ์งธ ๋์ฒด ํธ์ถ ์์
- oldFormModel <-
- surveyFA(
- oldformYData,
- autofix = F,
- SE = T,
- forceUIRT = T
- )
+ try(
+ oldFormModel <-
+ surveyFA(
+ oldformYData,
+ autofix = F,
+ SE = T,
+ forceUIRT = T
+ ),
+ silent = TRUE
+ )๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by normal MMLE/EM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| forceNormalEM = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/QMCEM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| unstable = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/MHRM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| forceMHRM = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics' | |
| ) | |
| try(rm(oldFormModel)) | |
| try( | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T | |
| ), | |
| silent = TRUE | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by normal MMLE/EM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| forceNormalEM = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/QMCEM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| unstable = T | |
| ) | |
| } | |
| if ( | |
| (!exists("oldFormModel", inherits = FALSE)) || (!isTRUE(oldFormModel@OptimInfo$secondordertest) && | |
| itemtype != 'ideal') | |
| ) { | |
| message( | |
| 'Estimation failed. trying to remove weird items by itemfit statistics by MMLE/MHRM' | |
| ) | |
| try(rm(oldFormModel)) | |
| oldFormModel <- | |
| surveyFA( | |
| oldformYData, | |
| autofix = F, | |
| SE = T, | |
| forceUIRT = T, | |
| forceMHRM = T | |
| ) | |
| } |
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R` around lines 293 - 366, Wrap each
fallback surveyFA() invocation in the shown retry blocks, including the
corresponding fallback calls in the new-form path, so a stop() from one attempt
does not abort autoFIPC(). Preserve the existing exists("oldFormModel", inherits
= FALSE) and OptimInfo$secondordertest checks so execution proceeds to the next
fallback whenever the attempt fails.
| IPDData <- | ||
| data.frame(matrix(nrow = length(IPDgroup), ncol = IPDItemCount)) | ||
| colnames(IPDData) <- paste0('X', 1:IPDItemCount) | ||
| print(IPDItemNamesOldForm) | ||
| print(IPDItemNamesNewForm) | ||
| IPDData[1:nrow(oldformYDataK), ] <- | ||
| oldformYDataK[, IPDItemNamesOldForm] | ||
| IPDData[nrow(oldformYDataK) + 1:nrow(newformXDataK), ] <- | ||
| newformXDataK[, IPDItemNamesNewForm] |
There was a problem hiding this comment.
๐ฉบ Stability & Availability | ๐ก Minor | โก Quick win
IPDItemCount๊ฐ 0์ด๋ฉด colnames() ํ ๋น์ด ์คํจํฉ๋๋ค.
Line 641์ 1:IPDItemCount๋ฅผ ์ฌ์ฉํฉ๋๋ค. ๊ณตํต ๋ฌธํญ ์ด๋ฆ์ด ์ด๋ ์ด ์ด๋ฆ๊ณผ๋ ์ผ์นํ์ง ์์ผ๋ฉด IPDItemCount๋ 0์ด ๋ฉ๋๋ค. ๊ทธ๋ฌ๋ฉด 1:0์ด ๊ธธ์ด 2 ๋ฒกํฐ๋ฅผ ๋ง๋ค๊ณ , 0์ด ๋ฐ์ดํฐ ํ๋ ์์ ๋ํ colnames() ํ ๋น์ด ์ค๋ฅ๋ฅผ ๋
๋๋ค. seq_len()์ ์ฌ์ฉํ๊ณ ๋งค์นญ ์คํจ๋ฅผ ๋จผ์ ๊ฒ์ฌํ์ญ์์ค.
๐ง ์ ์ ์์
+ if (IPDItemCount == 0) {
+ stop('No common items matched the model column names for IPD analysis.')
+ }
IPDData <-
data.frame(matrix(nrow = length(IPDgroup), ncol = IPDItemCount))
- colnames(IPDData) <- paste0('X', 1:IPDItemCount)
+ colnames(IPDData) <- paste0('X', seq_len(IPDItemCount))๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IPDData <- | |
| data.frame(matrix(nrow = length(IPDgroup), ncol = IPDItemCount)) | |
| colnames(IPDData) <- paste0('X', 1:IPDItemCount) | |
| print(IPDItemNamesOldForm) | |
| print(IPDItemNamesNewForm) | |
| IPDData[1:nrow(oldformYDataK), ] <- | |
| oldformYDataK[, IPDItemNamesOldForm] | |
| IPDData[nrow(oldformYDataK) + 1:nrow(newformXDataK), ] <- | |
| newformXDataK[, IPDItemNamesNewForm] | |
| if (IPDItemCount == 0) { | |
| stop('No common items matched the model column names for IPD analysis.') | |
| } | |
| IPDData <- | |
| data.frame(matrix(nrow = length(IPDgroup), ncol = IPDItemCount)) | |
| colnames(IPDData) <- paste0('X', seq_len(IPDItemCount)) | |
| print(IPDItemNamesOldForm) | |
| print(IPDItemNamesNewForm) | |
| IPDData[1:nrow(oldformYDataK), ] <- | |
| oldformYDataK[, IPDItemNamesOldForm] | |
| IPDData[nrow(oldformYDataK) + 1:nrow(newformXDataK), ] <- | |
| newformXDataK[, IPDItemNamesNewForm] |
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R` around lines 639 - 647, Update the
IPDData construction around IPDItemCount to handle zero matching items safely:
check for no matches before creating or naming the data frame, and use
seq_len(IPDItemCount) instead of 1:IPDItemCount when generating column names.
Preserve the existing assignment flow for cases where common items are found.
| for (i in seq_along(oldformCommonItemNames)) { | ||
| newFormItemStr <- newformCommonItemNames[i] | ||
| oldFormItemStr <- oldformCommonItemNames[i] | ||
|
|
||
| newFormItemName <- newFormColNames[idxNew_all[i]] | ||
| oldFormItemName <- oldFormColNames[idxOld_all[i]] | ||
|
|
||
| if ( | ||
| !is.na(newFormItemName) && | ||
| !is.na(oldFormItemName) && | ||
| (length(stats::na.omit(unique(newFormModel@Data$data[, newFormItemName]))) == | ||
| length(stats::na.omit(unique(oldFormModel@Data$data[, oldFormItemName])))) | ||
| ) { | ||
| message( | ||
| 'applying ', | ||
| newFormItemStr, | ||
| ' <<< ', | ||
| oldFormItemStr, | ||
| ' as common item use' | ||
| ) | ||
|
|
||
| # โก Bolt: Use cached O(1) dictionary lookups instead of O(N) which() scans | ||
| newIdx <- newScaleParmsItemIdxCache[[newFormItemStr]] | ||
| oldIdx <- oldScaleParmsItemIdxCache[[oldFormItemStr]] | ||
|
|
||
| # โก Bolt: Remove unnecessary paste0() array string generation overhead | ||
| message(' Newform Parms: ', paste(NewScaleParms[newIdx, "value"], collapse = ' ')) | ||
| message(' Oldform Parms: ', paste(OldScaleParms[oldIdx, "value"], collapse = ' ')) | ||
|
|
||
| NewScaleParms[newIdx, "value"] <- | ||
| OldScaleParms[oldIdx, "value"] | ||
| message(' Linkedform Parms: ', paste(NewScaleParms[newIdx, "value"], collapse = ' '), '\n') | ||
|
|
||
| NewScaleParms[newIdx, "est"] <- | ||
| FALSE | ||
| } else { | ||
| message( | ||
| 'skipping ', | ||
| newFormItemStr, | ||
| ' <<< ', | ||
| oldFormItemStr, | ||
| ' as common item use' | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
๐ฏ Functional Correctness | ๐ก Minor | โก Quick win
๊ณตํต ๋ฌธํญ ์ธ๋ฑ์ค ์บ์ ์กฐํ ํค๊ฐ ์ด ์ด๋ฆ ๊ฒ์ฆ๊ณผ ๋ค๋ฆ ๋๋ค.
Line 767, 768์ ๋ชจํ ๋ฐ์ดํฐ์ ์ด ์ด๋ฆ(newFormColNames)์ผ๋ก ์กด์ฌ๋ฅผ ํ์ธํฉ๋๋ค. Line 785, 786์ ์ฌ์ฉ์ ์
๋ ฅ ๋ฌธ์์ด(newFormItemStr)๋ก ์บ์๋ฅผ ์กฐํํฉ๋๋ค. ๋ ๊ฐ์ด ๋ค๋ฅด๋ฉด (์: data.frame()์ด make.names()๋ก ์ด๋ฆ์ ๋ณ๊ฒฝํ ๊ฒฝ์ฐ) ์ธ๋ฑ์ค๊ฐ NULL์ด ๋๊ณ , Line 792-797์ ํ ๋น์ด ์๋ฌด ํ๋ ๋ณ๊ฒฝํ์ง ์์ต๋๋ค. ๊ทธ๋๋ ๋ก๊ทธ๋ ๋งํน ์ฑ๊ณต์ผ๋ก ํ์๋ฉ๋๋ค.
ํด๊ฒฐ๋ ์ด๋ฆ(newFormItemName, oldFormItemName)์ผ๋ก ์บ์๋ฅผ ์กฐํํ๊ณ , ์ธ๋ฑ์ค๊ฐ ๋น์ด ์์ผ๋ฉด ๊ฒฝ๊ณ ๋ฅผ ๋ฐ์์ํค์ญ์์ค.
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R` around lines 763 - 807, The
common-item parameter cache uses input strings instead of the validated model
column names, allowing NULL indices and false successful-link logs. In the loop
over oldformCommonItemNames, update the newScaleParmsItemIdxCache and
oldScaleParmsItemIdxCache lookups to use newFormItemName and oldFormItemName,
and warn when either resolved index is empty before attempting the parameter
assignments.
| linked <- aFIPC::autoFIPC( | ||
| newformXData = new_model, | ||
| oldformYData = old_model, | ||
| newformCommonItemNames = new_common_items, | ||
| oldformCommonItemNames = old_common_items, | ||
| itemtype = "2PL", | ||
| checkIPD = FALSE, | ||
| tryEM = TRUE, | ||
| freeMEAN = FALSE, | ||
| forceNormalZeroOne = TRUE, | ||
| confirmCommonItems = TRUE | ||
| ) | ||
|
|
||
| linked_vcov <- as.matrix(linked$LinkedModel@vcov) | ||
| expect_gt(nrow(linked_vcov), 0) | ||
| expect_true(all(is.finite(diag(linked_vcov)))) | ||
| expect_true(isTRUE(linked$LinkedModel@OptimInfo$secondordertest)) | ||
|
|
||
| old_values <- mirt::mod2values(old_model) | ||
| linked_values <- mirt::mod2values(linked$LinkedModel) | ||
| linked_structural <- linked_values[ | ||
| linked_values$item %in% new_item_names[5:6] & | ||
| linked_values$name %in% c("g", "u"), | ||
| "est" | ||
| ] | ||
| expect_false(any(linked_structural)) |
There was a problem hiding this comment.
๐ฏ Functional Correctness | ๐ก Minor | โก Quick win
forceNormalZeroOne = TRUE์ ํจ๊ณผ๋ฅผ ๊ฒ์ฆํ์ง ์์ต๋๋ค.
์ด ํ
์คํธ๋ forceNormalZeroOne = TRUE๋ฅผ ์ ๋ฌํ์ง๋ง, ์ ์ฌ ๋ถํฌ ๋ชจ์๋ฅผ ํ์ธํ์ง ์์ต๋๋ค. R/aFIPC.R Line 856-868์ ํ๊ท ๊ณ ์ ๊ฒฐํจ์ด ์ด ํ
์คํธ๋ก ๊ฒ์ถ๋์ง ์์ต๋๋ค. ์ฐ๊ฒฐ ๋ชจํ์ ๊ทธ๋ฃน ๋ชจ์์ ๋ํ ๊ธฐ๋๊ฐ์ ์ถ๊ฐํ์ญ์์ค.
๐ง ์ ์ ์ถ๊ฐ ๊ฒ์ฆ
linked_values <- mirt::mod2values(linked$LinkedModel)
+ group_values <- linked_values[linked_values$item == "GROUP", ]
+ expect_equal(group_values$value[group_values$name == "MEAN_1"], 0)
+ expect_equal(group_values$value[group_values$name == "COV_11"], 1)
+ expect_false(any(group_values$est))๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| linked <- aFIPC::autoFIPC( | |
| newformXData = new_model, | |
| oldformYData = old_model, | |
| newformCommonItemNames = new_common_items, | |
| oldformCommonItemNames = old_common_items, | |
| itemtype = "2PL", | |
| checkIPD = FALSE, | |
| tryEM = TRUE, | |
| freeMEAN = FALSE, | |
| forceNormalZeroOne = TRUE, | |
| confirmCommonItems = TRUE | |
| ) | |
| linked_vcov <- as.matrix(linked$LinkedModel@vcov) | |
| expect_gt(nrow(linked_vcov), 0) | |
| expect_true(all(is.finite(diag(linked_vcov)))) | |
| expect_true(isTRUE(linked$LinkedModel@OptimInfo$secondordertest)) | |
| old_values <- mirt::mod2values(old_model) | |
| linked_values <- mirt::mod2values(linked$LinkedModel) | |
| linked_structural <- linked_values[ | |
| linked_values$item %in% new_item_names[5:6] & | |
| linked_values$name %in% c("g", "u"), | |
| "est" | |
| ] | |
| expect_false(any(linked_structural)) | |
| linked <- aFIPC::autoFIPC( | |
| newformXData = new_model, | |
| oldformYData = old_model, | |
| newformCommonItemNames = new_common_items, | |
| oldformCommonItemNames = old_common_items, | |
| itemtype = "2PL", | |
| checkIPD = FALSE, | |
| tryEM = TRUE, | |
| freeMEAN = FALSE, | |
| forceNormalZeroOne = TRUE, | |
| confirmCommonItems = TRUE | |
| ) | |
| linked_vcov <- as.matrix(linked$LinkedModel@vcov) | |
| expect_gt(nrow(linked_vcov), 0) | |
| expect_true(all(is.finite(diag(linked_vcov)))) | |
| expect_true(isTRUE(linked$LinkedModel@OptimInfo$secondordertest)) | |
| old_values <- mirt::mod2values(old_model) | |
| linked_values <- mirt::mod2values(linked$LinkedModel) | |
| group_values <- linked_values[linked_values$item == "GROUP", ] | |
| expect_equal(group_values$value[group_values$name == "MEAN_1"], 0) | |
| expect_equal(group_values$value[group_values$name == "COV_11"], 1) | |
| expect_false(any(group_values$est)) | |
| linked_structural <- linked_values[ | |
| linked_values$item %in% new_item_names[5:6] & | |
| linked_values$name %in% c("g", "u"), | |
| "est" | |
| ] | |
| expect_false(any(linked_structural)) |
๐ค 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
`@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R`
around lines 63 - 88, Extend the test around the autoFIPC call and
linked$LinkedModel to assert the latent distribution group parameters produced
with forceNormalZeroOne = TRUE, including the expected fixed mean values. Use
the existing mirt model-value inspection pattern and verify the relevant
group-parameter estimates so a regression in the mean-fixing behavior is
detected, while preserving the current covariance and structural-parameter
checks.
| test_that("surveyFA reports bounded recovery exhaustion when unrecoverable", { | ||
| skip_if_not_installed("mirt") | ||
|
|
||
| raw <- as.data.frame( | ||
| matrix( | ||
| c(rbinom(80, 1, 0.5), rbinom(80, 1, 0.4)), | ||
| ncol = 2 | ||
| ) | ||
| ) | ||
| names(raw) <- paste0("item", 1:2) |
There was a problem hiding this comment.
๐ Maintainability & Code Quality | ๐ก Minor | โก Quick win
set.seed()๋ฅผ ์ถ๊ฐํ์ฌ ์ฌํ์ฑ์ ํ๋ณดํ์ญ์์ค.
Line 66์ rbinom()์ ์๋ ์์ด ํธ์ถํฉ๋๋ค. ๊ทธ๋ฌ๋ฏ๋ก ๊ฐ ์คํ๋ง๋ค ์๋ต ๋ฐ์ดํฐ๊ฐ ๋ฌ๋ผ์ง๊ณ , ์คํจ๊ฐ ์ฌํ๋์ง ์์ ์ ์์ต๋๋ค. ๋ค๋ฅธ ํ
์คํธ์ ๋์ผํ๊ฒ ์๋๋ฅผ ๊ณ ์ ํ์ญ์์ค.
๐ง ์ ์ ์์
test_that("surveyFA reports bounded recovery exhaustion when unrecoverable", {
skip_if_not_installed("mirt")
+ set.seed(20260703)๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_that("surveyFA reports bounded recovery exhaustion when unrecoverable", { | |
| skip_if_not_installed("mirt") | |
| raw <- as.data.frame( | |
| matrix( | |
| c(rbinom(80, 1, 0.5), rbinom(80, 1, 0.4)), | |
| ncol = 2 | |
| ) | |
| ) | |
| names(raw) <- paste0("item", 1:2) | |
| test_that("surveyFA reports bounded recovery exhaustion when unrecoverable", { | |
| skip_if_not_installed("mirt") | |
| set.seed(20260703) | |
| raw <- as.data.frame( | |
| matrix( | |
| c(rbinom(80, 1, 0.5), rbinom(80, 1, 0.4)), | |
| ncol = 2 | |
| ) | |
| ) | |
| names(raw) <- paste0("item", 1:2) |
๐ค 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 `@aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-surveyFA.R` around lines 61
- 70, Set a deterministic seed before the rbinom calls in the surveyFA
exhaustion test so the generated response data is reproducible across runs.
Follow the seed-setting convention used by the surrounding tests and leave the
test data generation otherwise unchanged.
| span.rlang { | ||
| font-family: Courier New, Courier; | ||
| color: #666666; | ||
| } |
There was a problem hiding this comment.
๐ Maintainability & Code Quality | ๐ก Minor | โก Quick win
Stylelint ์ค๋ฅ๋ฅผ ์์ ํ์ธ์.
Line 127์ Courier New๋ ๊ณต๋ฐฑ์ด ์๋ ๊ธ๊ผด ์ด๋ฆ์
๋๋ค. ๋ฐ์ดํ๋ก ๊ฐ์ธ์ font-family-name-quotes ์ค๋ฅ๋ฅผ ์ ๊ฑฐํ์ธ์.
์์ ์์
span.rlang {
- font-family: Courier New, Courier;
+ font-family: "Courier New", Courier;
color: `#666666`;
}๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| span.rlang { | |
| font-family: Courier New, Courier; | |
| color: #666666; | |
| } | |
| span.rlang { | |
| font-family: "Courier New", Courier; | |
| color: `#666666`; | |
| } |
๐งฐ Tools
๐ช Stylelint (17.14.1)
[error] 127-127: Expected quotes around "Courier New" (font-family-name-quotes)
(font-family-name-quotes)
๐ค 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 `@aFIPC.Rcheck/aFIPC/html/R.css` around lines 126 - 129, Update the font-family
declaration in span.rlang to quote the spaced font name Courier New, while
preserving the existing fallback Courier and color styling.
Source: Linters/SAST tools
| export(autoFIPC) | ||
| export(surveyFA) | ||
| import(mirt) |
There was a problem hiding this comment.
๐ Maintainability & Code Quality | ๐ Major | โก Quick win
R CMD check ์ฐ์ถ๋ฌผ ๋๋ ํฐ๋ฆฌ๊ฐ ์ ์ฅ์์ ์ปค๋ฐ๋์์ต๋๋ค. aFIPC.Rcheck/๋ R CMD check๊ฐ ์์ฑํ๋ ๋๋ ํฐ๋ฆฌ์
๋๋ค. ๊ทธ ๊ฒฐ๊ณผ ์์ค ํ์ผ์ด ์ฌ๋ฌ ์์น์ ์ค๋ณต๋๊ณ , ํธ์งํ ์ฌ๋ณธ๊ณผ ์์ฑ๋ ์ฌ๋ณธ์ด ์๋ก ์ด๊ธ๋ ์ ์์ต๋๋ค.
aFIPC.Rcheck/aFIPC/NAMESPACE#L3-L5: ์ด ์์ฑ๋ ์ฌ๋ณธ์ ์ถ์ ๋์์์ ์ ๊ฑฐํ๊ณ ,.gitignore์*.Rcheck/๋ฅผ ์ถ๊ฐํ์ญ์์ค.aFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.R#L1-L123: ์ด ํ์ผ์aFIPC.Rcheck/00_pkg_src/aFIPC/tests/testthat/test-fixed-parameter-calibration.R์ ๋์ผํฉ๋๋ค. ํ ์คํธ ์๋ณธ์ ์ ์ฅ์ ๋ฃจํธ์tests/testthat/์๋ง ์ ์งํ๊ณ ์ด ์ฌ๋ณธ์ ์ ๊ฑฐํ์ญ์์ค.
๐ Affects 2 files
aFIPC.Rcheck/aFIPC/NAMESPACE#L3-L5(this comment)aFIPC.Rcheck/tests/testthat/test-fixed-parameter-calibration.R#L1-L123
๐ค 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 `@aFIPC.Rcheck/aFIPC/NAMESPACE` around lines 3 - 5, Remove the generated
aFIPC.Rcheck directory contents from version control, including
aFIPC.Rcheck/aFIPC/NAMESPACE and the duplicate
test-fixed-parameter-calibration.R under aFIPC.Rcheck/tests/testthat; retain the
test only in the repositoryโs root tests/testthat directory. Add *.Rcheck/ to
.gitignore so future R CMD check output is ignored.
| test_that("autoFIPC raises error in non-interactive session for inputs", { | ||
| # interactive() should be FALSE by default in testthat environments | ||
| expect_error( | ||
| aFIPC::autoFIPC( | ||
| newformXData = data.frame(A=1), | ||
| oldformYData = data.frame(A=2), | ||
| newformCommonItemNames = c('A'), | ||
| oldformCommonItemNames = c('A') | ||
| ), | ||
| "Common item confirmation requires an interactive session" | ||
| ) | ||
| }) |
There was a problem hiding this comment.
๐ฉบ Stability & Availability | ๐ Major | โก Quick win
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
set -euo pipefail
# autoFIPC()์ interactive() ๋ถ๊ธฐ์ ํ
์คํธ์ ๊ธฐ๋ ์ค๋ฅ๊ฐ ๊ฐ์ ๊ฒฝ๋ก์ธ์ง ํ์ธํ๋ค.
ast-grep outline aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R --items all || true
rg -n -C 5 'interactive\(\)|readline|Common item confirmation requires an interactive session' \
aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R \
aFIPC.Rcheck/tests/testthat/test-autoFIPC.RRepository: ContextualWisdomLab/aFIPC
Length of output: 5143
๐ Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package metadata and test configuration ---'
fd -i '^(DESCRIPTION|NAMESPACE|.*test.*|.*config.*)$' . | sed -n '1,120p'
rg -n -C 4 'testthat|devtools::test|autoFIPC|confirmCommonItems' \
DESCRIPTION NAMESPACE .github tests aFIPC.Rcheck 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- relevant source and tests ---'
sed -n '110,155p' aFIPC.Rcheck/00_pkg_src/aFIPC/R/aFIPC.R
sed -n '1,80p' aFIPC.Rcheck/tests/testthat/test-autoFIPC.R
printf '%s\n' '--- available R tooling, without loading repository code ---'
command -v Rscript || true
Rscript --version 2>/dev/null || trueRepository: ContextualWisdomLab/aFIPC
Length of output: 28165
๐ Web query:
R testthat interactive() devtools::test interactive session behavior
๐ก Result:
When running tests via devtools::test or testthat, the R session is explicitly configured to simulate a non-interactive environment to ensure reproducible results [1][2]. Key behavioral details include: 1. Interaction Suppression: During tests, the testthat package automatically calls local_test_context(), which sets the option rlang_interactive = FALSE [1][2]. This ensures that rlang::is_interactive() returns FALSE, causing your code to behave as if it is not in an interactive session [1][2]. This override is intentionally designed to prevent tests from waiting for user input or behaving differently due to terminal capabilities [1]. 2. Environment Variables: local_test_context() also sets the environment variable TESTTHAT = "true" [1]. Historically, this was the primary mechanism used to signal to code that tests were running; modern R development practice prefers rlang::is_interactive() for this purpose [3][4]. 3. Testing vs. Interactive Development: Because interactive() (from base R) and rlang::is_interactive() will return FALSE during these tests, you cannot directly test code paths that require an interactive terminal by simply calling test() [3][5]. If you need to test code that behaves differently based on interactivity, it is recommended to use rlang::is_interactive() and control its behavior by setting the rlang_interactive option during your test, rather than trying to override the base interactive() function, which is difficult to mock reliably [3][5]. 4. Mocking: While it is possible to mock functions like interactive() by binding them to NULL in your package's namespace and using local_mocked_bindings(), this approach is generally discouraged in favor of the rlang abstraction, which is built to be configurable for testing environments [3][6][5]. In summary, devtools::test enforces a non-interactive state to guarantee consistency [1]. If your code relies on interactivity, you should refactor it to check rlang::is_interactive() and use testthat's built-in tools to manipulate that state within your tests [3][1].
Citations:
- 1: https://testthat.r-lib.org/reference/local_test_context.html
- 2: https://r-lib.r-universe.dev/testthat/doc/manual.html
- 3: https://testthat.r-lib.org/articles/test-fixtures.html
- 4: Make is_interactive() sensitive to TESTTHAT env varย r-lib/rlang#738
- 5: https://cran.r-project.org/web/packages/testthat/vignettes/test-fixtures.html
- 6: https://testthat.r-lib.org/reference/local_mocked_bindings.html
๐ Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bundled testthat/devtools sources ---'
fd -i '^(testthat|devtools)$' packrat/lib 2>/dev/null | sed -n '1,80p'
rg -n -C 5 'local_test_context|rlang_interactive|test_dir|interactive[[:space:]]*=' \
packrat/lib 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- project tests that exercise the same branch ---'
sed -n '1,55p' tests/testthat/test-package-api.R 2>/dev/null || true
sed -n '1,35p' tests/testthat/test-autoFIPC.R 2>/dev/null || trueRepository: ContextualWisdomLab/aFIPC
Length of output: 4954
๐ Web query:
site:testthat.r-lib.org testthat interactive() base R test_dir devtools::test
๐ก Result:
In the testthat ecosystem, the interaction between test execution and environment settings is managed primarily through automated test contexts. Interactive Execution vs. Testing When running tests, testthat attempts to create a reproducible, non-interactive environment to ensure consistent results regardless of the user's local settings [1]. The function testthat::local_test_context (called automatically by test_that) sets the option rlang_interactive = FALSE, ensuring that rlang::is_interactive returns FALSE [1]. This allows your code to behave as if it were running in a non-interactive, automated environment [1]. Note that base R's interactive function cannot be reliably controlled via test fixtures; it is recommended to use rlang::is_interactive instead [2]. test_dir vs. devtools::test - devtools::test: This is the standard, high-level function for running all tests in an R package [3]. It is designed for developers and is typically triggered by RStudio shortcuts (e.g., Ctrl+Shift+T) [3]. It automatically handles the loading of package code, dependencies, and reporting [3][4]. - test_dir: This is a lower-level function intended for executing test files within a specific directory [5]. It is a "workhorse" function that powers higher-level tools like test_local and test_package [5][6]. Users generally should not call test_dir directly unless building specialized testing infrastructure, as it requires manual configuration of package environments (e.g., how to load the package code) [5]. Key Takeaways - Snapshot Testing: Snapshot tests rely on the context of a full test execution (e.g., via devtools::test or test_dir) to locate snapshot files [7]. They function differently when run interactively at the console compared to when they are part of a full test suite [7]. - Testing Status: You can use testthat::is_testing to check if code is running within a test suite [8]. For package-level checks (like R CMD check), use test_check [6]. - Reproducibility: Always prefer testthat's built-in context management (like local_test_context) over manually altering global options to simulate interactive or non-interactive states [1].
Citations:
- 1: https://testthat.r-lib.org/reference/local_test_context.html
- 2: https://testthat.r-lib.org/articles/test-fixtures.html
- 3: https://testthat.r-lib.org/
- 4: https://testthat.r-lib.org/articles/parallel.html
- 5: https://testthat.r-lib.org/reference/test_dir.html
- 6: https://testthat.r-lib.org/reference/test_package.html
- 7: https://testthat.r-lib.org/reference/expect_snapshot.html
- 8: https://testthat.r-lib.org/reference/is_testing.html
๋ํํ ์ธ์ ์์๋ ์ด ํ ์คํธ๋ฅผ ๊ฑด๋๋ฐ์ธ์.
testthat๋ rlang::is_interactive()๋ง ์ ์ดํ๋ฉฐ base R์ interactive()๋ ๋ณ๊ฒฝํ์ง ์์ต๋๋ค. ๋ํํ R ์ธ์
์์ devtools::test()๋ฅผ ์คํํ๋ฉด autoFIPC()๊ฐ readline()์ ํธ์ถํ ์ ์์ต๋๋ค. skip_if(interactive())๋ฅผ ์ถ๊ฐํ๊ฑฐ๋ ๋น๋ํํ ํ์ ํ๋ก์ธ์ค์์ assertion์ ์คํํ์ธ์.
๐ค 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 `@aFIPC.Rcheck/tests/testthat/test-autoFIPC.R` around lines 1 - 12, Update the
test identified by โautoFIPC raises error in non-interactive session for inputsโ
to skip when base R interactive() is TRUE, before invoking autoFIPC. Keep the
existing non-interactive error assertion unchanged.
1 participant
๐จ Severity: CRITICAL
๐ก Vulnerability: Weak regex validation (
^[0-9]+$) in interactive integer prompts allowed large strings of digits that evaluate toNAwhen coerced byas.integer(). This can breakifconditions and crash the execution pipeline.๐ฏ Impact: Unexpected crashes or unhandled exceptions that function as a local Denial of Service (DoS) during model automation.
๐ง Fix: Replaced
^[0-9]+$with bounded exact-match regex^[12]$across all instances inautoFIPC().โ Verification: Ran
Rscript -e 'devtools::test()'and explicitly verified interactive fallback logic usingmockeryinside theregex-securitytest suite. All tests passed successfully.PR created automatically by Jules for task 675986151613676802 started by @seonghobae
Summary by CodeRabbit
์๋ก์ด ๊ธฐ๋ฅ
autoFIPC๋ฅผ ์ ๊ณตํฉ๋๋ค.surveyFA๊ธฐ๋ฅ์ ์ถ๊ฐํ์ต๋๋ค.๋ฒ๊ทธ ์์
1๋๋2๋ง ํ์ฉํ๋๋ก ๊ฒ์ฆ์ ๊ฐํํ์ต๋๋ค.ํ ์คํธ
๋ฌธ์