Skip to content

test(e2e): register e2e-live source require hook and surface cloud-onboard dcode identity failure#6343

Merged
cv merged 5 commits into
mainfrom
fix/e2e-live-source-require-hook
Jul 6, 2026
Merged

test(e2e): register e2e-live source require hook and surface cloud-onboard dcode identity failure#6343
cv merged 5 commits into
mainfrom
fix/e2e-live-source-require-hook

Conversation

@prekshivyas

@prekshivyas prekshivyas commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes two E2E-harness problems surfaced by the E2E dispatch on main:

  1. e2e-live source require hook — the hermes-inference-switch live suite (added in fix(hermes): use OpenAI frontend for custom Anthropic #6335) failed at collection with Cannot find module '../runner' because the e2e-live Vitest project never loaded the typed-source require hook.
  2. cloud-onboard observability — the cloud-experimental check-04 (added in fix(onboard): preserve fresh DCode routing on re-onboard #6332) fails on main with could not read initial dcode identity, but the real dcode identity error is captured into a shell var and discarded, so it can't be diagnosed.

Changes

  • vitest.config.ts: add setupFiles: ["test/helpers/onboard-script-mocks.cjs"] to the e2e-live project (mirrors cli). Registers the typed-source .ts require hook in-process — deliberately not via env.NODE_OPTIONS, so --require never leaks into the real CLI subprocesses live tests spawn.
  • test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh: on both dcode identity reads, print the captured stdout+stderr to stdout (so it lands in result.json) before fail. No change to pass/fail logic — only makes the existing failure observable.

Root cause — #1 (fixed here)

hermes-inference-switch is the first e2e-live suite to import a deep src graph — its helpers import src/lib/inference/config.ts, which transitively loads ollama-runtime-context.ts's runtime require("../runner"). Without the require hook, Node's native CJS resolver can't resolve the extensionless .ts → suite throws at collection (0 tests, ~37s). Only this suite hit it; others drive the CLI as a subprocess and import only fixtures. runner.ts has no circular dependency on the inference graph, so the in-process hook resolves it fully.

Root cause — #2 (observability only; product root cause pending)

cloud-onboard was green on main through 2026-07-06 00:56 UTC and failed on the first main E2E after #6332 landed (19:50 UTC) — #6332 added check-04, which has never passed on main. dcode identity is the NemoClaw wrapper (agents/langchain-deepagents-code/dcode-wrapper.sh); its identity path returns 0 in isolation and #6332 did not modify it, so the non-zero exit is a runtime condition in #6332's new "recreate/verify live identity" onboarding flow. That can't be pinned without the swallowed stderr — which this change surfaces. Product root cause is for the #6332 author to fix once the next run shows the real error.

Type of Change

  • Code change (feature, bug fix, or refactor)

Quality Gates

  • Existing tests cover changed behavior — justification: both changes are E2E-harness config/diagnostics. ci: auto-update release notes on push to main #1: the e2e-live suites exercise the hook — verified the previously-failing hermes-inference-switch suite now collects and all e2e-live suites report 0 collection errors, and the full e2e-all dispatch ran the switch job (hosted) green. feature: custom settings for using build endpoints #2: pure diagnostic output; no pass/fail change.
  • Docs not applicable — justification: internal test-harness config/diagnostics; no user-facing behavior.
  • Sensitive paths changed (onboarding/inference/runner adjacent via test config)
  • Sensitive-path review completed or maintainer-approved waiver recorded — justification: changes are confined to Vitest test-runner setup (vitest.config.ts) and an E2E diagnostic print; no product runtime code path is altered. The require hook is in-process only and does not touch product CLI subprocesses.

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed — note: pre-push tsc-cli skipped for a pre-existing local-only noImplicitAny false-positive in test/helpers/mcp-lifecycle-lock-properties.ts and src/lib/state/mcp-lifecycle-lock-identity.test.ts (unrelated to this diff); CI tsc-cli covers it. Check-04 passed bash -n and the shellcheck/shfmt pre-commit hooks.
  • Targeted behavior tests pass — command/result: NEMOCLAW_RUN_LIVE_E2E=1 npx vitest list --project e2e-live → switch suite collects; whole project 0 collection errors (before the fix it reproduced Cannot find module '../runner'). Full e2e-all dispatch on this branch ran hermes-inference-switch (hosted) green.
  • No secrets, API keys, or credentials committed

Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of live end-to-end test runs by loading the required hook inside the test process, avoiding leakage into real CLI subprocesses.
    • Made identity checks in sandboxed cloud experimental flows more robust, with clearer diagnostics when identity lookup fails.

The e2e-live Vitest project did not load the typed-source require hook
(test/helpers/onboard-script-mocks.cjs), unlike the cli and integration
projects. When a live suite imports a source module whose graph resolves
a sibling via a runtime require of an extensionless .ts file — e.g.
src/lib/inference/ollama-runtime-context.ts's require("../runner"), reached
transitively from the hermes-inference-switch helpers importing
src/lib/inference/config.ts — Node's native CJS resolver cannot find the
module and the suite fails at collection ("Cannot find module '../runner'",
0 tests).

Register the hook via setupFiles rather than NODE_OPTIONS so it stays
in-process and never leaks --require into the real CLI subprocesses the
live tests spawn. Mirrors the cli project.

Verified with `NEMOCLAW_RUN_LIVE_E2E=1 npx vitest list --project e2e-live`:
the switch suite now collects and all e2e-live suites report 0 collection
errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3be9c757-134a-4de6-93f9-00b25361f558

📥 Commits

Reviewing files that changed from the base of the PR and between 2bccc36 and 2fa213f.

📒 Files selected for processing (1)
  • test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh

📝 Walkthrough

Walkthrough

The e2e-live Vitest project now registers a typed-source require hook via setupFiles instead of relying on --require injection. A deepagents re-onboard check script uses an absolute dcode binary path and adds guarded identity capture with diagnostic output printed on failure.

Changes

Vitest configuration and e2e diagnostics

Layer / File(s) Summary
e2e-live setupFiles registration
vitest.config.ts
Adds setupFiles entry for the typed-source require hook to the e2e-live Vitest project, with comments explaining in-process registration to avoid leaking --require into CLI subprocesses.
Absolute dcode path and identity failure diagnostics
test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh
dcode_identity now invokes /usr/local/bin/dcode identity instead of an unqualified dcode; both the initial and post-re-onboard identity captures are guarded, printing a diagnostic: message with captured output before failing with a specific error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: bug-fix

Suggested reviewers: jyaunches, ericksoa

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes both main changes: the e2e-live require hook update and improved cloud-onboard dcode identity failure reporting.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/e2e-live-source-require-hook

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: ubuntu-repo-cloud-langchain-deepagents-code

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No merge-blocking E2E is required because this PR is tests/harness-only and does not change production runtime behavior or user flows. Running the Deep Agents Code registry target is optional to validate the modified live check itself.

Optional E2E

  • ubuntu-repo-cloud-langchain-deepagents-code (high): Useful confidence check for the touched Deep Agents Code live shell check and the e2e-live Vitest harness change. This registry target runs test/e2e/live/registry-targets.test.ts for the Deep Agents Code cloud target, including test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh.

New E2E recommendations

  • None.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: e2e-all
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref>

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • e2e-all: vitest.config.ts changes the shared e2e-live Vitest project setup used by registry-target live dispatches, so run the full supported E2E target fan-out. This also covers the Deep Agents Code fresh re-onboard check change under test/e2e/e2e-cloud-experimental.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref>

Optional E2E targets

  • None.

Relevant changed files

  • test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh
  • vitest.config.ts

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Require hook src/ directory boundary lacks regression test; then add or justify PRA-T1.
Open items: 3 required · 3 warnings · 2 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 5 still apply · 3 new items found

Action checklist

  • PRA-2 Fix: Require hook src/ directory boundary lacks regression test in test/helpers/register-source-require.ts:55
  • PRA-3 Fix: e2e-live require hook registration lacks runtime smoke test in vitest.config.ts:146
  • PRA-4 Fix: Source-of-truth review incomplete for src/ directory boundary guard in test/helpers/register-source-require.ts:55
  • PRA-1 Resolve or justify: Source-of-truth review needed: vitest.config.ts:146 (setupFiles require hook registration)
  • PRA-5 Resolve or justify: e2e-branch-validation project missing parity comment for setupFiles in vitest.config.ts:170
  • PRA-6 Resolve or justify: Diagnostic output on dcode_identity failure should document no-credential assumption in test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:157
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: e2e-live require hook registration lacks runtime smoke test
  • PRA-T5 Add or justify test follow-up: Acceptance clause
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause
  • PRA-7 In-scope improvement: Shorten dcode absolute path comment referencing Dockerfile in test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:40
  • PRA-8 In-scope improvement: Trim e2e-live setupFiles comment to focus on in-process rationale in vitest.config.ts:140

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Required security test/helpers/register-source-require.ts:55 Add test/helpers/register-source-require.test.ts with a case that attempts require("../../../outside-src") and expects the original MODULE_NOT_FOUND error to propagate.
PRA-3 Required tests vitest.config.ts:146 Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() (e.g., require("../../../src/lib/inference/ollama-runtime-context")) and asserts expected exports exist.
PRA-4 Required architecture test/helpers/register-source-require.ts:55 Complete the source-of-truth review: document the invalid state (Node CJS resolver cannot resolve .ts/extensionless sibling imports in deep inference graph), source boundary (src/lib/inference/ runtime require()), source-fix constraint (large refactor to convert runtime require() to ESM), regression test (missing — see PRA-1/PRA-2), and removal condition (when grep -r "require("../" src/lib/inference/" returns no matches).
PRA-5 Resolve/justify architecture vitest.config.ts:170 Add a comment to the e2e-branch-validation project config: 'branch validation only runs brev-e2e.test.ts which uses ESM imports; require hook not needed.'
PRA-6 Resolve/justify correctness test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:157 Add a comment before the diagnostic printf statements confirming dcode identity output never includes credentials (fields: Model, Route, Provider, Endpoint, Sandbox, Harness, Agent, Runtime only).
PRA-7 Improvement correctness test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:40 Shorten comment to: 'Invoke dcode by absolute path; non-login shell PATH lacks /usr/local/bin (see agents/langchain-deepagents-code/Dockerfile).'
PRA-8 Improvement correctness vitest.config.ts:140 Consider trimming the comment to focus on why setupFiles is used over NODE_OPTIONS (in-process, no leak into CLI subprocesses).

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-2 Required — Require hook src/ directory boundary lacks regression test

  • Location: test/helpers/register-source-require.ts:55
  • Category: security
  • Problem: The typed-source require hook restricts resolution to files under src/ directory (lines 55-65) via sourceCandidate.startsWith(sourceRoot). No unit test validates this boundary. If the guard regresses, the hook could transpile files outside src/ in test worker processes, violating least-privilege for test infrastructure.
  • Impact: A regression in the sourceCandidate.startsWith(sourceRoot) check could allow arbitrary file transpilation in test workers. Impact limited to test processes but is a security control gap.
  • Required action: Add test/helpers/register-source-require.test.ts with a case that attempts require("../../../outside-src") and expects the original MODULE_NOT_FOUND error to propagate.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Review test/helpers/register-source-require.ts lines 55-65 (resolveFilename override) — sourceCandidate.startsWith(sourceRoot) check before allowing resolution. Confirm test/helpers/register-source-require.test.ts does not exist.
  • Missing regression test: Add test/helpers/register-source-require.test.ts verifying require("../../../outside-src") is rejected and original MODULE_NOT_FOUND error propagates.
  • Done when: The required change is committed and verification passes: Review test/helpers/register-source-require.ts lines 55-65 (resolveFilename override) — sourceCandidate.startsWith(sourceRoot) check before allowing resolution. Confirm test/helpers/register-source-require.test.ts does not exist.
  • Evidence: Hook's resolveFilename override at lines 55-65 checks sourceCandidate.startsWith(sourceRoot) before allowing resolution. Previous review (PRA-2) identified this gap; remains unaddressed.

PRA-3 Required — e2e-live require hook registration lacks runtime smoke test

  • Location: vitest.config.ts:146
  • Category: tests
  • Problem: The e2e-live project registers the typed-source require hook via onboard-script-mocks.cjs (setupFiles), but no test directly exercises it in the vitest worker process. The openshell-version-pin.test.ts uses require() but spawns a separate Node process with --import tsx; no vitest-worker test imports source modules via require().
  • Impact: Future breakage of the require hook registration in e2e-live could go undetected until a new test imports the deep inference graph. Collection-only verification (vitest list) is insufficient.
  • Required action: Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() (e.g., require("../../../src/lib/inference/ollama-runtime-context")) and asserts expected exports exist.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live --reporter=verbose to verify the project loads; grep test/e2e/live for require() on src/ paths — currently none found.
  • Missing regression test: Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() and validates the hook resolves .ts siblings correctly.
  • Done when: The required change is committed and verification passes: Run NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live --reporter=verbose to verify the project loads; grep test/e2e/live for require() on src/ paths — currently none found.
  • Evidence: PR verification used vitest list --project e2e-live which only checks collection; no test asserts the require hook works at runtime. Previous review (PRA-1/PRA-T1/PRA-T2) identified this gap; remains unaddressed.

PRA-4 Required — Source-of-truth review incomplete for src/ directory boundary guard

  • Location: test/helpers/register-source-require.ts:55
  • Category: architecture
  • Problem: The require hook's src/ boundary guard (lines 55-65) is a localized workaround for Node's inability to resolve .ts/extensionless imports at runtime. The source-of-truth review lacks: invalid state definition, source boundary, why source cannot be fixed here, regression test, and removal condition.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear. Without documented removal condition, the workaround may become permanent technical debt.
  • Required action: Complete the source-of-truth review: document the invalid state (Node CJS resolver cannot resolve .ts/extensionless sibling imports in deep inference graph), source boundary (src/lib/inference/ runtime require()), source-fix constraint (large refactor to convert runtime require() to ESM), regression test (missing — see PRA-1/PRA-2), and removal condition (when grep -r "require("../" src/lib/inference/" returns no matches).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect test/helpers/register-source-require.ts lines 55-65 and src/lib/inference/ollama-runtime-context.ts for runtime require("../runner") pattern. Check if source-of-truth review fields are populated in PR or docs.
  • Missing regression test: Add test/helpers/register-source-require.test.ts verifying require("../../../outside-src") is rejected AND test/e2e/live/source-require-hook-smoke.test.ts verifying a real source module with runtime require() loads correctly.
  • Done when: The required change is committed and verification passes: Inspect test/helpers/register-source-require.ts lines 55-65 and src/lib/inference/ollama-runtime-context.ts for runtime require("../runner") pattern. Check if source-of-truth review fields are populated in PR or docs.
  • Evidence: Lines 55-65 in register-source-require.ts check sourceCandidate.startsWith(sourceRoot). Previous review (PRA-1) marked localized patch analysis as needs_followup.
Review findings by urgency: 3 required fixes, 3 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: vitest.config.ts:146 (setupFiles require hook registration)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — Need: (a) unit test for src/ boundary guard (require("../../../outside-src") rejected), (b) smoke test that a real source module with require("../sibling") loads via hook in e2e-live worker.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PR description root cause ci: auto-update release notes on push to main #1: hermes-inference-switch imports src/lib/inference/config.ts which transitively loads ollama-runtime-context.ts's runtime require("../runner"). Without hook, collection fails with "Cannot find module '../runner'".

PRA-5 Resolve/justify — e2e-branch-validation project missing parity comment for setupFiles

  • Location: vitest.config.ts:170
  • Category: architecture
  • Problem: The e2e-branch-validation project lacks setupFiles. Currently brev-e2e.test.ts only uses ESM imports (import { shellQuote } from "../../../src/lib/core/shell-quote"), so the hook is not needed. A comment explaining this would prevent future confusion when tests import source modules with require().
  • Impact: Future branch-validation tests importing source modules with require() would hit the same collection failure without clear documentation of why setupFiles is absent.
  • Recommended action: Add a comment to the e2e-branch-validation project config: 'branch validation only runs brev-e2e.test.ts which uses ESM imports; require hook not needed.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/e2e/brev-e2e.test.ts for any require() imports of source modules — currently only ESM imports found.
  • Missing regression test: N/A — architecture consistency check; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/e2e/brev-e2e.test.ts for any require() imports of source modules — currently only ESM imports found.
  • Evidence: brev-e2e.test.ts imports shellQuote via ESM (import { shellQuote } from "../../../src/lib/core/shell-quote"). Previous review (PRA-4) identified this gap; remains unaddressed.

PRA-6 Resolve/justify — Diagnostic output on dcode_identity failure should document no-credential assumption

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:157
  • Category: correctness
  • Problem: Diagnostic output prints captured stdout/stderr from dcode_identity on failure (lines 157-160, 207-208). dcode identity output shows Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime — no API keys — but the assumption that output contains no credentials is not documented.
  • Impact: Low risk — diagnostic is safe as-is but should document the assumption that dcode identity output contains no secrets. If dcode_identity output ever changes to include credentials, they would be printed to CI logs.
  • Recommended action: Add a comment before the diagnostic printf statements confirming dcode identity output never includes credentials (fields: Model, Route, Provider, Endpoint, Sandbox, Harness, Agent, Runtime only).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run dcode identity manually or check agents/langchain-deepagents-code/dcode-wrapper.sh print_identity() — output fields are Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime.
  • Missing regression test: N/A — diagnostic improvement; existing test verifies identity fields via assert_identity.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run dcode identity manually or check agents/langchain-deepagents-code/dcode-wrapper.sh print_identity() — output fields are Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime.
  • Evidence: Shell script lines 142-148 and 193-196 capture and print identity_before/identity_after on failure. dcode_identity() calls openshell sandbox exec -- dcode identity 2>&1. Previous review (PRA-5) partially unaddressed.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-7 Improvement — Shorten dcode absolute path comment referencing Dockerfile

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:40
  • Category: correctness
  • Problem: dcode_identity() function changed to use absolute path /usr/local/bin/dcode with explanatory comment. The fix is correct (non-login shell PATH lacks /usr/local/bin) but the comment duplicates information already in the Dockerfile.
  • Impact: Minor — verbose comment adds maintenance burden without additional value.
  • Suggested action: Shorten comment to: 'Invoke dcode by absolute path; non-login shell PATH lacks /usr/local/bin (see agents/langchain-deepagents-code/Dockerfile).'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check agents/langchain-deepagents-code/Dockerfile for dcode launcher install path.
  • Missing regression test: N/A — cosmetic improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Current comment spans lines 35-38 explaining PATH issue; Dockerfile installs dcode at /usr/local/bin/dcode.

PRA-8 Improvement — Trim e2e-live setupFiles comment to focus on in-process rationale

  • Location: vitest.config.ts:140
  • Category: correctness
  • Problem: e2e-live project setupFiles comment explains the require hook registration but could reference the cli project as the mirrored pattern more explicitly. The comment is adequate but could be tighter.
  • Impact: Minor — verbose comment adds noise.
  • Suggested action: Consider trimming the comment to focus on why setupFiles is used over NODE_OPTIONS (in-process, no leak into CLI subprocesses).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare with cli project setupFiles in vitest.config.ts.
  • Missing regression test: N/A — cosmetic improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Current comment lines 137-145 explain the hook registration and why NODE_OPTIONS is avoided. CLI project uses same pattern.
Simplification opportunities: 2 possible cuts, net -8 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-7 shrink (test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:40): Multi-line comment explaining PATH issue in detail
    • Replacement: Single-line comment referencing Dockerfile
    • Net: -3 lines
    • Safety boundary: Must preserve absolute path /usr/local/bin/dcode invocation
  • PRA-8 shrink (vitest.config.ts:140): Verbose explanation of hook registration and NODE_OPTIONS avoidance
    • Replacement: Concise: 'In-process require hook (mirrors cli project); avoids NODE_OPTIONS leak into CLI subprocesses.'
    • Net: -5 lines
    • Safety boundary: Must preserve setupFiles array and in-process hook registration
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — test/helpers/register-source-require.test.ts — require("../../../outside-src") expects MODULE_NOT_FOUND. Runtime/sandbox/infrastructure paths need behavioral runtime validation: vitest.config.ts require hook registration and shell script diagnostic paths. The src/ boundary guard is a security control that must be regression-tested. The require hook must be exercised at runtime in vitest worker to detect breakage.
  • PRA-T2 Runtime validation — test/e2e/live/source-require-hook-smoke.test.ts — require("../../../src/lib/inference/ollama-runtime-context") asserts exports exist. Runtime/sandbox/infrastructure paths need behavioral runtime validation: vitest.config.ts require hook registration and shell script diagnostic paths. The src/ boundary guard is a security control that must be regression-tested. The require hook must be exercised at runtime in vitest worker to detect breakage.
  • PRA-T3 Runtime validation — test/e2e/live/source-require-hook-sibling-resolution.test.ts — module with require("../runner") resolves .ts sibling correctly. Runtime/sandbox/infrastructure paths need behavioral runtime validation: vitest.config.ts require hook registration and shell script diagnostic paths. The src/ boundary guard is a security control that must be regression-tested. The require hook must be exercised at runtime in vitest worker to detect breakage.
  • PRA-T4 e2e-live require hook registration lacks runtime smoke test — Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() (e.g., require("../../../src/lib/inference/ollama-runtime-context")) and asserts expected exports exist.
  • PRA-T5 Acceptance clause — Verify src/ directory boundary guard has regression test — add test evidence or identify existing coverage. No test/helpers/register-source-require.test.ts exists. Previous review PRA-1/PRA-2 unaddressed.
  • PRA-T6 Acceptance clause — Verify e2e-live require hook works at runtime with smoke test — add test evidence or identify existing coverage. No test/e2e/live/source-require-hook-smoke.test.ts exists. Previous review PRA-3/PRA-T1/PRA-T2/PRA-T3 unaddressed.
  • PRA-T7 Acceptance clause — Document why e2e-branch-validation lacks setupFiles — add test evidence or identify existing coverage. No parity comment added. Previous review PRA-4 unaddressed.
  • PRA-T8 Acceptance clause — Confirm dcode_identity diagnostic output contains no credentials — add test evidence or identify existing coverage. Diagnostic output added but no comment confirming no credential leakage. Previous review PRA-5 partially unaddressed.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: vitest.config.ts:146 (setupFiles require hook registration)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — Need: (a) unit test for src/ boundary guard (require("../../../outside-src") rejected), (b) smoke test that a real source module with require("../sibling") loads via hook in e2e-live worker.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PR description root cause ci: auto-update release notes on push to main #1: hermes-inference-switch imports src/lib/inference/config.ts which transitively loads ollama-runtime-context.ts's runtime require("../runner"). Without hook, collection fails with "Cannot find module '../runner'".

PRA-2 Required — Require hook src/ directory boundary lacks regression test

  • Location: test/helpers/register-source-require.ts:55
  • Category: security
  • Problem: The typed-source require hook restricts resolution to files under src/ directory (lines 55-65) via sourceCandidate.startsWith(sourceRoot). No unit test validates this boundary. If the guard regresses, the hook could transpile files outside src/ in test worker processes, violating least-privilege for test infrastructure.
  • Impact: A regression in the sourceCandidate.startsWith(sourceRoot) check could allow arbitrary file transpilation in test workers. Impact limited to test processes but is a security control gap.
  • Required action: Add test/helpers/register-source-require.test.ts with a case that attempts require("../../../outside-src") and expects the original MODULE_NOT_FOUND error to propagate.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Review test/helpers/register-source-require.ts lines 55-65 (resolveFilename override) — sourceCandidate.startsWith(sourceRoot) check before allowing resolution. Confirm test/helpers/register-source-require.test.ts does not exist.
  • Missing regression test: Add test/helpers/register-source-require.test.ts verifying require("../../../outside-src") is rejected and original MODULE_NOT_FOUND error propagates.
  • Done when: The required change is committed and verification passes: Review test/helpers/register-source-require.ts lines 55-65 (resolveFilename override) — sourceCandidate.startsWith(sourceRoot) check before allowing resolution. Confirm test/helpers/register-source-require.test.ts does not exist.
  • Evidence: Hook's resolveFilename override at lines 55-65 checks sourceCandidate.startsWith(sourceRoot) before allowing resolution. Previous review (PRA-2) identified this gap; remains unaddressed.

PRA-3 Required — e2e-live require hook registration lacks runtime smoke test

  • Location: vitest.config.ts:146
  • Category: tests
  • Problem: The e2e-live project registers the typed-source require hook via onboard-script-mocks.cjs (setupFiles), but no test directly exercises it in the vitest worker process. The openshell-version-pin.test.ts uses require() but spawns a separate Node process with --import tsx; no vitest-worker test imports source modules via require().
  • Impact: Future breakage of the require hook registration in e2e-live could go undetected until a new test imports the deep inference graph. Collection-only verification (vitest list) is insufficient.
  • Required action: Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() (e.g., require("../../../src/lib/inference/ollama-runtime-context")) and asserts expected exports exist.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live --reporter=verbose to verify the project loads; grep test/e2e/live for require() on src/ paths — currently none found.
  • Missing regression test: Add test/e2e/live/source-require-hook-smoke.test.ts that imports a source module via require() and validates the hook resolves .ts siblings correctly.
  • Done when: The required change is committed and verification passes: Run NEMOCLAW_RUN_LIVE_E2E=1 npx vitest run --project e2e-live --reporter=verbose to verify the project loads; grep test/e2e/live for require() on src/ paths — currently none found.
  • Evidence: PR verification used vitest list --project e2e-live which only checks collection; no test asserts the require hook works at runtime. Previous review (PRA-1/PRA-T1/PRA-T2) identified this gap; remains unaddressed.

PRA-4 Required — Source-of-truth review incomplete for src/ directory boundary guard

  • Location: test/helpers/register-source-require.ts:55
  • Category: architecture
  • Problem: The require hook's src/ boundary guard (lines 55-65) is a localized workaround for Node's inability to resolve .ts/extensionless imports at runtime. The source-of-truth review lacks: invalid state definition, source boundary, why source cannot be fixed here, regression test, and removal condition.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear. Without documented removal condition, the workaround may become permanent technical debt.
  • Required action: Complete the source-of-truth review: document the invalid state (Node CJS resolver cannot resolve .ts/extensionless sibling imports in deep inference graph), source boundary (src/lib/inference/ runtime require()), source-fix constraint (large refactor to convert runtime require() to ESM), regression test (missing — see PRA-1/PRA-2), and removal condition (when grep -r "require("../" src/lib/inference/" returns no matches).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect test/helpers/register-source-require.ts lines 55-65 and src/lib/inference/ollama-runtime-context.ts for runtime require("../runner") pattern. Check if source-of-truth review fields are populated in PR or docs.
  • Missing regression test: Add test/helpers/register-source-require.test.ts verifying require("../../../outside-src") is rejected AND test/e2e/live/source-require-hook-smoke.test.ts verifying a real source module with runtime require() loads correctly.
  • Done when: The required change is committed and verification passes: Inspect test/helpers/register-source-require.ts lines 55-65 and src/lib/inference/ollama-runtime-context.ts for runtime require("../runner") pattern. Check if source-of-truth review fields are populated in PR or docs.
  • Evidence: Lines 55-65 in register-source-require.ts check sourceCandidate.startsWith(sourceRoot). Previous review (PRA-1) marked localized patch analysis as needs_followup.

PRA-5 Resolve/justify — e2e-branch-validation project missing parity comment for setupFiles

  • Location: vitest.config.ts:170
  • Category: architecture
  • Problem: The e2e-branch-validation project lacks setupFiles. Currently brev-e2e.test.ts only uses ESM imports (import { shellQuote } from "../../../src/lib/core/shell-quote"), so the hook is not needed. A comment explaining this would prevent future confusion when tests import source modules with require().
  • Impact: Future branch-validation tests importing source modules with require() would hit the same collection failure without clear documentation of why setupFiles is absent.
  • Recommended action: Add a comment to the e2e-branch-validation project config: 'branch validation only runs brev-e2e.test.ts which uses ESM imports; require hook not needed.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/e2e/brev-e2e.test.ts for any require() imports of source modules — currently only ESM imports found.
  • Missing regression test: N/A — architecture consistency check; no test needed.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/e2e/brev-e2e.test.ts for any require() imports of source modules — currently only ESM imports found.
  • Evidence: brev-e2e.test.ts imports shellQuote via ESM (import { shellQuote } from "../../../src/lib/core/shell-quote"). Previous review (PRA-4) identified this gap; remains unaddressed.

PRA-6 Resolve/justify — Diagnostic output on dcode_identity failure should document no-credential assumption

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:157
  • Category: correctness
  • Problem: Diagnostic output prints captured stdout/stderr from dcode_identity on failure (lines 157-160, 207-208). dcode identity output shows Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime — no API keys — but the assumption that output contains no credentials is not documented.
  • Impact: Low risk — diagnostic is safe as-is but should document the assumption that dcode identity output contains no secrets. If dcode_identity output ever changes to include credentials, they would be printed to CI logs.
  • Recommended action: Add a comment before the diagnostic printf statements confirming dcode identity output never includes credentials (fields: Model, Route, Provider, Endpoint, Sandbox, Harness, Agent, Runtime only).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run dcode identity manually or check agents/langchain-deepagents-code/dcode-wrapper.sh print_identity() — output fields are Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime.
  • Missing regression test: N/A — diagnostic improvement; existing test verifies identity fields via assert_identity.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run dcode identity manually or check agents/langchain-deepagents-code/dcode-wrapper.sh print_identity() — output fields are Sandbox, Harness, Agent, Route, Provider, Model, Endpoint, Runtime.
  • Evidence: Shell script lines 142-148 and 193-196 capture and print identity_before/identity_after on failure. dcode_identity() calls openshell sandbox exec -- dcode identity 2>&1. Previous review (PRA-5) partially unaddressed.

PRA-7 Improvement — Shorten dcode absolute path comment referencing Dockerfile

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:40
  • Category: correctness
  • Problem: dcode_identity() function changed to use absolute path /usr/local/bin/dcode with explanatory comment. The fix is correct (non-login shell PATH lacks /usr/local/bin) but the comment duplicates information already in the Dockerfile.
  • Impact: Minor — verbose comment adds maintenance burden without additional value.
  • Suggested action: Shorten comment to: 'Invoke dcode by absolute path; non-login shell PATH lacks /usr/local/bin (see agents/langchain-deepagents-code/Dockerfile).'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check agents/langchain-deepagents-code/Dockerfile for dcode launcher install path.
  • Missing regression test: N/A — cosmetic improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Current comment spans lines 35-38 explaining PATH issue; Dockerfile installs dcode at /usr/local/bin/dcode.

PRA-8 Improvement — Trim e2e-live setupFiles comment to focus on in-process rationale

  • Location: vitest.config.ts:140
  • Category: correctness
  • Problem: e2e-live project setupFiles comment explains the require hook registration but could reference the cli project as the mirrored pattern more explicitly. The comment is adequate but could be tighter.
  • Impact: Minor — verbose comment adds noise.
  • Suggested action: Consider trimming the comment to focus on why setupFiles is used over NODE_OPTIONS (in-process, no leak into CLI subprocesses).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare with cli project setupFiles in vitest.config.ts.
  • Missing regression test: N/A — cosmetic improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Current comment lines 137-145 explain the hook registration and why NODE_OPTIONS is avoided. CLI project uses same pattern.

Workflow run details

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.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: vitest.config.ts e2e-live typed-source require-hook registration.
Open items: 0 required · 3 warnings · 0 suggestions · 6 test follow-ups
Since last review: 0 prior items resolved · 4 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: vitest.config.ts e2e-live typed-source require-hook registration
  • PRA-2 Resolve or justify: Add a config regression for the e2e-live in-process source hook boundary in vitest.config.ts:149
  • PRA-3 Resolve or justify: Redact or prove safe the raw `dcode identity` failure diagnostics in test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:159
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Add a config regression for the e2e-live in-process source hook boundary
  • PRA-T5 Add or justify test follow-up: Acceptance clause
  • PRA-T6 Add or justify test follow-up: vitest.config.ts e2e-live typed-source require-hook registration

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify tests vitest.config.ts:149 Extend `test/e2e/support/e2e-live-project-config.test.ts` so its `ProjectConfig` shape includes `setupFiles` and `env`, then add a behavior-named assertion that `projectConfig("e2e-live").test?.setupFiles` equals `["test/helpers/onboard-script-mocks.cjs"]` and `projectConfig("e2e-live").test?.env?.NODE_OPTIONS` is absent. Include a short comment or test name that records the removal condition: remove this hook when live suites no longer import source modules through runtime CJS require, or when the source loader boundary changes.
PRA-3 Resolve/justify security test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:159 Keep the useful diagnostic, but make its safe logging boundary explicit: either pass the captured output through an existing credential redactor or a small shell redaction helper before printing, or add/identify automated evidence proving all `/usr/local/bin/dcode identity` non-zero outputs are constrained to non-secret identity metadata.
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: vitest.config.ts e2e-live typed-source require-hook registration

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing. `test/e2e/support/e2e-live-project-config.test.ts` should assert the e2e-live project uses `setupFiles: ["test/helpers/onboard-script-mocks.cjs"]` and leaves `env.NODE_OPTIONS` undefined.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `vitest.config.ts:143-149` documents the in-worker hook and `setupFiles` choice; `test/helpers/onboard-script-mocks.cjs` loads `register-source-require.ts`; `test/e2e/support/e2e-live-project-config.test.ts` currently lacks setupFiles/env assertions.

PRA-2 Resolve/justify — Add a config regression for the e2e-live in-process source hook boundary

  • Location: vitest.config.ts:149
  • Category: tests
  • Problem: The PR relies on `setupFiles` to install `test/helpers/onboard-script-mocks.cjs` only inside the `e2e-live` Vitest worker, while deliberately avoiding `env.NODE_OPTIONS` so the preload does not leak into real CLI subprocesses. The current code implements that shape, but the adjacent config test still only asserts include gating and retry policy; it does not assert either the new `setupFiles` entry or the absence of `NODE_OPTIONS`. This also leaves the localized require-hook workaround without a concrete regression guard or encoded removal boundary.
  • Impact: A future config edit could remove the live-project source hook and reintroduce collection failures, or move the hook into `NODE_OPTIONS` and weaken the trusted-code boundary for subprocess-based live E2E tests, without a focused test catching it.
  • Recommended action: Extend `test/e2e/support/e2e-live-project-config.test.ts` so its `ProjectConfig` shape includes `setupFiles` and `env`, then add a behavior-named assertion that `projectConfig("e2e-live").test?.setupFiles` equals `["test/helpers/onboard-script-mocks.cjs"]` and `projectConfig("e2e-live").test?.env?.NODE_OPTIONS` is absent. Include a short comment or test name that records the removal condition: remove this hook when live suites no longer import source modules through runtime CJS require, or when the source loader boundary changes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `test/e2e/support/e2e-live-project-config.test.ts` and confirm it asserts both the e2e-live setup file and the absence of `NODE_OPTIONS`; then read `vitest.config.ts:141-149` to confirm those assertions match the project config.
  • Missing regression test: Add `it("keeps the e2e-live source hook in-process without NODE_OPTIONS", ...)` under `test/e2e/support/e2e-live-project-config.test.ts`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `test/e2e/support/e2e-live-project-config.test.ts` and confirm it asserts both the e2e-live setup file and the absence of `NODE_OPTIONS`; then read `vitest.config.ts:141-149` to confirm those assertions match the project config.
  • Evidence: `vitest.config.ts:141-149` adds `setupFiles: ["test/helpers/onboard-script-mocks.cjs"]` under `name: "e2e-live"` and does not add an `env` block there. `test/e2e/support/e2e-live-project-config.test.ts:14-18` only models `name`, `include`, and `retry`, and its existing assertions cover include gating and single-shot retry behavior only.

PRA-3 Resolve/justify — Redact or prove safe the raw `dcode identity` failure diagnostics

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:159
  • Category: security
  • Problem: The new failure paths print the raw captured stdout and stderr from `/usr/local/bin/dcode identity` before failing. The expected managed identity output is sanitized by `agents/langchain-deepagents-code/dcode-wrapper.sh`, but this shell diagnostic prints the combined output specifically when the command exits non-zero, and the changed script does not redact token-like substrings or known secret values before writing to CI/result logs.
  • Impact: If the launcher, wrapper preflight, shell, or a dependency emits provider configuration, environment details, or token-like values on a non-zero path, this diagnostic can write secrets to CI logs or result artifacts.
  • Recommended action: Keep the useful diagnostic, but make its safe logging boundary explicit: either pass the captured output through an existing credential redactor or a small shell redaction helper before printing, or add/identify automated evidence proving all `/usr/local/bin/dcode identity` non-zero outputs are constrained to non-secret identity metadata.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:155-160` and `:206-208` for redaction before `printf`; inspect `agents/langchain-deepagents-code/dcode-wrapper.sh` only as supporting proof if the diagnostic remains raw.
  • Missing regression test: Add a focused diagnostic-path test or shell fixture that feeds representative stderr containing `COMPATIBLE_API_KEY`, `DEEPAGENTS_CODE_OPENAI_API_KEY`, and token-shaped values, then asserts the emitted diagnostic redacts them; alternatively identify an existing test that exercises non-zero `dcode identity` output and proves it cannot contain secrets.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:155-160` and `:206-208` for redaction before `printf`; inspect `agents/langchain-deepagents-code/dcode-wrapper.sh` only as supporting proof if the diagnostic remains raw.
  • Evidence: `04-deepagents-code-fresh-reonboard.sh:155-160` prints `${identity_before:-<no output captured>}` and `:206-208` prints `${identity_after:-<no output captured>}` directly. Nearby `agents/langchain-deepagents-code/dcode-wrapper.sh:624-672` filters normal identity fields, but that does not by itself prove raw non-zero stderr from the whole command path is secret-free.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Add `keeps the e2e-live source hook in-process without NODE_OPTIONS` in `test/e2e/support/e2e-live-project-config.test.ts` to assert `setupFiles` and absence of `env.NODE_OPTIONS` for the `e2e-live` project.. The changed behavior affects live E2E worker setup, sandbox command execution, and failure diagnostics. Static review supports the intended shape, but the trusted-code boundary and diagnostic safety need targeted behavioral checks rather than relying only on external E2E job outcomes.
  • PRA-T2 Runtime validation — Add or identify a diagnostic-path test that proves non-zero `dcode identity` output containing `COMPATIBLE_API_KEY`, `DEEPAGENTS_CODE_OPENAI_API_KEY`, or token-shaped values is redacted before the shell check prints it.. The changed behavior affects live E2E worker setup, sandbox command execution, and failure diagnostics. Static review supports the intended shape, but the trusted-code boundary and diagnostic safety need targeted behavioral checks rather than relying only on external E2E job outcomes.
  • PRA-T3 Runtime validation — Add or identify runtime collection validation for an e2e-live suite that imports the source graph containing runtime `require("../runner")`, without reporting external E2E pass/fail status in the review.. The changed behavior affects live E2E worker setup, sandbox command execution, and failure diagnostics. Static review supports the intended shape, but the trusted-code boundary and diagnostic safety need targeted behavioral checks rather than relying only on external E2E job outcomes.
  • PRA-T4 Add a config regression for the e2e-live in-process source hook boundary — Extend `test/e2e/support/e2e-live-project-config.test.ts` so its `ProjectConfig` shape includes `setupFiles` and `env`, then add a behavior-named assertion that `projectConfig("e2e-live").test?.setupFiles` equals `["test/helpers/onboard-script-mocks.cjs"]` and `projectConfig("e2e-live").test?.env?.NODE_OPTIONS` is absent. Include a short comment or test name that records the removal condition: remove this hook when live suites no longer import source modules through runtime CJS require, or when the source loader boundary changes.
  • PRA-T5 Acceptance clause — Registers the typed-source `.ts` require hook in-process — deliberately not via `env.NODE_OPTIONS`, so `--require` never leaks into the real CLI subprocesses live tests spawn. — add test evidence or identify existing coverage. `vitest.config.ts:146-149` implements the in-process `setupFiles` path and does not add `env.NODE_OPTIONS` to `e2e-live`, but `test/e2e/support/e2e-live-project-config.test.ts` does not assert this trusted-code boundary.
  • PRA-T6 vitest.config.ts e2e-live typed-source require-hook registration — Missing. `test/e2e/support/e2e-live-project-config.test.ts` should assert the e2e-live project uses `setupFiles: ["test/helpers/onboard-script-mocks.cjs"]` and leaves `env.NODE_OPTIONS` undefined.. `vitest.config.ts:143-149` documents the in-worker hook and `setupFiles` choice; `test/helpers/onboard-script-mocks.cjs` loads `register-source-require.ts`; `test/e2e/support/e2e-live-project-config.test.ts` currently lacks setupFiles/env assertions.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: vitest.config.ts e2e-live typed-source require-hook registration

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing. `test/e2e/support/e2e-live-project-config.test.ts` should assert the e2e-live project uses `setupFiles: ["test/helpers/onboard-script-mocks.cjs"]` and leaves `env.NODE_OPTIONS` undefined.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `vitest.config.ts:143-149` documents the in-worker hook and `setupFiles` choice; `test/helpers/onboard-script-mocks.cjs` loads `register-source-require.ts`; `test/e2e/support/e2e-live-project-config.test.ts` currently lacks setupFiles/env assertions.

PRA-2 Resolve/justify — Add a config regression for the e2e-live in-process source hook boundary

  • Location: vitest.config.ts:149
  • Category: tests
  • Problem: The PR relies on `setupFiles` to install `test/helpers/onboard-script-mocks.cjs` only inside the `e2e-live` Vitest worker, while deliberately avoiding `env.NODE_OPTIONS` so the preload does not leak into real CLI subprocesses. The current code implements that shape, but the adjacent config test still only asserts include gating and retry policy; it does not assert either the new `setupFiles` entry or the absence of `NODE_OPTIONS`. This also leaves the localized require-hook workaround without a concrete regression guard or encoded removal boundary.
  • Impact: A future config edit could remove the live-project source hook and reintroduce collection failures, or move the hook into `NODE_OPTIONS` and weaken the trusted-code boundary for subprocess-based live E2E tests, without a focused test catching it.
  • Recommended action: Extend `test/e2e/support/e2e-live-project-config.test.ts` so its `ProjectConfig` shape includes `setupFiles` and `env`, then add a behavior-named assertion that `projectConfig("e2e-live").test?.setupFiles` equals `["test/helpers/onboard-script-mocks.cjs"]` and `projectConfig("e2e-live").test?.env?.NODE_OPTIONS` is absent. Include a short comment or test name that records the removal condition: remove this hook when live suites no longer import source modules through runtime CJS require, or when the source loader boundary changes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `test/e2e/support/e2e-live-project-config.test.ts` and confirm it asserts both the e2e-live setup file and the absence of `NODE_OPTIONS`; then read `vitest.config.ts:141-149` to confirm those assertions match the project config.
  • Missing regression test: Add `it("keeps the e2e-live source hook in-process without NODE_OPTIONS", ...)` under `test/e2e/support/e2e-live-project-config.test.ts`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `test/e2e/support/e2e-live-project-config.test.ts` and confirm it asserts both the e2e-live setup file and the absence of `NODE_OPTIONS`; then read `vitest.config.ts:141-149` to confirm those assertions match the project config.
  • Evidence: `vitest.config.ts:141-149` adds `setupFiles: ["test/helpers/onboard-script-mocks.cjs"]` under `name: "e2e-live"` and does not add an `env` block there. `test/e2e/support/e2e-live-project-config.test.ts:14-18` only models `name`, `include`, and `retry`, and its existing assertions cover include gating and single-shot retry behavior only.

PRA-3 Resolve/justify — Redact or prove safe the raw `dcode identity` failure diagnostics

  • Location: test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:159
  • Category: security
  • Problem: The new failure paths print the raw captured stdout and stderr from `/usr/local/bin/dcode identity` before failing. The expected managed identity output is sanitized by `agents/langchain-deepagents-code/dcode-wrapper.sh`, but this shell diagnostic prints the combined output specifically when the command exits non-zero, and the changed script does not redact token-like substrings or known secret values before writing to CI/result logs.
  • Impact: If the launcher, wrapper preflight, shell, or a dependency emits provider configuration, environment details, or token-like values on a non-zero path, this diagnostic can write secrets to CI logs or result artifacts.
  • Recommended action: Keep the useful diagnostic, but make its safe logging boundary explicit: either pass the captured output through an existing credential redactor or a small shell redaction helper before printing, or add/identify automated evidence proving all `/usr/local/bin/dcode identity` non-zero outputs are constrained to non-secret identity metadata.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:155-160` and `:206-208` for redaction before `printf`; inspect `agents/langchain-deepagents-code/dcode-wrapper.sh` only as supporting proof if the diagnostic remains raw.
  • Missing regression test: Add a focused diagnostic-path test or shell fixture that feeds representative stderr containing `COMPATIBLE_API_KEY`, `DEEPAGENTS_CODE_OPENAI_API_KEY`, and token-shaped values, then asserts the emitted diagnostic redacts them; alternatively identify an existing test that exercises non-zero `dcode identity` output and proves it cannot contain secrets.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh:155-160` and `:206-208` for redaction before `printf`; inspect `agents/langchain-deepagents-code/dcode-wrapper.sh` only as supporting proof if the diagnostic remains raw.
  • Evidence: `04-deepagents-code-fresh-reonboard.sh:155-160` prints `${identity_before:-<no output captured>}` and `:206-208` prints `${identity_after:-<no output captured>}` directly. Nearby `agents/langchain-deepagents-code/dcode-wrapper.sh:624-672` filters normal identity fields, but that does not by itself prove raw non-zero stderr from the whole command path is secret-free.

Workflow run details

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.

@github-code-quality

github-code-quality Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the branch is 96%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 2fa213f +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the branch is 73%. Coverage data for the branch is not yet available.

Show a code coverage summary of the most covered files.
File 2fa213f +/-
src/lib/shields...nsition-lock.ts 87%
src/lib/onboard/preflight.ts 83%
src/lib/actions...all/run-plan.ts 81%
src/lib/state/o...oard-session.ts 81%
src/lib/state/sandbox.ts 75%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/shields/index.ts 68%
src/lib/policy/index.ts 66%
src/lib/actions...licy-channel.ts 63%
src/lib/onboard.ts 28%

Updated July 06, 2026 23:19 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ⚠️ Run cancelled — no signal

Run: 28826327875
Workflow ref: fix/e2e-live-source-require-hook
Requested targets: (default — all supported)
Requested jobs: (selector rejected by workflow validation)
Summary: 0 passed, 0 failed, 73 cancelled, 0 skipped

Job Result
agent-turn-latency ⚠️ cancelled
bedrock-runtime-compatible-anthropic ⚠️ cancelled
brave-search ⚠️ cancelled
channels-add-remove ⚠️ cancelled
channels-stop-start ⚠️ cancelled
cloud-inference ⚠️ cancelled
cloud-onboard ⚠️ cancelled
common-egress-agent ⚠️ cancelled
concurrent-gateway-ports ⚠️ cancelled
credential-migration ⚠️ cancelled
credential-sanitization ⚠️ cancelled
cron-preflight-inference-local ⚠️ cancelled
device-auth-health ⚠️ cancelled
diagnostics ⚠️ cancelled
docs-validation ⚠️ cancelled
double-onboard ⚠️ cancelled
full-e2e ⚠️ cancelled
gateway-drift-preflight ⚠️ cancelled
gateway-guard-recovery ⚠️ cancelled
gateway-health-honest ⚠️ cancelled
gpu-double-onboard ⚠️ cancelled
gpu-e2e ⚠️ cancelled
hermes-dashboard ⚠️ cancelled
hermes-discord ⚠️ cancelled
hermes-e2e ⚠️ cancelled
hermes-gpu-startup ⚠️ cancelled
hermes-inference-switch ⚠️ cancelled
hermes-slack ⚠️ cancelled
inference-routing ⚠️ cancelled
issue-2478-crash-loop-recovery ⚠️ cancelled
issue-4434-tui-unreachable-inference ⚠️ cancelled
issue-4462-scope-upgrade-approval ⚠️ cancelled
jetson-nvmap-gpu ⚠️ cancelled
kimi-inference-compat ⚠️ cancelled
launchable-smoke ⚠️ cancelled
live ⚠️ cancelled
mcp-bridge ⚠️ cancelled
mcp-bridge-dev ⚠️ cancelled
messaging-compatible-endpoint ⚠️ cancelled
messaging-providers ⚠️ cancelled
model-router-provider-routed-inference ⚠️ cancelled
network-policy ⚠️ cancelled
ollama-auth-proxy ⚠️ cancelled
onboard-negative-paths ⚠️ cancelled
onboard-repair ⚠️ cancelled
onboard-resume ⚠️ cancelled
openclaw-discord-pairing ⚠️ cancelled
openclaw-inference-switch ⚠️ cancelled
openclaw-skill-cli ⚠️ cancelled
openclaw-slack-pairing ⚠️ cancelled
openclaw-tui-chat-correlation ⚠️ cancelled
openshell-gateway-auth-contract ⚠️ cancelled
openshell-gateway-upgrade ⚠️ cancelled
openshell-version-pin ⚠️ cancelled
overlayfs-autofix ⚠️ cancelled
rebuild-hermes ⚠️ cancelled
rebuild-hermes-stale-base ⚠️ cancelled
rebuild-openclaw ⚠️ cancelled
sandbox-operations ⚠️ cancelled
sandbox-rebuild ⚠️ cancelled
sandbox-rlimits-connect ⚠️ cancelled
sandbox-survival ⚠️ cancelled
security-posture ⚠️ cancelled
sessions-agents-cli ⚠️ cancelled
shields-config ⚠️ cancelled
skill-agent ⚠️ cancelled
snapshot-commands ⚠️ cancelled
spark-install ⚠️ cancelled
state-backup-restore ⚠️ cancelled
telegram-injection ⚠️ cancelled
token-rotation ⚠️ cancelled
tunnel-lifecycle ⚠️ cancelled
upgrade-stale-sandbox ⚠️ cancelled

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ❌ Some jobs failed

Run: 28826486785
Workflow ref: fix/e2e-live-source-require-hook
Requested targets: (default — all supported)
Requested jobs: (default — all default-enabled free-standing jobs; explicit-only jobs openshell-gateway-auth-contract, mcp-bridge-dev, hermes-gpu-startup, sandbox-rlimits-connect, and jetson-nvmap-gpu are skipped unless selected)
Summary: 65 passed, 4 failed, 0 cancelled, 5 skipped

Job Result
agent-turn-latency ✅ success
bedrock-runtime-compatible-anthropic ✅ success
brave-search ✅ success
channels-add-remove ✅ success
channels-stop-start ✅ success
cloud-inference ✅ success
cloud-onboard ❌ failure
common-egress-agent ✅ success
concurrent-gateway-ports ✅ success
credential-migration ✅ success
credential-sanitization ✅ success
cron-preflight-inference-local ✅ success
device-auth-health ✅ success
diagnostics ✅ success
docs-validation ✅ success
double-onboard ✅ success
full-e2e ❌ failure
gateway-drift-preflight ✅ success
gateway-guard-recovery ✅ success
gateway-health-honest ✅ success
generate-matrix ✅ success
gpu-double-onboard ✅ success
gpu-e2e ✅ success
hermes-dashboard ✅ success
hermes-discord ✅ success
hermes-e2e ✅ success
hermes-gpu-startup ⏭️ skipped
hermes-inference-switch ❌ failure
hermes-slack ✅ success
inference-routing ✅ success
issue-2478-crash-loop-recovery ✅ success
issue-4434-tui-unreachable-inference ✅ success
issue-4462-scope-upgrade-approval ✅ success
jetson-nvmap-gpu ⏭️ skipped
kimi-inference-compat ✅ success
launchable-smoke ✅ success
live ✅ success
mcp-bridge ✅ success
mcp-bridge-dev ⏭️ skipped
messaging-compatible-endpoint ✅ success
messaging-providers ✅ success
model-router-provider-routed-inference ✅ success
network-policy ✅ success
ollama-auth-proxy ✅ success
onboard-negative-paths ✅ success
onboard-repair ✅ success
onboard-resume ✅ success
openclaw-discord-pairing ✅ success
openclaw-inference-switch ✅ success
openclaw-skill-cli ✅ success
openclaw-slack-pairing ✅ success
openclaw-tui-chat-correlation ✅ success
openshell-gateway-auth-contract ⏭️ skipped
openshell-gateway-upgrade ❌ failure
openshell-version-pin ✅ success
overlayfs-autofix ✅ success
rebuild-hermes ✅ success
rebuild-hermes-stale-base ✅ success
rebuild-openclaw ✅ success
sandbox-operations ✅ success
sandbox-rebuild ✅ success
sandbox-rlimits-connect ⏭️ skipped
sandbox-survival ✅ success
security-posture ✅ success
sessions-agents-cli ✅ success
shields-config ✅ success
skill-agent ✅ success
snapshot-commands ✅ success
spark-install ✅ success
state-backup-restore ✅ success
telegram-injection ✅ success
token-rotation ✅ success
tunnel-lifecycle ✅ success
upgrade-stale-sandbox ✅ success

Explicit-only jobs skipped: openshell-gateway-auth-contract (default dispatch excludes the resource-heavy OpenShell auth-contract probe unless selected; validate with jobs=openshell-gateway-auth-contract or targets=openshell-gateway-auth-contract), mcp-bridge-dev (default dispatch excludes moving OpenShell dev artifacts unless explicitly selected; validate with jobs=mcp-bridge-dev or targets=mcp-bridge-dev), hermes-gpu-startup (default dispatch excludes this explicit-only job unless selected; validate with jobs=hermes-gpu-startup or targets=hermes-gpu-startup), sandbox-rlimits-connect (default dispatch excludes the destructive rlimit fork/connect probe unless selected; validate with jobs=sandbox-rlimits-connect or targets=sandbox-rlimits-connect), jetson-nvmap-gpu (default dispatch excludes Jetson until a stable Jetson runner is available; validate with jobs=jetson-nvmap-gpu or targets=jetson-nvmap-gpu).

Failed jobs: cloud-onboard, full-e2e, hermes-inference-switch, openshell-gateway-upgrade. Check run artifacts for logs.

The cloud-experimental check-04 (fresh DCode re-onboard) captures
`dcode identity` stdout+stderr into a shell variable via command
substitution, but the `|| fail` handler prints only a generic message and
discards the captured output. When `dcode identity` exits non-zero the real
reason is lost — CI logs and result.json show stdout "" — which is why the
current cloud-onboard failure on main (introduced with this check in #6332)
cannot be diagnosed.

Print the captured output to stdout (so it lands in result.json) before
failing, on both the initial and post-re-onboard identity reads. No behavior
change to the check's pass/fail logic; this only makes the existing failure
observable so the root cause in the managed DCode onboarding flow can be found.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@prekshivyas prekshivyas changed the title test(e2e): register typed-source require hook for the e2e-live project test(e2e): register e2e-live source require hook and surface cloud-onboard dcode identity failure Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ❌ Some jobs failed

Run: 28828274507
Workflow ref: fix/e2e-live-source-require-hook
Requested targets: (default — all supported)
Requested jobs: cloud-onboard
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ❌ failure

Failed jobs: cloud-onboard. Check run artifacts for logs.

prekshivyas and others added 3 commits July 6, 2026 16:00
…rd check

check-04 read the live identity with `openshell sandbox exec -- dcode identity`.
That runs without a login shell, so /usr/local/bin is not on PATH and `dcode`
resolves to "command not found" — the real reason cloud-onboard has failed on
main since #6332 added this check (surfaced by the preceding observability
commit). The image installs the launcher at /usr/local/bin/dcode, so invoke it
by absolute path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
The anthropic variant of the inference-switch suite asserts that
`openshell provider get compatible-anthropic-endpoint` reports `Type: openai`
with an anchored regex. That command wraps field labels in ANSI escapes
(`\e[2mType:\e[0m openai`), so the anchored match failed even though the
provider is correctly registered as Type: openai. Strip ANSI first. The check
only runs in the anthropic variant, which is why the hosted variant passed.

This failure was masked on main by the collection error the e2e-live require
hook fixed; with the suite now collecting, the assertion is reachable.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…equire-hook

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

# Conflicts:
#	test/e2e/live/hermes-inference-switch.test.ts
@prekshivyas

prekshivyas commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

openshell-gateway-upgrade E2E failure needs an owner decision (cc @jyaunches @cv )

While making e2e green for this PR I root-caused the openshell-gateway-upgrade live failure, and it's a behavior-contract conflict, not a mechanical bug — so I'm flagging rather than patching.

Symptom: openshell-gateway-upgrade fails at the current-version install.sh step:

Backup recovery blocked:
  e2e-gateway-upgrade-survivor  registry has no NemoClaw-managed image fingerprint
  (pre-fingerprint and custom images are not auto-recreated)
[ERROR] Installation incomplete: one or more existing sandboxes failed to upgrade.

Chain:

  1. The E2E test builds a legacy pre-fingerprint survivor sandbox (old NemoClaw v2026.4.24) and expects install.sh to upgrade + restore it.
  2. fix(sandbox): recover gateway-orphaned sandboxes during in-place upgrade #6305 (34f504eb) broadened recovery eligibility to absent-but-bound sandboxes (isPreparedRecoveryCandidate + confirmAbsentRecoveryCandidates). The orphaned legacy survivor is now routed into prepareBackupRecovery. Before fix(sandbox): recover gateway-orphaned sandboxes during in-place upgrade #6305, absence was explicitly not treated as failure, so it was left alone.
  3. prepareBackupRecovery hits the fix(installer): recover sandboxes before onboarding #6132 gate (upgrade-sandboxes.ts:103, hasPositiveManagedImageEvidence) → rejected → counted as failedprocess.exit(1).

The contradiction: the E2E test expects this scenario to succeed, but the unit tests deliberately assert it must fail closedupgrade-sandboxes-recovery.test.ts:202 "fails closed for an absent same-gateway legacy sandbox without a managed fingerprint" (and :183). So the live test now exercises a scenario the product was intentionally changed to reject.

Timeline: openshell-gateway-upgrade was green on main through 2026-07-06 00:56 UTC and failed on the first e2e-all after #6305 landed.

Decision needed (yours, not mine to guess):

  • If fail-closed is intended for legacy sandboxes on in-place upgrade → the E2E test should seed a fingerprinted/modern managed survivor (test the supported path) or assert the fail-closed block.
  • If legacy orphaned sandboxes should still upgrade → a product change in fix(sandbox): recover gateway-orphaned sandboxes during in-place upgrade #6305's recovery routing (and the "fails closed" unit tests) is needed.

I deliberately did not change the fail-closed behavior, since that would invert a deliberate security posture and rewrite the named "fails closed" tests. Happy to implement whichever direction you confirm.

@prekshivyas

prekshivyas commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Fresh e2e-all triggered — run (https://github.com/NVIDIA/NemoClaw/actions/runs/28829724087), tied to #6343. This is the full live fan-out on the post-merge branch (~25 min).

Expected result:

  • ✅ hermes-inference-switch (hosted + anthropic), cloud-onboard — should be green (main's merged fixes)
  • ❌ openshell-gateway-upgrade — still red (pending owner decision, now flagged)
  • ⚠️ full-e2e — likely green (intermittent flake)

@prekshivyas
prekshivyas requested review from cv and jyaunches July 6, 2026 23:20
@prekshivyas

Copy link
Copy Markdown
Contributor Author

E2E status on this branch — green except the owner-flagged gateway-upgrade

Ran a full e2e-all dispatch on the current commit (2fa213f, post-merge with main): run 28829724087.

Result: every previously-failing job is green except openshell-gateway-upgrade.

Job Before Now
hermes-inference-switch (hosted) ❌ collection error ✅ pass
hermes-inference-switch (anthropic) ❌ collection error ✅ pass
cloud-onboard dcode identity ✅ pass
full-e2e ⚠️ perf-budget flake ✅ pass (did not recur)
openshell-gateway-upgrade still red — needs owner decision

(73+ success, 5 skipped-by-design explicit-only jobs; mcp-bridge + token-rotation were still finishing at time of writing and are unrelated to this PR.)

How each got fixed

  • hermes-inference-switch (both variants): the e2e-live collection error is gone. main fixed the root cause directly (ollama-runtime-context.ts now uses a static import instead of a runtime require("../runner")); the setupFiles require-hook this PR added to the e2e-live Vitest project remains as defense-in-depth for any future runtime require() in a live suite. The anthropic Type: openai assertion is fixed by main's stripAnsi (merged in).
  • cloud-onboard: fixed by main's skip-guard (the generic cloud-onboard sandbox is OpenClaw, so the DCode check now skips). This PR also adds the observability that surfaced the original swallowed dcode: command not found, plus an absolute-path dcode invocation for real DCode targets.
  • full-e2e: intermittent 5s cold-onboard budget overshoot; passed cleanly this run.

Remaining: openshell-gateway-upgrade

Deliberately not fixed here — see the detailed analysis above. It's a fail-closed behavior conflict between #6305 (recovery broadening) and #6132 (fingerprint gate) that the E2E test contradicts; inverting it would rewrite the named "fails closed" safety tests. Needs @laitingsheng / @ericksoa to choose the test-side vs product-side resolution.

@cv
cv merged commit b15eb2d into main Jul 6, 2026
122 of 126 checks passed
@cv
cv deleted the fix/e2e-live-source-require-hook branch July 6, 2026 23:39
@cv

cv commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Following up on the owner decision for openshell-gateway-upgrade: the product-side resolution is implemented in #6362 without weakening ordinary fail-closed rebuild behavior.

Legacy recovery is limited to exact confirmed managed sandbox names after a complete backup, requires the canonical provider/credential binding and an available host credential, revalidates provider absence and credential availability at the delete edge, and verifies the exact route after reconstruction.

The exact-head gateway-upgrade E2E rerun passed 4/4 tests, including the old OpenClaw upgrade and survivor-state restoration.

Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…board dcode identity failure (NVIDIA#6343)

## Summary
Fixes two E2E-harness problems surfaced by the E2E dispatch on `main`:

1. **e2e-live source require hook** — the `hermes-inference-switch` live
suite (added in NVIDIA#6335) failed at collection with `Cannot find module
'../runner'` because the `e2e-live` Vitest project never loaded the
typed-source require hook.
2. **cloud-onboard observability** — the cloud-experimental check-04
(added in NVIDIA#6332) fails on `main` with `could not read initial dcode
identity`, but the real `dcode identity` error is captured into a shell
var and discarded, so it can't be diagnosed.

## Changes
- `vitest.config.ts`: add `setupFiles:
["test/helpers/onboard-script-mocks.cjs"]` to the `e2e-live` project
(mirrors `cli`). Registers the typed-source `.ts` require hook
**in-process** — deliberately not via `env.NODE_OPTIONS`, so `--require`
never leaks into the real CLI subprocesses live tests spawn.
-
`test/e2e/e2e-cloud-experimental/checks/04-deepagents-code-fresh-reonboard.sh`:
on both `dcode identity` reads, print the captured stdout+stderr to
stdout (so it lands in `result.json`) before `fail`. No change to
pass/fail logic — only makes the existing failure observable.

### Root cause — rh-ai-quickstart#1 (fixed here)
`hermes-inference-switch` is the first `e2e-live` suite to import a deep
`src` graph — its helpers import `src/lib/inference/config.ts`, which
transitively loads `ollama-runtime-context.ts`'s runtime
`require("../runner")`. Without the require hook, Node's native CJS
resolver can't resolve the extensionless `.ts` → suite throws at
collection (`0 tests`, ~37s). Only this suite hit it; others drive the
CLI as a subprocess and import only fixtures. `runner.ts` has no
circular dependency on the inference graph, so the in-process hook
resolves it fully.

### Root cause — NVIDIA#2 (observability only; product root cause pending)
`cloud-onboard` was green on `main` through 2026-07-06 00:56 UTC and
failed on the first main E2E after NVIDIA#6332 landed (19:50 UTC) — NVIDIA#6332
added check-04, which has never passed on main. `dcode identity` is the
NemoClaw wrapper (`agents/langchain-deepagents-code/dcode-wrapper.sh`);
its identity path returns 0 in isolation and NVIDIA#6332 did not modify it, so
the non-zero exit is a runtime condition in NVIDIA#6332's new "recreate/verify
live identity" onboarding flow. That can't be pinned without the
swallowed stderr — which this change surfaces. Product root cause is for
the NVIDIA#6332 author to fix once the next run shows the real error.

## Type of Change
- [x] Code change (feature, bug fix, or refactor)

## Quality Gates
- [x] Existing tests cover changed behavior — justification: both
changes are E2E-harness config/diagnostics. rh-ai-quickstart#1: the `e2e-live` suites
exercise the hook — verified the previously-failing
`hermes-inference-switch` suite now collects and all `e2e-live` suites
report 0 collection errors, and the full `e2e-all` dispatch ran the
switch job (hosted) green. NVIDIA#2: pure diagnostic output; no pass/fail
change.
- [x] Docs not applicable — justification: internal test-harness
config/diagnostics; no user-facing behavior.
- [x] Sensitive paths changed (onboarding/inference/runner adjacent via
test config)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — justification: changes are confined to Vitest test-runner
setup (`vitest.config.ts`) and an E2E diagnostic print; no product
runtime code path is altered. The require hook is in-process only and
does not touch product CLI subprocesses.

## Verification
- [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 —
note: `pre-push` `tsc-cli` skipped for a pre-existing local-only
`noImplicitAny` false-positive in
`test/helpers/mcp-lifecycle-lock-properties.ts` and
`src/lib/state/mcp-lifecycle-lock-identity.test.ts` (unrelated to this
diff); CI `tsc-cli` covers it. Check-04 passed `bash -n` and the
`shellcheck`/`shfmt` pre-commit hooks.
- [x] Targeted behavior tests pass — command/result:
`NEMOCLAW_RUN_LIVE_E2E=1 npx vitest list --project e2e-live` → switch
suite collects; whole project 0 collection errors (before the fix it
reproduced `Cannot find module '../runner'`). Full `e2e-all` dispatch on
this branch ran `hermes-inference-switch (hosted)` green.
- [x] No secrets, API keys, or credentials committed

---
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved reliability of live end-to-end test runs by loading the
required hook inside the test process, avoiding leakage into real CLI
subprocesses.
* Made identity checks in sandboxed cloud experimental flows more
robust, with clearer diagnostics when identity lookup fails.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@senthilr-nv senthilr-nv added v0.0.89 Release target and removed v0.0.88 v0.0.89 Release target labels Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants