🛡️ 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.
|
Closing as superseded by #193. The maintained replacement contains the same three exact |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
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만 허용하도록 검증을 강화했습니다.테스트
문서