fix(dcode): enforce empty prompt exit contract - #6441
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
📝 WalkthroughWalkthroughAdds an "empty non-interactive prompt" exit-code contract check for ChangesEmpty-prompt exit contract enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
E2E Target Results — ❌ Some jobs failedRun: 28917811899
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/agent/onboard-terminal-fixtures.test.ts (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the local issue-reference suffix to the test title.
The sibling contract test in this PR stack (
test/dcode-wrapper-empty-prompt.test.ts) tags its empty-prompt describe block with(#6440). This new test title omits that reference, which is inconsistent with the established convention for this feature.As per coding guidelines, "Write behavior-oriented test titles, and put local issue references in a final
(#1234)suffix."🤖 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 `@src/lib/agent/onboard-terminal-fixtures.test.ts` at line 158, The empty-prompt smoke test title in the on-board terminal fixtures suite is missing the local issue-reference suffix, which should match the established naming convention used by the sibling empty-prompt contract test. Update the `it(...)` title in `onboard-terminal-fixtures.test.ts` to include the final `(`#1234`)`-style suffix, keeping the title behavior-oriented and consistent with the existing `test/dcode-wrapper-empty-prompt.test.ts` pattern.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/dcode-wrapper-empty-prompt.test.ts`:
- Around line 20-37: Add a Bash 4+ availability check in wrapperFixtureCanRun
before enabling the fixture, because dcode-wrapper.sh relies on Bash-specific
lowercase expansion and can break on older macOS bash 3.2. Update the helper to
validate the version returned by bash --version (or equivalent) is at least 4,
alongside the existing python3 check, and only set canRun when both
prerequisites are satisfied.
---
Nitpick comments:
In `@src/lib/agent/onboard-terminal-fixtures.test.ts`:
- Line 158: The empty-prompt smoke test title in the on-board terminal fixtures
suite is missing the local issue-reference suffix, which should match the
established naming convention used by the sibling empty-prompt contract test.
Update the `it(...)` title in `onboard-terminal-fixtures.test.ts` to include the
final `(`#1234`)`-style suffix, keeping the title behavior-oriented and consistent
with the existing `test/dcode-wrapper-empty-prompt.test.ts` pattern.
🪄 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: d3634e5a-f7e6-4e30-8d79-b30c9afece37
📒 Files selected for processing (10)
agents/langchain-deepagents-code/Dockerfileagents/langchain-deepagents-code/manifest.yamlsrc/lib/agent/defs.test.tssrc/lib/agent/onboard-terminal-fixtures.test.tssrc/lib/agent/onboard-terminal-fixtures.tstest/cli/connect-terminal-agent.test.tstest/dcode-wrapper-empty-prompt.test.tstest/e2e/e2e-cloud-experimental/checks/07-deepagents-code-headless-inference.shtest/langchain-deepagents-code-image.test.tstest/langchain-deepagents-code-proxy-launcher.test.ts
| import { loadAgent } from "../src/lib/agent/defs"; | ||
|
|
||
| const AGENT_DIR = path.join(import.meta.dirname, "..", "agents", "langchain-deepagents-code"); | ||
| const WRAPPER = path.join(AGENT_DIR, "dcode-wrapper.sh"); | ||
| const EMPTY_PROMPT_DIAGNOSTIC = | ||
| "NemoClaw: empty non-interactive prompt for -n; provide prompt text."; | ||
|
|
||
| function python3Available(): boolean { | ||
| function wrapperFixtureCanRun(): boolean { | ||
| try { | ||
| return spawnSync("python3", ["--version"], { timeout: 5000 }).status === 0; | ||
| return ( | ||
| spawnSync("bash", ["--version"], { timeout: 5000 }).status === 0 && | ||
| spawnSync("python3", ["--version"], { timeout: 5000 }).status === 0 | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
| const canRun = process.platform === "linux" && python3Available(); | ||
| const canRun = wrapperFixtureCanRun(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check dcode-wrapper.sh for bash4+-only constructs
fd dcode-wrapper.sh agents/langchain-deepagents-code --exec rg -n 'mapfile|readarray|declare -A|local -n|\$\{[A-Za-z_]+,,\}|\$\{[A-Za-z_]+\^\^\}' {}Repository: NVIDIA/NemoClaw
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant wrapper section and any other bash-4+ syntax in the script.
script=agents/langchain-deepagents-code/dcode-wrapper.sh
printf '\n== around lowercasing usages ==\n'
sed -n '620,670p' "$script" | cat -n
printf '\n== broader bash-4+ feature search ==\n'
rg -n '\$\{[A-Za-z_][A-Za-z0-9_]*,,\}|\$\{[A-Za-z_][A-Za-z0-9_]*\^\^\}|mapfile|readarray|declare -A|local -n' "$script"Repository: NVIDIA/NemoClaw
Length of output: 2353
Require a bash 4+ check here. dcode-wrapper.sh already uses ${var,,}, so this fixture can fail on macOS’s stock bash 3.2 unless a newer bash is guaranteed on PATH.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/dcode-wrapper-empty-prompt.test.ts` around lines 20 - 37, Add a Bash 4+
availability check in wrapperFixtureCanRun before enabling the fixture, because
dcode-wrapper.sh relies on Bash-specific lowercase expansion and can break on
older macOS bash 3.2. Update the helper to validate the version returned by bash
--version (or equivalent) is at least 4, alongside the existing python3 check,
and only set canRun when both prerequisites are satisfied.
E2E Target Results — ✅ All selected jobs passedRun: 28917811899
|
cv
left a comment
There was a problem hiding this comment.
Reviewed the current head. The empty-prompt exit-2 contract is enforced across image build, manifest smoke, direct-exec, and login-shell acceptance paths with focused regression coverage. The previously cancelled commit-lint run was rerun successfully; current-head CI, live E2E, advisors, and contributor-compliance gates are green.
<!-- markdownlint-disable MD041 --> ## Summary Adds a shared deterministic regression risk plan for the PR head commit and changed-file set to PR Review Advisor and E2E Advisor. Known high-risk runtime surfaces now restore a reviewed floor of required E2E jobs even when a model omits or downgrades them. Execution is intentionally separated from plan generation: stacked follow-up #6447 consumes this contract in a trusted post-merge shadow gate without exposing repository secrets to unmerged code. PR Review Advisor analysis is split into seven bounded, evidence-focused turns with completion logs and per-turn response artifacts, improving review visibility and traceability while preserving one read-only session and the existing final JSON/comment contract. The seven stages now live in one ordered array. Stage names, evidence, prompt text, runtime numbering, and artifact ordering derive from that single declaration instead of parallel hard-coded topology. ## Changes - Add a stable risk-plan contract with lifecycle, upgrade, agent, inference, messaging, platform, credential, and security invariants. - Keep the canonical cloud-onboard live target in the platform-install floor through an explicit test-file allowlist, while ordinary tests remain excluded. - Feed the plan into PR Review Advisor scope/risk, security/trust, and tests/regressions contexts and both E2E Advisor normalizers.\n- Deduplicate optional recommendations against deterministic required jobs by both selector ID and workflow job. - Clarify commit references in advisor, E2E, and security-review documentation. - Emit `risk-plan.json` from both E2E Advisor lanes with a plan hash, required jobs, capped automatic candidates, and manual-expansion state; PR Review Advisor exposes the same plan through its scoped context artifacts. - Replace four uneven advisor turns with seven bounded stages: scope/risk, correctness/state, security/trust, tests/regressions, CI/operations, reconciliation, and final synthesis. - Derive turn numbering and prompt-artifact ordering from the ordered stage declaration, and remove five duplicate standalone context JSONs while retaining the same evidence in per-stage synthetic-tool artifacts. - Log every turn boundary and persist each settled response immediately, including partial output and failure/timeout status, while treating missing trace artifacts as fatal. - Leave live enforcement to stacked follow-up #6447 so this PR remains a reviewable deterministic planning primitive. - Remove unused PR-advisor check-wait configuration and align contributor-facing documentation with actual workflow behavior. - Add focused normalization, prompt, risk-mapping, workflow-inventory, and review-depth regression tests. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Internal CI/advisor behavior only; contributor-facing tool and E2E documentation was updated, with no Fern user documentation required. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Automated and local security review confirmed the deterministic floor is additive, bound to the PR head commit and changed-file set, capped, and does not itself execute secret-bearing live E2E work; enforcement is isolated in #6447. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 112 focused risk-plan, PR-advisor, E2E-advisor, workflow-boundary, and maintainer-policy tests passed; the E2E-advisor suite was rerun after the final normalization fix (7/7); CLI typechecking, Biome, test-size, and Vitest project-membership checks passed - [ ] Applicable broad gate passed — CI for PR head commit `9700a6ba` has 39 successful checks and 2 expected skips. `static-checks` fails only because merged #6441 left `test/langchain-deepagents-code-image.test.ts` at 1501 lines against the 1500-line mainline ceiling; aggregate `checks` mirrors that failure. The repair is tracked in #6493. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 sentences: what this PR does and why. --> Enforces the existing `dcode -n ""` exit-code contract across final image assembly, terminal-agent readiness, and live login/direct execution. Clean v0.0.75 and current arm64 builds already return the canonical diagnostic with exit 2, so this change closes the acceptance gap by rejecting stale or misassembled images instead of rewriting the wrapper's existing `exit 2` guard. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Fixes NVIDIA#6440 ## Changes <!-- Bullet list of key changes. --> - Fail the DCode image build and terminal-agent smoke check unless `dcode -n ""` returns exit 2 with the canonical diagnostic, with bounded timeouts. - Exercise the wrapper through the installed launcher chain and model the reported correct-message/exit-0 regression explicitly. - Require both login-shell and direct-exec empty-prompt rejection in the live DCode E2E check. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this restores the established invalid-input exit contract; no command, flag, configuration, or workflow changed. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: pending maintainer review; no waiver claimed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: no acceptance claimed; required CI must pass. ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 38 CLI tests and 88 integration tests passed; focused live `ubuntu-repo-cloud-langchain-deepagents-code` E2E passed on exact head in [run 28917811899, attempt 2](https://github.com/NVIDIA/NemoClaw/actions/runs/28917811899/attempts/2). - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: exact-head GitHub CI passed all five CLI test shards, installer integration, plugin tests, static checks, build/typecheck, and the `cli-tests` aggregate at `2a8d0cac`. - [ ] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds a shared deterministic regression risk plan for the PR head commit and changed-file set to PR Review Advisor and E2E Advisor. Known high-risk runtime surfaces now restore a reviewed floor of required E2E jobs even when a model omits or downgrades them. Execution is intentionally separated from plan generation: stacked follow-up NVIDIA#6447 consumes this contract in a trusted post-merge shadow gate without exposing repository secrets to unmerged code. PR Review Advisor analysis is split into seven bounded, evidence-focused turns with completion logs and per-turn response artifacts, improving review visibility and traceability while preserving one read-only session and the existing final JSON/comment contract. The seven stages now live in one ordered array. Stage names, evidence, prompt text, runtime numbering, and artifact ordering derive from that single declaration instead of parallel hard-coded topology. ## Changes - Add a stable risk-plan contract with lifecycle, upgrade, agent, inference, messaging, platform, credential, and security invariants. - Keep the canonical cloud-onboard live target in the platform-install floor through an explicit test-file allowlist, while ordinary tests remain excluded. - Feed the plan into PR Review Advisor scope/risk, security/trust, and tests/regressions contexts and both E2E Advisor normalizers.\n- Deduplicate optional recommendations against deterministic required jobs by both selector ID and workflow job. - Clarify commit references in advisor, E2E, and security-review documentation. - Emit `risk-plan.json` from both E2E Advisor lanes with a plan hash, required jobs, capped automatic candidates, and manual-expansion state; PR Review Advisor exposes the same plan through its scoped context artifacts. - Replace four uneven advisor turns with seven bounded stages: scope/risk, correctness/state, security/trust, tests/regressions, CI/operations, reconciliation, and final synthesis. - Derive turn numbering and prompt-artifact ordering from the ordered stage declaration, and remove five duplicate standalone context JSONs while retaining the same evidence in per-stage synthetic-tool artifacts. - Log every turn boundary and persist each settled response immediately, including partial output and failure/timeout status, while treating missing trace artifacts as fatal. - Leave live enforcement to stacked follow-up NVIDIA#6447 so this PR remains a reviewable deterministic planning primitive. - Remove unused PR-advisor check-wait configuration and align contributor-facing documentation with actual workflow behavior. - Add focused normalization, prompt, risk-mapping, workflow-inventory, and review-depth regression tests. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: Internal CI/advisor behavior only; contributor-facing tool and E2E documentation was updated, with no Fern user documentation required. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Automated and local security review confirmed the deterministic floor is additive, bound to the PR head commit and changed-file set, capped, and does not itself execute secret-bearing live E2E work; enforcement is isolated in NVIDIA#6447. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 112 focused risk-plan, PR-advisor, E2E-advisor, workflow-boundary, and maintainer-policy tests passed; the E2E-advisor suite was rerun after the final normalization fix (7/7); CLI typechecking, Biome, test-size, and Vitest project-membership checks passed - [ ] Applicable broad gate passed — CI for PR head commit `9700a6ba` has 39 successful checks and 2 expected skips. `static-checks` fails only because merged NVIDIA#6441 left `test/langchain-deepagents-code-image.test.ts` at 1501 lines against the 1500-line mainline ceiling; aggregate `checks` mirrors that failure. The repair is tracked in NVIDIA#6493. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Enforces the existing
dcode -n ""exit-code contract across final image assembly, terminal-agent readiness, and live login/direct execution. Clean v0.0.75 and current arm64 builds already return the canonical diagnostic with exit 2, so this change closes the acceptance gap by rejecting stale or misassembled images instead of rewriting the wrapper's existingexit 2guard.Related Issue
Fixes #6440
Changes
dcode -n ""returns exit 2 with the canonical diagnostic, with bounded timeouts.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableubuntu-repo-cloud-langchain-deepagents-codeE2E passed on exact head in run 28917811899, attempt 2.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: exact-head GitHub CI passed all five CLI test shards, installer integration, plugin tests, static checks, build/typecheck, and thecli-testsaggregate at2a8d0cac.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com