fix(dcode): supply the Ultra template argument from the managed resolver - #7463
fix(dcode): supply the Ultra template argument from the managed resolver#7463vyncint wants to merge 12 commits into
Conversation
The hardened _get_provider_kwargs override discards the mutable config.toml params table, so the per-model extra_body entry that generate-config.ts writes for the managed Nemotron Ultra IDs never reached the model constructor. Managed dcode runs could then return newline-only completions when a turn combined reasoning and tool calls. Derive extra_body.chat_template_kwargs.force_nonempty_content from a language-local Ultra ID set inside the patched resolver, mirroring the use_responses_api handling. A model name can enable exactly this reviewed template argument and nothing else; credentials, endpoint, and the rest of the request shape stay fixed. Register the patcher in the model ID drift test and prove the resolver contract in a focused managed-model-params patch test. Fixes NVIDIA#7441 Signed-off-by: vyncint <vyncint@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe managed Deep Agents patch adds Ultra-specific provider parameters and a bounded JSON boundary for non-interactive execution. Tests and profile checks validate resolver behavior, allowlist synchronization, blocked providers, fail-closed networking, and execution results. ChangesManaged Deep Agents runtime contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run_non_interactive
participant JSONBoundary
participant DeepAgentsCode
participant stdout
run_non_interactive->>JSONBoundary: output_format=json
JSONBoundary->>DeepAgentsCode: run with quiet and stream settings
DeepAgentsCode->>JSONBoundary: assistant text and completion metadata
JSONBoundary->>stdout: single bounded JSON envelope
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The patch design and local suites look correct, but the accepted issue’s runtime criterion is not yet proved by the required E2E. Check 03 currently exercises upstream ModelConfig TOML rather than the installed patched _get_provider_kwargs, and the later inference run occurs only after re-onboarding away from the initial Ultra model. Please add bounded evidence that the installed patch returns the exact managed contract for both Ultra IDs (while unrelated/OpenRouter shaping stays rejected), then run an Ultra reasoning/tool turn before re-onboarding and reject a whitespace-only persisted response without repository-level retries. An explicit maintainer waiver for that acceptance criterion would be the alternative. Refresh and exact-head receipts/CI/E2E are still required afterward.
The live DCode check asserted ModelConfig.get_kwargs against config.toml. The hardened managed resolver never consumes that params table, so the assertion passed with or without the NVIDIA#7441 fix and could not accept the issue's runtime criterion. Call the installed deepagents_code.config._get_provider_kwargs directly and pin the whole returned contract: both managed Ultra IDs are shaped under the OpenAI adapter, the same IDs stay unshaped on the managed OpenRouter adapter, a neighbouring Nemotron generation and an unrelated model are unshaped, unsupported providers still raise ModelConfigError, and each call returns a fresh contract. A socket guard fails the block closed on any network use, so the check stays inference-free. Mirror the same matrix in the unit test and bind the live check to the installed resolver, so it cannot regress to config round-trip evidence. Signed-off-by: vyncint <vyncint@users.noreply.github.com>
|
Thanks — both points were right, and I could reproduce the first one. On check 03: it asserted Pushed 4d35373. Check 03 now also calls the installed
Local results: 73 tests pass across the four affected files. Negative control: removing the On the Ultra turn before re-onboarding: you are right that none exists today. Check 03 is inference-free by construction, and check 04 takes model_a Ultra to model_b Receipt refreshed to 4d35373, and the PR body now states plainly what is proven and what is still open. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact head 87ba6b682790e6d8f8eabb8d80e6f2614a1ceab0 against base d4a859a886f36da3ecf953edf6b5121c1ea84842.
Verdict: PASS — no findings. The managed resolver adds one fixed request-shaping argument only for the two reviewed Ultra model IDs on the OpenAI adapter. It still derives the synthetic credential and inference.local endpoint from the root-owned managed boundary, does not consume mutable TOML parameters, rejects unsupported providers, and returns fresh nested state on every call.
Nine-category result:
- Secrets and credentials — PASS: no secret is added or logged; the literal credential remains the existing synthetic in-sandbox value.
- Input validation and sanitization — PASS: behavior is gated by an immutable exact-ID
frozenset; near-miss models andNoneremain unshaped. - Authentication and authorization — PASS: no authorization surface changes; provider restrictions remain fail-closed.
- Dependencies — PASS: no dependency or lockfile change.
- Error handling and logging — PASS: unsupported providers keep raising
ModelConfigError; no sensitive diagnostics are added. - Cryptography and data protection — PASS: no cryptographic or data-at-rest behavior changes.
- Configuration and security headers — PASS: no network, container, CORS, port, or privilege configuration changes.
- Security testing — PASS: focused and installed-sandbox contracts cover both allowed IDs, OpenRouter and unrelated-model exclusions, blocked providers, immutable allowlisting, fresh-object isolation, and fail-closed network use.
- System security — PASS: the root-owned patch supplies only the reviewed argument and cannot widen credentials, endpoints, providers, or arbitrary request parameters.
Files reviewed: agents/langchain-deepagents-code/patch-managed-deepagents-code.py, agents/langchain-deepagents-code/dependency-review.md, test/e2e/e2e-cloud-experimental/checks/03-deepagents-code-nemotron-ultra-profile.sh, test/langchain-deepagents-code-managed-model-params.test.ts, and test/langchain-deepagents-code-nemotron-profile-plugin.test.ts.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer acceptance waiver for exact head 87ba6b682790e6d8f8eabb8d80e6f2614a1ceab0.
Waived criterion: only the issue acceptance item requesting a bounded repeated managed dcode inference turn that returns non-empty Nemotron Ultra content without repository-side retries.
Justification: the installed-runtime check now invokes deepagents_code.config._get_provider_kwargs inside the real DCode sandbox and pins the complete request-shaping contract for both managed Ultra IDs, both provider paths, adjacent and unrelated models, immutable allowlist behavior, fresh-return behavior, unsupported-provider rejection, and zero network use. Focused tests independently exercise the same exact-ID matrix. The change is limited to deterministic request shaping; check 04 deliberately re-onboards to GPT-5.5 before its live turn, so adding an Ultra inference turn would create a new credential-bearing E2E surface beyond this patch.
This waiver does not waive E2E / PR Gate, protected fork authorization, ordinary CI, exact-head review, or any failing product assertion. Those hard gates must still pass before approval.
<!-- markdownlint-disable MD041 --> ## Summary Deep Agents Code runtime changes could produce an empty deterministic E2E plan even when the PR Review Advisor selected the existing DCode typed target. Risk-plan v6 now selects that target directly from trusted changed-file paths, so a fork revision cannot pass E2E coordination without entering the protected approval path. ## Related Issue Related to #7463 ## Changes - Select `ubuntu-repo-cloud-langchain-deepagents-code` for non-documentation runtime changes under `agents/langchain-deepagents-code/` and for the existing headless-inference live check. - Keep documentation and ordinary test changes alone at tier 0. - Advance the deterministic risk-plan version to 6 because selector semantics changed. - Add an exact regression for #7463's four changed paths and exercise the fork controller path with the managed DCode runtime patch. - Document the expanded selector boundary and empty-plan prevention in `test/e2e/README.md`. ## 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: [Nine-category security review](#7517 (comment)) passed exact head `ed8eab5ed65f9bd539a0221aadf54a8de6705d91` against base `e833bd863f0b06be97bedaf5343188ee19923384` with no findings; target selection remains allowlisted and exact-plan-bound, and the Hermes image is pinned to the trusted published remediation digest. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `test/e2e/README.md` accurately documents DCode runtime target selection, explicit protected credentialed-E2E approval for risky fork plans, and empty-plan behavior. The immutable Hermes digest refresh and conflict-free upstream retry-history merge require no additional user documentation. Changed text follows `WRITING.md`; `git diff --check` passes. - Agent: Codex Desktop documentation writer subagent <!-- docs-review-head-sha: ed8eab5 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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 — command/result or justification: the combined risk-plan, fork-controller, advisor, security-boundary, and typed-target suite passed 166 tests; the affected E2E-support workflow boundary passed 31 tests. After the current-main refresh, four focused integration files passed 125 tests and the final test commit's pre-commit hooks passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Pull-request CI will provide the applicable broad gate. - [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: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated E2E gate guidance for when Deep Agents Code runtime changes select the typed end-to-end target, including the headless inference requirement and credentialed E2E approval behavior. * **Bug Fixes** * Refined deterministic PR risk-plan targeting so relevant managed runtime changes select the canonical typed target, while documentation-only or test-only changes no longer do. * **Tests** * Updated the deterministic risk-plan version assertion and added coverage for canonical Deep Agents typed target selection scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Security and maintainer review — exact head 56b6b7d2705d4750157fe4109219802326cc46fb: PASS across all nine categories.
- Credential and secret handling: PASS. The resolver retains the existing synthetic managed credential and managed base URL; no secret-bearing value or persistence path is added.
- Input validation and injection resistance: PASS. Request shaping is gated by an immutable exact-ID
frozensetcontaining only the two reviewed Ultra model IDs, applies only to the managed OpenAI adapter, and leaves near-miss, unrelated, provider-only, and OpenRouter resolution unshaped. - Authentication and authorization: PASS. No authentication, permission, identity, or reviewer-routing behavior changes.
- Dependencies and supply chain: PASS. No package, image dependency, lockfile, download, or external source changes. The dependency-boundary note records both supply points and the removal condition.
- Error handling and information exposure: PASS. Unsupported providers continue to fail closed with
ModelConfigError; no sensitive data is added to diagnostics. - Cryptography: PASS / not applicable. No cryptographic behavior changes.
- Configuration and infrastructure: PASS. The root-owned patch deliberately does not consume mutable
config.tomlparameters; the managed credential, endpoint, provider restrictions, and remaining constructor contract stay fixed. - Security testing: PASS. Exact focused resolver/profile/image/E2E-support validation passes 73/73 and covers both allowed IDs, near-miss/unrelated IDs, OpenRouter, blocked providers, immutable allowlist state, fresh nested return objects, and a zero-network guard.
npm run check:diff, CLI and checked-JavaScript typechecks, shell syntax, and Python compilation pass. - System security and mutable-state safety: PASS. Every resolver call constructs fresh nested
extra_bodystate; mutation of one result cannot poison later calls, and installed-runtime evidence calls the actual patched resolver.
The only new commit is a signed mechanical merge of current main; the intended five-file product diff is unchanged. All eight PR commits are GitHub Verified. The documentation-writer review passed exact head 56b6b7d against AGENTS blob be20a0952410431f1039cb893d2b9168d2ceacd8 with result docs-updated.
The previously recorded maintainer waiver is carried forward without expansion. It remains limited to the repeated managed Ultra inference turn and does not waive protected E2E / PR Gate, ordinary CI, exact-head review, or any product assertion. The older human changes-requested review remains in force for that reviewer to resolve; this review does not dismiss or override it.
|
Exact-head ordinary CI is green, but credentialed E2E is blocked by repository environment configuration rather than PR code.
No selected E2E ran and no repository credential was exposed. A repository admin must configure required reviewers for that environment; the workflow then requires a new PR head and fresh CI before the exact plan can be approved. I have not weakened or changed the environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
agents/langchain-deepagents-code/patch-managed-deepagents-code.py (4)
946-953: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPatcher does not fail closed on the four newly required upstream symbols.
_write_text,_write_newline,generate_thread_id, and_run_agent_loopare captured at import time, but_require_functions(paths["non_interactive"], ...)still only requiresrun_non_interactiveand_run_startup_command. If upstream renames or relocates any of them, patching andcompile()both succeed and the failure appears as an import-timeNameErrorin the managed CLI. Extend the pre-patch assertion so drift is rejected at patch time.🛡️ Proposed fix (outside the reviewed range, near Line 1976)
_require_functions( paths["non_interactive"], texts["non_interactive"], - {"run_non_interactive", "_run_startup_command"}, + { + "run_non_interactive", + "_run_startup_command", + "_write_text", + "_write_newline", + "generate_thread_id", + "_run_agent_loop", + }, )As per path instructions, "Preserve deny-by-default behavior, least privilege, redaction, and fail-closed handling."
🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 946 - 953, Extend the pre-patch _require_functions assertion for paths["non_interactive"] to also require _write_text, _write_newline, generate_thread_id, and _run_agent_loop. Keep the existing run_non_interactive and _run_startup_command requirements, ensuring any missing or relocated upstream symbol causes patching to fail before managed CLI import-time execution.Source: Path instructions
889-944: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPipe descriptors leak if
os.dup(1)fails.
read_fd/write_fdare created before thetry, so an error fromos.dup(1)(Line 921) leaves both open with no cleanup path.🛡️ Proposed fix
- saved_stdout_fd = _nemoclaw_os.dup(1) + try: + saved_stdout_fd = _nemoclaw_os.dup(1) + except BaseException: + _nemoclaw_os.close(read_fd) + _nemoclaw_os.close(write_fd) + raise redirected = False🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 889 - 944, Update _nemoclaw_capture_process_stdout so failures from _nemoclaw_os.dup(1), as well as other setup failures before the drain thread owns cleanup, close both pipe descriptors. Structure the setup cleanup around saved_stdout_fd and the redirected state while preserving the existing restoration, drain signaling, and thread-join behavior after successful setup.
988-999: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse one status string for the timeout outcome.
exit_code == 124returnsturn_limithere, but thewait_forpath returnstimeoutfor the same case. Pick a single name and use it in both places.🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 988 - 999, Update _nemoclaw_json_status so the exit_code == 124 branch returns the same timeout status string used by the wait_for path. Keep the associated exit code and all other status mappings unchanged.Source: Coding guidelines
1071-1076: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the timeout handler on an armed deadline. When
timeout_secondsisNone, this also catches agent-raisedTimeoutErrorand logsafter Nones.; only treat it as a timeout whenwait_foris active.🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 1071 - 1076, Update the TimeoutError handler around the wait_for flow so it handles the exception as a process timeout only when timeout_seconds is not None, meaning a deadline is armed. Preserve agent-raised TimeoutError behavior when no deadline is configured, and keep the existing timeout status, exit code, and message for actual deadline expirations.
🧹 Nitpick comments (2)
agents/langchain-deepagents-code/patch-managed-deepagents-code.py (2)
2005-2029: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the call/timeout marker counts on the text you actually mutate.
Counts are checked on
texts["main"], while the replacements are applied totransformed["main"]afterMAIN_PATCHinsertion. Re-checking ontransformed["main"]keeps the single-occurrence guarantee tied to the mutated 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 2005 - 2029, Update the marker-count validations in the transformation flow to inspect transformed["main"] after MAIN_PATCH is inserted, rather than texts["main"]. Ensure NON_INTERACTIVE_CALL_MARKER and NON_INTERACTIVE_TIMEOUT_MARKER each occur exactly once in the text that will be mutated, before applying their replacements.
956-968: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winContext-scoped gating can silently drop assistant text.
Any
_write_textcall that happens outside the JSON run's context (plain worker thread, executor callback, or a task created beforeset()) takes the original-writer branch, lands in the suppressed stdout guard, and is dropped fromresponsewhile only flippingunexpected_stdout. For a single non-interactive process a module-level run holder is context-independent and simpler. Please confirm streaming text always reaches this writer on the managed run's context, ideally with a negative-path test that asserts no assistant text is lost.As per path instructions, "Require negative-path tests that prove the boundary rejects bypasses".
🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py` around lines 956 - 968, The context-local gating in _write_text can drop assistant text arriving outside _nemoclaw_json_run, so use a module-level active-run holder for the managed process instead of relying on context propagation. Update _write_text and _write_newline to consult that holder while preserving original-writer behavior when no run is active, and add a negative-path test proving bypassed or non-contextual streaming text is retained in the response rather than only setting unexpected_stdout.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.
Outside diff comments:
In `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py`:
- Around line 946-953: Extend the pre-patch _require_functions assertion for
paths["non_interactive"] to also require _write_text, _write_newline,
generate_thread_id, and _run_agent_loop. Keep the existing run_non_interactive
and _run_startup_command requirements, ensuring any missing or relocated
upstream symbol causes patching to fail before managed CLI import-time
execution.
- Around line 889-944: Update _nemoclaw_capture_process_stdout so failures from
_nemoclaw_os.dup(1), as well as other setup failures before the drain thread
owns cleanup, close both pipe descriptors. Structure the setup cleanup around
saved_stdout_fd and the redirected state while preserving the existing
restoration, drain signaling, and thread-join behavior after successful setup.
- Around line 988-999: Update _nemoclaw_json_status so the exit_code == 124
branch returns the same timeout status string used by the wait_for path. Keep
the associated exit code and all other status mappings unchanged.
- Around line 1071-1076: Update the TimeoutError handler around the wait_for
flow so it handles the exception as a process timeout only when timeout_seconds
is not None, meaning a deadline is armed. Preserve agent-raised TimeoutError
behavior when no deadline is configured, and keep the existing timeout status,
exit code, and message for actual deadline expirations.
---
Nitpick comments:
In `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py`:
- Around line 2005-2029: Update the marker-count validations in the
transformation flow to inspect transformed["main"] after MAIN_PATCH is inserted,
rather than texts["main"]. Ensure NON_INTERACTIVE_CALL_MARKER and
NON_INTERACTIVE_TIMEOUT_MARKER each occur exactly once in the text that will be
mutated, before applying their replacements.
- Around line 956-968: The context-local gating in _write_text can drop
assistant text arriving outside _nemoclaw_json_run, so use a module-level
active-run holder for the managed process instead of relying on context
propagation. Update _write_text and _write_newline to consult that holder while
preserving original-writer behavior when no run is active, and add a
negative-path test proving bypassed or non-contextual streaming text is retained
in the response rather than only setting unexpected_stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e818379-fb72-48a5-b65c-26b990615a73
📒 Files selected for processing (2)
agents/langchain-deepagents-code/dependency-review.mdagents/langchain-deepagents-code/patch-managed-deepagents-code.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/langchain-deepagents-code/dependency-review.md
|
Exact-head protected E2E is complete for
The trusted gate is still red because the unrelated Hermes half of the generic Run: https://github.com/NVIDIA/NemoClaw/actions/runs/30499877079 The controller has terminalized the first-attempt verdict, so rerunning the child alone would not turn the required gate green. I am not submitting a Changes Requested review and I am not asking for a code change based on this unrelated Hermes timeout. The current head remains unapproved while the required gate is red; a future legitimate head/base refresh can run the same deterministic plan again, or a maintainer can record an explicit non-success waiver through the repository's normal release process. |
|
Hi @cjagwani, Thanks for running that down and for the detail. Understood on all three points: nothing to change in the code, rerunning the child will not help, and the head stays unapproved while the generic matrix is red. Since main is still at d52d459, there is no legitimate head or base refresh available to me right now, and I would rather not push a no-op commit just to retrigger the gate. Is the waiver route something you can start, or should I wait for main to move and let the plan run again on a real rebase? |
|
Maintainer gate follow-up at exact head |
|
Retriggered at the current base: new exact head On why this had to mint a new head/base pair instead of re-running in place: the empty
State at the new head, from
So two steps remain on your side: Approve and run the fork The reviewed diff is unchanged. Separately, and independent of the gate: the 2026-07-26 Changes Requested review, from two head refreshes ago, still sets |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Exact-head security review for 380daff4c96f047bc9c176c5cb3812d9c3209b8b: PASS — no findings.
- Secrets and credentials — PASS. The resolver retains the fixed synthetic credential and managed inference URL; no real credential or mutable credential source is introduced.
- Input validation — PASS. Request shaping is gated by exact membership in an immutable two-ID
frozenset; providers remain allowlisted toopenaiandopenrouter, with the new argument limited toopenai. - Authentication and authorization — PASS. No authentication or authorization boundary changes.
- Dependencies — PASS. No dependency or version change; the existing hash-locked boundary and removal condition remain documented.
- Error handling and logging — PASS. The product diff adds no exception reflection, logging, or sensitive-output path.
- Cryptography and data protection — PASS. Not applicable; no cryptographic operation changes.
- Configuration and security posture — PASS. Mutable
config.tomlprovider classes, credentials, endpoints, and params remain ignored by the hardened resolver. - Security testing — PASS. Exact IDs, near-miss IDs, OpenRouter non-shaping, unsupported providers, immutable allowlist, fresh-return/mutation poisoning, allowlist drift, installed-runtime behavior, and a socket-denial guard are covered.
- Holistic posture — PASS. The new nested request body is freshly allocated per call and cannot widen provider/model authority or alter the managed credential/endpoint contract. The latest merge from
mainhad no conflict resolution.
Product scope is established by accepted bug #7441. This is a comment-only exact-head security result, not an approval: ordinary CI and the required E2E gate are still pending, and the stale Changes Requested review must be cleared or superseded by its author. I will continue monitoring without submitting Changes Requested.
|
Exact-head gate update for
No code change is requested by this update. Remaining work is safe exact-head E2E/coordination evidence (or an explicit maintainer-approved safe waiver/backfill), unambiguous substantive |
|
Ack on
The checker is binding the newer Observer: expires Two asks:
|
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed current head 380daff in light of the earlier requested re-review. The installed-runtime contract now calls the patched resolver directly and covers the exact Ultra IDs, provider exclusions, immutable allowlist, fresh-return behavior, unsupported-provider rejection, and zero-network boundary. The remaining repeated live Ultra-turn criterion has an explicit maintainer waiver without expanding any other gate. Ordinary CI, typecheck, focused tests, and security review pass. The E2E / PR Gate failure is a six-hour trusted-verdict coordination timeout before product jobs ran, not a failing product assertion; that required gate still needs a successful rerun or repository-owned acceptance before merge. I found no remaining blocking defect attributable to this PR.
|
@apurvvkumaria @cjagwani — the observer at |
Summary
The managed Deep Agents Code provider resolver dropped the reviewed Nemotron Ultra
extra_body.chat_template_kwargs.force_nonempty_contentrequest parameter, so manageddcoderuns against the Ultra models could persist newline-only completions withfinish_reason: stop. The patched_get_provider_kwargsnow derives that argument from a language-local Ultra ID set, mirroring its existinguse_responses_apihandling, while still never consuming the mutableconfig.tomlparams table.Related Issue
Fixes #7441
Changes
agents/langchain-deepagents-code/patch-managed-deepagents-code.py: the managed_get_provider_kwargsoverride suppliesextra_body = {"chat_template_kwargs": {"force_nonempty_content": True}}for the two managed Ultra model IDs on theopenaiadapter, from a new language-local_NEMOCLAW_NEMOTRON_ULTRA_MODEL_IDSconstant. A model name can enable exactly this reviewed template argument and nothing else: the synthetic credential, managed base URL, anduse_responses_apihandling are unchanged,--model-paramsstays disabled, and the mutable TOML params table is still never read, so arbitrary or unsupported parameters keep failing closed.test/e2e/e2e-cloud-experimental/checks/03-deepagents-code-nemotron-ultra-profile.sh: the check assertedModelConfig.get_kwargsagainstconfig.toml, which the hardened resolver never consumes, so that assertion passed with or without this fix. It now also calls the installeddeepagents_code.config._get_provider_kwargsand pins the whole returned contract: both managed Ultra IDs are shaped on theopenaiadapter, the same IDs stay unshaped on the managedopenrouteradapter, a neighbouring Nemotron generation and an unrelated model are unshaped, unsupported providers still raiseModelConfigError, the allowlist is an immutablefrozenset, and each call returns a fresh contract that a previous caller cannot poison. A socket guard fails the block closed on any network use, so the check stays inference-free.test/langchain-deepagents-code-managed-model-params.test.ts: focused patch test proving both Ultra IDs receive the argument and that unrelated models, provider-only resolution, and theopenrouteradapter do not, now covering the same matrix as the live check. It lives in a new file becausetest/langchain-deepagents-code-direct-module-patch.test.tsis at the test-file size budget. Detection gap: the existing direct-module assertions called_get_provider_kwargswithoutmodel_name, so the dropped per-model argument was invisible to them; this test closes that gap and fails without the patcher change. A second case binds the live check to the installed resolver so it cannot regress to config round-trip evidence.test/langchain-deepagents-code-nemotron-profile-plugin.test.ts: registers the patcher in the language-local Ultra model ID drift test, so every production consumer of the two IDs must stay in sync.agents/langchain-deepagents-code/dependency-review.md: records the second supply point and its removal condition under "Managed Ultra compatibility workarounds".The upstream
deepagents-code==0.1.34resolver merges per-modelparamsfromconfig.tomlwhenmodel_nameis passed, which is how the entry written bygenerate-config.tswas intended to reach the constructor; the hardening override intentionally refuses that mutable source, so the reviewed argument has to come from the root-owned patch itself.Maintainer waiver recorded: exact-head acceptance review waives only the bounded repeated managed
dcodelive Nemotron Ultra turn. The installed-runtime contract, exact model/provider matrix, immutable allowlist, fresh-return behavior, unsupported-provider rejection, and zero-network guard remain required evidence. This waiver does not waive the protectedE2E / PR Gate, which must still pass before approval.Type of Change
Quality Gates
docs/page describes the managed resolver or the buggy behavior; the in-repodependency-review.mdboundary record is updated in this diff.56b6b7d2705d4750157fe4109219802326cc46fbacross all nine categories; model-triggered request shaping is exact-ID allowlisted, does not consume mutable parameters, and adds no credential or dependency surface.Documentation Writer Review
docs-updatedagents/langchain-deepagents-code/dependency-review.mdand the exact five-file diff at56b6b7d27. The review confirms the exact two-ID OpenAI resolver scope, unchanged OpenRouter and unrelated-model contracts, installed inference-free proof, source boundaries, lifecycle and two-supply-point removal condition, and writing rules. Focused resolver/profile/image/E2E-support validation passed 73/73;build:cli,typecheck:cli,check:diff,bash -n,py_compile, and diff check passed. No further user-doc or code-comment edit is required.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 unavailable —npm run check:diffpassed at refreshed head56b6b7d27; the normal non-force push then passed plugin and CLI TypeScript plus tag-version pre-push hooks.vitest runoverlangchain-deepagents-code-managed-model-params,langchain-deepagents-code-nemotron-profile-plugin,langchain-deepagents-code-image, ande2e/support/platform-parity-cloud-experimental→ 73 passed, 0 failed. Negative control: removing theextra_bodyblock from the patcher makes the resolver test fail with the exact missing-key diff, and restoring it passes. The check script passesbash -n, and its extracted Python heredoc passespy_compile. The sandbox E2E itself needsNVIDIA_INFERENCE_API_KEYon NVIDIA's runners and was not run here.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: Vyncint Ng 115854244+vyncint@users.noreply.github.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
Tests
Documentation