test(e2e): hermeticize DCode fresh re-onboard inference#6581
test(e2e): hermeticize DCode fresh re-onboard inference#6581apurvvkumaria wants to merge 2 commits into
Conversation
Co-authored-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
area: e2e
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 |
📝 WalkthroughWalkthroughThe fresh re-onboard e2e check script now starts hermetic compatible inference, uses that endpoint during re-onboarding, and verifies the endpoint was consulted. The cloud-experimental runner withholds API keys for that script, and the e2e support tests cover the new key handling and request behavior. ChangesFresh re-onboard hermetic inference flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Script as 04-deepagents-code-fresh-reonboard.sh
participant Hermetic as HermeticCompatibleInference
participant Onboard as onboard --fresh
Script->>Hermetic: nemoclaw_e2e_start_hermetic_compatible_inference
Hermetic-->>Script: startup result
Script->>Onboard: run with NEMOCLAW_ENDPOINT_URL
Onboard-->>Script: re-onboard complete
Script->>Hermetic: nemoclaw_e2e_assert_hermetic_compatible_inference_used
Script->>Hermetic: cleanup() on EXIT trap
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor (Nemotron Ultra) — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Advisor SDK provider error: session: 429 status code (no body); turn: analysis: 429 status code (no body) |
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorFailed: Target advisor SDK provider error: session: omitted required tool result(s): e2e_target_metadata, e2e_target_changed_files, e2e_target_risk_plan, e2e_target_git_diff, e2e_target_response_schema; turn: target-analysis: 429 status code (no body) |
PR Review Advisor — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
E2E Target Results — ❌ Some jobs failedRun: 29033459812
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Required exact-head live evidence failed deterministically on 22614a84fb4b3a11719112e278f597e16d9fccd6 (run 29033459812, job 86172239210).
The changed fresh-re-onboard check reached its new hermetic assertion, but the fake endpoint log contained only GET /v1/models with authorizationSent:false and no inference POST. That GET is the fake server helper's own readiness probe, so the real nemoclaw onboard --fresh path did not exercise the hermetic endpoint at all. The support test currently masks this because its fake nemoclaw executable explicitly manufactures a POST that production onboarding did not make.
Please make the acceptance path prove the real behavior—for example, explicitly drive a managed inference request after re-onboard and then assert the authenticated fake-endpoint POST, or redefine and test the intended onboarding contract without a synthetic fixture-only POST. Then rerun ubuntu-repo-cloud-langchain-deepagents-code. I am not retrying the unchanged head because this failure is code/test-specific, not infrastructure.
Separately, the remaining static-checks failure is the known external release-fetch 504 and is not this review blocker.
Treat the authenticated models probe as provider-validation evidence. Do not claim that onboarding runs inference. Co-authored-by: Ho Lim <subhoya@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 29034363838
|
![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
🧹 Nitpick comments (1)
test/e2e/lib/hermetic-compatible-inference.sh (1)
46-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated request-log parsing logic.
This function repeats the same file-existence check, JSON-line parsing, and error-throwing structure as
nemoclaw_e2e_assert_hermetic_compatible_inference_used(lines 69-96). Consider extracting a shared helper (bash function returning the parsed entries, or a small shared Node snippet) to avoid the two implementations drifting.🤖 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/e2e/lib/hermetic-compatible-inference.sh` around lines 46 - 68, The request-log parsing and validation logic in nemoclaw_e2e_assert_hermetic_compatible_endpoint_consulted duplicates the same flow used by nemoclaw_e2e_assert_hermetic_compatible_inference_used, so extract the shared file-existence check and JSONL parsing into a common helper and have both functions call it. Keep the endpoint-specific consultation assertion in nemoclaw_e2e_assert_hermetic_compatible_endpoint_consulted, but move the repeated Node parsing/error handling into one reusable bash or Node helper to prevent drift.
🤖 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/e2e/lib/hermetic-compatible-inference.sh`:
- Line 27: The readiness probe request to /v1/models is being counted in the
shared request log with auth: "missing", which causes the hermetic inference
auth assertion to fail. Update the startup/test flow around
NEMOCLAW_FAKE_OPENAI_REQUIRE_AUTH_MODELS=1 so the readiness probe is excluded
from the request-log auth check, either by filtering that probe in
nemoclaw_e2e_assert_hermetic_compatible_inference_used or by clearing the log
after startup. Use the existing readiness probe and request log handling in
hermetic-compatible-inference.sh to locate the fix.
---
Nitpick comments:
In `@test/e2e/lib/hermetic-compatible-inference.sh`:
- Around line 46-68: The request-log parsing and validation logic in
nemoclaw_e2e_assert_hermetic_compatible_endpoint_consulted duplicates the same
flow used by nemoclaw_e2e_assert_hermetic_compatible_inference_used, so extract
the shared file-existence check and JSONL parsing into a common helper and have
both functions call it. Keep the endpoint-specific consultation assertion in
nemoclaw_e2e_assert_hermetic_compatible_endpoint_consulted, but move the
repeated Node parsing/error handling into one reusable bash or Node helper to
prevent drift.
🪄 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: e300f726-6887-41fe-a992-f1ff65623357
📒 Files selected for processing (4)
test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.shtest/e2e/lib/hermetic-compatible-inference.shtest/e2e/lib/openai-compatible-api-proof.shtest/e2e/support/platform-parity-cloud-experimental.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/support/platform-parity-cloud-experimental.test.ts
- test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh
| export FAKE_OPENAI_API_KEY="$fake_key" | ||
| export FAKE_OPENAI_REQUIRE_AUTH=1 | ||
| export NEMOCLAW_FAKE_OPENAI_REQUESTS_FILE="$FAKE_OPENAI_REQUESTS_FILE" | ||
| export NEMOCLAW_FAKE_OPENAI_REQUIRE_AUTH_MODELS=1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'FAKE_OPENAI_REQUESTS_FILE' test/e2e --type sh --type ts
rg -n 'nemoclaw_e2e_assert_hermetic_compatible_inference_used|nemoclaw_e2e_assert_hermetic_compatible_endpoint_consulted' test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh -A2 -B2Repository: NVIDIA/NemoClaw
Length of output: 1364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## hermetic-compatible-inference.sh\n'
cat -n test/e2e/lib/hermetic-compatible-inference.sh | sed -n '1,220p'
printf '\n## openai-compatible-api-proof.sh\n'
cat -n test/e2e/lib/openai-compatible-api-proof.sh | sed -n '1,220p'
printf '\n## 04-deepagents-code-fresh-reonboard.sh relevant section\n'
cat -n test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh | sed -n '230,320p'Repository: NVIDIA/NemoClaw
Length of output: 13242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## fake-openai-compatible.ts outline\n'
ast-grep outline test/e2e/fixtures/fake-openai-compatible.ts --view expanded
printf '\n## auth/request logging references\n'
rg -n 'auth|requestsFile|REQUESTS_FILE|require_auth_models|requireAuthModels|models' test/e2e/fixtures/fake-openai-compatible.ts -n -A3 -B3
printf '\n## request log write path\n'
cat -n test/e2e/fixtures/fake-openai-compatible.ts | sed -n '1,260p'Repository: NVIDIA/NemoClaw
Length of output: 11824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## server script outline\n'
ast-grep outline test/e2e/lib/fake-openai-compatible-api.mts --view expanded
printf '\n## logging and readiness references\n'
rg -n 'requestsFile|auth:|authorizationSent|requireAuthModels|/v1/models|writeFileSync|appendFileSync|readiness' test/e2e/lib/fake-openai-compatible-api.mts -A4 -B4
printf '\n## relevant server body\n'
cat -n test/e2e/lib/fake-openai-compatible-api.mts | sed -n '1,260p'Repository: NVIDIA/NemoClaw
Length of output: 15843
Exclude the readiness probe from the request-log auth check. NEMOCLAW_FAKE_OPENAI_REQUIRE_AUTH_MODELS=1 makes the unauthenticated /v1/models readiness request record auth: "missing" in the shared requests file, and nemoclaw_e2e_assert_hermetic_compatible_inference_used fails on any missing-auth entry. Filter that probe out or clear the log after startup.
🤖 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/e2e/lib/hermetic-compatible-inference.sh` at line 27, The readiness
probe request to /v1/models is being counted in the shared request log with
auth: "missing", which causes the hermetic inference auth assertion to fail.
Update the startup/test flow around NEMOCLAW_FAKE_OPENAI_REQUIRE_AUTH_MODELS=1
so the readiness probe is excluded from the request-log auth check, either by
filtering that probe in nemoclaw_e2e_assert_hermetic_compatible_inference_used
or by clearing the log after startup. Use the existing readiness probe and
request log handling in hermetic-compatible-inference.sh to locate the fix.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
The replacement head e4165b4d63cdaf1b234b3f1d3120c85cddeb06b3 still has the same acceptance-test fidelity blocker. Required exact-head run 29034363838 (job 86175352196) failed deterministically after the first seven assertions. Its complete fake-endpoint record was only GET /v1/models, auth: missing, authorizationSent: false.
That request is the helper's startup readiness probe (openai-compatible-api-proof.sh), which now intentionally accepts the 401. Production onboard --fresh still made no request to the hermetic endpoint. The support test masks this by changing its fake nemoclaw executable to manufacture an authenticated GET /models, just as the previous head manufactured a POST. Replacing the synthetic request type does not validate production behavior.
Please exercise a real production boundary after re-onboard, or accurately redefine the contract without asserting a network consultation production onboarding does not perform. The fixture must not create the sole evidence itself. CodeRabbit's current major finding about the readiness record is related, but merely clearing/filtering readiness would leave zero production requests and would not fix the live failure.
Both commits are Verified and DCO-compliant; this request is about correctness/test fidelity. I am not retrying the unchanged head.
|
Closing after exact-head live validation exposed a conceptual mismatch with #5747. The only request in the artifact is the helper readiness probe: an unauthenticated GET /v1/models that correctly receives 401 when model-list auth is enabled. Production re-onboard intentionally skips host-side validation for the sandbox-internal host and makes no endpoint request, so accepting or authenticating that readiness probe would make fixture setup the sole false-positive evidence. More importantly, this ordered target runs check #7 after #4 in the same sandbox. #4 changes compatible-endpoint to the ephemeral fake and stops it on EXIT; #7 then requires /v1/models HTTP 200 plus a real dcode -n PONG. The route would be dead, and even a kept-alive default fake response would replace the required live-inference contract. A correct Pattern-C conversion needs an isolated sandbox/target or a target-level lifecycle redesign, not a narrow patch here. Ho Lim remains credited as co-author on the preserved investigation commit. |
2 participants
Summary
Move the Deep Agents Code fresh re-onboard live check to the existing hermetic OpenAI-compatible endpoint instead of forwarding a hosted inference key into that path. This is a clean, verified replacement for #6530; Ho Lim's original implementation is preserved and credited as co-author.
Related Issue
Supersedes #6530 because its published branch conflicts with current
mainand its PR description cannot satisfy the current DCO declaration gate.Changes
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablee2e-supportvalidation passed 25/25 tests.npm run check:diff, CLI build, and CLI type-check passed on the replacement commit.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests