Add ModelOpt agent plugin marketplace - #2025
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThe PR packages ModelOpt skills as installable Claude and Codex plugins. It relocates canonical skills under ChangesModelOpt plugin packaging and discovery
ModelOpt workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2025 +/- ##
==========================================
- Coverage 78.73% 77.95% -0.79%
==========================================
Files 522 522
Lines 60357 60692 +335
==========================================
- Hits 47523 47313 -210
- Misses 12834 13379 +545
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (36)
plugins/modelopt/skills/eagle3-validate/SKILL.md-36-53 (1)
36-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire explicit completion and artifact checks.
“ No error” in the last log tail is insufficient to establish success, and log messages do not prove that expected files exist. Require an explicit successful exit/completion marker and run filesystem checks such as
test -s/findon the relevant workspace; otherwise report the artifact status as unverified.🤖 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 `@plugins/modelopt/skills/eagle3-validate/SKILL.md` around lines 36 - 53, Update the validation instructions in the “Verify artifacts exist” section to require explicit successful completion or exit markers for every task, not merely an error-free log tail. Add filesystem checks using test -s, find, or equivalent for each expected artifact under /scratchspace, and report any missing or empty artifact as unverified.plugins/modelopt/skills/eagle3-review-logs/SKILL.md-75-80 (1)
75-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResume from the actual failed task.
This example only skips
task_0; iftask_2ortask_3failed, it reruns successful earlier tasks unnecessarily. Generate skip overrides for every task before the failed task so the command truly resumes from the reported failure.🤖 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 `@plugins/modelopt/skills/eagle3-review-logs/SKILL.md` around lines 75 - 80, The rerun guidance should resume from the reported failed pipeline task rather than always skipping only task_0. Update the instruction around the `pipeline.task_0.skip=true` override to generate skip overrides for every task preceding the failed task, while leaving the failed task and subsequent tasks enabled.plugins/modelopt/skills/eagle3-new-model/SKILL.md-42-42 (1)
42-42: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAlign the
trust_remote_codeforwarding contract before documenting this workaround.The instructions currently tell users to pass the flag to task_3 while also stating that
quick_check.shdoes not forward it. Update the wrapper/config contract so the flag reaches the benchmark, then document the supported invocation.
plugins/modelopt/skills/eagle3-new-model/SKILL.md#L42-L42: document the actual task_3 forwarding mechanism.plugins/modelopt/skills/eagle3-triage/SKILL.md#L117-L118: replace the ineffective “pass the flag” fix unlessquick_check.shforwards it.plugins/modelopt/skills/eagle3-triage/SKILL.md#L129-L129: keep task_0/task_3 guidance consistent with the implemented forwarding path.🤖 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 `@plugins/modelopt/skills/eagle3-new-model/SKILL.md` at line 42, Align the trust_remote_code forwarding contract by updating quick_check.sh or its configuration so the flag reaches task_3 benchmark arguments while remaining correctly passed to task_0 before the separator. In plugins/modelopt/skills/eagle3-new-model/SKILL.md:42, document the actual supported task_3 forwarding mechanism and invocation. In plugins/modelopt/skills/eagle3-triage/SKILL.md:117-118, replace the ineffective workaround with instructions matching the implemented forwarding path; at :129, keep task_0/task_3 guidance consistent with that path.plugins/modelopt/skills/ptq/references/unsupported-models.md-252-280 (1)
252-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when FP8 scale metadata is missing or unsupported.
When
scale is None, and again for unsupported scale dimensions, the helper silently casts FP8 values to BF16 without applying a scale. That can corrupt weights while reporting successful dequantization. Raise an error unless a no-scale representation is explicitly supported, and reject unsupported dimensions instead of silently continuing.🤖 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 `@plugins/modelopt/skills/ptq/references/unsupported-models.md` around lines 252 - 280, Update dequantize_fp8_params so missing FP8 scale metadata (scale is None) raises an error rather than casting values, unless an explicitly supported no-scale representation is defined. Replace the unsupported scale-dimension fallback with an error that identifies the parameter and scale shape, while preserving the existing 1D and 3D scaling paths.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-100-110 (1)
100-110: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSupport single-file safetensors exports in the coverage gate.
This script unconditionally opens
model.safetensors.index.json, unlike the VLM check above, which supports both sharded and non-sharded exports. Valid single-file checkpoints have no index and will fail before producing the required validation report.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 100 - 110, Update the layer-coverage script around the index/config loading to support both sharded exports with model.safetensors.index.json and single-file model.safetensors exports without an index. Detect which layout exists, derive the checkpoint tensor names appropriately for each, and preserve the required validation report and coverage checks.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-74-98 (1)
74-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject empty or missing safetensors before computing the ratio.
When
src == 0, the ratio becomesnan, which does not satisfy the>= 1.0blocking condition. Whendst == 0, the ratio is0.0, incorrectly suggesting successful compression. Require both source and output byte counts to be greater than zero before reporting a passing ratio.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 74 - 98, Update the checkpoint size-check script around safetensor_bytes, src, and dst to reject missing or empty source/output safetensors before calculating or reporting the ratio. Require both byte counts to be greater than zero, emit a failing result with clear context otherwise, and only apply the existing ratio and compression threshold when both counts are valid.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-37-61 (1)
37-61: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the VLM detector generic or explicitly limit its supported model names.
The prose claims to validate multimodal checkpoints, but the predicate only recognizes
model.visual,vision_tower, andvision_model. Models using names such asvision_encoderorvisual_encodercan therefore pass with a quantized vision branch. Derive the branch from model metadata or document/enforce the supported naming contract explicitly.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 37 - 61, The VLM validation snippet’s vision-branch detection is incomplete for generic multimodal checkpoints. Update the detector to derive vision-tower keys from available model metadata, or explicitly enforce and document a supported naming contract that includes only the recognized prefixes; ensure unsupported naming schemes cannot silently report zero quantized vision tensors.plugins/modelopt/skills/ptq/tests.json-77-86 (1)
77-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpand eval 6 to cover the complete checkpoint gate.
The documented gate also requires recording the exact workspace/path, every PTQ compatibility category (or
none), and a deployment serving canary before evaluation. Without expectations for those behaviors, an implementation can pass this test while violating the required deployment/evaluation handoff contract.Proposed additions
"Checks metadata consistency against the source checkpoint and records any diffs", + "Reports the exact checkpoint workspace and path and preserves them for deployment/evaluation", + "Records dependency upgrades, source patches, custom code, environment variables, and launcher/container changes, using none when absent", + "Runs the deployment canary from the recorded workspace/path and stops before eval submission if it fails", "Stops before eval submission if the size ratio is >= 1.0 for a compression recipe, if intended layer coverage is missing, or if metadata changes unexpectedly"🤖 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 `@plugins/modelopt/skills/ptq/tests.json` around lines 77 - 86, Expand test case id 6 expectations to require recording the exact workspace/path, reporting every PTQ compatibility category with “none” when applicable, and running a deployment serving canary before any evaluation submission. Keep the existing checkpoint validation, compression, layer coverage, metadata checks, and stop conditions unchanged.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-127-154 (1)
127-154: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't present this as actual precision validation.
algois only the declared config, andhas_scalesonly checks for scale-key presence, so a checkpoint with FP8 scales but a misdeclared NVFP4 algo would still be counted as NVFP4. Either inspect real quantization metadata / tensor dtype / scale layout, or rename this to a declaration-vs-presence check.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 127 - 154, Rename this validation logic and its output to describe declaration-versus-scale-presence checking rather than actual precision validation. In the loop using declared_algo and has_scales, avoid labeling algo values as verified precisions unless the implementation is extended to inspect tensor dtypes, scale metadata, or scale layouts; otherwise update counters, mismatch messages, and surrounding documentation to use declaration/presence terminology.plugins/modelopt/skills/common/remote-execution.md-102-108 (1)
102-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe SCP command does not reuse the configured SSH session.
It omits
ControlPath,ssh_key, andssh_proxy; clusters requiring either configured auth option will fail, and successful calls create a new connection. Show an invocation using the same SSH options asremote_exec.sh, or direct users toremote_sync_to.🤖 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 `@plugins/modelopt/skills/common/remote-execution.md` around lines 102 - 108, Update the SCP guidance near the “SCP (alternative to rsync)” section to reuse the configured SSH session and authentication settings by including the same ControlPath, ssh_key, and ssh_proxy options used by remote_exec.sh, or replace the example with a recommendation to use remote_sync_to.plugins/modelopt/skills/common/slurm-setup.md-260-278 (1)
260-278: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict Enroot credential-file permissions.
These examples write registry tokens but never enforce
0600; a permissive umask can expose them to other users. Addchmod 600 ~/.config/enroot/.credentialsafter creating/appending it.🤖 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 `@plugins/modelopt/skills/common/slurm-setup.md` around lines 260 - 278, Update the enroot credential setup examples after the DockerHub and NGC credential append commands to run chmod 600 on ~/.config/enroot/.credentials, ensuring the file containing registry tokens is readable and writable only by the owner.plugins/modelopt/skills/common/remote_exec.sh-372-390 (1)
372-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCreate remote sync destinations before invoking rsync.
The documented first-use workflows sync into
<session_id>/<model>/..., butremote_sync_toonly assumes the path exists; rsync cannot create missing intermediate directories.
plugins/modelopt/skills/common/remote_exec.sh#L372-L390: create${REMOTE_WORKSPACE}/${remote_subdir}withmkdir -p --before rsync.plugins/modelopt/skills/common/remote-execution.md#L119-L124: rely on the helper’s destination creation or explicitly create the scripts directory first.plugins/modelopt/skills/common/workspace-management.md#L79-L87: ensure the remote session/model workspace exists before the initial source and script sync.🤖 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 `@plugins/modelopt/skills/common/remote_exec.sh` around lines 372 - 390, Update remote_sync_to in plugins/modelopt/skills/common/remote_exec.sh (lines 372-390) to create ${REMOTE_WORKSPACE}/${remote_subdir} with mkdir -p -- before invoking rsync. In plugins/modelopt/skills/common/remote-execution.md (lines 119-124), rely on the helper’s destination creation or explicitly create the scripts directory first. In plugins/modelopt/skills/common/workspace-management.md (lines 79-87), ensure the remote session/model workspace exists before the initial source and script sync.plugins/modelopt/skills/common/environment-setup.md-13-19 (1)
13-19: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not mutate an existing checkout to “update” it.
git pull origin maincan merge into the user’s active branch, andgit checkout mainneither preserves branch intent nor clears patches. Require a clean, dedicated clone/worktree or ask before changing branches.🤖 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 `@plugins/modelopt/skills/common/environment-setup.md` around lines 13 - 19, Update the environment setup guidance around “If found, ensure the source is up to date” to prohibit mutating an existing checkout with git pull or git checkout. Require a clean, dedicated clone/worktree, or instruct the user to obtain explicit confirmation before changing branches or applying resets; preserve existing patches and branch intent unless the user approves otherwise.plugins/modelopt/skills/common/credentials.md-50-54 (1)
50-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid exposing the NGC key in process arguments.
-p <NGC_API_KEY>can leak through process listings and shell history. Use stdin, as the SLURM guide already does.Proposed fix
-docker login nvcr.io -u '$oauthtoken' -p <NGC_API_KEY> +printf '%s\n' "$NGC_API_KEY" | docker login nvcr.io -u '$oauthtoken' --password-stdin🤖 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 `@plugins/modelopt/skills/common/credentials.md` around lines 50 - 54, Update the Docker login command in the Docker section of credentials.md to pass NGC_API_KEY through stdin rather than the -p option, matching the existing SLURM guide pattern and preventing exposure in process arguments or shell history.plugins/modelopt/skills/accessing-mlflow/SKILL.md-5-9 (1)
5-9: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required licensing headers for vendored content.
This file identifies itself as vendored third-party source but contains only frontmatter licensing metadata and a source pointer. Add the original copyright/license notice and the required NVIDIA Apache 2.0 header, and ensure the file is excluded from the license-insertion hook.
As per coding guidelines, copied third-party source must include a source reference with commit hash, the original copyright/license, and the NVIDIA Apache 2.0 header.
🤖 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 `@plugins/modelopt/skills/accessing-mlflow/SKILL.md` around lines 5 - 9, Add the original upstream copyright/license notice and the required NVIDIA Apache 2.0 header to the vendored content in SKILL.md, while preserving its commit-pinned source reference. Update the repository’s license-insertion hook configuration to exclude this vendored file from automatic header insertion.Source: Coding guidelines
plugins/modelopt/skills/day0-release/scripts/gate_ptq.py-83-93 (1)
83-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSize gate never validates
output_bytes; a zero/negative output silently passes.Only
src <= 0is checked. Ifoutput_bytesis0(or negative, e.g. from a truncated/failed checkpoint export),ratio = out/src = 0.0 < 1.0, so no "output not smaller than source" failure is recorded — an empty/corrupted checkpoint passes the size gate.🐛 Proposed fix
- if not isinstance(src, (int, float)) or not isinstance(out, (int, float)) or src <= 0: + if not isinstance(src, (int, float)) or not isinstance(out, (int, float)) or src <= 0 or out <= 0: checks["size"] = "missing/invalid source or output bytes" failures.append(("USER_CONFIG_ERROR", "missing source/output sizes"))🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_ptq.py` around lines 83 - 93, Update the size validation in the gate’s size-check block to require both source and output byte values to be positive numeric values, including rejecting zero or negative output_bytes as a USER_CONFIG_ERROR before calculating the ratio. Preserve the existing ratio check for valid sizes.plugins/modelopt/skills/day0-release/SKILL.md-67-77 (1)
67-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse plugin-root-relative skill paths
plugins/modelopt/skills/day0-release/SKILL.mdandplugins/modelopt/skills/compare-results/SKILL.mdstill reference.agents/skills/.... In the canonical plugin tree, those are only symlinked repo paths, so these references should be switched toskills/...or${CLAUDE_PLUGIN_ROOT}/skills/....🤖 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 `@plugins/modelopt/skills/day0-release/SKILL.md` around lines 67 - 77, Replace repo-relative “.agents/skills/...” references with plugin-root-relative “skills/...” or “${CLAUDE_PLUGIN_ROOT}/skills/...” paths. Update the command and references in plugins/modelopt/skills/day0-release/SKILL.md at lines 67-77, 100-104, and 118-124, and in plugins/modelopt/skills/compare-results/SKILL.md at lines 32-41; preserve the referenced skill and script targets.Source: Coding guidelines
plugins/modelopt/skills/release-cherry-pick/SKILL.md-81-88 (1)
81-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMark PRs done only after the release PR merges.
Step 2 excludes
cherry-pick-done; applying it immediately after opening the release PR means a closed or failed release PR permanently hides those source PRs from later runs. Apply the label after the release PR is merged.🤖 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 `@plugins/modelopt/skills/release-cherry-pick/SKILL.md` around lines 81 - 88, Update Step 7 in the release cherry-pick workflow so the cherry-pick-done label is applied only after confirming the release PR has merged, not immediately after it is opened. Preserve the existing loop and labeling behavior for every successfully cherry-picked source PR once the merge condition is satisfied.plugins/modelopt/skills/release-cherry-pick/SKILL.md-64-65 (1)
64-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCreate the feature branch before cherry-picking.
The current order advances local
release/<VERSION>with every pick, then force-resets/creates the feature branch afterward. Create and check outcherry-picks/release-<VERSION>fromorigin/release/<VERSION>before Step 5, and do not usecheckout -Bhere.🤖 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 `@plugins/modelopt/skills/release-cherry-pick/SKILL.md` around lines 64 - 65, Update the release cherry-pick workflow before Step 5 to create and check out cherry-picks/release-<VERSION> from origin/release/<VERSION>, then push it as needed; remove the later checkout -B command so the local release branch is not advanced during cherry-picking.plugins/modelopt/skills/release-cherry-pick/SKILL.md-61-79 (1)
61-79: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire approval before publishing.
Present the branch, PR title, and body, then wait for explicit approval before
git pushorgh pr create; a request to cherry-pick does not necessarily authorize publishing. Based on learnings, never rungit pushorgh pr createwithout explicit approval in the current turn.🤖 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 `@plugins/modelopt/skills/release-cherry-pick/SKILL.md` around lines 61 - 79, Update the cherry-pick publishing workflow around the git push and gh pr create commands to first present the proposed branch, PR title, and exact body for review, then pause for explicit approval in the current turn. Do not execute git push or gh pr create based solely on a cherry-pick request; proceed only after approval is explicitly provided.Source: Learnings
plugins/modelopt/skills/release-cherry-pick/SKILL.md-21-23 (1)
21-23: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPaginate the pending-PR search. This query only fetches the first 50 results, so larger releases can silently skip eligible cherry-picks. Use
gh api --paginatewithper_page=100.🤖 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 `@plugins/modelopt/skills/release-cherry-pick/SKILL.md` around lines 21 - 23, Update the pending cherry-pick search command to use gh api --paginate and increase per_page from 50 to 100, preserving the existing query filters, jq transformation, and sorting behavior.plugins/modelopt/skills/debug/SKILL.md-3-31 (1)
3-31: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire confirmation before state-changing remote commands.
This skill permits arbitrary relay execution but does not require confirmation before writes, process termination, or network-affecting commands. Require explicit confirmation immediately before such commands and never pass copied/untrusted strings to
run.🤖 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 `@plugins/modelopt/skills/debug/SKILL.md` around lines 3 - 31, Update the Remote Docker Debugger instructions to require explicit user confirmation immediately before any state-changing remote command, including writes, process termination, or network-affecting operations. Ensure commands passed to tools/debugger/client.sh run are not copied or untrusted strings, while preserving read-only command usage and the existing relay workflow.Source: Linters/SAST tools
plugins/modelopt/skills/deployment/scripts/deploy.sh-191-203 (1)
191-203: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not treat arbitrary Python processes as this server.
A stale/reused PID pointing to any Python process passes this check, after which
stop_servercan terminate it or its process group. Start the server in a dedicated session and persist/verify a run-specific identity before signaling it.Also applies to: 391-410
🤖 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 `@plugins/modelopt/skills/deployment/scripts/deploy.sh` around lines 191 - 203, Update is_server_running and the related stop_server flow to avoid identifying arbitrary Python processes as the managed server. Start the server in a dedicated session and persist a run-specific identity, then verify that identity before returning success or sending termination signals; retain stale-PID cleanup when verification fails.plugins/modelopt/skills/deployment/scripts/deploy.sh-333-355 (1)
333-355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not advertise TRT-LLM as an automated start backend.
start --framework trtllmreaches this function, which returns failure and never starts an endpoint. Implement the lifecycle path or reject/removetrtllmfrom the script’s supported framework list and direct users to the reference.🤖 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 `@plugins/modelopt/skills/deployment/scripts/deploy.sh` around lines 333 - 355, Update start_trtllm and the framework-selection flow so TRT-LLM is not presented as a supported automated backend: either implement a functioning server lifecycle that starts an endpoint, or remove/reject trtllm from the supported framework list and direct users to the existing TRT-LLM reference guidance. Do not leave trtllm selectable while start_trtllm only logs instructions and returns failure.plugins/modelopt/skills/deployment/SKILL.md-213-218 (1)
213-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the canonical plugin skill path.
The deployment skill instructs installed-plugin users to source a compatibility path that may not exist outside this repository, and the eval locks that behavior in.
plugins/modelopt/skills/deployment/SKILL.md#L213-L218: sourceplugins/modelopt/skills/common/remote_exec.sh(or an install-location-resolved canonical equivalent).plugins/modelopt/skills/deployment/tests/evals.json#L29-L31: expect the canonical plugin path.Based on learnings, use paths relative to the repository root. As per coding guidelines,
plugins/modelopt/skills/is the canonical installable tree and.agents/skillsis only a compatibility symlink.🤖 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 `@plugins/modelopt/skills/deployment/SKILL.md` around lines 213 - 218, Update the deployment instructions in plugins/modelopt/skills/deployment/SKILL.md at lines 213-218 to source the canonical plugin skill path plugins/modelopt/skills/common/remote_exec.sh, or an equivalent path resolved from the install location, while keeping subsequent remote_* commands unchanged. Update the corresponding expectation in plugins/modelopt/skills/deployment/tests/evals.json at lines 29-31 to require the canonical plugin path; both locations should use repository-root-relative paths, with no direct change needed to the .agents/skills compatibility symlink.Sources: Coding guidelines, Learnings
plugins/modelopt/skills/deployment/scripts/deploy.sh-25-32 (1)
25-32: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake external binding opt-in
deploy.shand these examples bind the service to0.0.0.0explicitly; SGLang’s own default is127.0.0.1, so this widens exposure unnecessarily. Keep public binding behind an explicit opt-in and document the auth/firewall/SSH-tunnel requirement next to the remote-access examples.
plugins/modelopt/skills/deployment/scripts/deploy.shplugins/modelopt/skills/deployment/SKILL.mdplugins/modelopt/skills/deployment/references/setup.mdplugins/modelopt/skills/deployment/references/sglang.mdplugins/modelopt/skills/deployment/references/vllm.md🤖 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 `@plugins/modelopt/skills/deployment/scripts/deploy.sh` around lines 25 - 32, Make external binding opt-in across deploy.sh and the documented deployment examples: default the deploy script to SGLang’s loopback address, add an explicit opt-in mechanism for 0.0.0.0, and update every referenced example to avoid public binding by default. In plugins/modelopt/skills/deployment/SKILL.md (117-123, 142-147, 232-236), references/setup.md (57-61, 84-90, 99-105), references/sglang.md (36-40), and references/vllm.md (15-20), document that remote/public access requires explicit binding plus appropriate authentication, firewalling, or an SSH tunnel.plugins/modelopt/skills/deployment/references/support-matrix.md-63-64 (1)
63-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the Blackwell image tag explicit
support-matrix.mdsayscu130is now the vLLM default, butSKILL.mdstill says the default build iscu12. That mismatch can send B300/GB300 users to an image without thesm_103FP4 kernel; drop the default claim here and require an explicit-cu130tag.🤖 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 `@plugins/modelopt/skills/deployment/references/support-matrix.md` around lines 63 - 64, Update the NVFP4 support note in the deployment support matrix to remove the claim that cu130 is the vLLM default and state that B300/GB300 deployments must explicitly use a -cu130 serving image. Preserve the warning that older cu12 images lack the sm_103 FP4 kernel.plugins/modelopt/skills/evaluation/tests/evals.json-23-23 (1)
23-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the expected
targetpath.
targetis a sibling ofconfigunderevaluation.nemo_evaluator_config; expectingconfig.target.api_endpointtrains the evaluator to produce an invalid config shape.🤖 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 `@plugins/modelopt/skills/evaluation/tests/evals.json` at line 23, Update the expected path in the evaluation test description to reference evaluation.nemo_evaluator_config.target.api_endpoint.adapter_config, removing the incorrect config segment while preserving the existing interceptor field-name expectations.plugins/modelopt/skills/launching-evals/SKILL.md-55-55 (1)
55-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute config creation to the installed
evaluationskill.
nel-assistantis not the skill introduced in this plugin; the canonical skill in this cohort isevaluation. This route will fail to hand users to the config-authoring workflow.🤖 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 `@plugins/modelopt/skills/launching-evals/SKILL.md` at line 55, Update the config-creation instruction in the launching-evals skill to route users through the installed evaluation skill instead of nel-assistant, while preserving the existing past-run config.yml starting-point behavior.plugins/modelopt/skills/evaluation/recipes/examples/example_eval.yaml-158-160 (1)
158-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not emit both thinking-toggle keys.
The adjacent guidance says model families accept only one key and that an unused DeepSeek key can error, but this template sets both. Leave this block commented out or retain only the model-card-selected key.
🤖 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 `@plugins/modelopt/skills/evaluation/recipes/examples/example_eval.yaml` around lines 158 - 160, Update the chat_template_kwargs block in the evaluation recipe so it does not emit both thinking-toggle keys: leave the block commented out or retain only the model-card-selected key, removing the unused alternative.plugins/modelopt/skills/launching-evals/SKILL.md-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComplete the required vendored-source attribution.
This declares the document “Vendored verbatim” but lacks the original copyright/license notice and NVIDIA Apache-2.0 header required for copied third-party source.
As per coding guidelines, copied third-party source must include the source commit, original copyright/license, and NVIDIA Apache 2.0 header.
🤖 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 `@plugins/modelopt/skills/launching-evals/SKILL.md` around lines 5 - 7, Add the required NVIDIA copyright and Apache-2.0 license attribution to the header of SKILL.md, preserving the existing source repository, commit, and re-sync information in the vendored-source notice.Source: Coding guidelines
plugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-general-info.md-102-104 (1)
102-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not present eight 1-sample runs as equivalent to one 8-sample run.
Independent runs change run-level aggregation, including reported
pass_at_k. Either document the supported aggregation/reconstruction procedure or label this throughput workaround as non-comparable to the canonical multi-sample result.🤖 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 `@plugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-general-info.md` around lines 102 - 104, Update the “8x1 pattern” guidance to state that independent single-sample runs are not equivalent to one 8-sample run because run-level metrics such as pass_at_k differ. Either document how to aggregate or reconstruct the canonical result, or explicitly label this pattern as a throughput workaround whose results are non-comparable.plugins/modelopt/skills/monitor/SKILL.md-74-83 (1)
74-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse type-specific terminal states during cleanup.
The generic removal list omits NEL’s documented terminal states
SUCCESS,ERROR, andNOT FOUNDfrom Lines 111-115. Successful NEL jobs can therefore remain in the registry and keep the monitor alive indefinitely. Delegate cleanup to the per-type terminal predicates.🤖 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 `@plugins/modelopt/skills/monitor/SKILL.md` around lines 74 - 83, Update the “Remove completed jobs” step in the monitor workflow to use each job type’s terminal-state predicate instead of the generic terminal-state list. Ensure NEL jobs recognize SUCCESS, ERROR, and NOT FOUND as terminal, while preserving the existing cleanup behavior for other job types and allowing the monitor to exit when none remain.plugins/modelopt/skills/monitor/SKILL.md-37-57 (1)
37-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord the launcher log path.
active_jobs.jsondoesn’t store any output-file location, sotype: launcherjobs can’t be resumed from the registry alone unless that path is derived elsewhere. Add a required field or document the exact derivation.🤖 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 `@plugins/modelopt/skills/monitor/SKILL.md` around lines 37 - 57, Update the active_jobs.json schema documentation to include a required launcher log/output path field for type: launcher jobs, or specify the exact deterministic derivation used to recover it from the registry. Ensure launcher jobs can be resumed using only their recorded registry data, while leaving non-launcher job fields unchanged.plugins/modelopt/skills/evaluation/references/parallelism.md-109-112 (1)
109-112: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the current nel-next field for the KV-cache flag.
This reference tells users to place
--kv-cache-dtype fp8indeployment.command, butnel-next.mddefines the active schema asservices.<svc>.extra_argsand explicitly maps serving flags there. Update this example to prevent schema validation failures or ignored settings.🤖 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 `@plugins/modelopt/skills/evaluation/references/parallelism.md` around lines 109 - 112, Update the KV-cache quantization example in the referenced parallelism guidance to use the active nel-next schema, placing the serving flag through services.<svc>.extra_args rather than deployment.command. Preserve the existing fp8 dtype options and surrounding concurrency guidance.plugins/modelopt/skills/evaluation/recipes/tasks/aa/tau2_bench_telecom.md-52-52 (1)
52-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not leave the required runtime value unresolved in the YAML fragment.
parallelism: ???is not an executable configuration value. Provide a concrete conservative default, or keep the placeholder outside the copy-pastable YAML and explicitly require the user to replace it before validation.🤖 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 `@plugins/modelopt/skills/evaluation/recipes/tasks/aa/tau2_bench_telecom.md` at line 52, Replace the unresolved parallelism value in the configuration fragment with a concrete conservative default that respects the cap of 512, and ensure the surrounding guidance still instructs users to recompute --max-num-seqs after setting it.
🧹 Nitpick comments (2)
plugins/modelopt/skills/ptq/references/slurm-setup-ptq.md (1)
31-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the Transformers version used by the SLURM workflow.
Both upgrade paths install the latest available package, and
PIP_CONSTRAINTis explicitly removed. Repeated runs can therefore resolve different Transformers and dependency versions, while--no-depscan leave an incomplete environment. Use a tested exact pin, consistent with the launcher guide’s==requirement, and document any intentional constraint override.Also applies to: 51-61
🤖 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 `@plugins/modelopt/skills/ptq/references/slurm-setup-ptq.md` around lines 31 - 35, Update both Transformers installation paths in the SLURM workflow to use the tested exact version pin from the launcher guide, retaining the == requirement. Remove any --no-deps usage so dependencies are installed completely, and document the intentional PIP_CONSTRAINT override wherever the workflow clears it.plugins/modelopt/skills/day0-release/scripts/gate_compare.py (1)
1-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNone of the three gate scripts define
__all__. Same gap repeated ingate_compare.py,gate_ptq.py, andgate_run.py.
plugins/modelopt/skills/day0-release/scripts/gate_compare.py#L1-L37: add__all__ = ["evaluate_comparison", "main"].plugins/modelopt/skills/day0-release/scripts/gate_ptq.py#L1-L48: add__all__ = ["evaluate_checkpoint", "main"].plugins/modelopt/skills/day0-release/scripts/gate_run.py#L1-L46: add__all__ = ["evaluate_run", "main"].As per coding guidelines, "Define each module's public API with
__all__ = [...]."🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_compare.py` around lines 1 - 37, Add the module public API declarations in all three listed files: define __all__ in gate_compare.py with evaluate_comparison and main, in gate_ptq.py with evaluate_checkpoint and main, and in gate_run.py with evaluate_run and main.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f082fb76-572c-42aa-9867-c0c759222ccf
📒 Files selected for processing (89)
.agents/README.md.agents/TOOLING.md.agents/plugins/marketplace.json.agents/scripts/sync-upstream-skills.sh.agents/skills.claude-plugin/marketplace.json.markdownlint-cli2.yaml.pre-commit-config.yamlAGENTS.mdREADME.mdplugins/modelopt/.claude-plugin/plugin.jsonplugins/modelopt/.codex-plugin/plugin.jsonplugins/modelopt/skills/accessing-mlflow/SKILL.mdplugins/modelopt/skills/common/SKILL.mdplugins/modelopt/skills/common/credentials.mdplugins/modelopt/skills/common/environment-setup.mdplugins/modelopt/skills/common/remote-execution.mdplugins/modelopt/skills/common/remote_exec.shplugins/modelopt/skills/common/slurm-setup.mdplugins/modelopt/skills/common/workspace-management.mdplugins/modelopt/skills/compare-results/SKILL.mdplugins/modelopt/skills/compare-results/tests/evals.jsonplugins/modelopt/skills/day0-release/SKILL.mdplugins/modelopt/skills/day0-release/scripts/gate_compare.pyplugins/modelopt/skills/day0-release/scripts/gate_ptq.pyplugins/modelopt/skills/day0-release/scripts/gate_run.pyplugins/modelopt/skills/day0-release/tests/evals.jsonplugins/modelopt/skills/day0-release/tests/test_gates.pyplugins/modelopt/skills/debug/SKILL.mdplugins/modelopt/skills/deployment/SKILL.mdplugins/modelopt/skills/deployment/references/benchmarking.mdplugins/modelopt/skills/deployment/references/setup.mdplugins/modelopt/skills/deployment/references/sglang.mdplugins/modelopt/skills/deployment/references/support-matrix.mdplugins/modelopt/skills/deployment/references/trtllm.mdplugins/modelopt/skills/deployment/references/unsupported-models.mdplugins/modelopt/skills/deployment/references/vllm.mdplugins/modelopt/skills/deployment/scripts/deploy.shplugins/modelopt/skills/deployment/tests/evals.jsonplugins/modelopt/skills/eagle3-new-model/SKILL.mdplugins/modelopt/skills/eagle3-review-logs/SKILL.mdplugins/modelopt/skills/eagle3-triage/SKILL.mdplugins/modelopt/skills/eagle3-validate/SKILL.mdplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/evaluation/recipes/env.exampleplugins/modelopt/skills/evaluation/recipes/examples/example_eval.yamlplugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yamlplugins/modelopt/skills/evaluation/recipes/tasks/aa/gpqa_diamond.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/hle.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/ifbench.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/lcr.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/mmmu_pro.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/omniscience.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/scicode.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa/tau2_bench_telecom.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/swebench_verified.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.mdplugins/modelopt/skills/evaluation/recipes/tasks/aime_2025.mdplugins/modelopt/skills/evaluation/recipes/tasks/livecodebench.mdplugins/modelopt/skills/evaluation/recipes/tasks/mmlu_pro.mdplugins/modelopt/skills/evaluation/references/model-card-research.mdplugins/modelopt/skills/evaluation/references/multi-node.mdplugins/modelopt/skills/evaluation/references/nel-next.mdplugins/modelopt/skills/evaluation/references/parallelism.mdplugins/modelopt/skills/evaluation/references/quantization-benchmarks.mdplugins/modelopt/skills/evaluation/references/run-validation.mdplugins/modelopt/skills/evaluation/references/slurm.mdplugins/modelopt/skills/evaluation/tests/evals.jsonplugins/modelopt/skills/launching-evals/SKILL.mdplugins/modelopt/skills/launching-evals/references/analyze-results.mdplugins/modelopt/skills/launching-evals/references/benchmarks/swebench-general-info.mdplugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-general-info.mdplugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-trace-analysis.mdplugins/modelopt/skills/launching-evals/references/check-progress.mdplugins/modelopt/skills/launching-evals/references/debug-failed-runs.mdplugins/modelopt/skills/launching-evals/references/run-evaluation.mdplugins/modelopt/skills/launching-evals/tests.jsonplugins/modelopt/skills/monitor/SKILL.mdplugins/modelopt/skills/ptq/SKILL.mdplugins/modelopt/skills/ptq/references/checkpoint-validation.mdplugins/modelopt/skills/ptq/references/launcher-guide.mdplugins/modelopt/skills/ptq/references/slurm-setup-ptq.mdplugins/modelopt/skills/ptq/references/unsupported-models.mdplugins/modelopt/skills/ptq/tests.jsonplugins/modelopt/skills/quant-recipe-search/SKILL.mdplugins/modelopt/skills/quant-recipe-search/references/qwen36_case_study.mdplugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.mdplugins/modelopt/skills/release-cherry-pick/SKILL.mdpyproject.toml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (4)
plugins/modelopt/skills/common/remote_exec.sh (1)
110-126: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Replace string-built SSH/rsync commands with argument arrays.
Configuration values and caller paths are interpolated into strings that
evalre-parses. A crafted cluster config (ssh_proxy,ssh_key, host/user) or a path containing shell syntax can execute commands on the local agent machine. Build SSH and rsync invocations as Bash arrays and invoke them with"${args[@]}"; remove everyeval.
plugins/modelopt/skills/common/remote_exec.sh#L110-L126: return structured SSH arguments instead of a shell-escaped string.plugins/modelopt/skills/common/remote_exec.sh#L159-L160: invokesshdirectly with the argument array.plugins/modelopt/skills/common/remote_exec.sh#L269-L269: invoke the connectivity probe directly with the argument array.plugins/modelopt/skills/common/remote_exec.sh#L356-L356: invoke the encoded remote payload directly with the argument array.plugins/modelopt/skills/common/remote_exec.sh#L379-L401: build rsync arguments as arrays so local and remote paths remain data, not shell source.📍 Affects 1 file
plugins/modelopt/skills/common/remote_exec.sh#L110-L126(this comment)plugins/modelopt/skills/common/remote_exec.sh#L159-L160plugins/modelopt/skills/common/remote_exec.sh#L269-L269plugins/modelopt/skills/common/remote_exec.sh#L356-L356plugins/modelopt/skills/common/remote_exec.sh#L379-L401🤖 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 `@plugins/modelopt/skills/common/remote_exec.sh` around lines 110 - 126, Replace string-built SSH and rsync commands with Bash argument arrays to prevent configuration values and paths from being re-parsed as shell syntax. In _ssh_base_opts, return structured SSH arguments; update the SSH invocations at plugins/modelopt/skills/common/remote_exec.sh lines 159-160, 269-269, and 356-356 to invoke the arrays directly without eval. At lines 379-401, build rsync arguments as arrays for both local and remote paths, preserving each value as a single argument.Source: Linters/SAST tools
plugins/modelopt/skills/evaluation/SKILL.md (2)
111-117: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Require explicit opt-in before executing model-supplied code.
The default flow infers trust from
auto_map, and the nel-next template unconditionally adds--trust-remote-code. A malicious or untrusted checkpoint can therefore execute supplied Python during deployment.
plugins/modelopt/skills/evaluation/SKILL.md#L111-L117: instruct users to opt in only after confirming the model source is trusted; do not infer consent fromauto_map.plugins/modelopt/skills/evaluation/recipes/examples/example_eval.yaml#L95-L98: make remote-code execution an explicit, commented opt-in.plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml#L26-L31: remove the default--trust-remote-codeflag and require an explicit caller-selected setting.As per coding guidelines, remote-code trust must be caller-configurable and default to false.
📍 Affects 3 files
plugins/modelopt/skills/evaluation/SKILL.md#L111-L117(this comment)plugins/modelopt/skills/evaluation/recipes/examples/example_eval.yaml#L95-L98plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml#L26-L31🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 111 - 117, Require explicit caller opt-in for remote-code execution and never infer trust from auto_map in plugins/modelopt/skills/evaluation/SKILL.md lines 111-117; document verification of the model source before enabling it. In plugins/modelopt/skills/evaluation/recipes/examples/example_eval.yaml lines 95-98, make the trust flag an explicit commented opt-in. In plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml lines 26-31, remove the default --trust-remote-code setting and expose it only through an explicitly selected caller configuration, with false as the default.Source: Coding guidelines
353-365: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not authorize arbitrary
pre_cmdexecution by default.The generated
.envenablesNEMO_EVALUATOR_TRUST_PRE_CMD=1, and Step 8 sources it before executing a config whosepre_cmdcan run arbitrary commands. The test also makes adding such a command expected behavior.
plugins/modelopt/skills/evaluation/SKILL.md#L353-L365: require explicit user confirmation of the exact reviewedpre_cmdbefore setting the trust variable.plugins/modelopt/skills/evaluation/recipes/env.example#L17-L18: leave the trust variable commented out, with an opt-in warning.plugins/modelopt/skills/evaluation/tests/evals.json#L16-L17: expect an approval gate rather than unconditionalpre_cmdinsertion.📍 Affects 3 files
plugins/modelopt/skills/evaluation/SKILL.md#L353-L365(this comment)plugins/modelopt/skills/evaluation/recipes/env.example#L17-L18plugins/modelopt/skills/evaluation/tests/evals.json#L16-L17🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 353 - 365, The evaluation workflow must not authorize arbitrary pre_cmd execution by default. In plugins/modelopt/skills/evaluation/SKILL.md lines 353-365, require explicit user confirmation of the exact reviewed pre_cmd before setting NEMO_EVALUATOR_TRUST_PRE_CMD=1; in plugins/modelopt/skills/evaluation/recipes/env.example lines 17-18, keep that variable commented out with an opt-in warning; and in plugins/modelopt/skills/evaluation/tests/evals.json lines 16-17, update expectations to require the approval gate rather than unconditional pre_cmd insertion.plugins/modelopt/skills/monitor/SKILL.md (1)
133-139: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Prevent remote command injection through
jid.Line 136 interpolates
$jidinto a remote shell string. Because job IDs can originate from user input or the registry, a crafted value can execute arbitrary commands on the cluster. Validate the expected SLURM ID format and invokesacctwithout embedding untrusted text in a shell command.🤖 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 `@plugins/modelopt/skills/monitor/SKILL.md` around lines 133 - 139, Harden extract_slurm_state by validating jid against the expected SLURM job-ID format before making the SSH call, rejecting invalid values without executing a remote command. Replace the interpolated remote command string with a safe invocation that passes the validated job ID as an argument rather than embedding untrusted text in shell syntax, while preserving the existing sacct output normalization.
169be38 to
71a9c33
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
71a9c33 to
ab56a31
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
plugins/modelopt/skills/day0-release/scripts/gate_run.py-58-74 (1)
58-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject malformed run-summary structures before processing them.
A valid JSON file can set
tasksto[], a task record tonull, orerrorsto{}. The current code then calls.items(),.get(), or" ".join()on incompatible values and raises instead of returning a non-passing verdict. A sufficiently large JSON integer score can also raiseOverflowErrorinmath.isfinite.Validate
summary,tasks, task records, anderrorsagainst the documented schema at the interface boundary. Return a structured configuration error for invalid input. Applymath.isfiniteonly after safe numeric validation.Also applies to: 102-104
🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_run.py` around lines 58 - 74, Update the run-summary validation at the gate_run interface before iterating tasks: require summary and tasks to be mappings, each task record to be a mapping, and errors to be a sequence of valid error values rather than accepting []/null/{} blindly. Return the existing structured USER_CONFIG_ERROR verdict for every schema violation, and validate numeric fields—including score for finite, range-safe numeric values—before calling math.isfinite or processing them.plugins/modelopt/skills/day0-release/scripts/gate_run.py-95-97 (1)
95-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire valid sample counts.
The
is not Noneguard accepts omitted counts and equal non-integer values. For example, aSUCCESStask with a numeric score and no sample counts passes this gate. This allows an incompletely accounted evaluation run into the release decision.Reject missing, Boolean, non-integer, and negative sample counts before comparing them. Classify these cases as
SAMPLE_ACCOUNTING_FAILED. Add tests for omitted and string-valued counts.Proposed fix
- if expected is not None and scored is not None and scored != expected: + valid_counts = ( + isinstance(expected, int) + and not isinstance(expected, bool) + and expected >= 0 + and isinstance(scored, int) + and not isinstance(scored, bool) + and scored >= 0 + ) + if not valid_counts: + ok = False + reasons.append("sample accounting: invalid or missing counts") + elif scored != expected: ok = False reasons.append(f"sample accounting: scored {scored} of {expected}")🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_run.py` around lines 95 - 97, Validate scored and expected sample counts before the comparison in the gate logic: reject missing, Boolean, non-integer, and negative values, classify any such case as SAMPLE_ACCOUNTING_FAILED, and retain the mismatch failure for valid unequal counts. Add coverage for omitted counts and string-valued counts, while preserving valid integer accounting behavior.plugins/modelopt/skills/eagle3-validate/SKILL.md-17-24 (1)
17-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSearch the configured experiment location.
The command only searches
experiments/cicd/cicd_*. Valid runs can be underexperiments/or the path supplied through--job-dir.Ask for the experiment path first. If no path is available, search the launcher output location without requiring the
cicd_*name.plugins/modelopt/skills/eagle3-review-logs/SKILL.md:15-18documents both locations.🤖 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 `@plugins/modelopt/skills/eagle3-validate/SKILL.md` around lines 17 - 24, Update the experiment-directory discovery instructions around the “most recent experiment directory” step to ask for the experiment path first, then search the configured launcher output location when no path is provided. Support both the default experiments location and the path supplied through --job-dir, without requiring a cicd_* directory name; align the search behavior with the locations documented in eagle3-review-logs.plugins/modelopt/skills/day0-release/scripts/gate_compare.py-92-126 (1)
92-126: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the complete JSON schema at the CLI boundary.
Malformed JSON objects can reach unchecked mapping and numeric operations. The gates then raise exceptions instead of returning structured configuration errors.
plugins/modelopt/skills/day0-release/scripts/gate_compare.py#L92-L126: require mapping inputs, preserve invalidscalesvalues for rejection, and require a finite non-negativethreshold.plugins/modelopt/skills/day0-release/scripts/gate_ptq.py#L66-L103: require a summary mapping, mapping precision counts, finite non-boolean sizes, non-negative output size, and non-negative integer counts.- Both CLIs: return exit code
2when the resulting failure class isUSER_CONFIG_ERROR.🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_compare.py` around lines 92 - 126, Validate the complete CLI input schema before processing: in gate_compare.py, require baseline and candidate mappings, preserve invalid scales for rejection, and require threshold to be finite and non-negative; in gate_ptq.py, validate the summary mapping, precision-count mappings, finite non-boolean sizes, non-negative output size, and non-negative integer counts. Update both CLIs to exit with code 2 whenever the resulting failure_class is USER_CONFIG_ERROR.plugins/modelopt/skills/deployment/scripts/deploy.sh-199-203 (1)
199-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCreate and verify a dedicated server process group.
A reused PID for any Python process passes
is_server_running. Also,nohupdoes not make the launched process a process-group leader, sokill -- -"$pid"usually fails and cleanup only kills the parent.
plugins/modelopt/skills/deployment/scripts/deploy.sh#L199-L203: verify the expectedvllmorsglangcommand and persist a process identity marker such as start time.plugins/modelopt/skills/deployment/scripts/deploy.sh#L292-L293: launch vLLM in a dedicated session or process group.plugins/modelopt/skills/deployment/scripts/deploy.sh#L319-L320: launch SGLang in the same way.🤖 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 `@plugins/modelopt/skills/deployment/scripts/deploy.sh` around lines 199 - 203, Update plugins/modelopt/skills/deployment/scripts/deploy.sh lines 199-203 in is_server_running to accept only the expected vLLM or SGLang command, persist and validate a process identity marker such as start time, and reject reused Python PIDs; update lines 292-293 in the vLLM launch and lines 319-320 in the SGLang launch to start each server in a dedicated session or process group so cleanup via the server PID terminates the full process group.plugins/modelopt/skills/common/environment-setup.md-37-40 (1)
37-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the helper from the installed plugin root.
.agents/skillsexists only as a repository compatibility symlink. Use the plugin-root resolver instead, such as${CLAUDE_PLUGIN_ROOT}/skills/common/remote_exec.shfor Claude Code. Apply this toenvironment-setup.md,deployment/SKILL.md, andcommon/remote-execution.md.🤖 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 `@plugins/modelopt/skills/common/environment-setup.md` around lines 37 - 40, Resolve remote_exec.sh from the installed plugin root instead of the repository-relative .agents/skills path. Update plugins/modelopt/skills/common/environment-setup.md lines 37-40 and plugins/modelopt/skills/deployment/SKILL.md lines 219-223, preserving the existing remote_load_cluster, remote_check_ssh, and remote_detect_env calls while using the plugin-root-based path; apply the same path correction in common/remote-execution.md.Source: Coding guidelines
plugins/modelopt/skills/evaluation/references/parallelism.md-3-7 (1)
3-7: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not state that topology and concurrency never affect scores.
The document later describes preemption, truncation, timeouts, and retries. These failures can change the number and content of scored responses. Replace “never scores” with a qualified statement such as “primarily affect throughput; validate score integrity after tuning.”
🤖 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 `@plugins/modelopt/skills/evaluation/references/parallelism.md` around lines 3 - 7, Update the “Two decisions, in order” statement in the parallelism guidance to remove the claim that topology and concurrency never affect scores. Qualify their impact as primarily affecting throughput and instruct readers to validate score integrity after tuning, consistent with the documented preemption, truncation, timeout, and retry behavior.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-109-110 (1)
109-110: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSupport single-file exports and missing quantization metadata.
The layer-coverage script unconditionally opens
model.safetensors.index.jsonandhf_quant_config.json. A valid non-sharded export may have onlymodel.safetensors, and some exports may store quantization metadata only inconfig.json. The required validation gate then crashes instead of producing a result. Reuse the single-file header scan from the VLM check and fail with an explicit remediation message when metadata is unavailable.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 109 - 110, Update the layer-coverage validation script around the `model.safetensors.index.json` and `hf_quant_config.json` loads to support non-sharded exports by reusing the VLM check’s single-file header scan. Fall back to quantization metadata from `config.json` when `hf_quant_config.json` is absent, and fail with an explicit remediation message when required metadata cannot be found.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-85-91 (1)
85-91: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when the source contains no supported weight files.
For a directory containing
.binor.ptshards,safetensor_bytes()returns zero. The ratio becomesNaN, so the blocking rule at line 98 does not run. Raise an error for an empty source set or explicitly support the source formats used by the validation gate.🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 85 - 91, Update safetensor_bytes and the validation flow to handle supported source weight files beyond .safetensors, including .bin and .pt shards, or reject directories with no supported weight files before calculating the ratio. Ensure an empty source set raises an error and cannot produce NaN that bypasses the blocking validation rule.plugins/modelopt/skills/evaluation/references/parallelism.md-94-98 (1)
94-98: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the worked example below the request ceiling.
The rules state that
parallelismcannot exceeddataset_size × repeats, but the example usesparallelism=256for 198 requests. Use198(or document a different client-side meaning for 256). The example should also derive--max-num-seqsfrom the selected value.Also applies to: 154-157
🤖 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 `@plugins/modelopt/skills/evaluation/references/parallelism.md` around lines 94 - 98, Update the worked example’s parallelism value to remain at or below the 198-request ceiling, or explicitly define a different client-side meaning for 256. Derive the example’s --max-num-seqs setting from the selected parallelism value, and apply the same correction to the corresponding example section around the later referenced lines.plugins/modelopt/skills/ptq/SKILL.md-38-42 (1)
38-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScan all remote-code Python modules.
The command only scans
modeling_*.py, butauto_mapcan reference configuration, tokenizer, processor, or other custom modules. Missing imports will be discovered only when model loading fails. Inspect every file listed byauto_map, or scan all custom Python files.Proposed correction
-grep -h "^from \|^import " <model_path>/modeling_*.py | sort -u +find <model_path> -type f -name '*.py' -exec \ + grep -hE '^(from|import) ' {} + | sort -u🤖 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 `@plugins/modelopt/skills/ptq/SKILL.md` around lines 38 - 42, Update the remote-code dependency inspection guidance in the trust_remote_code section to scan every custom Python module referenced by config.json auto_map, or all custom Python files when appropriate, rather than limiting the command to modeling_*.py; preserve the existing import extraction and deduplication behavior.plugins/modelopt/skills/ptq/references/checkpoint-validation.md-54-55 (1)
54-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck the full quantization scale-key set in the VLM gate.
The VLM scan checks only
weight_scaleandinput_scale, but the coverage script recognizesweight_scale_2,activation_scale, andweight_scale_inv. A vision tensor with only an omitted key can pass the required VLM gate.Proposed correction
-vis_q = [k for k in keys if any(t in k for t in ('model.visual', 'vision_tower', 'vision_model')) - and any(s in k for s in ('weight_scale', 'input_scale'))] +vis_q = [k for k in keys + if any(t in k for t in ('model.visual', 'vision_tower', 'vision_model')) + and any(s in k for s in ( + 'weight_scale', 'weight_scale_2', 'input_scale', + 'activation_scale', 'weight_scale_inv'))]🤖 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 `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md` around lines 54 - 55, Update the VLM scale-key filter in the checkpoint validation scan to recognize the complete quantization key set: weight_scale, input_scale, weight_scale_2, activation_scale, and weight_scale_inv. Keep the existing vision-key matching unchanged so the VLM gate evaluates all relevant vision quantization tensors.plugins/modelopt/skills/ptq/SKILL.md-21-22 (1)
21-22: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse an unambiguous shared-skill path convention.
No repository-root
skills/commondirectory exists. Useplugins/modelopt/skills/common/...for repository-root-relative references, or document that the loader resolvesskills/common/...from the plugin root. Apply the same convention at lines 21–22 and 199–200.Expand the custom-code dependency scan beyond
modeling_*.py, or state its limitation. Custom checkpoints can also providetokenization_*.pyand other imported modules.🤖 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 `@plugins/modelopt/skills/ptq/SKILL.md` around lines 21 - 22, Update the skill references at the indicated introductory and later sections to use the unambiguous plugins/modelopt/skills/common/... convention, or explicitly document plugin-root resolution. Also expand the custom-code dependency scan beyond modeling_*.py to include tokenization_*.py and other imported modules, or clearly state the scan’s limitation.Sources: Coding guidelines, Learnings
plugins/modelopt/skills/evaluation/references/parallelism.md-86-88 (1)
86-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one
--max-num-seqsformula.When
num_instances > 1, use:Proposed correction
--max-num-seqs = ceil(max parallelism across tasks / DP) +--max-num-seqs = ceil(max parallelism across tasks / (DP × num_instances))
parallelismis global across instances, while--max-num-seqsapplies per instance. Update the duplicate guidance inplugins/modelopt/skills/evaluation/SKILL.md.🤖 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 `@plugins/modelopt/skills/evaluation/references/parallelism.md` around lines 86 - 88, Update the duplicate max-num-seqs guidance in SKILL.md to use a single formula that divides global parallelism by DP multiplied by num_instances, applying ceiling as needed. Keep the serving_capacity relationship consistent and ensure the documentation reflects that max-num-seqs is per instance while parallelism is global.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py-845-855 (1)
845-855: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
combined_results.csvis lost when any step after case execution raises.mainwrites the combined table as its final action, so every failure-prone step that runs before_write_resultscan discard results that were already measured.SKILL.mddocuments the opposite contract: the table is always written and the command then exits nonzero.
plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py#L845-L855: wrap the_attach_quant_timescall in a handler that records the failure as anERROR: ...quant_result, then always call_write_results.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py#L776-L780: guard thefloat(case_rows[-1]["median_time"])conversion withtry/except (KeyError, TypeError, ValueError)and fall back to_failure_messageinstead of propagating.🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py` around lines 845 - 855, Ensure main always writes combined_results.csv: wrap _attach_quant_times in failure handling that records an ERROR: ... quant_result, then call _write_results regardless of attachment failures; additionally, in the result-building logic around float(case_rows[-1]["median_time"]), catch KeyError, TypeError, and ValueError and use _failure_message instead of propagating. Apply both changes in plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py at lines 845-855 and 776-780.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py-32-32 (1)
32-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the
# nosecmarkers or obtain explicit codeowner approval.Three Bandit suppressions are present:
# nosec B404at Line 32,# nosec B603at Line 235, and# nosec B603 B607at Line 297. The repository guidelines prohibit# nosecas a bypass.Both call sites already avoid a shell and pass a fixed executable with an argument list, so the underlying pattern is defensible. The suppression mechanism is the problem. Configure the Bandit skip in the project configuration, or record the approval in the PR description.
As per coding guidelines: "Any use of '# nosec' comments to bypass Bandit security checks is not allowed. If a security-sensitive pattern is genuinely necessary, the PR must be reviewed and approved by
@NVIDIA/modelopt-setup-codeownerswith an explicit justification in the PR description."Run the following script to check the repository's Bandit configuration and existing precedent:
#!/bin/bash # Description: Inspect Bandit configuration and existing nosec usage. fd -H -t f 'pyproject.toml|.bandit|bandit.yaml|.pre-commit-config.yaml' --exec sh -c 'echo "== {} =="; rg -n -A6 -i "bandit|skips|B603|B404|B607" "{}"' # Existing nosec precedent elsewhere in the repository. rg -n --type=py 'nosec' | head -40Also applies to: 235-235, 297-297
🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py` at line 32, Remove the inline # nosec suppressions associated with subprocess usage in benchmark_via_builtin.py, including the import and both subprocess call sites. Configure the applicable Bandit exclusions in the project’s existing Bandit configuration instead, following repository precedent and preserving the non-shell, fixed-argument-list execution behavior.Source: Coding guidelines
plugins/modelopt/skills/common/remote_exec.sh-104-107 (1)
104-107: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse portable encodings for
base64and the host hash.
base64 -w0andmd5sumare GNU coreutils features. BSD/macOSbase64does not accept-w, and macOS shipsmd5instead ofmd5sum. On a macOS workstation,remote_runproduces an empty or wrapped payload and_ssh_control_pathproduces an empty hash. Both failures are silent.Use
tr -d '\n'for wrapping and a portable hash helper.🔧 Proposed portable fallbacks
- host_hash=$(echo "${REMOTE_USER}@${REMOTE_HOST}" | md5sum | cut -c1-12) + if command -v md5sum >/dev/null 2>&1; then + host_hash=$(printf '%s' "${REMOTE_USER}@${REMOTE_HOST}" | md5sum | cut -c1-12) + else + host_hash=$(printf '%s' "${REMOTE_USER}@${REMOTE_HOST}" | md5 -q | cut -c1-12) + fi- encoded=$(printf '%s' "$full_cmd" | base64 -w0) + encoded=$(printf '%s' "$full_cmd" | base64 | tr -d '\n')Also applies to: 348-352
🤖 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 `@plugins/modelopt/skills/common/remote_exec.sh` around lines 104 - 107, Update the base64 encoding in remote_run to remove line breaks with tr -d '\n' instead of the GNU-only -w0 option, and update _ssh_control_path to compute host_hash through a portable hash helper that supports macOS md5 and GNU md5sum. Preserve the existing payload and short control-path behavior while ensuring neither command silently produces empty or wrapped output.plugins/modelopt/skills/common/remote_exec.sh-476-495 (1)
476-495: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSingle quotes break when
$cmdor$container_or_imagecontains an apostrophe.
remote_docker_runwraps$cmdin single quotes inside a remote command string. A single quote in$cmdcloses the quote early. The remainder is then executed as separate remote shell words. The same problem applies to$container_or_imagein thegrep -xcall at Line 486.
remote_runalready solves quoting with base64. Apply the same approach to the inner container command.🔒 Proposed fix using a base64 payload for the container command
local container_or_image="$1" local cmd="$2" + local encoded_cmd + encoded_cmd=$(printf '%s' "$cmd" | base64 | tr -d '\n') + local inner="echo ${encoded_cmd} | base64 -d | bash" # Check if it's a running container local is_running - is_running=$(remote_run "docker ps --format '{{.Names}}' | grep -x '$container_or_image' 2>/dev/null" 2>&1 || true) + local encoded_name + encoded_name=$(printf '%s' "$container_or_image" | base64 | tr -d '\n') + is_running=$(remote_run "docker ps --format '{{.Names}}' | grep -xF \"\$(echo ${encoded_name} | base64 -d)\" 2>/dev/null" 2>&1 || true) if [[ -n "$is_running" ]]; then echo "Executing in running container: $container_or_image" - remote_run "docker exec $container_or_image bash -c '$cmd'" + remote_run "docker exec ${container_or_image} bash -c ${inner@Q}" else echo "Running in new container: $container_or_image" - remote_run "docker run --rm --gpus all -v ${REMOTE_WORKSPACE}:${REMOTE_WORKSPACE} -w ${REMOTE_WORKSPACE} $container_or_image bash -c '$cmd'" + remote_run "docker run --rm --gpus all -v ${REMOTE_WORKSPACE}:${REMOTE_WORKSPACE} -w ${REMOTE_WORKSPACE} ${container_or_image} bash -c ${inner@Q}" 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 `@plugins/modelopt/skills/common/remote_exec.sh` around lines 476 - 495, Update remote_docker_run to avoid embedding raw container_or_image or cmd values in single-quoted remote command strings. Reuse remote_run’s existing base64 quoting approach for the inner container command, and safely encode or pass container_or_image for the grep check and Docker invocation so apostrophes cannot alter shell parsing.
🟡 Minor comments (4)
plugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-trace-analysis.md-15-15 (1)
15-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the invalid run-level agent-log path.
Terminal-Bench 2.1 documents agent logs only under
artifacts/terminal-bench/{task_name}/{trial_name}/agent-logs/episode-N/. Use this path for trace analysis.🤖 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 `@plugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-trace-analysis.md` at line 15, Update the agent-log reference in the trace-analysis documentation to use the Terminal-Bench 2.1 task/trial episode path under artifacts/terminal-bench/{task_name}/{trial_name}/agent-logs/episode-N/, replacing the invalid run-level tasks.jsonl path.plugins/modelopt/skills/deployment/scripts/deploy.sh-545-553 (1)
545-553: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the TCP port range.
The numeric check accepts
0and values above65535. Port0makes the server choose a port, butwait_for_serverstill probeslocalhost:0. Validate1 <= PORT <= 65535before launch.🤖 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 `@plugins/modelopt/skills/deployment/scripts/deploy.sh` around lines 545 - 553, Update the PORT validation in the numeric-argument validation block to require a value between 1 and 65535 inclusive, rejecting zero and values above 65535 before launch. Preserve the existing error logging and exit behavior, while leaving TP_SIZE validation unchanged.plugins/modelopt/skills/evaluation/references/parallelism.md-21-22 (1)
21-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winChange “must be a power of 2” to “prefer a power of 2.” vLLM supports non-power-of-two TP sizes when the model dimensions are divisible by the selected TP size.
🤖 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 `@plugins/modelopt/skills/evaluation/references/parallelism.md` around lines 21 - 22, Update the TP guidance in the parallelism documentation to say that power-of-two sizes are preferred rather than required. Preserve the requirements that TP divides the model’s attention dimensions and does not cross nodes, while acknowledging non-power-of-two sizes when dimensions are divisible.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py-518-525 (1)
518-525: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a 64-aligned intermediate size for gated
nvfp4_trtllm. The TRT-LLM NVFP4 path requires the gated gate/up dimension (2 * intermediate) to be divisible by 128.fp8_intermediateonly aligns gated sizes to 16; for example, 65 becomes 80 instead of 128. Compute a separate 64-aligned value and add a regression test.🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py` around lines 518 - 525, Update the nvfp4_trtllm registration for trtllm_fp4_block_scale_moe to compute and use a separate 64-aligned intermediate size, rather than fp8_intermediate, so the gated 2 * intermediate dimension is divisible by 128; add a regression test covering a value such as 65 that must round to 128.
🧹 Nitpick comments (7)
plugins/modelopt/skills/day0-release/scripts/gate_run.py (1)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the module public API.
Add
__all__ = ["evaluate_run", "main"]after the imports. This prevents imported standard-library names from becoming unintended exports.As per coding guidelines: “Define each module's public API with
__all__ = [...].”🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_run.py` around lines 41 - 46, Add the module-level __all__ declaration after the imports in gate_run.py, exposing only evaluate_run and main so imported standard-library names are not part of the public API.Source: Coding guidelines
plugins/modelopt/skills/day0-release/scripts/gate_compare.py (1)
49-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the module public APIs.
Add
__all__for the callable gate APIs and exported decision constants.
plugins/modelopt/skills/day0-release/scripts/gate_compare.py#L49-L75: declareACCEPT,REGRESSION,ANOMALOUS,evaluate_comparison, andmain.plugins/modelopt/skills/day0-release/scripts/gate_ptq.py#L50-L61: declareevaluate_checkpointandmain.As per coding guidelines: “Define each module's public API with
__all__ = [...].”🤖 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 `@plugins/modelopt/skills/day0-release/scripts/gate_compare.py` around lines 49 - 75, Declare the module public APIs with __all__: in gate_compare.py, include ACCEPT, REGRESSION, ANOMALOUS, evaluate_comparison, and main; in gate_ptq.py, include evaluate_checkpoint and main. No direct changes are required beyond adding the corresponding export declarations at the affected sites.Source: Coding guidelines
plugins/modelopt/skills/benchmark-model-kernels/tests/test_benchmark_model.py (2)
223-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant assertion after the skip.
Line 224 already exits the test when either class is missing. The
asserton Line 225 can never fail.As per coding guidelines: "Validate external input once at the interface boundary; let internal code trust those checks and avoid redundant assertions."
🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/tests/test_benchmark_model.py` around lines 223 - 225, Remove the redundant assert after the pytest.skip guard in the test setup, leaving the existing config_cls/model_cls availability check and skip behavior unchanged.Source: Coding guidelines
159-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
problemsis empty in the Mixtral and GPT-OSS tests.Both tests discard the third return value of
_inspect_model. If a future change makes these layouts report an unsupported decoder Linear or an expert-count mismatch, the MoE assertion still passes and the regression goes unnoticed. Other tests in this file already assertproblems == [].💚 Proposed change
- _, moe, _ = benchmark_model._inspect_model(model, config, tp=2, ep=2) + _, moe, problems = benchmark_model._inspect_model(model, config, tp=2, ep=2) + assert problems == [] assert moe == benchmark_model._MoeShape(32, 48, 2, 2, "Swiglu")Also applies to: 180-182
🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/tests/test_benchmark_model.py` around lines 159 - 161, Update the Mixtral and GPT-OSS tests around _inspect_model to retain the third return value as problems and assert that problems == [] alongside the existing MoE shape assertions. Do not discard the diagnostic result in either test.plugins/modelopt/skills/common/remote_exec.sh (1)
437-465: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
remote_wait_jobcan poll forever.The loop exits only on a terminal state. A job stuck in
PENDING,CONFIGURING, orSUSPENDEDkeeps the caller blocked with no upper bound. Add an optional maximum wait argument.🤖 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 `@plugins/modelopt/skills/common/remote_exec.sh` around lines 437 - 465, Update remote_wait_job to accept an optional maximum wait duration in addition to the existing job ID and polling interval, track elapsed waiting time across polls, and exit with an error after the limit is reached while preserving current terminal-state handling. Ensure the default behavior remains compatible when no maximum wait is provided.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py (2)
149-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe multimodal fallback hides the original construction error.
Line 152 catches every exception and retries with
text_config. If the retry also fails, the outer handler at Line 159 reports only the second failure. The first failure is usually the more informative one for a plain text model whose config happens to exposetext_config.Chain the original error into the retry failure.
🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py` around lines 149 - 160, Update the inner fallback around AutoModelForCausalLM.from_config so that if the retry using text_config also fails, its exception is chained from the original construction exception. Preserve re-raising the original error when text_config is unavailable, and ensure the outer ShapeError retains the chained error context.
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new script modules omit
__all__. The repository guidelines require every module to declare its public API explicitly. Neither new module does.
plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py#L27-L34: add__all__ = ["ShapeError", "main"]after the imports.plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py#L26-L36: add__all__ = ["main"]after the imports.As per coding guidelines: "Define each module's public API with
__all__ = [...]."🤖 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 `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py` around lines 27 - 34, Define each new script module’s public API with __all__ immediately after its imports: in plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py:27-34, declare ShapeError and main; in plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py:26-36, declare main.Source: Coding guidelines
🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md`:
- Around line 140-174: Require explicit user approval before adding
--trust-remote-code when auto_map or custom remote code is detected, and pin the
reviewed model revision after approval; update the guidance in
plugins/modelopt/skills/evaluation/SKILL.md lines 140-174. Update the comment in
plugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml
lines 109-113 to require the same approval while keeping remote code disabled by
default.
- Around line 84-86: Remove all instructions to source the workspace .env,
including the setup guidance and the corresponding section around the later
referenced steps. Rely on NEL’s automatic workspace-root .env loading as
documented by the launching-evals run-evaluation guidance, while retaining safe
creation via cp and the prohibition on opening .env with Read/Write/Edit.
In `@plugins/modelopt/skills/ptq/references/unsupported-models.md`:
- Around line 13-15: Update the guidance around custom modeling/tokenization
files to require reviewing the checkpoint-supplied source before enabling remote
code execution. Replace the unconditional “always use” wording and ensure
diagnostic commands explicitly pass trust_remote_code=True only after review,
while preserving the existing CLI and helper defaults of False.
---
Major comments:
In
`@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py`:
- Around line 845-855: Ensure main always writes combined_results.csv: wrap
_attach_quant_times in failure handling that records an ERROR: ... quant_result,
then call _write_results regardless of attachment failures; additionally, in the
result-building logic around float(case_rows[-1]["median_time"]), catch
KeyError, TypeError, and ValueError and use _failure_message instead of
propagating. Apply both changes in
plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py
at lines 845-855 and 776-780.
- Line 32: Remove the inline # nosec suppressions associated with subprocess
usage in benchmark_via_builtin.py, including the import and both subprocess call
sites. Configure the applicable Bandit exclusions in the project’s existing
Bandit configuration instead, following repository precedent and preserving the
non-shell, fixed-argument-list execution behavior.
In `@plugins/modelopt/skills/common/environment-setup.md`:
- Around line 37-40: Resolve remote_exec.sh from the installed plugin root
instead of the repository-relative .agents/skills path. Update
plugins/modelopt/skills/common/environment-setup.md lines 37-40 and
plugins/modelopt/skills/deployment/SKILL.md lines 219-223, preserving the
existing remote_load_cluster, remote_check_ssh, and remote_detect_env calls
while using the plugin-root-based path; apply the same path correction in
common/remote-execution.md.
In `@plugins/modelopt/skills/common/remote_exec.sh`:
- Around line 104-107: Update the base64 encoding in remote_run to remove line
breaks with tr -d '\n' instead of the GNU-only -w0 option, and update
_ssh_control_path to compute host_hash through a portable hash helper that
supports macOS md5 and GNU md5sum. Preserve the existing payload and short
control-path behavior while ensuring neither command silently produces empty or
wrapped output.
- Around line 476-495: Update remote_docker_run to avoid embedding raw
container_or_image or cmd values in single-quoted remote command strings. Reuse
remote_run’s existing base64 quoting approach for the inner container command,
and safely encode or pass container_or_image for the grep check and Docker
invocation so apostrophes cannot alter shell parsing.
In `@plugins/modelopt/skills/day0-release/scripts/gate_compare.py`:
- Around line 92-126: Validate the complete CLI input schema before processing:
in gate_compare.py, require baseline and candidate mappings, preserve invalid
scales for rejection, and require threshold to be finite and non-negative; in
gate_ptq.py, validate the summary mapping, precision-count mappings, finite
non-boolean sizes, non-negative output size, and non-negative integer counts.
Update both CLIs to exit with code 2 whenever the resulting failure_class is
USER_CONFIG_ERROR.
In `@plugins/modelopt/skills/day0-release/scripts/gate_run.py`:
- Around line 58-74: Update the run-summary validation at the gate_run interface
before iterating tasks: require summary and tasks to be mappings, each task
record to be a mapping, and errors to be a sequence of valid error values rather
than accepting []/null/{} blindly. Return the existing structured
USER_CONFIG_ERROR verdict for every schema violation, and validate numeric
fields—including score for finite, range-safe numeric values—before calling
math.isfinite or processing them.
- Around line 95-97: Validate scored and expected sample counts before the
comparison in the gate logic: reject missing, Boolean, non-integer, and negative
values, classify any such case as SAMPLE_ACCOUNTING_FAILED, and retain the
mismatch failure for valid unequal counts. Add coverage for omitted counts and
string-valued counts, while preserving valid integer accounting behavior.
In `@plugins/modelopt/skills/deployment/scripts/deploy.sh`:
- Around line 199-203: Update
plugins/modelopt/skills/deployment/scripts/deploy.sh lines 199-203 in
is_server_running to accept only the expected vLLM or SGLang command, persist
and validate a process identity marker such as start time, and reject reused
Python PIDs; update lines 292-293 in the vLLM launch and lines 319-320 in the
SGLang launch to start each server in a dedicated session or process group so
cleanup via the server PID terminates the full process group.
In `@plugins/modelopt/skills/eagle3-validate/SKILL.md`:
- Around line 17-24: Update the experiment-directory discovery instructions
around the “most recent experiment directory” step to ask for the experiment
path first, then search the configured launcher output location when no path is
provided. Support both the default experiments location and the path supplied
through --job-dir, without requiring a cicd_* directory name; align the search
behavior with the locations documented in eagle3-review-logs.
In `@plugins/modelopt/skills/evaluation/references/parallelism.md`:
- Around line 3-7: Update the “Two decisions, in order” statement in the
parallelism guidance to remove the claim that topology and concurrency never
affect scores. Qualify their impact as primarily affecting throughput and
instruct readers to validate score integrity after tuning, consistent with the
documented preemption, truncation, timeout, and retry behavior.
- Around line 94-98: Update the worked example’s parallelism value to remain at
or below the 198-request ceiling, or explicitly define a different client-side
meaning for 256. Derive the example’s --max-num-seqs setting from the selected
parallelism value, and apply the same correction to the corresponding example
section around the later referenced lines.
- Around line 86-88: Update the duplicate max-num-seqs guidance in SKILL.md to
use a single formula that divides global parallelism by DP multiplied by
num_instances, applying ceiling as needed. Keep the serving_capacity
relationship consistent and ensure the documentation reflects that max-num-seqs
is per instance while parallelism is global.
In `@plugins/modelopt/skills/ptq/references/checkpoint-validation.md`:
- Around line 109-110: Update the layer-coverage validation script around the
`model.safetensors.index.json` and `hf_quant_config.json` loads to support
non-sharded exports by reusing the VLM check’s single-file header scan. Fall
back to quantization metadata from `config.json` when `hf_quant_config.json` is
absent, and fail with an explicit remediation message when required metadata
cannot be found.
- Around line 85-91: Update safetensor_bytes and the validation flow to handle
supported source weight files beyond .safetensors, including .bin and .pt
shards, or reject directories with no supported weight files before calculating
the ratio. Ensure an empty source set raises an error and cannot produce NaN
that bypasses the blocking validation rule.
- Around line 54-55: Update the VLM scale-key filter in the checkpoint
validation scan to recognize the complete quantization key set: weight_scale,
input_scale, weight_scale_2, activation_scale, and weight_scale_inv. Keep the
existing vision-key matching unchanged so the VLM gate evaluates all relevant
vision quantization tensors.
In `@plugins/modelopt/skills/ptq/SKILL.md`:
- Around line 38-42: Update the remote-code dependency inspection guidance in
the trust_remote_code section to scan every custom Python module referenced by
config.json auto_map, or all custom Python files when appropriate, rather than
limiting the command to modeling_*.py; preserve the existing import extraction
and deduplication behavior.
- Around line 21-22: Update the skill references at the indicated introductory
and later sections to use the unambiguous plugins/modelopt/skills/common/...
convention, or explicitly document plugin-root resolution. Also expand the
custom-code dependency scan beyond modeling_*.py to include tokenization_*.py
and other imported modules, or clearly state the scan’s limitation.
---
Minor comments:
In
`@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py`:
- Around line 518-525: Update the nvfp4_trtllm registration for
trtllm_fp4_block_scale_moe to compute and use a separate 64-aligned intermediate
size, rather than fp8_intermediate, so the gated 2 * intermediate dimension is
divisible by 128; add a regression test covering a value such as 65 that must
round to 128.
In `@plugins/modelopt/skills/deployment/scripts/deploy.sh`:
- Around line 545-553: Update the PORT validation in the numeric-argument
validation block to require a value between 1 and 65535 inclusive, rejecting
zero and values above 65535 before launch. Preserve the existing error logging
and exit behavior, while leaving TP_SIZE validation unchanged.
In `@plugins/modelopt/skills/evaluation/references/parallelism.md`:
- Around line 21-22: Update the TP guidance in the parallelism documentation to
say that power-of-two sizes are preferred rather than required. Preserve the
requirements that TP divides the model’s attention dimensions and does not cross
nodes, while acknowledging non-power-of-two sizes when dimensions are divisible.
In
`@plugins/modelopt/skills/launching-evals/references/benchmarks/terminal-bench-trace-analysis.md`:
- Line 15: Update the agent-log reference in the trace-analysis documentation to
use the Terminal-Bench 2.1 task/trial episode path under
artifacts/terminal-bench/{task_name}/{trial_name}/agent-logs/episode-N/,
replacing the invalid run-level tasks.jsonl path.
---
Nitpick comments:
In `@plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py`:
- Around line 149-160: Update the inner fallback around
AutoModelForCausalLM.from_config so that if the retry using text_config also
fails, its exception is chained from the original construction exception.
Preserve re-raising the original error when text_config is unavailable, and
ensure the outer ShapeError retains the chained error context.
- Around line 27-34: Define each new script module’s public API with __all__
immediately after its imports: in
plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_model.py:27-34,
declare ShapeError and main; in
plugins/modelopt/skills/benchmark-model-kernels/scripts/benchmark_via_builtin.py:26-36,
declare main.
In
`@plugins/modelopt/skills/benchmark-model-kernels/tests/test_benchmark_model.py`:
- Around line 223-225: Remove the redundant assert after the pytest.skip guard
in the test setup, leaving the existing config_cls/model_cls availability check
and skip behavior unchanged.
- Around line 159-161: Update the Mixtral and GPT-OSS tests around
_inspect_model to retain the third return value as problems and assert that
problems == [] alongside the existing MoE shape assertions. Do not discard the
diagnostic result in either test.
In `@plugins/modelopt/skills/common/remote_exec.sh`:
- Around line 437-465: Update remote_wait_job to accept an optional maximum wait
duration in addition to the existing job ID and polling interval, track elapsed
waiting time across polls, and exit with an error after the limit is reached
while preserving current terminal-state handling. Ensure the default behavior
remains compatible when no maximum wait is provided.
In `@plugins/modelopt/skills/day0-release/scripts/gate_compare.py`:
- Around line 49-75: Declare the module public APIs with __all__: in
gate_compare.py, include ACCEPT, REGRESSION, ANOMALOUS, evaluate_comparison, and
main; in gate_ptq.py, include evaluate_checkpoint and main. No direct changes
are required beyond adding the corresponding export declarations at the affected
sites.
In `@plugins/modelopt/skills/day0-release/scripts/gate_run.py`:
- Around line 41-46: Add the module-level __all__ declaration after the imports
in gate_run.py, exposing only evaluate_run and main so imported standard-library
names are not part of the public API.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (3)
plugins/modelopt/skills/evaluation/SKILL.md (2)
84-86: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not source the workspace
.envfile.
source .envexecutes command substitutions and shell code in any existing workspace file. A malicious or copied.envcan therefore execute code on the submitting host.Let NEL load the workspace-root
.envinstead.plugins/modelopt/skills/launching-evals/references/run-evaluation.md:11-26states that NEL automatically reads that file. Remove the instruction to source it.Also applies to: 389-394
🧰 Tools
🪛 SkillSpector (2.5.1)
[error] 84: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 392: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 394: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 84 - 86, Remove all instructions to source the workspace .env, including the setup guidance and the corresponding section around the later referenced steps. Rely on NEL’s automatic workspace-root .env loading as documented by the launching-evals run-evaluation guidance, while retaining safe creation via cp and the prohibition on opening .env with Read/Write/Edit.
140-174: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Require explicit approval for remote model code.
auto_mapand “custom-code model” identify a remote-code requirement. They do not establish that the model repository is trusted. Do not add--trust-remote-codeautomatically.
plugins/modelopt/skills/evaluation/SKILL.md#L140-L174: Require an explicit user trust decision before adding--trust-remote-code. Pin the reviewed model revision when approval is given.plugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml#L109-L113: Change the comment to require the same explicit approval. Keep remote code disabled by default.🧰 Tools
🪛 SkillSpector (2.5.1)
[error] 84: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 392: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 394: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
📍 Affects 2 files
plugins/modelopt/skills/evaluation/SKILL.md#L140-L174(this comment)plugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml#L109-L113🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 140 - 174, Require explicit user approval before adding --trust-remote-code when auto_map or custom remote code is detected, and pin the reviewed model revision after approval; update the guidance in plugins/modelopt/skills/evaluation/SKILL.md lines 140-174. Update the comment in plugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml lines 109-113 to require the same approval while keeping remote code disabled by default.plugins/modelopt/skills/ptq/references/unsupported-models.md (1)
13-15: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail rg -n -C 3 --glob '*.py' --glob '*.md' \ 'trust_remote_code\s*=\s*True|--trust_remote_code' \ plugins/modelopt/skillsRepository: NVIDIA/Model-Optimizer
Length of output: 3185
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- unsupported-models.md ---' cat -n plugins/modelopt/skills/ptq/references/unsupported-models.md printf '%s\n' '--- all trust_remote_code references in the skill tree ---' rg -n -C 4 --glob '*.py' --glob '*.md' --glob '*.yaml' --glob '*.yml' \ 'trust_remote_code|hf_ptq\.py' plugins/modelopt/skills printf '%s\n' '--- likely CLI definitions and call sites ---' rg -n -C 5 --glob '*.py' \ 'add_argument\([^)]*trust_remote_code|trust_remote_code|hf_ptq' .Repository: NVIDIA/Model-Optimizer
Length of output: 50379
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- documentation context ---' sed -n '1,55p' plugins/modelopt/skills/ptq/references/unsupported-models.md sed -n '45,65p' plugins/modelopt/skills/ptq/references/launcher-guide.md printf '%s\n' '--- source references outside tests ---' rg -n -C 6 --glob '*.py' --glob '!tests/**' \ 'trust_remote_code|add_argument.*remote|hf_ptq' examples modelopt plugins 2>/dev/null | head -n 500 printf '%s\n' '--- exact files containing the option ---' rg -l --glob '*.py' --glob '!tests/**' 'trust_remote_code|remote_code' . | sortRepository: NVIDIA/Model-Optimizer
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- hf_ptq option definition and propagation ---' rg -n -C 10 --glob 'hf_ptq.py' \ 'trust_remote_code|AutoConfig|AutoTokenizer|AutoModel|from_pretrained' examples/hf_ptq printf '%s\n' '--- helper defaults and propagation ---' rg -n -C 8 --glob '*.py' --glob '!tests/**' \ 'def (get_model|get_tokenizer|load_model)|trust_remote_code\s*:\s*bool|trust_remote_code\s*=' examples/hf_ptq printf '%s\n' '--- relevant tests for default and opt-in behavior ---' rg -n -C 8 --glob 'test_*.py' \ 'trust_remote_code|remote code|DeepSeek' tests/examples/hf_ptq | head -n 350Repository: NVIDIA/Model-Optimizer
Length of output: 50378
🌐 Web query:
Hugging Face Transformers trust_remote_code AutoConfig from_pretrained executes remote modeling code security user opt-in official documentation💡 Result:
In Hugging Face Transformers,
trust_remote_code=Trueis a security-critical parameter used when loading models or configurations via methods likefrom_pretrained()[1][2][3]. Because Hugging Face allows repositories to ship custom Python modeling or configuration files (e.g.,modeling_*.py,configuration_*.py), setting this flag toTrueexplicitly authorizes the library to download and execute that arbitrary code on your local machine [1][4][5]. Official documentation emphasizes that this option should only be enabled for repositories you trust and have manually inspected [2][6][3]. Whentrust_remote_codeis set toFalse(the default), the library is intended to restrict loading to built-in, pre-verified model architectures, preventing the execution of arbitrary remote code [7][8]. Security considerations include: 1. Risk of Remote Code Execution (RCE): Loading models withtrust_remote_code=Truefrom untrusted or compromised repositories can lead to full system compromise [5]. 2. Security Boundaries: Whiletrust_remote_code=Falseis intended as a security barrier, historical vulnerabilities have demonstrated that configuration-based injections or improper handling of local file paths can occasionally bypass this protection [9][7][8]. 3. Recommended Practices: * Always inspect the source code of the modeling/configuration files in the repository before enabling the flag [1][2][3]. * Pin the specific repositoryrevision(commit hash) to prevent the silent loading of updated, potentially malicious code [1][2]. * Usesafetensorsformat for model weights whenever possible, as this format is designed to avoid the risks associated with older, pickle-based serialization [1][4][2]. Users should treattrust_remote_code=Trueas an explicit opt-in to potential arbitrary code execution and apply defense-in-depth measures, such as running in isolated environments (e.g., containers) and monitoring runtime activity [5][7].Citations:
- 1: https://github.com/huggingface/transformers/blob/main/SECURITY.md
- 2: https://huggingface.co/docs/transformers/models
- 3: https://huggingface.co/docs/transformers/en/model_doc/auto.md
- 4: https://github.com/huggingface/transformers/security
- 5: trailofbits/semgrep-rules#83
- 6: https://github.com/huggingface/transformers/blob/08cd694ef07d53f6e08e60ea6e1483dbb156924d/src/transformers/models/auto/configuration_auto.py
- 7: https://pluto.security/blog/unauthenticated-remote-code-execution-in-huggingface-transformers-via-config-injection/
- 8: huggingface/sentence-transformers#3801
- 9: https://nvd.nist.gov/vuln/detail/CVE-2026-5241
Require explicit opt-in for remote code execution.
trust_remote_code=Truecan execute checkpoint-supplied Python, so thealways useinstruction and diagnostic commands must require source review first. Keep the existing CLI and helper defaults atFalse.🤖 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 `@plugins/modelopt/skills/ptq/references/unsupported-models.md` around lines 13 - 15, Update the guidance around custom modeling/tokenization files to require reviewing the checkpoint-supplied source before enabling remote code execution. Replace the unconditional “always use” wording and ensure diagnostic commands explicitly pass trust_remote_code=True only after review, while preserving the existing CLI and helper defaults of False.Source: Coding guidelines
88da754 to
a54e457
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/modelopt/skills/evaluation/SKILL.md (1)
84-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not source the workspace
.env. Replaceset -a && source .env && set +awith a non-executing loader that validates and exports onlyKEY=VALUEentries. The conditionalcppreserves an existing malicious.env, andsourceexecutes command substitutions and other shell code beforenelstarts. Preserve the exports required byhost:...values.🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 84 - 86, Update the `.env` loading instructions in the evaluation setup to remove `source` and any `set -a` execution flow. Use a non-executing loader that validates and exports only KEY=VALUE entries while preserving the environment variables required by `host:` values, and ensure the setup does not trust an existing `.env` created by the conditional copy.Source: Linters/SAST tools
🤖 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 `@plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml`:
- Around line 5-10: Update the example commands in the configuration comments to
reference the repository-root-relative path
plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml,
including both dry-run and submit invocations, without relying on a
working-directory change.
In `@plugins/modelopt/skills/evaluation/SKILL.md`:
- Around line 43-46: Update the evaluation instructions around the nel-next
setup and execution commands to resolve `.agents` resources through the
installed plugin or a documented resolver, rather than directly assuming
`.agents/scripts/nel-next.sh` exists. Continue using the canonical
`plugins/modelopt/skills/evaluation/recipes/env.example` and preserve the
existing setup, evaluation, and MLflow-push flow.
---
Outside diff comments:
In `@plugins/modelopt/skills/evaluation/SKILL.md`:
- Around line 84-86: Update the `.env` loading instructions in the evaluation
setup to remove `source` and any `set -a` execution flow. Use a non-executing
loader that validates and exports only KEY=VALUE entries while preserving the
environment variables required by `host:` values, and ensure the setup does not
trust an existing `.env` created by the conditional copy.
🪄 Autofix
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: a03a2c12-6ce1-40d0-bdb0-b567d01cc89c
📒 Files selected for processing (6)
.agents/README.mdplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yamlplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/swebench_verified.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.mdplugins/modelopt/skills/evaluation/references/nel-next.md
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/modelopt/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.md
- .agents/README.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/modelopt/skills/evaluation/SKILL.md (1)
84-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not source the workspace
.env. Replaceset -a && source .env && set +awith a non-executing loader that validates and exports onlyKEY=VALUEentries. The conditionalcppreserves an existing malicious.env, andsourceexecutes command substitutions and other shell code beforenelstarts. Preserve the exports required byhost:...values.🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 84 - 86, Update the `.env` loading instructions in the evaluation setup to remove `source` and any `set -a` execution flow. Use a non-executing loader that validates and exports only KEY=VALUE entries while preserving the environment variables required by `host:` values, and ensure the setup does not trust an existing `.env` created by the conditional copy.Source: Linters/SAST tools
🤖 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 `@plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml`:
- Around line 5-10: Update the example commands in the configuration comments to
reference the repository-root-relative path
plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml,
including both dry-run and submit invocations, without relying on a
working-directory change.
In `@plugins/modelopt/skills/evaluation/SKILL.md`:
- Around line 43-46: Update the evaluation instructions around the nel-next
setup and execution commands to resolve `.agents` resources through the
installed plugin or a documented resolver, rather than directly assuming
`.agents/scripts/nel-next.sh` exists. Continue using the canonical
`plugins/modelopt/skills/evaluation/recipes/env.example` and preserve the
existing setup, evaluation, and MLflow-push flow.
---
Outside diff comments:
In `@plugins/modelopt/skills/evaluation/SKILL.md`:
- Around line 84-86: Update the `.env` loading instructions in the evaluation
setup to remove `source` and any `set -a` execution flow. Use a non-executing
loader that validates and exports only KEY=VALUE entries while preserving the
environment variables required by `host:` values, and ensure the setup does not
trust an existing `.env` created by the conditional copy.
🪄 Autofix
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: a03a2c12-6ce1-40d0-bdb0-b567d01cc89c
📒 Files selected for processing (6)
.agents/README.mdplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yamlplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/swebench_verified.mdplugins/modelopt/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.mdplugins/modelopt/skills/evaluation/references/nel-next.md
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/modelopt/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.md
- .agents/README.md
🛑 Comments failed to post (2)
plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml (1)
5-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a repository-root-relative configuration path.
When run from the repository root, Line [8] passes
recipes/examples/example_eval_next.yaml, but this file is located atplugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml. The command fails unless.agents/scripts/nel-next.shchanges its working directory. Update the path or add an explicit working-directory step.Proposed path fix
-# .agents/scripts/nel-next.sh eval run recipes/examples/example_eval_next.yaml --dry-run +# .agents/scripts/nel-next.sh eval run plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml --dry-runBased on learnings: Use relative paths from the repository root in commands and file references.
📝 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.# Run via the isolated nel-next venv: # .agents/scripts/nel-next.sh --setup-only # set -a && source .env && set +a # HF_TOKEN, AWS_*, NEL_NEXT_EVAL_IMAGE, HARBOR_*_ECR_REPOSITORY (from modelopttools:eval-config) # .agents/scripts/nel-next.sh eval run plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml --dry-run # ... --submit -O benchmarks.0.max_problems=2 -O benchmarks.0.repeats=1 -O benchmarks.0.max_concurrent=2 # canary # ... --submit # full🤖 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 `@plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml` around lines 5 - 10, Update the example commands in the configuration comments to reference the repository-root-relative path plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml, including both dry-run and submit invocations, without relying on a working-directory change.Source: Learnings
plugins/modelopt/skills/evaluation/SKILL.md (1)
43-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail for path in \ .agents/scripts/nel-next.sh \ .agents/skills/evaluation/recipes/env.example \ plugins/modelopt/skills/evaluation/recipes/env.example do test -e "$path" || { printf 'Missing path: %s\n' "$path" >&2 exit 1 } doneRepository: NVIDIA/Model-Optimizer
Length of output: 160
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- path types and targets ---' for path in \ .agents/scripts/nel-next.sh \ .agents/skills/evaluation \ .agents/skills/evaluation/recipes/env.example \ plugins/modelopt/skills/evaluation \ plugins/modelopt/skills/evaluation/recipes/env.example do if test -L "$path"; then printf '%s -> %s\n' "$path" "$(readlink "$path")" else stat -c '%F %n' "$path" fi done printf '%s\n' '--- install/package references ---' rg -n --hidden -S \ 'plugins/modelopt/skills|\.agents/skills|SKILL\.md|env\.example|nel-next\.sh' \ -g '!node_modules' -g '!dist' -g '!build' \ . | head -n 240 printf '%s\n' '--- relevant skill instructions ---' cat -n plugins/modelopt/skills/evaluation/SKILL.md | sed -n '35,95p'Repository: NVIDIA/Model-Optimizer
Length of output: 234
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- path types and targets ---' for path in \ .agents/scripts/nel-next.sh \ .agents/skills/evaluation \ .agents/skills/evaluation/recipes/env.example \ plugins/modelopt/skills/evaluation \ plugins/modelopt/skills/evaluation/recipes/env.example do if test -L "$path"; then printf 'symlink: %s -> %s\n' "$path" "$(readlink "$path")" elif test -e "$path"; then ls -ld "$path" else printf 'missing: %s\n' "$path" fi done printf '%s\n' '--- install/package references ---' rg -n --hidden -S \ 'plugins/modelopt/skills|\.agents/skills|SKILL\.md|env\.example|nel-next\.sh' \ -g '!node_modules' -g '!dist' -g '!build' \ . | head -n 240 printf '%s\n' '--- relevant skill instructions ---' cat -n plugins/modelopt/skills/evaluation/SKILL.md | sed -n '35,95p'Repository: NVIDIA/Model-Optimizer
Length of output: 22669
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- compatibility-tree targets ---' for path in .agents .agents/skills .agents/scripts .claude .claude/skills; do if test -L "$path"; then printf 'symlink: %s -> %s\n' "$path" "$(readlink "$path")" elif test -e "$path"; then ls -ld "$path" else printf 'missing: %s\n' "$path" fi done printf '%s\n' '--- plugin tree and script locations ---' find plugins/modelopt -maxdepth 4 -type f \( -name 'nel-next.sh' -o -name 'env.example' -o -name 'SKILL.md' \) -print | sort git ls-files .agents/scripts/nel-next.sh plugins/modelopt/skills/evaluation/recipes/env.example plugins/modelopt/skills/evaluation/SKILL.md printf '%s\n' '--- packaging metadata ---' rg -n -C 5 --hidden -S \ 'plugins/modelopt|package-data|include-package-data|plugin|skills' \ pyproject.toml setup.cfg setup.py MANIFEST.in plugins/modelopt .github \ 2>/dev/null | head -n 260 printf '%s\n' '--- repository path contract ---' cat -n AGENTS.md | sed -n '1,35p' cat -n .agents/README.md | sed -n '1,48p'Repository: NVIDIA/Model-Optimizer
Length of output: 23457
Resolve
.agentspaths through the installed plugin.The repository exposes
.agents/skillsthrough a symlink, but.agents/scripts/nel-next.shis outside the canonical plugin tree. Useplugins/modelopt/skills/evaluation/recipes/env.exampleand providenel-next.shthrough the installed plugin or a documented resolver.🧰 Tools
🪛 SkillSpector (2.5.1)
[error] 84: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 392: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 393: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 394: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
🤖 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 `@plugins/modelopt/skills/evaluation/SKILL.md` around lines 43 - 46, Update the evaluation instructions around the nel-next setup and execution commands to resolve `.agents` resources through the installed plugin or a documented resolver, rather than directly assuming `.agents/scripts/nel-next.sh` exists. Continue using the canonical `plugins/modelopt/skills/evaluation/recipes/env.example` and preserve the existing setup, evaluation, and MLflow-push flow.Source: Coding guidelines
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
The goal is to distribute the existing repository-local ModelOpt skills as installable Claude/Codex marketplace plugins while retaining checkout compatibility. The move is cohesive and the compatibility symlink is a sensible direction, but the installed plugin is not currently self-contained: several skills still execute checkout-only .agents/... paths, and two required evaluation helpers remain outside plugins/modelopt, so workflows advertised for “any workspace” fail when invoked from a normal marketplace installation. The validator runs do not exercise this runtime path; please add a smoke test from a temporary non-ModelOpt workspace.
Design review: the PR body explains the desired packaging and single canonical tree, but does not compare this architecture with the existing in-repo symlink/sync system. Obvious alternatives are (1) retain .agents/skills as canonical and expose it through a thin plugin tree, extending tools/precommit/sync_claude_skills.sh; (2) generate a standalone plugin artifact/tree from .agents during validation/release; or (3) distribute the skills from a dedicated marketplace repository/submodule. The Python dependencies in pyproject.toml do not provide relevant plugin packaging machinery, so the main existing alternative is the repository’s current filesystem symlink/sync pattern. Please document why making plugins/modelopt canonical is preferable and, importantly, how runtime asset resolution is intended to work portably in both Claude and Codex.
This also adds Apache-2.0 license declarations to the plugin manifests and redistributes externally-derived skill material under a new plugin boundary. Per licensing policy, that needs human/legal-owner sign-off even after the functional issue is fixed.
| ## AI Agents | ||
|
|
||
| For AI-assisted development setup, see the [agent tooling notes](./.agents/TOOLING.md). | ||
| ModelOpt's agent skills can be installed from this repository and used in any |
There was a problem hiding this comment.
Bot comment.
The plugin is not currently usable from an arbitrary workspace as advertised. Only plugins/modelopt is packaged, but installed skills still execute checkout-relative paths such as .agents/skills/benchmark-model-kernels/scripts/..., .agents/skills/day0-release/scripts/..., and .agents/scripts/{nel-next,gdpval-sif}.sh; the latter two helpers are not under the plugin tree at all. deployment also shows scripts/deploy.sh, which resolves against the user's workspace rather than the installed skill directory. These commands therefore fail unless the current workspace happens to be a Model-Optimizer checkout. Please package every runtime helper and resolve it through a Claude/Codex-portable plugin-root mechanism (or ensure every affected workflow explicitly obtains and enters a checkout before using checkout paths), then add a smoke test that installs/loads the plugin from a temporary non-ModelOpt directory and verifies representative helper paths.
There was a problem hiding this comment.
[agent comment]
Addressed in b79d701. The nel-next and GDPVal helpers now ship under plugins/modelopt/skills/evaluation/scripts/; bundled helper and cross-skill references resolve from $SKILL_DIR rather than the workspace; and a pre-commit validator rejects checkout-relative plugin paths or missing packaged helpers. I also installed the rebased plugin with both Codex and Claude from an unrelated temporary workspace and exercised representative helper entry points successfully.
| ## AI Agents | ||
|
|
||
| For AI-assisted development setup, see the [agent tooling notes](./.agents/TOOLING.md). | ||
| ModelOpt's agent skills can be installed from this repository and used in any |
There was a problem hiding this comment.
Installed marketplace copies only contain plugins/modelopt, but several bundled skills still execute checkout-relative paths such as .agents/skills/..., .agents/scripts/nel-next.sh, and scripts/deploy.sh; some required helpers are outside the plugin tree entirely. From a non-ModelOpt workspace these commands fail, contradicting this claim. Please package the required runtime helpers and resolve them relative to the installed skill/plugin, then add a smoke test from an unrelated working directory.
There was a problem hiding this comment.
[agent comment]
Addressed in b79d701. The nel-next and GDPVal helpers now ship under plugins/modelopt/skills/evaluation/scripts/; bundled helper and cross-skill references resolve from $SKILL_DIR rather than the workspace; and a pre-commit validator rejects checkout-relative plugin paths or missing packaged helpers. I also installed the rebased plugin with both Codex and Claude from an unrelated temporary workspace and exercised representative helper entry points successfully.
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
Focused follow-up only: repository-local use remains functionally equivalent on normal symlink-capable checkouts, aside from the already-reported CI path-filter regression. I found two additional checkout-local regressions caused by making .agents/skills a symlink.
| @@ -0,0 +1 @@ | |||
| ../plugins/modelopt/skills No newline at end of file | |||
There was a problem hiding this comment.
Bot comment.
This is not fully backward-compatible for repository-local users on Git-for-Windows checkouts with core.symlinks=false (a common default): Git materializes this as a one-line regular file, so .agents/skills/<name> discovery and the skill test command no longer work. Previously .agents/skills was a real directory, so this specifically regresses users who work from the checkout without installing the marketplace. Please retain a Windows-safe compatibility mechanism or document/enforce the symlink requirement and cover it in checkout validation.
| # Requires: gh, base64, awk. Run from the repo root. | ||
| # | ||
| # The script overwrites .agents/skills/<skill>/ with upstream contents and | ||
| # The script overwrites plugins/modelopt/skills/<skill>/ through the |
There was a problem hiding this comment.
Bot comment.
Although writes through .agents/skills reach the new canonical tree on a symlink-capable checkout, the script still ends with git diff $DEST_BASE/launching-evals $DEST_BASE/accessing-mlflow. Git pathspec traversal does not follow a tracked directory symlink, so this advertised review command now fails (or shows no canonical changes) after a sync. Please make DEST_BASE and the final review paths plugins/modelopt/skills, using .agents/skills only as the compatibility entry point.
a54e457 to
b79d701
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/precommit/check_modelopt_plugin_paths.py (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine the module public API.
Add
__all__ = ["main"]after the imports.mainis the module public entry point.As per coding guidelines, "
**/*.py: Define each module's public API with__all__ = [...]."🤖 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/precommit/check_modelopt_plugin_paths.py` around lines 18 - 19, Add the module-level __all__ declaration after the imports in check_modelopt_plugin_paths.py, exposing only the existing main entry point and preserving the rest of the module unchanged.Source: Coding guidelines
🤖 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 `@tools/precommit/check_modelopt_plugin_paths.py`:
- Around line 26-30: Extend FORBIDDEN_REFERENCES with a regular-expression entry
that rejects checkout-relative `.claude/skills/` paths, using the same behavior
and diagnostic style as the existing `.agents/skills/` rule while leaving the
canonical plugins/modelopt/skills path allowed.
---
Nitpick comments:
In `@tools/precommit/check_modelopt_plugin_paths.py`:
- Around line 18-19: Add the module-level __all__ declaration after the imports
in check_modelopt_plugin_paths.py, exposing only the existing main entry point
and preserving the rest of the module unchanged.
🪄 Autofix
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: dec12286-38c3-457a-bd1d-16e9e6d21622
📒 Files selected for processing (28)
.pre-commit-config.yamlplugins/modelopt/skills/benchmark-model-kernels/SKILL.mdplugins/modelopt/skills/common/SKILL.mdplugins/modelopt/skills/common/environment-setup.mdplugins/modelopt/skills/common/remote-execution.mdplugins/modelopt/skills/common/remote_exec.shplugins/modelopt/skills/common/slurm-setup.mdplugins/modelopt/skills/compare-results/SKILL.mdplugins/modelopt/skills/day0-release/SKILL.mdplugins/modelopt/skills/day0-release/tests/test_gates.pyplugins/modelopt/skills/deployment/SKILL.mdplugins/modelopt/skills/deployment/tests/evals.jsonplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/evaluation/recipes/env.exampleplugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yamlplugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yamlplugins/modelopt/skills/evaluation/recipes/tasks/aa_gym/gdpval.mdplugins/modelopt/skills/evaluation/references/gym-gdpval.mdplugins/modelopt/skills/evaluation/references/nel-next.mdplugins/modelopt/skills/evaluation/scripts/gdpval-sif.shplugins/modelopt/skills/evaluation/scripts/nel-next.shplugins/modelopt/skills/ptq/SKILL.mdplugins/modelopt/skills/ptq/references/slurm-setup-ptq.mdplugins/modelopt/skills/ptq/references/unsupported-models.mdplugins/modelopt/skills/qad/SKILL.mdplugins/modelopt/skills/quant-recipe-search/SKILL.mdplugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.mdtools/precommit/check_modelopt_plugin_paths.py
🚧 Files skipped from review as they are similar to previous changes (20)
- plugins/modelopt/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml
- plugins/modelopt/skills/common/SKILL.md
- plugins/modelopt/skills/common/environment-setup.md
- plugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.md
- plugins/modelopt/skills/ptq/references/slurm-setup-ptq.md
- plugins/modelopt/skills/evaluation/recipes/examples/example_eval_next.yaml
- plugins/modelopt/skills/common/slurm-setup.md
- plugins/modelopt/skills/evaluation/references/nel-next.md
- plugins/modelopt/skills/qad/SKILL.md
- plugins/modelopt/skills/evaluation/references/gym-gdpval.md
- plugins/modelopt/skills/compare-results/SKILL.md
- plugins/modelopt/skills/common/remote_exec.sh
- plugins/modelopt/skills/evaluation/recipes/env.example
- plugins/modelopt/skills/common/remote-execution.md
- plugins/modelopt/skills/quant-recipe-search/SKILL.md
- plugins/modelopt/skills/deployment/tests/evals.json
- plugins/modelopt/skills/ptq/SKILL.md
- plugins/modelopt/skills/day0-release/SKILL.md
- plugins/modelopt/skills/evaluation/recipes/tasks/aa_gym/gdpval.md
- plugins/modelopt/skills/benchmark-model-kernels/SKILL.md
|
The My main recommendation is to make plugin installation enable the existing ModelOpt MCP as well. The current Codex manifest declares only For predictable releases, I would pin that Git source to a compatible tag or commit rather than implicitly following With that wiring, the expected user experience becomes just marketplace registration plus Separately, the new path validator should also reject legacy |
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
926f955 to
69dc2bd
Compare
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
69dc2bd to
1b0947f
Compare
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
@ChenhanYu This is done! |
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 6
🤖 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/unit_tests.yml:
- Line 18: Update the workflow path filters used by check-file-changes so plugin
manifest files are included in the plugin validation workflow, ensuring
manifest-only pull requests trigger validation instead of skipping the skills
job. Reuse the existing plugin manifest path pattern or add the validator
workflow if no such workflow exists.
- Line 59: Add least-privilege GitHub Actions permissions to the workflow by
defining a workflow-level permissions block granting only contents: read, or
equivalent minimum permissions on every job including skills. Preserve the
existing skills job checkout and execution behavior.
In `@plugins/modelopt/.claude-plugin/plugin.json`:
- Line 25: Pin the shared Model-Optimizer MCP source to the same tested tag or
full commit instead of the mutable main branch in
plugins/modelopt/.claude-plugin/plugin.json:25 and plugins/modelopt/.mcp.json:6;
update both configuration URLs consistently.
In `@plugins/modelopt/skills/ptq/references/unsupported-models.md`:
- Line 9: Update the workspace-management.md reference in the download-first
guidance to resolve from the sibling common skill directory rather than the PTQ
skill's common subdirectory. Use the established common-skill root variable or
an explicit sibling path so agents can locate the guide when $SKILL_DIR points
to the PTQ skill root.
In `@plugins/modelopt/skills/quant-recipe-search/SKILL.md`:
- Around line 56-58: Update the default success rule in the quant-recipe-search
skill to require metric-specific acceptance criteria: define whether each listed
objective is maximized or minimized, its comparison unit, and its allowable
tolerance relative to the matching BF16/FP16 baseline before promoting a
candidate. Remove the universal “1 percentage point” rule and preserve reruns
for near-threshold or noisy results.
- Around line 136-156: Add an explicit default execution budget before the
Iteration Loop, covering maximum candidates plus GPU-hour and wall-time limits,
and define a stop condition when any limit is reached or the goal is met.
Require user confirmation before launching candidates or sweeps that would
exceed the default budget, while preserving the existing promotion criteria in
the iteration loop.
🪄 Autofix
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: 2d6c0194-e937-414d-9e04-9bf015c23a4c
📒 Files selected for processing (22)
.github/workflows/unit_tests.yml.pre-commit-config.yamlplugins/modelopt/.claude-plugin/plugin.jsonplugins/modelopt/.codex-plugin/plugin.jsonplugins/modelopt/.mcp.jsonplugins/modelopt/skills/benchmark-model-kernels/SKILL.mdplugins/modelopt/skills/common/SKILL.mdplugins/modelopt/skills/common/environment-setup.mdplugins/modelopt/skills/common/remote-execution.mdplugins/modelopt/skills/common/remote_exec.shplugins/modelopt/skills/common/slurm-setup.mdplugins/modelopt/skills/compare-results/SKILL.mdplugins/modelopt/skills/day0-release/SKILL.mdplugins/modelopt/skills/deployment/SKILL.mdplugins/modelopt/skills/deployment/tests/evals.jsonplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/ptq/SKILL.mdplugins/modelopt/skills/ptq/references/slurm-setup-ptq.mdplugins/modelopt/skills/ptq/references/unsupported-models.mdplugins/modelopt/skills/qad/SKILL.mdplugins/modelopt/skills/quant-recipe-search/SKILL.mdplugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.md
🚧 Files skipped from review as they are similar to previous changes (15)
- plugins/modelopt/.codex-plugin/plugin.json
- plugins/modelopt/skills/common/slurm-setup.md
- plugins/modelopt/skills/common/remote-execution.md
- plugins/modelopt/skills/qad/SKILL.md
- plugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.md
- plugins/modelopt/skills/common/remote_exec.sh
- plugins/modelopt/skills/day0-release/SKILL.md
- plugins/modelopt/skills/ptq/SKILL.md
- plugins/modelopt/skills/compare-results/SKILL.md
- plugins/modelopt/skills/deployment/tests/evals.json
- plugins/modelopt/skills/common/environment-setup.md
- plugins/modelopt/skills/common/SKILL.md
- .pre-commit-config.yaml
- plugins/modelopt/skills/ptq/references/slurm-setup-ptq.md
- plugins/modelopt/skills/benchmark-model-kernels/SKILL.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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/unit_tests.yml:
- Line 18: Update the workflow path filters used by check-file-changes so plugin
manifest files are included in the plugin validation workflow, ensuring
manifest-only pull requests trigger validation instead of skipping the skills
job. Reuse the existing plugin manifest path pattern or add the validator
workflow if no such workflow exists.
- Line 59: Add least-privilege GitHub Actions permissions to the workflow by
defining a workflow-level permissions block granting only contents: read, or
equivalent minimum permissions on every job including skills. Preserve the
existing skills job checkout and execution behavior.
In `@plugins/modelopt/.claude-plugin/plugin.json`:
- Line 25: Pin the shared Model-Optimizer MCP source to the same tested tag or
full commit instead of the mutable main branch in
plugins/modelopt/.claude-plugin/plugin.json:25 and plugins/modelopt/.mcp.json:6;
update both configuration URLs consistently.
In `@plugins/modelopt/skills/ptq/references/unsupported-models.md`:
- Line 9: Update the workspace-management.md reference in the download-first
guidance to resolve from the sibling common skill directory rather than the PTQ
skill's common subdirectory. Use the established common-skill root variable or
an explicit sibling path so agents can locate the guide when $SKILL_DIR points
to the PTQ skill root.
In `@plugins/modelopt/skills/quant-recipe-search/SKILL.md`:
- Around line 56-58: Update the default success rule in the quant-recipe-search
skill to require metric-specific acceptance criteria: define whether each listed
objective is maximized or minimized, its comparison unit, and its allowable
tolerance relative to the matching BF16/FP16 baseline before promoting a
candidate. Remove the universal “1 percentage point” rule and preserve reruns
for near-threshold or noisy results.
- Around line 136-156: Add an explicit default execution budget before the
Iteration Loop, covering maximum candidates plus GPU-hour and wall-time limits,
and define a stop condition when any limit is reached or the goal is met.
Require user confirmation before launching candidates or sweeps that would
exceed the default budget, while preserving the existing promotion criteria in
the iteration loop.
🪄 Autofix
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: 2d6c0194-e937-414d-9e04-9bf015c23a4c
📒 Files selected for processing (22)
.github/workflows/unit_tests.yml.pre-commit-config.yamlplugins/modelopt/.claude-plugin/plugin.jsonplugins/modelopt/.codex-plugin/plugin.jsonplugins/modelopt/.mcp.jsonplugins/modelopt/skills/benchmark-model-kernels/SKILL.mdplugins/modelopt/skills/common/SKILL.mdplugins/modelopt/skills/common/environment-setup.mdplugins/modelopt/skills/common/remote-execution.mdplugins/modelopt/skills/common/remote_exec.shplugins/modelopt/skills/common/slurm-setup.mdplugins/modelopt/skills/compare-results/SKILL.mdplugins/modelopt/skills/day0-release/SKILL.mdplugins/modelopt/skills/deployment/SKILL.mdplugins/modelopt/skills/deployment/tests/evals.jsonplugins/modelopt/skills/evaluation/SKILL.mdplugins/modelopt/skills/ptq/SKILL.mdplugins/modelopt/skills/ptq/references/slurm-setup-ptq.mdplugins/modelopt/skills/ptq/references/unsupported-models.mdplugins/modelopt/skills/qad/SKILL.mdplugins/modelopt/skills/quant-recipe-search/SKILL.mdplugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.md
🚧 Files skipped from review as they are similar to previous changes (15)
- plugins/modelopt/.codex-plugin/plugin.json
- plugins/modelopt/skills/common/slurm-setup.md
- plugins/modelopt/skills/common/remote-execution.md
- plugins/modelopt/skills/qad/SKILL.md
- plugins/modelopt/skills/quant-recipe-search/references/recipe_iteration.md
- plugins/modelopt/skills/common/remote_exec.sh
- plugins/modelopt/skills/day0-release/SKILL.md
- plugins/modelopt/skills/ptq/SKILL.md
- plugins/modelopt/skills/compare-results/SKILL.md
- plugins/modelopt/skills/deployment/tests/evals.json
- plugins/modelopt/skills/common/environment-setup.md
- plugins/modelopt/skills/common/SKILL.md
- .pre-commit-config.yaml
- plugins/modelopt/skills/ptq/references/slurm-setup-ptq.md
- plugins/modelopt/skills/benchmark-model-kernels/SKILL.md
🛑 Comments failed to post (2)
plugins/modelopt/skills/quant-recipe-search/SKILL.md (2)
56-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the acceptance rule metric-aware.
The skill lists latency and custom metrics at Line 48 through Line 50. The default rule at Line 56 through Line 58 applies a fixed “1 percentage point” tolerance to every benchmark. That unit does not apply to latency, memory, perplexity, or other non-percentage metrics.
Define the comparison direction, unit, and tolerance for each metric before promoting a candidate.
Proposed fix
-Default success rule: maximize the chosen performance objective while keeping -each benchmark within 1 percentage point of the matching BF16/FP16 baseline. +For percentage-based accuracy metrics, keep each benchmark within 1 percentage +point of the matching BF16/FP16 baseline. For other metrics, define a +metric-specific direction, unit, and tolerance before evaluating candidates.📝 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.For percentage-based accuracy metrics, keep each benchmark within 1 percentage point of the matching BF16/FP16 baseline. For other metrics, define a metric-specific direction, unit, and tolerance before evaluating candidates. Near-threshold or noisy regressions require reruns before making a decision.🧰 Tools
🪛 LanguageTool
[style] ~58-~58: ‘making a decision’ might be wordy. Consider a shorter alternative.
Context: ...noisy regressions require reruns before making a decision. ## Search Space Keep the search spac...(EN_WORDINESS_PREMIUM_MAKING_A_DECISION)
🤖 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 `@plugins/modelopt/skills/quant-recipe-search/SKILL.md` around lines 56 - 58, Update the default success rule in the quant-recipe-search skill to require metric-specific acceptance criteria: define whether each listed objective is maximized or minimized, its comparison unit, and its allowable tolerance relative to the matching BF16/FP16 baseline before promoting a candidate. Remove the universal “1 percentage point” rule and preserve reruns for near-threshold or noisy results.
136-156: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound candidate execution before the iteration loop.
Line 138 through Line 153 directs the agent to evaluate every candidate and launch larger sweeps, but the skill defines no maximum candidate count, GPU-hour budget, wall-time budget, or confirmation gate. A broad “find the best” request can therefore trigger unbounded expensive jobs.
Require a default budget and stop condition. Ask for confirmation before exceeding that budget.
Proposed fix
## Iteration Loop +Before launching candidates, define maximum candidates, GPU-hours, and wall time. +Stop when the budget is reached. Ask for confirmation before expanding it. + 1. Run cheap screen evals for every candidate that passes the gates.🤖 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 `@plugins/modelopt/skills/quant-recipe-search/SKILL.md` around lines 136 - 156, Add an explicit default execution budget before the Iteration Loop, covering maximum candidates plus GPU-hour and wall-time limits, and define a stop condition when any limit is reached or the goal is met. Require user confirmation before launching candidates or sweeps that would exceed the default budget, while preserving the existing promotion criteria in the iteration loop.
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
Re-review: the installed-plugin portability concern was substantially addressed in b79d701d46 (helpers now ship under the plugin, most commands use $SKILL_DIR, and canonical skill tests run from plugins/modelopt/skills). The earlier CI skill-path filter comment was also addressed. However, the existing feedback is not fully resolved:
- Critical/security findings remain:
remote_exec.shstill builds commands as strings and executes them witheval; evaluation instructions stillsourcea workspace.env; remote model code is still enabled based onauto_map/custom files without explicit trust approval; andNEMO_EVALUATOR_TRUST_PRE_CMD=1remains enabled by default. - Correctness findings remain in the day-0 gates and deployment helper (malformed summaries can raise or pass incomplete sample accounting, non-positive output size can pass, PID reuse/process-group handling is unsafe, and TRT-LLM is still advertised although start always fails).
- The FP8 dequantization example still silently casts when scale metadata is missing/unsupported, which can corrupt weights.
- The claimed plugin-path validator is not present in
tools/precommit/or.pre-commit-config.yaml, despite the author reply and PR testing text. The upstream sync script also still uses the tracked symlink asDEST_BASEand its finalgit diffpath, so that earlier checkout-local regression remains. - New/latest comments remain open: plugin manifests do not trigger the skills job; workflow-level least-privilege permissions are absent; MCP sources follow mutable
main; and quant-recipe search remains metric-insensitive and unbounded.
Design review: the problem is to distribute repository-local skills as a self-contained Claude/Codex plugin while retaining checkout compatibility. Existing alternatives are (1) keep .agents/skills canonical and extend the existing tools/precommit/sync_claude_skills.sh/symlink mechanism, (2) generate a standalone plugin artifact from .agents during validation/release, or (3) publish from a dedicated marketplace repository/submodule. pyproject.toml has no relevant plugin-packaging dependency; the primary in-repo alternative is the existing sync/symlink tooling. The PR body still does not justify making plugins/modelopt canonical versus those alternatives or document portable cross-skill asset resolution for both hosts, so the required design question remains unaddressed.
Finally, this GitHub PR adds Apache-2.0 manifest declarations and redistributes externally-derived skills. Licensing requires human/legal-owner sign-off and cannot be auto-approved.
Additional comments (outside the PR diff):
plugins/modelopt/skills/common/remote_exec.sh:116— > Bot comment.
This critical prior finding is still unresolved. _ssh_base_opts builds shell source from config values, and callers execute it with eval (including remote_start_session, remote_check_ssh, remote_run, and both rsync helpers). A crafted ssh_key, ssh_proxy, host/user, or local path can therefore execute commands on the local machine. Build SSH/rsync invocations as Bash arrays and call them directly with "${args[@]}"; remove the eval path entirely.
plugins/modelopt/skills/ptq/references/unsupported-models.md:13— > Bot comment.
The prior remote-code finding remains: the presence of custom Python files establishes that remote code may be required, not that it is trusted. Require the user to review and explicitly approve the checkpoint-supplied code (and pin the reviewed revision) before adding --trust_remote_code / trust_remote_code=True; keep the default disabled.
| Run `nel --version`; if missing, instruct `pip install nemo-evaluator-launcher`. If user has an existing config, skip to Step 8 (optionally review for `???` and quantization flags first). | ||
|
|
||
| **Set up `.env` now (not Step 8).** The working `.env` lives at the **workspace root** — the directory you run `nel` from — matching `modelopttools:eval-config`'s convention; do **not** create it under the skill dir. (NEL does not discover `.env` by path: it reads secrets from the shell env via the `host:` prefix after you `source`, so the location is purely *which file you source* before `nel run`. Keeping the single `.env` at the workspace root avoids a stale duplicate under the symlinked, shared `.agents/` skill tree.) For judge-scored / user-sim tasks (HLE, AA-LCR, Tau2), seed it from the template if absent — the template ships under the skill dir, the working `.env` does not: `[ -f .env ] || cp .agents/skills/evaluation/recipes/env.example .env`. Then try `modelopttools:eval-config` (if available) to fill the judge `model_id`/`url` rows (user adds the secret key). Needed before Step 5, which substitutes those values into task `<VAR>` placeholders. | ||
| **Set up `.env` now (not Step 8).** The working `.env` lives at the **workspace root** — the directory you run `nel` from — matching `modelopttools:eval-config`'s convention; do **not** create it under the skill dir. (NEL does not discover `.env` by path: it reads secrets from the shell env via the `host:` prefix after you `source`, so the location is purely *which file you source* before `nel run`. Keeping the single `.env` at the workspace root avoids a stale duplicate under the symlinked, shared `.agents/` skill tree.) For judge-scored / user-sim tasks (HLE, AA-LCR, Tau2), seed it from the template if absent — the template ships under the skill dir, the working `.env` does not: `[ -f .env ] || cp "$SKILL_DIR/recipes/env.example" .env`. Then try `modelopttools:eval-config` (if available) to fill the judge `model_id`/`url` rows (user adds the secret key). Needed before Step 5, which substitutes those values into task `<VAR>` placeholders. |
There was a problem hiding this comment.
Bot comment.
The previous security comment is still unresolved: the workflow later runs set -a && source .env, so an existing workspace .env can execute arbitrary shell code on the submitting host. Do not source this file. Either rely on NEL's supported env-file loading or use a non-executing parser that validates only KEY=VALUE records. The same guidance must be removed from recipes/env.example and references/nel-next.md.
| - "tools/mcp/**" | ||
| - "tools/resource_monitor.py" | ||
| - ".agents/skills/**" | ||
| - "plugins/modelopt/skills/**" |
There was a problem hiding this comment.
Bot comment.
The latest manifest-validation comment is still open. plugins/modelopt/.claude-plugin/**, .codex-plugin/**, .mcp.json, and the marketplace manifests are not included in either the push paths or changed-files input, so a manifest-only change skips the skills/plugin checks. Include these paths and run an actual manifest/plugin validator in the skills job.
| "command": "uvx", | ||
| "args": [ | ||
| "--from", | ||
| "git+https://github.com/NVIDIA/Model-Optimizer.git#subdirectory=tools/mcp", |
There was a problem hiding this comment.
Bot comment.
This existing supply-chain/reproducibility comment is unresolved in both this manifest and .mcp.json: uvx installs from the repository's mutable default branch. Pin the Git source to the same tested release tag or full commit in both configurations.
Edwardf0t1
left a comment
Edwardf0t1
left a comment
There was a problem hiding this comment.
LGTM - will the skills still be loaded automatically under modelopt repo?
cc @Trenton-Starkey we are adding modelopt skills to marketplace.
@Edwardf0t1 Yes |
4 participants
What does this PR do?
Type of change: new feature
Packages the existing ModelOpt agent skills as installable Codex and Claude plugins:
plugins/modelopt/plugin tree and manifests..agents/skillsas a compatibility symlink.commonplaceholder skill required by Codex validation.Usage
Then open
/plugins, select themodeloptmarketplace, and installmodelopt.For Claude Code:
Testing
claude plugin validate . --strictclaude plugin validate plugins/modelopt --strictuv run --frozen --extra dev python -m pytest -q plugins/modelopt/skills/day0-release/tests/test_gates.py plugins/modelopt/skills/benchmark-model-kernels/tests.Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Skills remain available through
.agents/skills; bundled helpers are packaged under the plugin and resolved from$SKILL_DIRso installed workflows do not depend on the current workspace.Summary by CodeRabbit