fix(dcode): close inherited route-probe descriptor - #7032
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
Recommended Blocker
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe managed-exec launcher now closes file descriptor 3 before running commands. Unit and end-to-end tests verify direct managed execution, descriptor closure, and fail-closed rejection of untrusted route evidence before session attachment. ChangesDCode execution and route validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-managed-exec.test.ts`:
- Around line 164-181: Update the regression test around makeLauncherFixture and
launcherPath so it invokes the installed
/usr/local/lib/nemoclaw/dcode-managed-exec artifact directly, or add an
installation step that produces that exact artifact before execution. Ensure the
FD 3 closure assertion runs against the installed managed helper rather than
only a temporary copy generated from LAUNCHER_PATH.
🪄 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: 126e95cb-8c7d-4a9d-b64e-b37e16a6e5ed
📒 Files selected for processing (2)
agents/langchain-deepagents-code/dcode-launcher.shtest/dcode-managed-exec.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Blocking exact head 11382f8 on the remaining #7031 acceptance gap. The exec 3>&- fix is correct and the focused managed-exec/probe/image suites pass, but #7031 explicitly requires one image-backed regression that executes the installed managed helper through the route probe and then proves bare connect exits nonzero without attaching when route evidence is untrusted. The current tests cover the temp-copy helper and mocked connect flow separately, so they do not protect that shipped composition. This matches exact-head Advisor PRA-1. Please add that installed-image boundary test, refresh onto current main, rerun exact-diff CI/E2E, and request rereview.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/langchain-deepagents-code-image.test.ts (1)
805-812: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid locking the image test to E2E script source text.
These literals can all be present while fail-closed behavior remains broken, and harmless shell refactors will cause failures. Keep only an artifact-presence check here and rely on the Vitest E2E result to prove nonzero connect status, probe execution, and no session attachment.
As per path instructions, prefer observable outcomes through the public boundary over source-text assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/langchain-deepagents-code-image.test.ts` around lines 805 - 812, Remove the assertions in the image-backed connect test that inspect E2E script source literals such as probe, session-attach, and shell-command text. Retain only the artifact-presence check, and use the Vitest E2E result through the public connect boundary to verify nonzero status, probe execution, and absence of session attachment.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/langchain-deepagents-code-image.test.ts`:
- Around line 805-812: Remove the assertions in the image-backed connect test
that inspect E2E script source literals such as probe, session-attach, and
shell-command text. Retain only the artifact-presence check, and use the Vitest
E2E result through the public connect boundary to verify nonzero status, probe
execution, and absence of session attachment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b54c3db4-0541-4c13-86ff-6378413d64a6
📒 Files selected for processing (2)
test/e2e/e2e-cloud-experimental/checks/07-deepagents-code-headless-inference.shtest/langchain-deepagents-code-image.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results — ✅ All selected tests passedRun: 29534487768
|
|
Rereview requested on exact head |
<!-- markdownlint-disable MD041 --> ## Summary PR E2E risk plans now select the exact Deep Agents Code typed target when its headless-inference check changes, so #7032 can receive its required live regression instead of only the control-plane floor jobs. The controller dispatches jobs and that target in one bound run while preserving exact-SHA authorization, fork, stale-revision, and secret-scope boundaries. ## Changes - Extend deterministic risk plan v4 with `requiredTargets` for the current Deep Agents Code check-to-target requirement. The PR gate consumes this field because a free-standing workflow job cannot address the registry target; `pr-risk-plan.test.ts` and `pr-e2e-gate-typed-target.test.ts` protect the mapping and evidence contract. - Permit one workflow run to combine selected jobs with the exact allowlisted `ubuntu-repo-cloud-langchain-deepagents-code` target. The trusted workflow independently rejects unapproved controller target input and requires the generated matrix IDs to exactly match that selection before exposing any matrix output or starting credential-bearing work. - Bind typed-target evidence to `matrix.id`, upload its `default`-shard signal, and fail closed for missing, duplicate, skipped, malformed, stale, or unauthorized evidence. - Carry targets through E2E recommendations and PR review context, and update the maintainer runbook and merge-gate guidance for combined selections. ## 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 - [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 changes internal maintainer CI selection and evidence. Maintainer runbooks were updated; the documentation review found no supported product behavior, Fern page, or changelog change. - [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: The exact target is independently allowlisted in the trusted controller workflow and gate code, and the workflow requires exact equality between the trusted controller selection and the PR-generated matrix before any credential-bearing job. PR-modified execution remains behind exact-head/base `run-control-plane` authorization, forks and stale revisions do not dispatch, and the NVIDIA inference secret remains scoped to the existing live-test step. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line 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 — focused controller integration: 161 passed; workflow/E2E support: 81 passed; advisor consumers: 123 passed; final split target-gate regression: 29 passed; `npm run test:changed`: 208 passed; `npm run build:cli && npm run typecheck:cli`: passed. After binding the generated matrix to the controller selection: focused controller integration: 142 passed; workflow/E2E support: 113 passed; `npm run typecheck:cli`: passed. CodeRabbit PATH follow-up: workflow-boundary test 31 passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Focused control-plane contracts and the changed-test lane were used for this narrow prerequisite. - [x] 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added typed E2E target selection alongside jobs, supporting combined job+target plans and unified PR E2E gate dispatch/state handling. * Risk planning now tracks and recommends required targets (including Deep Agents headless typed checks). * **Bug Fixes** * Strengthened gate authorization and “fail-closed” behavior for both jobs and typed targets. * Improved controller-vs-matrix correlation, including stricter selector/matrix consistency checks. * **Documentation** * Updated the PR E2E gate workflow contract to reflect typed targets and “no run” semantics. * **Tests** * Expanded E2E/PR-gate coverage for typed targets, trusted controller matrix validation, and risk-signal/artifact contracts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
3 participants
Summary
Close the legacy auxiliary descriptor before the image-owned DCode managed-exec boundary launches a diagnostic command. Route-probe evidence now remains confined to the existing stdout/stderr contract, with an installed-helper regression against inherited descriptor output.
Related Issue
Closes #7031
Changes
dcode-managed-execexact-execs its command.usr/local/lib/nemoclaw/dcode-managed-execartifact.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/dcode-managed-exec.test.ts(6 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
connect --probe-onlyto fail closed when presented with untrusted evidence.Tests