fix(advisor): install from trusted dependency lockfile - #6786
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe advisor workflow now installs dependencies with ChangesAdvisor dependency pinning
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pr-review-advisor.yaml:
- Line 253: Update the advisor dependency installation command in the workflow
to use a committed lockfile and npm ci, ensuring the complete direct and
transitive dependency graph is pinned and reproducible. Remove the ad-hoc npm
install behavior and preserve the existing dependency version constraints where
compatible.
In `@tools/pr-review-advisor/workflow-boundary.mts`:
- Line 371: Update the assertion around requireRunContains in the workflow
boundary checks to verify that the actual npm install invocation includes
"vitest@${VITEST_VERSION}", rather than accepting matching text from comments,
echo commands, or unrelated commands. Add a regression test covering decoy text
that contains the package string without installing Vitest.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 34b1ea60-218f-4ff1-ac54-64119cea6027
📒 Files selected for processing (3)
.github/workflows/pr-review-advisor.yamltest/pr-review-advisor-workflow-boundary.test.tstools/pr-review-advisor/workflow-boundary.mts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> # Conflicts: # .github/workflows/pr-review-advisor.yaml # test/pr-review-advisor-workflow-boundary.test.ts # tools/pr-review-advisor/workflow-boundary.mts
cjagwani
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pr-review-advisor/workflow-boundary.mts (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVersion pins duplicated between the array and
requireEnvcalls.
ADVISOR_RUNTIME_PACKAGE_PINS(Lines 14-19) and therequireEnv(..., "PI_SDK_VERSION"/"TYPEBOX_VERSION"/"YAML_VERSION"/"VITEST_VERSION", ...)calls (Lines 286-291) independently hardcode the same four version strings. A partial bump (updating one but forgetting the other) can silently create an inconsistent ratchet between the env-var check and the lockfile-pin check.As per path instructions, "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."
♻️ Proposed fix: derive requireEnv checks from the pins array
const ADVISOR_RUNTIME_PACKAGE_PINS = [ - ["`@earendil-works/pi-coding-agent`", "0.80.6"], - ["typebox", "1.1.38"], - ["yaml", "2.8.3"], - ["vitest", "4.1.9"], + ["`@earendil-works/pi-coding-agent`", "PI_SDK_VERSION", "0.80.6"], + ["typebox", "TYPEBOX_VERSION", "1.1.38"], + ["yaml", "YAML_VERSION", "2.8.3"], + ["vitest", "VITEST_VERSION", "4.1.9"], ] as const;- const packages = asRecord(lock.packages); - for (const [name, expectedVersion] of ADVISOR_RUNTIME_PACKAGE_PINS) { + const packages = asRecord(lock.packages); + for (const [name, , expectedVersion] of ADVISOR_RUNTIME_PACKAGE_PINS) { const actualVersion = asRecord(packages[`node_modules/${name}`]).version;- requireEnv(errors, "review job", reviewJob, "PI_SDK_VERSION", "0.80.6"); requireEnv(errors, "review job", reviewJob, "FD_FIND_VERSION", "9.0.0-1"); requireEnv(errors, "review job", reviewJob, "RIPGREP_VERSION", "14.1.0-1"); - requireEnv(errors, "review job", reviewJob, "TYPEBOX_VERSION", "1.1.38"); - requireEnv(errors, "review job", reviewJob, "YAML_VERSION", "2.8.3"); - requireEnv(errors, "review job", reviewJob, "VITEST_VERSION", "4.1.9"); + for (const [, envKey, version] of ADVISOR_RUNTIME_PACKAGE_PINS) { + requireEnv(errors, "review job", reviewJob, envKey, version); + }🤖 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 `@tools/pr-review-advisor/workflow-boundary.mts` around lines 14 - 19, Derive the version values used by the requireEnv checks from the canonical ADVISOR_RUNTIME_PACKAGE_PINS array instead of hardcoding them again. Update the logic around the requireEnv calls to map each package pin to its corresponding environment-variable name, preserving the existing package-to-variable associations and validation behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In `@tools/pr-review-advisor/workflow-boundary.mts`:
- Around line 14-19: Derive the version values used by the requireEnv checks
from the canonical ADVISOR_RUNTIME_PACKAGE_PINS array instead of hardcoding them
again. Update the logic around the requireEnv calls to map each package pin to
its corresponding environment-variable name, preserving the existing
package-to-variable associations and validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29105cd0-3ca6-421e-9a9c-dcb4533a3772
📒 Files selected for processing (3)
.github/workflows/pr-review-advisor.yamltest/pr-review-advisor-workflow-boundary.test.tstools/pr-review-advisor/workflow-boundary.mts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence E2E guidanceAdvisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output. Recommended coverage: None Why no E2E coverage is recommended: No deterministic or trusted-inventory E2E coverage was selected. Why no selector is recommended: No trusted E2E selector was selected. This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
wscurran
added
area: ci
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
@cv Correction to this handoff: exact head |
3 participants
Summary
Replace PR Review Advisor's ad-hoc runtime install with the trusted checkout's repository-locked dependency graph. This keeps the recently restored Vitest inventory path working while pinning the complete direct and transitive graph used by the secret-bearing job.
Bootstrap note: this PR's automatic
pull_request_targetAdvisor run uses the pre-lockfile trusted base and therefore cannot exercise the newnpm cipath. A healthy post-merge run is required before the shared lane is considered fully restored.Changes
npm cifrom the trusted checkout's committed lockfile, with lifecycle scripts disabledType of Change
Quality Gates
npm cicommand, with positive, drift, and decoy mutation coverage; an isolated runtime reproduction exercised the exact previously failing discovery commandVerification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/pr-review-advisor-workflow-boundary.test.ts(21 passed); isolated live-enabledvitest list --filesOnly --json --project e2e-live --project integration(exit 0, 591 files)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Additional verification:
npm run build:clinpm run typecheck:clinpm ci --ignore-scripts --no-audit --no-fundfrom the trusted repository lock (440 packages), followed by the exact live-enabled inventory command (591 files)Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
VITEST_VERSIONand verify runtime dependency pins (includingvitest@4.1.9) from the repository lockfile.npm ci --ignore-scripts --no-audit --no-fundinvocation and rejecting misplaced/decoy install text and lock drift.VITEST_VERSIONpinning and the exactnpm ci --ignore-scripts --no-audit --no-fundcommand and ordering.