fix(coverage): resolve npm workspace lock owners - #703
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughnpm workspace 설치 루트 해석기와 hardening 테스트를 추가했습니다. 부트스트랩 스크립트는 검증된 workspace 루트에서 오프라인 설치를 수행합니다. 관련 계약 테스트와 일회성 GitHub Actions 워크플로도 추가했습니다. Changesnpm workspace 설치 루트 검증
부트스트랩 설치 연결
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant bootstrap_patch_workflow
participant npm_workspace_install_root
participant Git
participant npm
GitHubActions->>bootstrap_patch_workflow: 패치 스크립트 실행
bootstrap_patch_workflow->>npm_workspace_install_root: 패키지 및 base/head SHA 전달
npm_workspace_install_root->>Git: manifest, lockfile, revision 검증
npm_workspace_install_root-->>bootstrap_patch_workflow: workspace 설치 루트 반환
bootstrap_patch_workflow->>npm: 검증된 루트에서 오프라인 npm ci 실행
GitHubActions->>GitHubActions: 결과 및 diff 검증
GitHubActions->>Git: 성공 변경 또는 실패 진단 push
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@jules Please finish this focused central coverage fix on the current branch. Wire
The intended verified command shape is equivalent to: npm_install_root_relative="$(python3 -I "$GITHUB_WORKSPACE/scripts/ci/npm_workspace_install_root.py" --repo-root "$COVERAGE_SOURCE_WORKDIR" --package-dir "$PWD")"
# map `.` or a safe relative result beneath COVERAGE_SOURCE_WORKDIR
trusted_npm_lock_is_materialized "$npm_install_root"
run_and_capture "JavaScript/TypeScript dependencies (npm workspace-root offline ci, lifecycle hooks disabled)" \
bash -c 'cd "$1" && npm ci --offline --ignore-scripts --cache "$2" --no-audit --no-fund' \
bash "$npm_install_root" "$writable_npm_cache_dir"Verify the resolver at 100% line coverage and docstring coverage, the full relevant pytest contract set, Bash syntax of extracted workflow run blocks, and |
|
@jules While completing the workflow wiring, also close these resolver trust/correctness gaps on the same branch:
Keep the PR focused on the resolver, workflow integration, contracts/tests, and one accurate 2026 journal entry. |
|
@jules Complete this PR now rather than leaving the resolver unwired. Required implementation:
Keep the patch fail-closed and limited to the central coverage path. Push the completed implementation to this PR branch. |
|
One fail-closed bug remains in the current resolver head Replace |
|
A second compatibility/security-contract issue is visible in the current resolver: The workspace resolver should establish ownership from the live-validated HEAD tree/worktree and return the lock-owner path; the workflow's existing |
|
@jules Complete this PR into its final reviewable state. Run |
|
@jules The branch now has the resolver and focused tests, but the actual central workflow wiring is still absent from the PR diff. Please apply the existing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
tests/test_npm_workspace_install_root_hardening.py (2)
15-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value테스트 헬퍼가 중복됩니다.
_git,_write_json,_commit은tests/test_npm_workspace_install_root.py의 동일한 헬퍼와 중복됩니다. 두 픽스처 구현이 시간이 지나며 달라질 수 있습니다. 헬퍼를tests/conftest.py의 공유 픽스처나 작은 헬퍼 모듈로 이동하십시오.🤖 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/test_npm_workspace_install_root_hardening.py` around lines 15 - 67, Remove the duplicated _git, _write_json, and _commit helpers from this test module and reuse shared implementations from tests/conftest.py or a small helper module, updating _workspace_repo and its callers to use them while preserving existing fixture behavior.
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
rm -rf서브프로세스 대신shutil.rmtree를 사용하십시오.이 호출은 외부
rm실행 파일에 의존합니다. Windows 개발 환경에서는 실패합니다. 또한 Ruff가 S603과 S607로 표시합니다. 표준 라이브러리shutil.rmtree가 동일한 작업을 이식 가능하게 수행합니다.♻️ 제안 리팩터링
import json +import shutil import subprocess- subprocess.run(["rm", "-rf", str(repo / "apps")], check=True) + shutil.rmtree(repo / "apps")🤖 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/test_npm_workspace_install_root_hardening.py` at line 193, Replace the subprocess-based recursive deletion in the test with the standard-library shutil.rmtree call, updating imports as needed. Preserve deletion of the repo / "apps" directory and its current test behavior without invoking an external rm executable.Source: Linters/SAST tools
scripts/ci/npm_workspace_install_root.py (2)
195-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value루프 내부에서
lru_cache데코레이터를 정의하지 마십시오.
matches는 루프 반복마다 새로 정의됩니다. 이 함수는 자유 변수pattern_parts를 캡처합니다. Ruff는 이를 B023으로 표시합니다. 현재는 함수가 정의된 반복 안에서만 호출되므로 동작은 정확합니다. 그러나 이 구조는 향후 리팩터링에서 늦은 바인딩 버그를 유발할 수 있습니다. 또한 반복마다 새 캐시 객체를 생성합니다.매처를 모듈 수준 헬퍼로 추출하고 인자를 튜플로 전달하십시오. 그러면 캐시를 패턴 간에 재사용할 수 있고 B023 경고도 사라집니다.
♻️ 제안 리팩터링
+@lru_cache(maxsize=4096) +def _segments_match( + path_parts: tuple[str, ...], + pattern_parts: tuple[str, ...], +) -> bool: + """Match anchored single-segment globs and recursive ``**`` tokens.""" + if not pattern_parts: + return not path_parts + token = pattern_parts[0] + if token == "**": + return _segments_match(path_parts, pattern_parts[1:]) or ( + bool(path_parts) and _segments_match(path_parts[1:], pattern_parts) + ) + if not path_parts: + return False + return fnmatch.fnmatchcase(path_parts[0], token) and _segments_match( + path_parts[1:], + pattern_parts[1:], + ) + + def _is_declared_workspace(relative_package: PurePosixPath, patterns: list[str]) -> bool: """Return whether a path fully matches one anchored workspace pattern.""" path_parts = relative_package.parts - - for pattern in patterns: - pattern_parts = tuple(pattern.split("/")) - - `@lru_cache`(maxsize=None) - def matches(path_index: int, pattern_index: int) -> bool: - ... - - if matches(0, 0): - return True - return False + return any( + _segments_match(path_parts, tuple(pattern.split("/"))) for pattern in patterns + )🤖 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 `@scripts/ci/npm_workspace_install_root.py` around lines 195 - 218, Move the nested matches function out of the patterns loop into a module-level cached helper, passing path_parts and pattern_parts as explicit tuple arguments. Update the loop to call this helper for each pattern, preserving the existing anchored glob and recursive ** matching behavior while allowing the cache to be reused across patterns and eliminating the B023 warning.Source: Linters/SAST tools
334-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value중복 조건을 단순화하십시오.
PurePosixPath("")는PurePosixPath(".")로 정규화됩니다. 따라서parent != PurePosixPath("")조건의 두 분기가 동일한 값PurePosixPath(".")를 만듭니다. 이 조건은 동작에 영향을 주지 않습니다. 조건을 제거하면 상위 경로 탐색 의도가 명확해집니다.♻️ 제안 리팩터링
if candidate == PurePosixPath("."): break - parent = candidate.parent - candidate = parent if parent != PurePosixPath("") else PurePosixPath(".") + candidate = candidate.parent🤖 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 `@scripts/ci/npm_workspace_install_root.py` around lines 334 - 337, Update the parent-path assignment in the candidate traversal loop to remove the redundant PurePosixPath("") conditional. After the existing candidate == PurePosixPath(".") termination check, assign candidate directly to candidate.parent while preserving the current traversal behavior.tests/test_npm_workspace_install_root.py (1)
480-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
module.PurePosixPath대신 직접 임포트를 사용하십시오.이 테스트는 프로덕션 모듈의 임포트 재노출에 의존합니다.
npm_workspace_install_root.py가PurePosixPath임포트를 제거하거나 이름을 바꾸면, 실제 동작 변경이 없어도 테스트가 실패합니다. 이 파일은 이미pathlib에서Path를 임포트합니다.PurePosixPath도 같은 방식으로 임포트하십시오.♻️ 제안 리팩터링
-from pathlib import Path +from pathlib import Path, PurePosixPathmodule._tree_blob( tmp_path, "a" * 40, - module.PurePosixPath("package.json"), + PurePosixPath("package.json"), "fixture manifest", )🤖 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/test_npm_workspace_install_root.py` around lines 480 - 487, Update the test invoking _tree_blob to use a directly imported PurePosixPath from pathlib instead of module.PurePosixPath. Add PurePosixPath alongside the existing Path import and pass it directly, removing the dependency on the production module’s re-export..github/workflows/pr703-focused-tests.yml (1)
42-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPython 버전을 명시적으로 설정하십시오.
이 파일은
bootstrap_patch_workflow.py가 성공하면 삭제하는 일회성 워크플로이므로 별도 중앙 워크플로로 이관할 대상이 아닙니다. 그러나 현재ubuntu-latest의 기본python3에 의존합니다.actions/setup-python을 추가하고python-version: "3.12"를 설정하십시오.bootstrap-npm-workspace-wiring.yml에도 동일한 설정을 적용하십시오.🤖 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 @.github/workflows/pr703-focused-tests.yml around lines 42 - 62, Explicitly configure Python 3.12 in the workflow by adding actions/setup-python with python-version set to "3.12" before the Python-based steps, and apply the same setup to bootstrap-npm-workspace-wiring.yml. Keep the existing test and coverage commands unchanged..github/workflows/bootstrap-npm-workspace-wiring.yml (1)
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win실패 로그를 저장소에 커밋하지 말고 job summary로 보내세요.
현재 실패 로그는
.github/bootstrap-npm-workspace-failure.log로 기록되고, 이후 단계가 이를 브랜치에 push합니다. 이 파일은 저장소에 잔여 아티팩트로 남습니다.$GITHUB_STEP_SUMMARY또는 업로드 아티팩트를 사용하세요.♻️ 제안 변경
if [ "$patch_rc" -ne 0 ]; then { echo "bootstrap_patch_workflow.py failed with exit code $patch_rc" echo sed -n '1,200p' "$RUNNER_TEMP/bootstrap-patch.log" - } > .github/bootstrap-npm-workspace-failure.log + } >>"$GITHUB_STEP_SUMMARY" fi🤖 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 @.github/workflows/bootstrap-npm-workspace-wiring.yml around lines 64 - 70, Update the failure-handling block around patch_rc in the workflow to stop writing bootstrap failures to .github/bootstrap-npm-workspace-failure.log, which is later committed and pushed. Send the existing failure message and contents of $RUNNER_TEMP/bootstrap-patch.log to $GITHUB_STEP_SUMMARY instead, preserving the diagnostic details without leaving a repository artifact.
🤖 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 @.github/workflows/bootstrap-npm-workspace-wiring.yml:
- Line 124: Update the condition in the workflow’s patch result check to pass
steps.patch.outputs.patch_rc through the step’s env configuration, then
reference the resulting shell environment variable inside the if statement
instead of interpolating the GitHub Actions expression directly.
In @.github/workflows/pr703-focused-tests.yml:
- Around line 1-25: Move the resolver tests and coverage gate from the
PR-specific workflow into the repository’s central test workflow, preserving
their required triggers and checks. Then delete the temporary workflow defined
by “PR 703 Focused Resolver Tests,” including its PR-specific branch and path
configuration, so no one-off bootstrap or workflow remains under
.github/workflows.
In `@scripts/ci/bootstrap_patch_workflow.py`:
- Around line 278-289: Remove the one-time self-modifying bootstrap path: run
scripts/ci/bootstrap_patch_workflow.py locally, commit its generated final
contents into .github/workflows/opencode-review-dispatch.yml and
tests/test_opencode_agent_contract.py, then delete
scripts/ci/bootstrap_patch_workflow.py. Also delete
.github/workflows/bootstrap-npm-workspace-wiring.yml, including its contents:
write permission and branch-push behavior.
In `@scripts/ci/npm_workspace_install_root.py`:
- Around line 113-126: Update _worktree_blob to hash the worktree file with
Git’s path-aware normalization by passing the repository-relative relative_path
via --path to hash-object, instead of using --no-filters. Preserve the existing
regular-file validation and expected-blob comparison.
---
Nitpick comments:
In @.github/workflows/bootstrap-npm-workspace-wiring.yml:
- Around line 64-70: Update the failure-handling block around patch_rc in the
workflow to stop writing bootstrap failures to
.github/bootstrap-npm-workspace-failure.log, which is later committed and
pushed. Send the existing failure message and contents of
$RUNNER_TEMP/bootstrap-patch.log to $GITHUB_STEP_SUMMARY instead, preserving the
diagnostic details without leaving a repository artifact.
In @.github/workflows/pr703-focused-tests.yml:
- Around line 42-62: Explicitly configure Python 3.12 in the workflow by adding
actions/setup-python with python-version set to "3.12" before the Python-based
steps, and apply the same setup to bootstrap-npm-workspace-wiring.yml. Keep the
existing test and coverage commands unchanged.
In `@scripts/ci/npm_workspace_install_root.py`:
- Around line 195-218: Move the nested matches function out of the patterns loop
into a module-level cached helper, passing path_parts and pattern_parts as
explicit tuple arguments. Update the loop to call this helper for each pattern,
preserving the existing anchored glob and recursive ** matching behavior while
allowing the cache to be reused across patterns and eliminating the B023
warning.
- Around line 334-337: Update the parent-path assignment in the candidate
traversal loop to remove the redundant PurePosixPath("") conditional. After the
existing candidate == PurePosixPath(".") termination check, assign candidate
directly to candidate.parent while preserving the current traversal behavior.
In `@tests/test_npm_workspace_install_root_hardening.py`:
- Around line 15-67: Remove the duplicated _git, _write_json, and _commit
helpers from this test module and reuse shared implementations from
tests/conftest.py or a small helper module, updating _workspace_repo and its
callers to use them while preserving existing fixture behavior.
- Line 193: Replace the subprocess-based recursive deletion in the test with the
standard-library shutil.rmtree call, updating imports as needed. Preserve
deletion of the repo / "apps" directory and its current test behavior without
invoking an external rm executable.
In `@tests/test_npm_workspace_install_root.py`:
- Around line 480-487: Update the test invoking _tree_blob to use a directly
imported PurePosixPath from pathlib instead of module.PurePosixPath. Add
PurePosixPath alongside the existing Path import and pass it directly, removing
the dependency on the production module’s re-export.
🪄 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: 9607ef19-06dd-4eed-b62e-96958f32cc8d
📒 Files selected for processing (6)
.github/workflows/bootstrap-npm-workspace-wiring.yml.github/workflows/pr703-focused-tests.ymlscripts/ci/bootstrap_patch_workflow.pyscripts/ci/npm_workspace_install_root.pytests/test_npm_workspace_install_root.pytests/test_npm_workspace_install_root_hardening.py
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
|
@jules Finalize this PR directly on the current branch. Replace the temporary self-modifying bootstrap with the actual central workflow and contract changes it was meant to produce; delete |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![pr-auto-rebase[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
2175296 to
309ed34
Compare
Pull request was converted to draft
2 participants
What
Add a fail-closed resolver for nested npm workspace packages and wire it into the central OpenCode coverage sandbox. A selected package such as
apps/desktopcan now install from its nearest validated npm workspace lock owner instead of requiring an invalid duplicate lockfile beside every workspace package.Why
BandScope correctly owns one root npm workspace lock. The previous central coverage path selected
apps/desktop, attempted isolated installation there, failed to materialize Vitest, and consequently blocked every current-head approval. The final implementation resolves and validates the ancestor lock owner before running a lifecycle-disabled, networkless workspace-scopednpm ci.Security boundary
packagesentry.Verification
The exact final state was reconstructed, reviewed, and tested before being committed directly:
apps/desktopto the repository root (.).shutil.rmtreecleanup.git diff --checkpassed.aiohttp==3.14.3andcryptography==50.0.0to remove current high-severity audit blockers.Product impact
This removes the organization-level coverage deadlock for BandScope and other modular npm workspace repositories while preserving standalone package selection and centralized governance.