feat(agents): add nemo agents package container packaging command - #14
Conversation
|
1812b43 to
bba096d
Compare
- scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com>
* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <mschwab@nvidia.com> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com>
* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <mschwab@nvidia.com> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com> Signed-off-by: Alex Ray <alray@nvidia.com>
bba096d to
a13d1e5
Compare
a13d1e5 to
da18709
Compare
📝 WalkthroughWalkthroughAdds ChangesNeMo Agents Container Packaging
Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
plugins/nemo-agents/README.md (1)
231-492: 🏗️ Heavy liftSplit this section by Diataxis and add “Next Steps”.
This section mixes HOW-TO walkthroughs, REFERENCE tables, and EXPLANATION in one page. Move reference tables to a dedicated reference doc, keep this section task-oriented, and add a
Next Stepscross-link section at the end. As per coding guidelines, "Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead" and "Include 'Next Steps' section at the end with cross-links to related documentation content".🤖 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/nemo-agents/README.md` around lines 231 - 492, This section mixes HOW-TO walkthroughs, reference tables, and explanatory material; split it by Diataxis so this page becomes a task-oriented "How-to: package agents" (keep the progressive pipeline walkthroughs, render/build/publish examples, full example, and brief rendering modes explanation needed for the task) and move all reference material (the entire "Flag reference" tables, "Image tagging convention", "OCI image labels", "Security defaults", and the detailed "Rendering modes" table) into a separate reference doc (e.g., "Packaging reference"); then add a short "Next Steps" section at the end of this how-to that links to the new packaging reference, agent config validation docs, and any template/pyproject docs for further reading.
🤖 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/nemo-agents/agent_container_commands_3964e748.plan.md`:
- Line 212: The Markdown file has a broken relative link target using
"nmp/plugins/..." instead of the correct repository path
"plugins/nemo-agents/..."; update the link in
plugins/nemo-agents/agent_container_commands_3964e748.plan.md to point to the
correct path (replace "nmp/plugins/nemo-agents/..." with
"plugins/nemo-agents/...") so references to the AgentsCLI help panel and package
subcommand resolve correctly.
- Line 251: A few fenced code blocks lack a language specifier and required
surrounding blank lines; add an appropriate language token after the opening ```
for the fence currently written as ``` at Line 251, and ensure there is a blank
line before each opening fence and a blank line after each closing fence for the
code blocks spanning the regions around Lines 444–449 and 452–463 so they
satisfy markdownlint (i.e., replace ``` with ```<language> and insert empty
lines immediately before and after those fenced blocks).
In `@plugins/nemo-agents/nat_agent_build_requirements.md`:
- Line 318: Update the installation sample to match the implemented extras set:
change the pip install requirement that currently uses
"nvidia-nat[all]==${NAT_VERSION}" to use "nvidia-nat[most]==${NAT_VERSION}" so
it aligns with the template and build flow; locate the string
"nvidia-nat[all]==${NAT_VERSION}" in nat_agent_build_requirements.md and replace
the extras token "[all]" with "[most]".
- Around line 12-17: Update this spec to reflect the actual single-command
workflow: replace all references to the deprecated lifecycle commands 'nemo
agents render', 'nemo agents build', and 'nemo agents publish' with the shipped
'nemo agents package' command and adjust descriptions to explain that 'nemo
agents package' validates, renders (if needed), builds, and optionally
publishes; remove or consolidate duplicated sections that describe the old
multi-command flow (notably the blocks around the shown table and the other
occurrences you flagged) so the document only documents 'nemo agents package' as
the primary lifecycle command.
- Line 107: Several fenced code blocks in nat_agent_build_requirements.md are
missing language tags (violating MD040); edit each triple-backtick block and add
the appropriate language identifier (e.g., bash, text, json) after the opening
``` so the markdown linter recognizes the language—update every occurrence of
bare ``` in the file (the blocks called out in the review) to use a specific
language tag.
In `@plugins/nemo-agents/README.md`:
- Around line 356-357: The README table lists incorrect default values for
`--nat-version` and `--output`; update the table to reflect actual runtime
behavior implemented in _warn_if_nat_version_unpinned and _package_render_only:
state that `--nat-version` falls back to an internal default when $NAT_VERSION
is not set and emits a warning (rather than simply being `$NAT_VERSION`), and
that `--output` defaults to the pyproject parent directory in project mode (not
always `<config-dir>/Dockerfile`), while documenting the render-only/--no-build
behavior as implemented in _package_render_only.
- Around line 267-270: Add missing fenced-code block language specifiers (e.g.,
```text or ```bash) and ensure each fenced block has a blank line before and
after to satisfy MD040/MD031; specifically update the blocks containing the
lines "Dockerfile written to examples/Dockerfile" and ".dockerignore written to
examples/.dockerignore" and the other blocks referenced at ranges 282-286,
299-307, and 403-405 in the README so every ``` fence includes a language token
and is separated by surrounding blank lines.
In `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 275-279: The CLI's platform Option (variable platform) is declared
but not actually used, allowing the publish path to report success without any
push; update the command handler that performs the build/publish (the function
using the platform variable around the publish logic) to either pass platform
into the buildx/publish routine or short-circuit with an explicit error when
platform is provided but buildx wiring isn't implemented; ensure the guard is
applied in both places noted (the platform declaration usage and the duplicate
spots around lines 400-404) so the CLI returns a non-zero error if platform is
requested but no push/publish occurs.
- Around line 438-444: The CLI currently validates --format and --agent-whl but
always proceeds down the Docker build path; modify the build command handler
around the validation (where the variables format and agent_whl are checked) to
branch on format == "whl": when true, invoke the wheel build flow using
agent_whl (e.g., call or create a function like build_whl_package(agent_whl) or
reuse existing wheel packaging helpers) and skip any Docker-only steps; ensure
the whl branch uses agent_whl as the source path, exits with errors on
missing/invalid paths, and only runs the Docker build logic when format ==
"docker".
In `@plugins/nemo-agents/src/nemo_agents_plugin/container/builder.py`:
- Around line 155-167: The current flow can remove a user's existing
.dockerignore because render_dockerignore always returns a path that is unlinked
in the finally block; change the logic so you only unlink the .dockerignore if
we created it. Before calling render_dockerignore (in the block guarded by
generate_ignore) record whether a .dockerignore already existed (e.g. check
Path(context_dir / ".dockerignore").exists()) or change render_dockerignore to
return a (path, created_flag); then only call
ignore_file.unlink(missing_ok=True) in the finally when the created_flag is True
(or when the pre-check showed the file did not previously exist). Keep
tmp_dockerfile cleanup unchanged.
In `@plugins/nemo-agents/src/nemo_agents_plugin/container/template.py`:
- Around line 266-272: The current code in template.py silently falls back to a
non-build-context path when agent_config is outside the project root (in the
has_pyproject branch); change the except ValueError handler to fail fast by
raising a clear error (e.g., raise ValueError or RuntimeError) that includes
agent_config and pyproject paths rather than assigning Path(agent_config.name)
to relative_config, so that config_file_path is not set to a bogus
"/workspace/..." value; update the try/except around
agent_config.resolve().relative_to(pyproject.resolve().parent) accordingly
(references: has_pyproject, pyproject, agent_config, relative_config,
config_file_path).
In `@plugins/nemo-agents/src/nemo_agents_plugin/container/validator.py`:
- Around line 70-74: The validator currently lets configs with a workflow but no
"_type" pass; update the check in validator.py to treat a missing or empty
workflow["_type"] as an error: read wf_type = workflow.get("_type", "") and if
not wf_type append an error like "Missing required workflow._type. Expected one
of: ..." (use _KNOWN_WORKFLOW_TYPES to list allowed values), otherwise keep the
existing unknown-type check that appends to errors when wf_type not in
_KNOWN_WORKFLOW_TYPES.
- Around line 47-50: The current try/except in validate_agent_config silently
treats missing PyYAML as valid; change the ImportError handling in
validate_agent_config to return a ValidationResult with valid=False and an
explanatory error (e.g., "PyYAML not installed: <error>") instead of
ValidationResult(valid=True,...), so callers know YAML parsing/validation
couldn't run; keep using the ValidationResult class and include the ImportError
message in the errors list to aid debugging.
In `@plugins/nemo-agents/tests/unit/test_container.py`:
- Line 1109: The test method's parameter type hint is written as a string
("tuple[Path, Path]"); remove the quotes so the annotation is a concrete type
tuple[Path, Path] (e.g., change the signature that contains package_cli,
project_dir: "tuple[Path, Path]" to use package_cli, project_dir: tuple[Path,
Path]) to comply with concrete type-hint guidelines and ensure tuple and Path
are used directly.
---
Nitpick comments:
In `@plugins/nemo-agents/README.md`:
- Around line 231-492: This section mixes HOW-TO walkthroughs, reference tables,
and explanatory material; split it by Diataxis so this page becomes a
task-oriented "How-to: package agents" (keep the progressive pipeline
walkthroughs, render/build/publish examples, full example, and brief rendering
modes explanation needed for the task) and move all reference material (the
entire "Flag reference" tables, "Image tagging convention", "OCI image labels",
"Security defaults", and the detailed "Rendering modes" table) into a separate
reference doc (e.g., "Packaging reference"); then add a short "Next Steps"
section at the end of this how-to that links to the new packaging reference,
agent config validation docs, and any template/pyproject docs for further
reading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00451055-1213-4707-8e75-278347f6f255
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
plugins/nemo-agents/README.mdplugins/nemo-agents/agent_container_commands_3964e748.plan.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/nat_agent_build_requirements.mdplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/openshell_policy.yamlplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py
|
Actionable comments posted: 0 |
da18709 to
a49559b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/nemo-agents/src/nemo_agents_plugin/cli.py (2)
374-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--format whlis accepted but still runs the Docker build path.After validation, execution always calls
build_agent_image(...)and does not branch to a wheel flow, so--format whlbehavior is incorrect.Also applies to: 438-444
🤖 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/nemo-agents/src/nemo_agents_plugin/cli.py` around lines 374 - 390, The CLI always calls build_agent_image(...) regardless of the --format value; change the post-validation branch to check the format flag (e.g., format or build_format variable) and route to the correct flow: if format == "whl" invoke the wheel packaging flow (call the existing wheel packaging function or add a new package_agent_wheel/build_agent_whl function that accepts the same relevant args such as agent, pyproject, tag/agent_version, python_version, template_path, generate_ignore, etc.), otherwise call build_agent_image(...). Apply the same fix to the other occurrence that mirrors this call so --format whl actually triggers the wheel flow.
275-279:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
--platformis still a no-op and can report false publish success.
platformis never wired into the build call, and the multi-platform branch exits with a success message without performing an actual push in this path.Also applies to: 400-404
🤖 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/nemo-agents/src/nemo_agents_plugin/cli.py` around lines 275 - 279, The --platform Option defined as platform: Optional[list[str]] (typer Option) is never forwarded into the image build/publish flow and the multi-platform branch returns success without performing the push; update the publish/build flow to pass the platform list into the build invocation (e.g., call build_image/build or whatever function constructs the container with a platforms/platform argument) and ensure the multi-platform branch actually performs the push/publish (invoke the same push logic or docker buildx/push call) before emitting success; adjust any conditional that prints success to run only after push completes and handle empty/None platform by using the default linux/amd64 + linux/arm64 behavior.
🧹 Nitpick comments (2)
plugins/nemo-agents/README.md (2)
255-397: ⚡ Quick winAdd Python SDK alternatives alongside CLI examples via tab sets.
Packaging examples are CLI-only. Add parallel Python SDK examples in tabs for the same tasks (render-only, build, build+publish).
As per coding guidelines, "Provide both Python SDK and CLI examples in tab-sets for consistency and to support multiple user workflows."
🤖 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/nemo-agents/README.md` around lines 255 - 397, Update the "package — render, build, and publish in one command" section to present CLI and Python SDK examples side-by-side using tab-sets: keep the existing CLI code blocks for the three scenarios (render-only, build, build+publish) and add corresponding Python SDK snippets that demonstrate the SDK call(s) to render/write Dockerfile, build an image with a tag, and build+publish to a registry (match the same flags: --no-build, --tag, --publish/--registry equivalents); mirror the "**With an existing Dockerfile**" and "**Project mode**" examples with SDK alternatives as well, and ensure the "Full example — inspect, build, publish" sequence also has parallel CLI and Python tabs so users can choose either workflow.
231-490: 🏗️ Heavy liftSplit packaging docs into one Diataxis quadrant per page.
This section mixes HOW-TO walkthroughs with REFERENCE tables in one page segment. Move flag tables/spec details to a separate reference page and keep this section task-focused, cross-linking between pages.
As per coding guidelines, "Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead."
🤖 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/nemo-agents/README.md` around lines 231 - 490, This page mixes HOW-TO/tutorial content with reference/spec tables (Diataxis rule); split the current "Packaging command — containerize agents as Docker images" into two pages: a task-focused HOW-TO page that keeps the progressive pipeline, render/build/publish examples, full example, and project mode usage (sections containing step-by-step commands and walkthroughs), and a separate reference page that contains the Flag reference, Build options, Hardening overrides, OCI labels, Image tagging convention, Agent config validation, Security defaults, and Rendering modes tables; update cross-links from the HOW-TO to the new reference page and adjust the documentation index/TOC accordingly, ensuring headings like "Packaging command", "Flag reference", "Image tagging convention", "OCI image labels", "Agent config validation", and "Rendering modes" are moved intact to the reference file so callers (readers) can find exact flag names and label keys.
🤖 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/nemo-agents/tests/unit/test_container.py`:
- Around line 507-513: Replace realistic token prefixes in the test string
literals with non-signature placeholders to avoid secret scanning: change keys
like "https://user:glpat-xxx@gitlab-master.nvidia.com/org/repo.git",
"https://glpat-AAAA@gitlab-master.nvidia.com/org/repo.git", and
"https://oauth2:ghp_xxx@github.com/org/repo.git" to use neutral placeholders
such as "https://USER_X:TOKEN_X@gitlab-master.nvidia.com/org/repo.git",
"https://USER_X:TOKEN_X@gitlab-master.nvidia.com/org/repo.git", and
"https://USER_X:TOKEN_X@github.com/org/repo.git" (and make equivalent changes
for the similar entries referenced at the subsequent 519-523 region) so the
tests keep coverage but no realistic token prefixes remain.
---
Duplicate comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 374-390: The CLI always calls build_agent_image(...) regardless of
the --format value; change the post-validation branch to check the format flag
(e.g., format or build_format variable) and route to the correct flow: if format
== "whl" invoke the wheel packaging flow (call the existing wheel packaging
function or add a new package_agent_wheel/build_agent_whl function that accepts
the same relevant args such as agent, pyproject, tag/agent_version,
python_version, template_path, generate_ignore, etc.), otherwise call
build_agent_image(...). Apply the same fix to the other occurrence that mirrors
this call so --format whl actually triggers the wheel flow.
- Around line 275-279: The --platform Option defined as platform:
Optional[list[str]] (typer Option) is never forwarded into the image
build/publish flow and the multi-platform branch returns success without
performing the push; update the publish/build flow to pass the platform list
into the build invocation (e.g., call build_image/build or whatever function
constructs the container with a platforms/platform argument) and ensure the
multi-platform branch actually performs the push/publish (invoke the same push
logic or docker buildx/push call) before emitting success; adjust any
conditional that prints success to run only after push completes and handle
empty/None platform by using the default linux/amd64 + linux/arm64 behavior.
---
Nitpick comments:
In `@plugins/nemo-agents/README.md`:
- Around line 255-397: Update the "package — render, build, and publish in one
command" section to present CLI and Python SDK examples side-by-side using
tab-sets: keep the existing CLI code blocks for the three scenarios
(render-only, build, build+publish) and add corresponding Python SDK snippets
that demonstrate the SDK call(s) to render/write Dockerfile, build an image with
a tag, and build+publish to a registry (match the same flags: --no-build, --tag,
--publish/--registry equivalents); mirror the "**With an existing Dockerfile**"
and "**Project mode**" examples with SDK alternatives as well, and ensure the
"Full example — inspect, build, publish" sequence also has parallel CLI and
Python tabs so users can choose either workflow.
- Around line 231-490: This page mixes HOW-TO/tutorial content with
reference/spec tables (Diataxis rule); split the current "Packaging command —
containerize agents as Docker images" into two pages: a task-focused HOW-TO page
that keeps the progressive pipeline, render/build/publish examples, full
example, and project mode usage (sections containing step-by-step commands and
walkthroughs), and a separate reference page that contains the Flag reference,
Build options, Hardening overrides, OCI labels, Image tagging convention, Agent
config validation, Security defaults, and Rendering modes tables; update
cross-links from the HOW-TO to the new reference page and adjust the
documentation index/TOC accordingly, ensuring headings like "Packaging command",
"Flag reference", "Image tagging convention", "OCI image labels", "Agent config
validation", and "Rendering modes" are moved intact to the reference file so
callers (readers) can find exact flag names and label keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 83ab4e0e-e2af-4d88-88be-30943d05a5ad
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
plugins/nemo-agents/README.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py
✅ Files skipped from review due to trivial changes (2)
- plugins/nemo-agents/examples/hello_world.yaml
- plugins/nemo-agents/examples/.dockerignore
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/nemo-agents/pyproject.toml
- plugins/nemo-agents/examples/Dockerfile
- plugins/nemo-agents/src/nemo_agents_plugin/container/template.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/validator.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/builder.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/publisher.py
a49559b to
e09cfb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugins/nemo-agents/tests/unit/test_container.py (2)
1112-1113: ⚡ Quick winUse concrete type hint instead of string annotation.
Replace the string annotation with
tuple[Path, Path].Proposed fix
- def test_no_build_project_mode_writes_dockerfile_next_to_pyproject( - self, package_cli, project_dir: "tuple[Path, Path]" - ) -> None: + def test_no_build_project_mode_writes_dockerfile_next_to_pyproject( + self, package_cli, project_dir: tuple[Path, Path] + ) -> None:As per coding guidelines, "Always prefer concrete type hints over string-based ones."
🤖 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/nemo-agents/tests/unit/test_container.py` around lines 1112 - 1113, The annotation for the parameter currently uses a string "tuple[Path, Path]"; replace this string literal with the concrete type tuple[Path, Path] on the method signature (the parameters package_cli, project_dir) so the signature reads package_cli, project_dir: tuple[Path, Path] -> None; ensure Path is imported from pathlib if not already present.
1047-1049: ⚡ Quick win
test_allow_root_e2eassertion is effectively non-verifying.The test reads
Dockerfile.generatedafterbuild_agent_image()cleanup, socontentis usually empty and the assertion can pass without validatingallow_root.Proposed fix
`@patch`("nemo_agents_plugin.container.builder.docker_build") def test_allow_root_e2e(self, mock_build: MagicMock, agent_config: Path) -> None: from nemo_agents_plugin.container.builder import build_agent_image - mock_build.return_value = "root-agent:latest" + captured: dict[str, str] = {} + + def _inspect_dockerfile(**kwargs): + dockerfile = kwargs["dockerfile"] + captured["dockerfile_text"] = dockerfile.read_text() + return "root-agent:latest" + + mock_build.side_effect = _inspect_dockerfile build_agent_image(agent_config, nat_version="1.0.0", allow_root=True) - generated = agent_config.parent / "Dockerfile.generated" - content = generated.read_text() if generated.exists() else "" - assert "USER agent" not in content + assert "USER agent" not in captured["dockerfile_text"]🤖 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/nemo-agents/tests/unit/test_container.py` around lines 1047 - 1049, The test test_allow_root_e2e is reading Dockerfile.generated after build_agent_image() has already cleaned up, so content is often empty and the "USER agent" assertion is vacuous; modify the test to capture the generated Dockerfile before cleanup (either by calling build_agent_image with cleanup disabled or by reading generated.read_text() immediately after build_agent_image returns and before any cleanup), then assert that "USER agent" is absent/present according to allow_root, referencing the test function test_allow_root_e2e and the generated variable/Dockerfile.generated file to locate where to change the timing of the read.plugins/nemo-agents/README.md (1)
231-373: 🏗️ Heavy liftSplit packaging docs by Diataxis quadrant.
This section mixes HOW-TO flow (step-by-step invocations) and REFERENCE tables (flag matrix/defaults) in one page block. Split into separate pages/sections and cross-link.
As per coding guidelines, "Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead."
🤖 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/nemo-agents/README.md` around lines 231 - 373, The Packaging command docs currently mix HOW-TO walkthroughs and REFERENCE tables; split the content under "Packaging command — containerize agents as Docker images" into two (or more) pages/sections following Diataxis: move the step-by-step examples and progressive pipeline invocations (the examples under "Progressive pipeline" and the bash examples) into a HOW-TO / tutorial page (e.g., "Packaging: How-to" or "Package an agent"), and put all flag matrices (the tables under "Flag reference" and the individual flag groups like Pipeline control, Source inputs, Build options, Hardening overrides, OCI labels) into a separate REFERENCE page (e.g., "Packaging: Reference") and add clear cross-links between them; update the README headings and TOC accordingly and ensure sections like the examples, "Progressive pipeline", and flags reference are relocated and linked rather than duplicated.
🤖 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/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 570-584: Wrap the filesystem operations in the render-only path
(the call to output.write_text(content, encoding="utf-8") and the
render_dockerignore(output.parent) handling) with OSError handling: catch
OSError around both the write_text call and the subsequent render_dockerignore()
invocation, call typer.echo or processLogger to emit a concise, user-facing
error message that includes the exception message (and which file failed), and
exit the CLI cleanly (e.g., raise typer.Exit or sys.exit). Ensure you reference
output.write_text, render_dockerignore, and ignore_path so the error handling
surrounds those calls and preserves the existing messages when operations
succeed.
In `@plugins/nemo-agents/src/nemo_agents_plugin/container/template.py`:
- Around line 398-400: When reading the template file (the branch that sets
template_source from template_path using Path.read_text), catch OSError around
the Path(template_path).read_text(...) call and raise a user-facing ValueError
instead (preserving a short, clear message and include the original error text
if helpful). Update the code that assigns template_source to wrap
Path.read_text(encoding="utf-8") in a try/except OSError -> raise
ValueError(...) so callers only see the documented ValueError style rather than
raw OSError from the filesystem.
---
Nitpick comments:
In `@plugins/nemo-agents/README.md`:
- Around line 231-373: The Packaging command docs currently mix HOW-TO
walkthroughs and REFERENCE tables; split the content under "Packaging command —
containerize agents as Docker images" into two (or more) pages/sections
following Diataxis: move the step-by-step examples and progressive pipeline
invocations (the examples under "Progressive pipeline" and the bash examples)
into a HOW-TO / tutorial page (e.g., "Packaging: How-to" or "Package an agent"),
and put all flag matrices (the tables under "Flag reference" and the individual
flag groups like Pipeline control, Source inputs, Build options, Hardening
overrides, OCI labels) into a separate REFERENCE page (e.g., "Packaging:
Reference") and add clear cross-links between them; update the README headings
and TOC accordingly and ensure sections like the examples, "Progressive
pipeline", and flags reference are relocated and linked rather than duplicated.
In `@plugins/nemo-agents/tests/unit/test_container.py`:
- Around line 1112-1113: The annotation for the parameter currently uses a
string "tuple[Path, Path]"; replace this string literal with the concrete type
tuple[Path, Path] on the method signature (the parameters package_cli,
project_dir) so the signature reads package_cli, project_dir: tuple[Path, Path]
-> None; ensure Path is imported from pathlib if not already present.
- Around line 1047-1049: The test test_allow_root_e2e is reading
Dockerfile.generated after build_agent_image() has already cleaned up, so
content is often empty and the "USER agent" assertion is vacuous; modify the
test to capture the generated Dockerfile before cleanup (either by calling
build_agent_image with cleanup disabled or by reading generated.read_text()
immediately after build_agent_image returns and before any cleanup), then assert
that "USER agent" is absent/present according to allow_root, referencing the
test function test_allow_root_e2e and the generated
variable/Dockerfile.generated file to locate where to change the timing of the
read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d0c1e2a-7af2-46db-8cf6-a4fb2976f44e
📒 Files selected for processing (12)
plugins/nemo-agents/README.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py
✅ Files skipped from review due to trivial changes (1)
- plugins/nemo-agents/examples/.dockerignore
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/nemo-agents/src/nemo_agents_plugin/container/publisher.py
- plugins/nemo-agents/examples/hello_world.yaml
- plugins/nemo-agents/pyproject.toml
- plugins/nemo-agents/examples/Dockerfile
- plugins/nemo-agents/src/nemo_agents_plugin/container/validator.py
e09cfb9 to
91ab8d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/nemo-agents/README.md (1)
267-267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifiers and blank lines to fenced code blocks.
Lines 267, 282, 299, 402: Output blocks need language identifier (use
textorconsole) and blank lines before/after to satisfy MD040/MD031.📝 Example fix for line 267
-``` +```text Dockerfile written to examples/Dockerfile .dockerignore written to examples/.dockerignore
</details> Also applies to: 282-282, 299-299, 402-402 <details> <summary>🤖 Prompt for AI Agents</summary>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/nemo-agents/README.mdat line 267, The README contains several
fenced code blocks that lack language specifiers and surrounding blank lines
(examples showing output like "Dockerfile written to examples/Dockerfile",
".dockerignore written to examples/.dockerignore", etc. at the sections
referenced); update each affected fenced block to include a language identifier
such astext orconsole and ensure there is a blank line before the
openingand a blank line after the closingso the blocks satisfy
MD040/MD031 (apply this change to the blocks shown around the output lines at
the four reported locations).</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>plugins/nemo-agents/README.md (3)</summary><blockquote> `491-491`: _⚡ Quick win_ **Add "Next Steps" section with cross-links.** The packaging section ends abruptly without guiding readers to related content (e.g., deployment, evaluation). Per coding guidelines, include a "Next Steps" section with cross-links. As per coding guidelines: "Include 'Next Steps' section at the end with cross-links to related documentation content" <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/nemo-agents/README.md` at line 491, The Packaging section currently ends without guidance; add a "Next Steps" section at the end of README.md that contains cross-links to related documentation (e.g., Deployment, Evaluation, Troubleshooting, Contributing) so readers can follow up; create a short paragraph under the "Next Steps" heading and add internal markdown links pointing to the appropriate headings or docs (Deployment, Evaluation, Troubleshooting, Contributing) to satisfy the coding guideline requiring a Next Steps section. ``` </details> --- `328-372`: _⚡ Quick win_ **Use dropdowns for lengthy flag reference tables.** The flag reference spans 5 tables and 45 lines. Per coding guidelines, collapse optional/reference content into dropdowns to improve scannability. As per coding guidelines: "Use dropdowns for optional/advanced content, advanced sections, troubleshooting details, long examples, and content most users will skip" <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/nemo-agents/README.md` around lines 328 - 372, The "Flag reference" section currently displays five long tables; collapse these into a Markdown collapsible (using <details><summary>...</summary>...</details>) so the README shows a short visible label like "Flag reference (expand for full tables)" and the full tables remain inside the collapsed block; apply this to the entire "Flag reference" block including the subsections "Pipeline control", "Source inputs", "Build options", "Hardening overrides", and "OCI labels", keeping the exact tables and flag names (e.g., --no-build, --publish, --agent, --pyproject, --tag, --platform, --allow-root, --agent-version) intact inside the dropdown and ensure the summary text clearly indicates the content is optional/advanced. ``` </details> --- `259-307`: _⚡ Quick win_ **Use tab sets for alternative command invocations.** Lines 259-307 show parallel alternatives (render-only, build, publish). Per coding guidelines, use tab sets for parallel alternatives to support scannable progressive disclosure. As per coding guidelines: "Use tab sets for parallel alternatives or variants (platform/language variants, before/after comparisons, consecutive code blocks showing alternatives)" <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/nemo-agents/README.md` around lines 259 - 307, The README currently shows three parallel alternative command invocations (the render-only example, the Build (default) example, and the Full pipeline example) as separate code blocks; replace these parallel alternatives with a tab-set UI so readers can switch between "Render-only", "Build", and "Build & Publish" tabs — convert the three code blocks and their respective output blocks into tab panes (keeping the exact commands and outputs intact) and ensure the tab labels match the intent (“Render-only”, “Build (default)”, “Build & Publish”) to follow the tab-set guideline and improve progressive disclosure. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/nemo-agents/src/nemo_agents_plugin/container/validator.py:
- Around line 86-97: The validator currently allows malformed types to pass:
update the checks around workflow.tool_names, tool_names, workflow.llm_name and
llms so that incorrect types are rejected with errors before performing
membership checks; specifically, if workflow.get("tool_names") is present but
not a list append a descriptive error mentioning workflow.tool_names, otherwise
iterate the list and keep the existing available_tools membership check; if
workflow.get("llm_name") is present but not a string append an error mentioning
workflow.llm_name, otherwise check that llm_name exists in llms; and if
data.get("llms") is present but not a dict append an error mentioning llms
instead of silently skipping the check. Ensure you modify the blocks using the
tool_names, llm_name and llms variables to perform these type validations prior
to the existing lookups.- Around line 49-53: The code currently calls agent_config.read_text(...)
outside the YAML parse try/except so file I/O or decoding errors escape; wrap
the read_text call (or expand the existing try block) to catch
FileNotFoundError, OSError, UnicodeDecodeError (or a broad Exception if
preferred) and return a ValidationResult(valid=False, errors=[f"Config read
error: {exc}"]) instead of letting the exception propagate; ensure the change
references agent_config.read_text, yaml.safe_load, the raw/data variables and
returns ValidationResult in the same style as the existing YAML parse error
handling.
Duplicate comments:
In@plugins/nemo-agents/README.md:
- Line 267: The README contains several fenced code blocks that lack language
specifiers and surrounding blank lines (examples showing output like "Dockerfile
written to examples/Dockerfile", ".dockerignore written to
examples/.dockerignore", etc. at the sections referenced); update each affected
fenced block to include a language identifier such astext orconsole and
ensure there is a blank line before the openingand a blank line after the closingso the blocks satisfy MD040/MD031 (apply this change to the blocks
shown around the output lines at the four reported locations).
Nitpick comments:
In@plugins/nemo-agents/README.md:
- Line 491: The Packaging section currently ends without guidance; add a "Next
Steps" section at the end of README.md that contains cross-links to related
documentation (e.g., Deployment, Evaluation, Troubleshooting, Contributing) so
readers can follow up; create a short paragraph under the "Next Steps" heading
and add internal markdown links pointing to the appropriate headings or docs
(Deployment, Evaluation, Troubleshooting, Contributing) to satisfy the coding
guideline requiring a Next Steps section.- Around line 328-372: The "Flag reference" section currently displays five long
tables; collapse these into a Markdown collapsible (using) so the README shows a short visible label like "Flag reference (expand for full tables)" and the full tables remain inside the collapsed block; apply this to the entire "Flag reference" block including the subsections "Pipeline control", "Source inputs", "Build options", "Hardening overrides", and "OCI labels", keeping the exact tables and flag names (e.g., --no-build, --publish, --agent, --pyproject, --tag, --platform, --allow-root, --agent-version) intact inside the dropdown and ensure the summary text clearly indicates the content is optional/advanced. - Around line 259-307: The README currently shows three parallel alternative command invocations (the render-only example, the Build (default) example, and the Full pipeline example) as separate code blocks; replace these parallel alternatives with a tab-set UI so readers can switch between "Render-only", "Build", and "Build & Publish" tabs — convert the three code blocks and their respective output blocks into tab panes (keeping the exact commands and outputs intact) and ensure the tab labels match the intent (“Render-only”, “Build (default)”, “Build & Publish”) to follow the tab-set guideline and improve progressive disclosure. ````...
...🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID:
bdab7403-c5ae-4db3-a36f-6d508b22714c⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock📒 Files selected for processing (12)
plugins/nemo-agents/README.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py✅ Files skipped from review due to trivial changes (1)
- plugins/nemo-agents/examples/hello_world.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/nemo-agents/examples/.dockerignore
- plugins/nemo-agents/pyproject.toml
- plugins/nemo-agents/src/nemo_agents_plugin/container/publisher.py
- plugins/nemo-agents/examples/Dockerfile
- plugins/nemo-agents/src/nemo_agents_plugin/container/template.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/builder.py
- plugins/nemo-agents/src/nemo_agents_plugin/cli.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/metadata.py
- plugins/nemo-agents/tests/unit/test_container.py
ac7b1cf to
4092499
Compare
… harden validator, polish docs/examples Bundles the P0 + P1 review feedback into one follow-up commit. Validator (`container/validator.py` + `container/builder.py`): - Add `warnings: list[str]` field to `ValidationResult` and surface it from the builder's pre-build validate pass via `typer.echo(..., err=True)`. - Demote unknown `workflow._type` from a hard error to a soft warning. NAT's plugin system can register additional workflow types at runtime, and a closed allowlist here was effectively a denylist that forced operators to `--skip-validation` on otherwise-valid configs. Missing `_type` remains a hard error (truly unbuildable). (benmccown) - Wrap `agent_config.read_text(...)` in `try/except (OSError, UnicodeDecodeError)` so missing / unreadable / non-UTF-8 configs surface as a structured `ValidationResult` instead of leaking a raw traceback to the operator. (CodeRabbit `validator.py:49`) Metadata (`container/metadata.py`): - Narrow `_load_pyproject` exception handler from bare `Exception` to `tomllib.TOMLDecodeError`. Parse failures still fall back to an empty dict so OCI labels can be filled from CLI flags; real bugs (OSError, UnicodeDecodeError) now propagate instead of being silently swallowed. (mckornfield `metadata.py:94`) CLI (`cli.py`): - ASCII-ify the `package` docstring: replace `→` / `•` / `—` with `->` / `-` / `--` so the help text renders correctly on Windows consoles using cp1252. (mckornfield `cli.py:325`) Docs (`README.md`): - Rewrite the `--nat-version` flag row to mention the env-var → baked-in fallback chain and the warning the CLI prints when neither is set, matching actual behavior. (CodeRabbit `README.md:356`) - Tighten the `--platform` row to mention the actionable `docker buildx imagetools create` workaround for multi-arch. Examples (`examples/Dockerfile`, `examples/.dockerignore`): - Regenerate against the current tree so the OCI source label reads `git@github.com:NVIDIA-NeMo/nemo-platform.git` instead of the stale `ssh://git@gitlab-master.nvidia.com:12051/aire/microservices/nmp.git` carried over from the original NMP-tree checkin. (mckornfield `examples/Dockerfile:44` — "ruh roh"). `agent_id`, `revision`, `created`, and `nat-version` all update to match. - Add the plugin sentinel header to the shipped `.dockerignore` so subsequent `nemo agents package` runs recognise it as plugin-managed and safely regenerate it (the previous file pre-dated the sentinel system and would now be preserved as user-owned). Tests (`tests/unit/test_container.py`): - Invert `test_unknown_workflow_type` → `test_unknown_workflow_type_warns_does_not_fail`: asserts `valid=True`, message lands in `warnings`, and the known built-in types are still listed so operators can spot typos. - Add `test_unreadable_config_returns_structured_error` covering both missing-file and binary-blob paths. Existing test counts: 96 -> 97 (one new), all passing. Ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
… harden validator, polish docs/examples Bundles the P0 + P1 review feedback into one follow-up commit. Validator (`container/validator.py` + `container/builder.py`): - Add `warnings: list[str]` field to `ValidationResult` and surface it from the builder's pre-build validate pass via `typer.echo(..., err=True)`. - Demote unknown `workflow._type` from a hard error to a soft warning. NAT's plugin system can register additional workflow types at runtime, and a closed allowlist here was effectively a denylist that forced operators to `--skip-validation` on otherwise-valid configs. Missing `_type` remains a hard error (truly unbuildable). (benmccown) - Wrap `agent_config.read_text(...)` in `try/except (OSError, UnicodeDecodeError)` so missing / unreadable / non-UTF-8 configs surface as a structured `ValidationResult` instead of leaking a raw traceback to the operator. (CodeRabbit `validator.py:49`) Metadata (`container/metadata.py`): - Narrow `_load_pyproject` exception handler from bare `Exception` to `tomllib.TOMLDecodeError`. Parse failures still fall back to an empty dict so OCI labels can be filled from CLI flags; real bugs (OSError, UnicodeDecodeError) now propagate instead of being silently swallowed. (mckornfield `metadata.py:94`) CLI (`cli.py`): - ASCII-ify the `package` docstring: replace `→` / `•` / `—` with `->` / `-` / `--` so the help text renders correctly on Windows consoles using cp1252. (mckornfield `cli.py:325`) Docs (`README.md`): - Rewrite the `--nat-version` flag row to mention the env-var → baked-in fallback chain and the warning the CLI prints when neither is set, matching actual behavior. (CodeRabbit `README.md:356`) - Tighten the `--platform` row to mention the actionable `docker buildx imagetools create` workaround for multi-arch. Examples (`examples/Dockerfile`, `examples/.dockerignore`): - Regenerate against the current tree so the OCI source label reads `git@github.com:NVIDIA-NeMo/nemo-platform.git` instead of the stale `ssh://git@gitlab-master.nvidia.com:12051/aire/microservices/nmp.git` carried over from the original NMP-tree checkin. (mckornfield `examples/Dockerfile:44` — "ruh roh"). `agent_id`, `revision`, `created`, and `nat-version` all update to match. - Add the plugin sentinel header to the shipped `.dockerignore` so subsequent `nemo agents package` runs recognise it as plugin-managed and safely regenerate it (the previous file pre-dated the sentinel system and would now be preserved as user-owned). Tests (`tests/unit/test_container.py`): - Invert `test_unknown_workflow_type` → `test_unknown_workflow_type_warns_does_not_fail`: asserts `valid=True`, message lands in `warnings`, and the known built-in types are still listed so operators can spot typos. - Add `test_unreadable_config_returns_structured_error` covering both missing-file and binary-blob paths. Existing test counts: 96 -> 97 (one new), all passing. Ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
3bfe351 to
5880bd9
Compare
gabwow
left a comment
There was a problem hiding this comment.
Approving but I want you to think how tightly coupled we want this implementation to NAT. Is it too early to think about what a harness protocol would look like?
… harden validator, polish docs/examples Bundles the P0 + P1 review feedback into one follow-up commit. Validator (`container/validator.py` + `container/builder.py`): - Add `warnings: list[str]` field to `ValidationResult` and surface it from the builder's pre-build validate pass via `typer.echo(..., err=True)`. - Demote unknown `workflow._type` from a hard error to a soft warning. NAT's plugin system can register additional workflow types at runtime, and a closed allowlist here was effectively a denylist that forced operators to `--skip-validation` on otherwise-valid configs. Missing `_type` remains a hard error (truly unbuildable). (benmccown) - Wrap `agent_config.read_text(...)` in `try/except (OSError, UnicodeDecodeError)` so missing / unreadable / non-UTF-8 configs surface as a structured `ValidationResult` instead of leaking a raw traceback to the operator. (CodeRabbit `validator.py:49`) Metadata (`container/metadata.py`): - Narrow `_load_pyproject` exception handler from bare `Exception` to `tomllib.TOMLDecodeError`. Parse failures still fall back to an empty dict so OCI labels can be filled from CLI flags; real bugs (OSError, UnicodeDecodeError) now propagate instead of being silently swallowed. (mckornfield `metadata.py:94`) CLI (`cli.py`): - ASCII-ify the `package` docstring: replace `→` / `•` / `—` with `->` / `-` / `--` so the help text renders correctly on Windows consoles using cp1252. (mckornfield `cli.py:325`) Docs (`README.md`): - Rewrite the `--nat-version` flag row to mention the env-var → baked-in fallback chain and the warning the CLI prints when neither is set, matching actual behavior. (CodeRabbit `README.md:356`) - Tighten the `--platform` row to mention the actionable `docker buildx imagetools create` workaround for multi-arch. Examples (`examples/Dockerfile`, `examples/.dockerignore`): - Regenerate against the current tree so the OCI source label reads `git@github.com:NVIDIA-NeMo/nemo-platform.git` instead of the stale `ssh://git@gitlab-master.nvidia.com:12051/aire/microservices/nmp.git` carried over from the original NMP-tree checkin. (mckornfield `examples/Dockerfile:44` — "ruh roh"). `agent_id`, `revision`, `created`, and `nat-version` all update to match. - Add the plugin sentinel header to the shipped `.dockerignore` so subsequent `nemo agents package` runs recognise it as plugin-managed and safely regenerate it (the previous file pre-dated the sentinel system and would now be preserved as user-owned). Tests (`tests/unit/test_container.py`): - Invert `test_unknown_workflow_type` → `test_unknown_workflow_type_warns_does_not_fail`: asserts `valid=True`, message lands in `warnings`, and the known built-in types are still listed so operators can spot typos. - Add `test_unreadable_config_returns_structured_error` covering both missing-file and binary-blob paths. Existing test counts: 96 -> 97 (one new), all passing. Ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
…re preservation, except hardening, doc drift - Bump default base image from `ubuntu:22.04_20240212` to `ubuntu:noble-20260217` (24.04 LTS) in `_DEFAULTS` and regenerated `examples/Dockerfile`. - Preserve committed plugin-managed `.dockerignore` across build cleanup: snapshot `path.exists()` before `render_dockerignore` and only unlink in `finally` when the file did not exist beforehand. Regression test `test_build_preserves_committed_plugin_managed_dockerignore` locks both corners of the truth table. - Harden `metadata.py` exception handling: move `import yaml` to module top (matches validator.py), narrow two broad `except Exception` clauses to `yaml.YAMLError`, replace `pass` with explicit `return ""` in `_resolve_description`, and add explanatory comments to every remaining `except` (including the intentional `SOURCE_DATE_EPOCH` fall-through and the `urlsplit` `ValueError`). - Correct README rendering-modes table: project mode runs `uv pip install .` only (no `uv sync`), matching the template and the existing regression test. 98/98 unit tests pass; ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
5880bd9 to
6f49e5c
Compare
@gabwow Things have certainly changed between when feature request was made versus now. We dont have a view yet for what a generic harness protocol looks like. We can address it iteratively across our agent plugin where deploy also need to be decoupled from framework |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/nemo-agents/README.md (2)
356-356:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
--outputdefault description for project mode.This default is inaccurate. With
--pyproject, default output isDockerfilenext topyproject.toml; otherwise it’s next to the agent config.🤖 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/nemo-agents/README.md` at line 356, Update the README table entry for the `--output`/`-o` flag to correct its default description: explain that when `--pyproject` is used the default output is a `Dockerfile` placed next to `pyproject.toml`, otherwise the default is a `Dockerfile` placed next to the agent config (the `<config-dir>/Dockerfile`). Modify the table row containing `--output`, `-o` so it mentions both modes (`--pyproject` vs project/agent config) and the corresponding default output locations.
267-270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix fenced output blocks for markdownlint compliance.
These fences need a language token (e.g.,
text) and a blank line before each opening fence.As per coding guidelines: “Use active voice, present tense, professional tone, and scannable/purposeful formatting (PACE communication style: Professional, Active, Conversational, Engaging)”.Suggested patch
Output: -``` + +```text Dockerfile written to examples/Dockerfile .dockerignore written to examples/.dockerignoreOutput:
-+ +text
Building image 'my-agent:1.0' from context examples ...
Successfully built my-agent:1.0
Image ready: my-agent:1.0Output: -``` + +```text Building image 'my-agent:1.0' from context examples ... Successfully built my-agent:1.0 Image ready: my-agent:1.0 Tagging my-agent:1.0 -> nvcr.io/my-org/my-agent:1.0 Pushing nvcr.io/my-org/my-agent:1.0 ... Successfully pushed nvcr.io/my-org/my-agent:1.0 Published: nvcr.io/my-org/my-agent:1.0When
--tagis not provided, the image tag is computed automatically as:-
+text
-:Also applies to: 282-286, 299-307, 402-404
🤖 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/nemo-agents/README.md` around lines 267 - 270, Several fenced code blocks in plugins/nemo-agents/README.md (notably around the ranges containing the example outputs at 267-270, 282-286, 299-307, and 402-404) are missing a language token and the required blank line before the opening fence; update each affected block by inserting a blank line immediately before the opening fence and changing the opening triple-backtick to include the language token (use "text"), and ensure the matching closing triple-backticks remain; apply this consistently to every similar output block in the file so markdownlint passes and readability is preserved.
🤖 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/nemo-agents/examples/Dockerfile`:
- Line 41: Replace the personal name in the OCI image label
org.opencontainers.image.authors with a neutral identifier; locate the label
assignment that currently sets "Arpit Singh (SW-CLOUD)" and change it to a
generic value such as "unknown" or "NeMo Agents Team" so example image metadata
no longer contains an individual's real name.
In `@plugins/nemo-agents/README.md`:
- Around line 462-463: Update the README wording to clarify that only a missing
workflow._type is a hard failure while unknown workflow._type values produce a
warning and continue; specifically change the line about `workflow._type` being
a known NAT agent type to state that missing `_type` is an error but
unrecognized `_type` values are warned (mention `workflow._type` and the
validator behavior) so readers understand the validator emits warnings for
unknown types rather than failing.
---
Duplicate comments:
In `@plugins/nemo-agents/README.md`:
- Line 356: Update the README table entry for the `--output`/`-o` flag to
correct its default description: explain that when `--pyproject` is used the
default output is a `Dockerfile` placed next to `pyproject.toml`, otherwise the
default is a `Dockerfile` placed next to the agent config (the
`<config-dir>/Dockerfile`). Modify the table row containing `--output`, `-o` so
it mentions both modes (`--pyproject` vs project/agent config) and the
corresponding default output locations.
- Around line 267-270: Several fenced code blocks in
plugins/nemo-agents/README.md (notably around the ranges containing the example
outputs at 267-270, 282-286, 299-307, and 402-404) are missing a language token
and the required blank line before the opening fence; update each affected block
by inserting a blank line immediately before the opening fence and changing the
opening triple-backtick to include the language token (use "text"), and ensure
the matching closing triple-backticks remain; apply this consistently to every
similar output block in the file so markdownlint passes and readability is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57e58fbc-345c-4fd6-897a-0e2aba7dd5dd
📒 Files selected for processing (12)
plugins/nemo-agents/README.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py
✅ Files skipped from review due to trivial changes (2)
- plugins/nemo-agents/examples/hello_world.yaml
- plugins/nemo-agents/examples/.dockerignore
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/nemo-agents/src/nemo_agents_plugin/container/publisher.py
- plugins/nemo-agents/src/nemo_agents_plugin/cli.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/template.py
- plugins/nemo-agents/src/nemo_agents_plugin/container/metadata.py
…re preservation, except hardening, doc drift - Bump default base image from `ubuntu:22.04_20240212` to `ubuntu:noble-20260217` (24.04 LTS) in `_DEFAULTS` and regenerated `examples/Dockerfile`. - Preserve committed plugin-managed `.dockerignore` across build cleanup: snapshot `path.exists()` before `render_dockerignore` and only unlink in `finally` when the file did not exist beforehand. Regression test `test_build_preserves_committed_plugin_managed_dockerignore` locks both corners of the truth table. - Harden `metadata.py` exception handling: move `import yaml` to module top (matches validator.py), narrow two broad `except Exception` clauses to `yaml.YAMLError`, replace `pass` with explicit `return ""` in `_resolve_description`, and add explanatory comments to every remaining `except` (including the intentional `SOURCE_DATE_EPOCH` fall-through and the `urlsplit` `ValueError`). - Correct README rendering-modes table: project mode runs `uv pip install .` only (no `uv sync`), matching the template and the existing regression test. 98/98 unit tests pass; ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
6f49e5c to
5e3f439
Compare
…re preservation, except hardening, doc drift - Bump default base image from `ubuntu:22.04_20240212` to `ubuntu:noble-20260217` (24.04 LTS) in `_DEFAULTS` and regenerated `examples/Dockerfile`. - Preserve committed plugin-managed `.dockerignore` across build cleanup: snapshot `path.exists()` before `render_dockerignore` and only unlink in `finally` when the file did not exist beforehand. Regression test `test_build_preserves_committed_plugin_managed_dockerignore` locks both corners of the truth table. - Harden `metadata.py` exception handling: move `import yaml` to module top (matches validator.py), narrow two broad `except Exception` clauses to `yaml.YAMLError`, replace `pass` with explicit `return ""` in `_resolve_description`, and add explanatory comments to every remaining `except` (including the intentional `SOURCE_DATE_EPOCH` fall-through and the `urlsplit` `ValueError`). - Correct README rendering-modes table: project mode runs `uv pip install .` only (no `uv sync`), matching the template and the existing regression test. 98/98 unit tests pass; ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
5e3f439 to
a61f1ac
Compare
Adds a render → validate → build → publish pipeline for OCI agent
container images, surfaced via `nemo agents package` and the
`AgentsCLI` rich-help panel.
CLI surface:
- `--no-build` emit Dockerfile + .dockerignore and exit
- (default) render + validate + build the image
- `--publish --registry <r>` additionally push after a successful build
- `--platform` single-arch only (multi-arch deferred; >1 rejected)
- `--format` `docker` (default); `whl` rejected as not yet implemented
- `--agent-config`, `--pyproject`, `--output`, `--template`,
`--agent-version`, `--agent-author`, `--base-image-url`,
`--base-image-tag`, `--python-version`, `--nat-version`,
`--uv-version`, `--allow-root`
Modules under `plugins/nemo-agents/src/nemo_agents_plugin/container/`:
- `template.py` — Jinja2 Dockerfile + .dockerignore renderers with a
`dockerfile_escape` filter on every OCI label interpolation, a
plugin sentinel that prevents overwriting user-owned files, and
StrictUndefined for fail-fast template typos.
- `metadata.py` — agent metadata extraction with credential stripping
(basic-auth userinfo *and* query-string tokens), git revision /
source / author resolution scoped to the project root via `cwd`,
`SOURCE_DATE_EPOCH`-aware reproducible timestamps, and a
content-addressable `agent_id` hashed with domain separation across
config / pyproject / build env.
- `validator.py` — structural NAT-config validation; rejects missing
or unknown `workflow._type`, malformed `workflow.tool_names` /
`workflow.llm_name` / `llms` (wrong YAML type), and unresolved
tool/llm references — errors are collected, not short-circuited.
- `builder.py` — python-on-whales orchestration with explicit
BuildKit, sanitized default image name/tag, single metadata pass
shared with the renderer, and refusal to overwrite a pre-existing
`Dockerfile.generated`.
- `publisher.py` — docker push wrapper.
Hardening:
- Dockerfile label escape filter defeats injection via metadata.
- `.dockerignore` sentinel-based preservation of user-owned files.
- Default-path Dockerfile is never silently overwritten; explicit
`--output` is treated as informed consent.
- Credential stripping prevents PATs / OAuth tokens from leaking into
`org.opencontainers.image.source`.
- Build env is folded into `agent_id` so changing `nat_version` /
`python_version` / base image yields a distinct, reproducible id.
- `--format whl` and `>1 --platform` are rejected at flag-validation
time with actionable messages, replacing the previous false-positive
success path.
- All render-only filesystem writes are wrapped in `except OSError` →
clean `Error:` CLI line and `typer.Exit(1)`, no traceback to the
operator and no partial success messages before failure.
Tests (`plugins/nemo-agents/tests/unit/test_container.py`, 96 cases):
- Metadata extraction, credential-stripping matrix (URLs assembled
from variables at runtime so the source file does not contain
literal `scheme://user:pass@host` substrings that trip secret
scanners), reproducible-timestamp / build-env-in-agent-id, validator
(including malformed-type rejection), render-only path (including
OSError → clean CLI error), build path, publish path, multi-platform
rejection, format-whl rejection, end-to-end pipeline, package
command, and every safety-guard regression.
Docs:
- `plugins/nemo-agents/README.md` — flag matrix, examples,
reproducibility guarantees, credential-handling behavior.
- `plugins/nemo-agents/examples/{hello_world.yaml, Dockerfile,
.dockerignore}` — runnable sample agent.
Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
… harden validator, polish docs/examples Bundles the P0 + P1 review feedback into one follow-up commit. Validator (`container/validator.py` + `container/builder.py`): - Add `warnings: list[str]` field to `ValidationResult` and surface it from the builder's pre-build validate pass via `typer.echo(..., err=True)`. - Demote unknown `workflow._type` from a hard error to a soft warning. NAT's plugin system can register additional workflow types at runtime, and a closed allowlist here was effectively a denylist that forced operators to `--skip-validation` on otherwise-valid configs. Missing `_type` remains a hard error (truly unbuildable). (benmccown) - Wrap `agent_config.read_text(...)` in `try/except (OSError, UnicodeDecodeError)` so missing / unreadable / non-UTF-8 configs surface as a structured `ValidationResult` instead of leaking a raw traceback to the operator. (CodeRabbit `validator.py:49`) Metadata (`container/metadata.py`): - Narrow `_load_pyproject` exception handler from bare `Exception` to `tomllib.TOMLDecodeError`. Parse failures still fall back to an empty dict so OCI labels can be filled from CLI flags; real bugs (OSError, UnicodeDecodeError) now propagate instead of being silently swallowed. (mckornfield `metadata.py:94`) CLI (`cli.py`): - ASCII-ify the `package` docstring: replace `→` / `•` / `—` with `->` / `-` / `--` so the help text renders correctly on Windows consoles using cp1252. (mckornfield `cli.py:325`) Docs (`README.md`): - Rewrite the `--nat-version` flag row to mention the env-var → baked-in fallback chain and the warning the CLI prints when neither is set, matching actual behavior. (CodeRabbit `README.md:356`) - Tighten the `--platform` row to mention the actionable `docker buildx imagetools create` workaround for multi-arch. Examples (`examples/Dockerfile`, `examples/.dockerignore`): - Regenerate against the current tree so the OCI source label reads `git@github.com:NVIDIA-NeMo/nemo-platform.git` instead of the stale `ssh://git@gitlab-master.nvidia.com:12051/aire/microservices/nmp.git` carried over from the original NMP-tree checkin. (mckornfield `examples/Dockerfile:44` — "ruh roh"). `agent_id`, `revision`, `created`, and `nat-version` all update to match. - Add the plugin sentinel header to the shipped `.dockerignore` so subsequent `nemo agents package` runs recognise it as plugin-managed and safely regenerate it (the previous file pre-dated the sentinel system and would now be preserved as user-owned). Tests (`tests/unit/test_container.py`): - Invert `test_unknown_workflow_type` → `test_unknown_workflow_type_warns_does_not_fail`: asserts `valid=True`, message lands in `warnings`, and the known built-in types are still listed so operators can spot typos. - Add `test_unreadable_config_returns_structured_error` covering both missing-file and binary-blob paths. Existing test counts: 96 -> 97 (one new), all passing. Ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
…re preservation, except hardening, doc drift - Bump default base image from `ubuntu:22.04_20240212` to `ubuntu:noble-20260217` (24.04 LTS) in `_DEFAULTS` and regenerated `examples/Dockerfile`. - Preserve committed plugin-managed `.dockerignore` across build cleanup: snapshot `path.exists()` before `render_dockerignore` and only unlink in `finally` when the file did not exist beforehand. Regression test `test_build_preserves_committed_plugin_managed_dockerignore` locks both corners of the truth table. - Harden `metadata.py` exception handling: move `import yaml` to module top (matches validator.py), narrow two broad `except Exception` clauses to `yaml.YAMLError`, replace `pass` with explicit `return ""` in `_resolve_description`, and add explanatory comments to every remaining `except` (including the intentional `SOURCE_DATE_EPOCH` fall-through and the `urlsplit` `ValueError`). - Correct README rendering-modes table: project mode runs `uv pip install .` only (no `uv sync`), matching the template and the existing regression test. 98/98 unit tests pass; ruff lint + format clean. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
a61f1ac to
8e23bff
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
plugins/nemo-agents/tests/unit/test_container.py (1)
1248-1250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winString-based type hint still present.
def test_no_build_project_mode_writes_dockerfile_next_to_pyproject( - self, package_cli, project_dir: "tuple[Path, Path]" + self, package_cli, project_dir: tuple[Path, Path] ) -> None:🤖 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/nemo-agents/tests/unit/test_container.py` around lines 1248 - 1250, The test function test_no_build_project_mode_writes_dockerfile_next_to_pyproject contains a string-based type hint ("tuple[Path, Path]"); replace the quoted annotation with a real type hint (e.g., tuple[Path, Path] or Tuple[Path, Path] and add from typing import Tuple if you choose the latter) so the annotation is evaluated correctly at runtime and by type checkers—update the function signature to use the unquoted type and adjust imports if necessary.plugins/nemo-agents/README.md (2)
402-404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifier to tag format block.
The code block showing the tag format lacks a language identifier (MD040). Add
text:-``` +```text <agent-name>-<agent-id>:<agent-version><details> <summary>🤖 Prompt for AI Agents</summary>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/nemo-agents/README.mdaround lines 402 - 404, Update the fenced code
block that shows the tag format so it includes the language specifier "text";
locate the block containing "-:" in
README.md and change the opening fence fromtotext to satisfy the MD040
lint rule.</details> --- `267-270`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add language specifiers to output blocks.** Output blocks at lines 267, 282, 299 lack language identifiers. Add `text` or `console` and ensure blank lines surround each block (MD040/MD031). Note: These violations were previously addressed but appear to have regressed or the fix was incomplete. <details> <summary>📝 Proposed fix</summary> For line 267: ```diff -``` +```text Dockerfile written to examples/Dockerfile .dockerignore written to examples/.dockerignore ``` ``` Apply the same pattern to lines 282 and 299. </details> Also applies to: 282-286, 299-307 <details> <summary>🤖 Prompt for AI Agents</summary>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/nemo-agents/README.mdaround lines 267 - 270, Update the three
unlabeled fenced code blocks in README.md (the blocks that currently contain
"Dockerfile written to examples/Dockerfile / .dockerignore written to
examples/.dockerignore" and the similar outputs near the other two locations) to
include a language specifier (use "text" or "console") and ensure there is a
blank line before and after each fenced block so they satisfy MD040/MD031;
locate the three blocks around the existing snippets (the ones shown in the diff
and the two similar blocks later in the file) and replace the opening triple
backticks withtext (orconsole) and verify surrounding blank lines are
present.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>plugins/nemo-agents/README.md (2)</summary><blockquote> `231-495`: _💤 Low value_ **Section mixes HOW-TO examples with REFERENCE tables.** The packaging section combines step-by-step examples (HOW-TO) with comprehensive flag tables and OCI label specifications (REFERENCE). Per coding guidelines, each documentation section should fit one Diataxis quadrant; use cross-links to separate concerns. Consider splitting: keep usage examples here, move flag reference and OCI label specs to a separate reference section or collapsible dropdown. As per coding guidelines: "Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead." <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/nemo-agents/README.md` around lines 231 - 495, The "Packaging command — containerize agents as Docker images" section mixes HOW-TO examples with reference material; split this into two pages/sections: keep the step-by-step examples and progressive pipeline under the existing "Packaging command — containerize agents as Docker images" heading (HOW-TO) and move the "Flag reference" and "OCI image labels" tables (and any detailed label mappings under "OCI labels") into a new Reference page/section named e.g. "Packaging CLI reference"; update cross-links from the HOW-TO to the new Reference section and adjust the README table of contents accordingly (look for the headings "Packaging command — containerize agents as Docker images", "Flag reference", "OCI image labels" and "Rendering modes" in the diff to locate content to move and link). ``` </details> --- `231-495`: _⚡ Quick win_ **Add "Next Steps" section at end of packaging documentation.** The packaging section lacks a "Next Steps" section with cross-links to related content. Add links to deployment, evaluation, and log inspection sections. As per coding guidelines: "Include 'Next Steps' section at the end with cross-links to related documentation content." <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/nemo-agents/README.md` around lines 231 - 495, Append a new "Next Steps" subsection at the end of the "Packaging command — containerize agents as Docker images" section that provides concise cross-links to the related documentation pages: deployment (e.g. "Deployment / Running agents"), evaluation (e.g. "Evaluating agent performance / Benchmarks"), and log inspection (e.g. "Inspecting logs / Troubleshooting"); ensure the heading is formatted like "### Next Steps" and include one-line bullets with link text matching the target docs so readers can navigate from packaging to deployment, evaluation, and log inspection content. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@plugins/nemo-agents/README.md:
- Around line 402-404: Update the fenced code block that shows the tag format so
it includes the language specifier "text"; locate the block containing
"-:" in README.md and change the opening
fence fromtotext to satisfy the MD040 lint rule.- Around line 267-270: Update the three unlabeled fenced code blocks in
README.md (the blocks that currently contain "Dockerfile written to
examples/Dockerfile / .dockerignore written to examples/.dockerignore" and the
similar outputs near the other two locations) to include a language specifier
(use "text" or "console") and ensure there is a blank line before and after each
fenced block so they satisfy MD040/MD031; locate the three blocks around the
existing snippets (the ones shown in the diff and the two similar blocks later
in the file) and replace the opening triple backticks with ```text (orIn `@plugins/nemo-agents/tests/unit/test_container.py`: - Around line 1248-1250: The test function test_no_build_project_mode_writes_dockerfile_next_to_pyproject contains a string-based type hint ("tuple[Path, Path]"); replace the quoted annotation with a real type hint (e.g., tuple[Path, Path] or Tuple[Path, Path] and add from typing import Tuple if you choose the latter) so the annotation is evaluated correctly at runtime and by type checkers—update the function signature to use the unquoted type and adjust imports if necessary. --- Nitpick comments: In `@plugins/nemo-agents/README.md`: - Around line 231-495: The "Packaging command — containerize agents as Docker images" section mixes HOW-TO examples with reference material; split this into two pages/sections: keep the step-by-step examples and progressive pipeline under the existing "Packaging command — containerize agents as Docker images" heading (HOW-TO) and move the "Flag reference" and "OCI image labels" tables (and any detailed label mappings under "OCI labels") into a new Reference page/section named e.g. "Packaging CLI reference"; update cross-links from the HOW-TO to the new Reference section and adjust the README table of contents accordingly (look for the headings "Packaging command — containerize agents as Docker images", "Flag reference", "OCI image labels" and "Rendering modes" in the diff to locate content to move and link). - Around line 231-495: Append a new "Next Steps" subsection at the end of the "Packaging command — containerize agents as Docker images" section that provides concise cross-links to the related documentation pages: deployment (e.g. "Deployment / Running agents"), evaluation (e.g. "Evaluating agent performance / Benchmarks"), and log inspection (e.g. "Inspecting logs / Troubleshooting"); ensure the heading is formatted like "### Next Steps" and include one-line bullets with link text matching the target docs so readers can navigate from packaging to deployment, evaluation, and log inspection content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID:
c48242f0-7024-4eec-b4c2-9e9ca36ec820⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock📒 Files selected for processing (12)
plugins/nemo-agents/README.mdplugins/nemo-agents/examples/.dockerignoreplugins/nemo-agents/examples/Dockerfileplugins/nemo-agents/examples/hello_world.yamlplugins/nemo-agents/pyproject.tomlplugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/container/builder.pyplugins/nemo-agents/src/nemo_agents_plugin/container/metadata.pyplugins/nemo-agents/src/nemo_agents_plugin/container/publisher.pyplugins/nemo-agents/src/nemo_agents_plugin/container/template.pyplugins/nemo-agents/src/nemo_agents_plugin/container/validator.pyplugins/nemo-agents/tests/unit/test_container.py✅ Files skipped from review due to trivial changes (1)
- plugins/nemo-agents/examples/.dockerignore
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/nemo-agents/pyproject.toml
- plugins/nemo-agents/examples/hello_world.yaml
- plugins/nemo-agents/src/nemo_agents_plugin/container/publisher.py
- plugins/nemo-agents/examples/Dockerfile
- plugins/nemo-agents/src/nemo_agents_plugin/cli.py
Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Introduces a render → validate → build → publish pipeline for packaging NAT agents as OCI container images, driven by flags on a single command.
container/module: Jinja2 Dockerfile renderer (template.py), python-on-whales builder (builder.py) and publisher (publisher.py), structural config validator (validator.py), OCI metadata extractor (metadata.py), and a sample sandbox policy (openshell_policy.yaml).nemo agents packagewith--no-build,--publish,--registry,--format docker|whl,--platform, plus reproducibility knobs (--nat-version,--base-image-*,--python-version) and security defaults (--allow-rootopt-out, generated.dockerignore).[container]optional extra (jinja2, python-on-whales) and adds jinja2 to the test extra.examples/hello_world.yaml).Summary by CodeRabbit
nemo agents packageCLI to render → validate → build → (optional) publish container images with project/config modes, image tagging, and push.containerextras for packaging tooling.