Add ModelOpt agent plugin marketplace - #2025
Conversation
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (89)
📝 WalkthroughWalkthroughThis PR introduces installable ModelOpt plugins for Claude and Codex, relocates canonical skills under ChangesModelOpt plugin layout
Shared execution foundations
Release gates
Deployment
EAGLE3
Evaluation
Launching and monitoring
PTQ
Recipe search and release automation
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 66.83% 58.01% -8.83%
==========================================
Files 519 519
Lines 58916 58916
==========================================
- Hits 39376 34179 -5197
- Misses 19540 24737 +5197
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.
1 participant
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 --strictmarkdownlint-cli2check-symlinksruff-checkcheck-jsonBefore your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
All pre-existing skill files retain their original contents and remain available through
.agents/skills.Summary by CodeRabbit
New Features
Documentation