Skip to content

perf(e2e): extend trace timing artifacts to Vitest targets - #6153

Merged
cv merged 13 commits into
mainfrom
5341-perf-e2e-extend-trace-timing-artifacts-to-vitest-scenarios
Jul 3, 2026
Merged

perf(e2e): extend trace timing artifacts to Vitest targets#6153
cv merged 13 commits into
mainfrom
5341-perf-e2e-extend-trace-timing-artifacts-to-vitest-scenarios

Conversation

@amata-human

@amata-human amata-human commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds sanitized onboard trace timing artifacts to registry-driven Vitest live E2E targets so per-target timing evidence is uploaded without exposing raw traces.
Keeps Slack/GitHub timing aggregation scoped to the dedicated cloud-onboard artifact.

Related Issue

Fixes #5341

Changes

  • Configure NEMOCLAW_TRACE_DIR for each live matrix target via a workflow step, then sanitize trace timing and delete raw traces before upload.
  • Extend the live artifact upload allowlist and reusable upload-action contract to include only cloud-onboard-trace-timing-summary.json for trace timing.
  • Pass only NEMOCLAW_TRACE_DIR through the E2E fixture child-env boundary, without broad NEMOCLAW_TRACE_* or secret passthrough.
  • Add workflow, upload-contract, and fixture-env tests for trace setup, sanitizer, cleanup, ordering, and upload path guarantees.
  • Update E2E developer docs and contributor-agent PR guidance for the new artifact path and validation expectations.
  • Stabilize existing service-env and state-dir-guard tests observed during full-suite validation.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Self-review focused on workflow runner cleanup, artifact allowlist, and fixture env boundary. Tests assert only NEMOCLAW_TRACE_DIR crosses into child commands and raw trace paths are never uploaded.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Angel Mata amata@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added per-target live E2E onboard trace timing summaries by generating sanitized timing evidence and uploading it per target.
  • Bug Fixes
    • Hardened the live E2E workflow with target-scoped trace directories, forced sanitization and cleanup, and stricter per-target artifact upload paths.
  • Documentation
    • Updated E2E CI guidance and migration/retirement docs to reflect the new trace-evidence artifact locations and inputs for timing comparisons.
    • Updated targeted test instructions for E2E workflow-related changes.
  • Tests
    • Expanded boundary, workflow-trace, artifact allowlist, fixture environment propagation, and trace sanitization coverage.

Signed-off-by: Angel Mata <amata@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds live-target trace directory setup, sanitized timing summary generation, raw trace cleanup, artifact upload updates, boundary validation, fixture env passthrough, and matching test and documentation changes.

Changes

Live E2E trace timing pipeline

Layer / File(s) Summary
Workflow trace setup, sanitize, cleanup, and upload
.github/workflows/e2e.yaml, tools/e2e/workflow-boundary.mts, tools/e2e/upload-e2e-artifacts-workflow-boundary.mts
Sets and validates per-target raw trace directories, adds sanitizer and cleanup steps, and extends the live artifact upload contract with the per-target timing summary path.
Workflow and validator tests
test/e2e/support/e2e-workflow-trace.test.ts, test/e2e/support/e2e-workflow.test.ts, test/e2e/support/e2e-fixture-context.test.ts, test/e2e/support/e2e-redaction-entry.test.ts, test/e2e/support/e2e-scorecard.test.ts
Adds negative trace-boundary tests and updates existing workflow assertions for step order, guard fragments, artifact paths, env passthrough, and trace-summary sanitization behavior.
Fixture env allowlist and docs
test/e2e/fixtures/redaction.ts, test/e2e/README.md, test/e2e/docs/MIGRATION.md, test/e2e/docs/README.md, test/e2e/docs/RETIREMENT.md, .agents/skills/nemoclaw-contributor-create-pr/SKILL.md
Allows NEMOCLAW_TRACE_DIR through fixture child env construction and documents the per-target timing summary artifact path, raw trace lifecycle, scorecard comparison scope, and targeted test guidance.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#6012: Shares the same E2E cloud-onboard trace sanitization flow, including NEMOCLAW_TRACE_DIR, sanitize-trace-timing.py, and strict workflow-boundary checks.

Suggested labels: area: e2e, feature

Suggested reviewers: ericksoa, jason-ma-nv

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR changes live E2E artifact handling and summaries, but the linked issue requires Vitest scenario traces under the existing Vitest artifact structure. Retarget the workflow, docs, and tests to e2e-artifacts/vitest//traces/ and preserve the existing Vitest artifact upload contract.
Out of Scope Changes check ⚠️ Warning The new live-target trace-summary workflow, artifact path changes, and docs are outside the linked issue's Vitest-scenario scope. Remove or move the live-target timing-summary work unless it is explicitly part of the Vitest scenario trace-artifact requirement.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and clearly points to extending E2E trace timing artifacts for Vitest targets.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 5341-perf-e2e-extend-trace-timing-artifacts-to-vitest-scenarios

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

@github-actions

github-actions Bot commented Jul 1, 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: Issue #5341 acceptance clause drift — scope mismatch between issue and PR delivery; then add or justify PRA-T1.
Open items: 3 required · 7 warnings · 2 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 7 still apply · 3 new items found

Action checklist

  • PRA-2 Fix: Issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 acceptance clause drift — scope mismatch between issue and PR delivery in test/e2e/README.md:41
  • PRA-3 Fix: Artifact path structure differs from issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 expectation without documented justification in .github/workflows/e2e.yaml:417
  • PRA-4 Fix: Post-hoc trace sanitizer lacks completed source-of-truth review — 5-element analysis incomplete in scripts/e2e/sanitize-trace-timing.py:1
  • PRA-1 Resolve or justify: Source-of-truth review needed: scripts/e2e/sanitize-trace-timing.py (post-hoc trace sanitizer)
  • PRA-5 Resolve or justify: Duplicate source guard ordering validation logic between workflow boundary modules in tools/e2e/operations-workflow-boundary.mts:332
  • PRA-6 Resolve or justify: Sanitizer output permissions set but artifact directory permissions unverified in scripts/e2e/sanitize-trace-timing.py:130
  • PRA-7 Resolve or justify: Workflow boundary module growing — trace validation not extracted to dedicated module in tools/e2e/workflow-boundary.mts:3765
  • PRA-8 Resolve or justify: Live job step ordering lacks explicit inline documentation for trace setup/sanitize/cleanup sequence in .github/workflows/e2e.yaml:255
  • PRA-9 Resolve or justify: Missing explicit test for slowest_spans cap at 10 (MAX_SLOWEST_SPANS) in test/e2e/support/e2e-scorecard.test.ts:1
  • PRA-12 Resolve or justify: Cloud-onboard trace step ordering not covered by workflow trace tests in test/e2e/support/e2e-workflow-trace.test.ts:1
  • 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: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Missing explicit test for slowest_spans cap at 10 (MAX_SLOWEST_SPANS)
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause
  • PRA-10 In-scope improvement: Extract shared requireRunFragmentBefore helper to common module in tools/e2e/operations-workflow-boundary.mts:332
  • PRA-11 In-scope improvement: Doc naming consistency for trace summary files across targets in test/e2e/docs/MIGRATION.md:30

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 acceptance test/e2e/README.md:41 Update PR description to explicitly scope acceptance: either (a) note this PR covers registry-driven live matrix only and issue #5341 needs update, or (b) extend same pattern to remaining Vitest scenario suite targets in this PR. Do not claim 'Fixes #5341' without clarification.
PRA-3 Required acceptance .github/workflows/e2e.yaml:417 Document path difference in PR description or update issue #5341. If vitest/ prefix required by consumers, extend pattern to remaining scenario suite targets. Otherwise confirm current path is intentional for consolidated workflow.
PRA-4 Required architecture scripts/e2e/sanitize-trace-timing.py:1 Add to PR description or follow-up issue: (1) invalid state = sensitive data in raw traces, (2) source = TraceCollector.flush in src/lib/trace.ts, (3) why source fix deferred = local diagnosis needs full traces (make explicit), (4) add emission-boundary regression test for sanitizeTraceAttributes, (5) removal condition = when TraceArtifact emits timingOnly schema (define schema flag).
PRA-5 Resolve/justify security tools/e2e/operations-workflow-boundary.mts:332 Extract shared requireRunFragmentBefore helper to common module (e.g., tools/e2e/workflow-validate.mts) and import in both boundary files. Keep validation logic DRY.
PRA-6 Resolve/justify security scripts/e2e/sanitize-trace-timing.py:130 Verify prepare-e2e action creates E2E_ARTIFACT_DIR with restrictive permissions (mkdir -m 0700). If not, add it in workflow or fixture layer. Document assumption if GitHub-hosted runner isolation is deemed sufficient.
PRA-7 Resolve/justify architecture tools/e2e/workflow-boundary.mts:3765 Extract trace-specific validation helpers to tools/e2e/workflow-trace-boundary.mts now. Include: source guard ordering, step sequence validation, script path pinning, upload path allowlist for trace summaries. Both workflow-boundary.mts and operations-workflow-boundary.mts import from it.
PRA-8 Resolve/justify workflow .github/workflows/e2e.yaml:255 Add inline workflow comments at each trace-related step explaining the ordering constraint. Example: '# Trace setup before workspace prep: ensures child commands write raw traces to RUNNER_TEMP, not upload roots' before 'Configure live E2E trace directory'.
PRA-9 Resolve/justify tests test/e2e/support/e2e-scorecard.test.ts:1 Add test case in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts with >10 valid slowest spans in trace summary; verify output contains exactly 10, sorted by duration.
PRA-10 Improvement architecture tools/e2e/operations-workflow-boundary.mts:332 Create tools/e2e/workflow-validate.mts with shared requireRunFragmentBefore. Import in both boundary files.
PRA-11 Improvement docs test/e2e/docs/MIGRATION.md:30 Optional: rename output file to live-trace-timing-summary.json in sanitizer (OUTPUT_FILE constant) and update all references. Low priority, can be follow-up.
PRA-12 Resolve/justify security test/e2e/support/e2e-workflow-trace.test.ts:1 Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests.

🚨 Required before merge

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

PRA-2 Required — Issue #5341 acceptance clause drift — scope mismatch between issue and PR delivery

PRA-3 Required — Artifact path structure differs from issue #5341 expectation without documented justification

  • Location: .github/workflows/e2e.yaml:417
  • Category: acceptance
  • Problem: Issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 expects trace artifacts under e2e-artifacts/vitest/<scenario>/traces/. PR uploads single sanitized summary at e2e-artifacts/live/${{ matrix.id }}/cloud-onboard-trace-timing-summary.json. Path difference is real and undocumented in PR description.
  • Impact: Consumers expecting Vitest scenario artifact layout will not find trace timing summaries. Scorecard comparison remains tied to dedicated cloud-onboard artifact, so aggregation is stable, but per-target evidence location differs.
  • Required action: Document path difference in PR description or update issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341. If vitest/ prefix required by consumers, extend pattern to remaining scenario suite targets. Otherwise confirm current path is intentional for consolidated workflow.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check upload-e2e-artifacts step in .github/workflows/e2e.yaml line 417 for cloud-onboard-trace-timing-summary.json path. Compare with issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 'e2e-artifacts/vitest/...' expectation.
  • Missing regression test: N/A — path contract documentation, not testable behavior
  • Done when: The required change is committed and verification passes: Check upload-e2e-artifacts step in .github/workflows/e2e.yaml line 417 for cloud-onboard-trace-timing-summary.json path. Compare with issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 'e2e-artifacts/vitest/...' expectation.
  • Evidence: Workflow upload step includes 'e2e-artifacts/live/${{ matrix.id }}/cloud-onboard-trace-timing-summary.json'. Issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341: 'e2e-artifacts/vitest/<scenario-or-job>/traces/'. README acknowledges mapping.

PRA-4 Required — Post-hoc trace sanitizer lacks completed source-of-truth review — 5-element analysis incomplete

  • Location: scripts/e2e/sanitize-trace-timing.py:1
  • Category: architecture
  • Problem: Sanitizer strips sensitive data after emission rather than preventing collection at source. Required 5-element source-of-truth analysis missing: (3) explicit justification why emission-side fix is out of scope, (4) regression test at emission boundary (src/lib/trace.ts TraceCollector.flush) verifying sanitizeTraceAttributes doesn't leak sensitive keys, (5) concrete removal condition (e.g., 'when TraceArtifact emits timingOnly schema').
  • Impact: Workaround may preserve invalid state indefinitely if source boundary is unclear. If onboard telemetry can be fixed to not emit sensitive data, sanitizer becomes defense-in-depth rather than primary control. No removal trigger documented.
  • Required action: Add to PR description or follow-up issue: (1) invalid state = sensitive data in raw traces, (2) source = TraceCollector.flush in src/lib/trace.ts, (3) why source fix deferred = local diagnosis needs full traces (make explicit), (4) add emission-boundary regression test for sanitizeTraceAttributes, (5) removal condition = when TraceArtifact emits timingOnly schema (define schema flag).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read sanitize-trace-timing.py header comment and PR description for the five elements. Check src/lib/trace.ts for TraceCollector.flush and any sanitizeTraceAttributes function.
  • Missing regression test: Emission-boundary test in src/lib/trace.test.ts verifying sanitizeTraceAttributes (or equivalent) redacts sensitive keys before trace emission
  • Done when: The required change is committed and verification passes: Read sanitize-trace-timing.py header comment and PR description for the five elements. Check src/lib/trace.ts for TraceCollector.flush and any sanitizeTraceAttributes function.
  • Evidence: Sanitizer header comment notes 'raw trace shape is produced by src/lib/trace.ts TraceArtifact' and 'If the producer grows a timing-only artifact, this post-run reducer can be removed' but does not specify the three missing elements. PR description does not address them.
Review findings by urgency: 3 required fixes, 7 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: scripts/e2e/sanitize-trace-timing.py (post-hoc trace sanitizer)

  • 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: Emission-boundary test in src/lib/trace.test.ts verifying sanitizeTraceAttributes (or equivalent) redacts sensitive keys before trace emission
  • 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: Sanitizer header comment acknowledges source but lacks three required elements. PR description does not address them. No test in src/lib/trace.test.ts for emission redaction.

PRA-5 Resolve/justify — Duplicate source guard ordering validation logic between workflow boundary modules

  • Location: tools/e2e/operations-workflow-boundary.mts:332
  • Category: security
  • Problem: workflow-boundary.mts has shared requireRunFragmentBefore helper (line 375); operations-workflow-boundary.mts has manual indexOf check (lines 332-339) for cloud-onboard trace sanitizer source guard ordering.
  • Impact: DRY violation increases maintenance burden and risk of divergent validation logic. If guard ordering requirement changes, both locations must be updated consistently.
  • Recommended action: Extract shared requireRunFragmentBefore helper to common module (e.g., tools/e2e/workflow-validate.mts) and import in both boundary files. Keep validation logic DRY.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare workflow-boundary.mts:375-390 (requireRunFragmentBefore) with operations-workflow-boundary.mts:332-339 (manual sourceGuardIndex check).
  • Missing regression test: N/A — code structure improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare workflow-boundary.mts:375-390 (requireRunFragmentBefore) with operations-workflow-boundary.mts:332-339 (manual sourceGuardIndex check).
  • Evidence: operations-workflow-boundary.mts uses manual indexOf comparison; workflow-boundary.mts uses requireRunFragmentBefore for identical ordering check on live job sanitizer.

PRA-6 Resolve/justify — Sanitizer output permissions set but artifact directory permissions unverified

  • Location: scripts/e2e/sanitize-trace-timing.py:130
  • Category: security
  • Problem: Sanitizer creates output directory with mode 0o700 and summary file with 0o600, but prepare-e2e action does not create E2E_ARTIFACT_DIR with restrictive permissions. Base artifact directory (e2e-artifacts/live/) may have default permissions.
  • Impact: If artifact directory has loose permissions, other jobs or processes on the runner could read/write artifacts before sanitizer runs. Defense-in-depth expects restrictive permissions at creation.
  • Recommended action: Verify prepare-e2e action creates E2E_ARTIFACT_DIR with restrictive permissions (mkdir -m 0700). If not, add it in workflow or fixture layer. Document assumption if GitHub-hosted runner isolation is deemed sufficient.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/actions/prepare-e2e/action.yaml for artifact directory creation. Check workflow steps for mkdir -m 0700 on e2e-artifacts/live/.
  • Missing regression test: Test that E2E_ARTIFACT_DIR is created with 0o700 permissions before any job writes to it
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/actions/prepare-e2e/action.yaml for artifact directory creation. Check workflow steps for mkdir -m 0700 on e2e-artifacts/live/.
  • Evidence: prepare-e2e action only runs npm ci and build:cli; no mkdir for artifact dir. Sanitizer creates its output subdir with 0o700 but parent e2e-artifacts/live/ is created implicitly by workflow.

PRA-7 Resolve/justify — Workflow boundary module growing — trace validation not extracted to dedicated module

  • Location: tools/e2e/workflow-boundary.mts:3765
  • Category: architecture
  • Problem: Workflow boundary module growing — trace validation logic (requireRunFragmentBefore, step ordering, source guard checks) mixed with general workflow validation. New e2e-workflow-trace.test.ts validates trace steps but validation helpers remain in workflow-boundary.mts.
  • Impact: Monolith growth makes boundary validation harder to maintain and review. Trace-specific validation should be separated for clarity and reusability.
  • Recommended action: Extract trace-specific validation helpers to tools/e2e/workflow-trace-boundary.mts now. Include: source guard ordering, step sequence validation, script path pinning, upload path allowlist for trace summaries. Both workflow-boundary.mts and operations-workflow-boundary.mts import from it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check tools/e2e/workflow-boundary.mts for trace-specific validation functions (requireRunFragmentBefore, trace step ordering checks). Verify no dedicated workflow-trace-boundary.mts exists.
  • Missing regression test: N/A — architecture improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check tools/e2e/workflow-boundary.mts for trace-specific validation functions (requireRunFragmentBefore, trace step ordering checks). Verify no dedicated workflow-trace-boundary.mts exists.
  • Evidence: requireRunFragmentBefore defined in workflow-boundary.mts:375; used for live job sanitizer validation. operations-workflow-boundary.mts duplicates logic manually. No separate trace boundary module exists.

PRA-8 Resolve/justify — Live job step ordering lacks explicit inline documentation for trace setup/sanitize/cleanup sequence

  • Location: .github/workflows/e2e.yaml:255
  • Category: workflow
  • Problem: Live job trace setup step runs before workspace preparation (correct ordering), but cloud-onboard job also has trace setup before workspace prep. Both validated by workflow-boundary tests. Inline comments explain ordering but not at each step.
  • Impact: Maintainers may accidentally reorder steps without understanding the security constraint (raw traces must stay under RUNNER_TEMP, never under upload roots). Explicit inline comments at each step reduce regression risk.
  • Recommended action: Add inline workflow comments at each trace-related step explaining the ordering constraint. Example: '# Trace setup before workspace prep: ensures child commands write raw traces to RUNNER_TEMP, not upload roots' before 'Configure live E2E trace directory'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/workflows/e2e.yaml lines 255-350 for live job trace steps. Check cloud-onboard job lines 2799-2895 for equivalent steps. Verify e2e-workflow-trace.test.ts validates both jobs.
  • Missing regression test: N/A — documentation improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e.yaml lines 255-350 for live job trace steps. Check cloud-onboard job lines 2799-2895 for equivalent steps. Verify e2e-workflow-trace.test.ts validates both jobs.
  • Evidence: Live job has comment '# Configure NEMOCLAW_TRACE_DIR before workspace prep so every child command writes raw traces under runner temp, never under upload roots.' Cloud-onboard job has similar comment but not at each step.

PRA-9 Resolve/justify — Missing explicit test for slowest_spans cap at 10 (MAX_SLOWEST_SPANS)

  • Location: test/e2e/support/e2e-scorecard.test.ts:1
  • Category: tests
  • Problem: Sanitizer limits slowest_spans to 10 entries (MAX_SLOWEST_SPANS=10) but no test verifies this bound.
  • Impact: If cap logic regresses (e.g., constant changed, loop condition broken), sanitizer could emit unbounded slowest_spans array, bloating artifact and potentially exposing more span metadata.
  • Recommended action: Add test case in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts with >10 valid slowest spans in trace summary; verify output contains exactly 10, sorted by duration.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/e2e/support/ for MAX_SLOWEST_SPANS or slowest_spans cap test. Constant is 10 in sanitize-trace-timing.py.
  • Missing regression test: Test providing 15+ valid slowest_spans in trace summary; assert output slowest_spans.length === 10 and entries sorted by duration_ms descending
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/e2e/support/ for MAX_SLOWEST_SPANS or slowest_spans cap test. Constant is 10 in sanitize-trace-timing.py.
  • Evidence: sanitize-trace-timing.py enforces MAX_SLOWEST_SPANS=10 in extract_candidate loop. No test in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts exercises this bound.

PRA-12 Resolve/justify — Cloud-onboard trace step ordering not covered by workflow trace tests

  • Location: test/e2e/support/e2e-workflow-trace.test.ts:1
  • Category: security
  • Problem: e2e-workflow-trace.test.ts validates live job trace steps (configure, sanitize, cleanup, ordering, source guard, script path). Cloud-onboard job has equivalent trace steps but no equivalent test coverage.
  • Impact: Cloud-onboard trace step ordering, source guard position, and always() guards could regress without detection. Live job is validated; cloud-onboard is not.
  • Recommended action: Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check e2e-workflow-trace.test.ts validates live job trace steps. Check if cloud-onboard trace steps have equivalent test coverage in operations-workflow-boundary tests.
  • Missing regression test: Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check e2e-workflow-trace.test.ts validates live job trace steps. Check if cloud-onboard trace steps have equivalent test coverage in operations-workflow-boundary tests.
  • Evidence: e2e-workflow-trace.test.ts only validates live job (workflow.jobs.live). Cloud-onboard job is validated in operations-workflow-boundary.mts but not with the same granular step-ordering tests.

💡 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-10 Improvement — Extract shared requireRunFragmentBefore helper to common module

  • Location: tools/e2e/operations-workflow-boundary.mts:332
  • Category: architecture
  • Problem: Shared requireRunFragmentBefore helper exists in workflow-boundary.mts but not extracted to common module. Can be done in follow-up PR since behavior unchanged.
  • Impact: Minor code duplication; maintenance risk if validation logic evolves.
  • Suggested action: Create tools/e2e/workflow-validate.mts with shared requireRunFragmentBefore. Import in both boundary files.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if tools/e2e/workflow-validate.mts exists and exports requireRunFragmentBefore.
  • Missing regression test: N/A — refactoring, not testable behavior
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: requireRunFragmentBefore defined in workflow-boundary.mts:375-390. operations-workflow-boundary.mts duplicates logic at lines 332-339.

PRA-11 Improvement — Doc naming consistency for trace summary files across targets

  • Location: test/e2e/docs/MIGRATION.md:30
  • Category: docs
  • Problem: OUTPUT_FILE constant is 'cloud-onboard-trace-timing-summary.json' but used for both cloud-onboard and live matrix targets.
  • Impact: Minor naming confusion; file name suggests cloud-onboard-specific but used generically.
  • Suggested action: Optional: rename output file to live-trace-timing-summary.json in sanitizer (OUTPUT_FILE constant) and update all references. Low priority, can be follow-up.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check OUTPUT_FILE constant in sanitize-trace-timing.py and all references in workflow YAML and tests.
  • Missing regression test: N/A — documentation improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: sanitize-trace-timing.py:15 OUTPUT_FILE = 'cloud-onboard-trace-timing-summary.json'. Used in live job upload (e2e.yaml:346) and cloud-onboard job (e2e.yaml:2858).
Simplification opportunities: 1 possible cut, net -8 lines possible

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

  • PRA-10 shrink (tools/e2e/operations-workflow-boundary.mts:332): Duplicate requireRunFragmentBefore logic in operations-workflow-boundary.mts lines 332-339
    • Replacement: Import requireRunFragmentBefore from tools/e2e/workflow-validate.mts
    • Net: -8 lines
    • Safety boundary: Must not change validation behavior — only extract and reuse existing logic
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 test providing 15+ valid slowest_spans in trace summary; assert output slowest_spans.length === 10 and entries sorted by duration_ms descending (sanitize-trace-timing.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow YAML changes, Python sanitizer, workflow boundary validators. Static tests cover logic but not full CI execution flow.
  • PRA-T2 Runtime validation — Add emission-boundary regression test for sanitizeTraceAttributes in src/lib/trace.ts verifying sensitive keys redacted at emission (src/lib/trace.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow YAML changes, Python sanitizer, workflow boundary validators. Static tests cover logic but not full CI execution flow.
  • PRA-T3 Runtime validation — Verify prepare-e2e action or workflow creates E2E_ARTIFACT_DIR with mkdir -m 0700; add test (.github/actions/prepare-e2e/action.yaml or workflow step test). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow YAML changes, Python sanitizer, workflow boundary validators. Static tests cover logic but not full CI execution flow.
  • PRA-T4 Runtime validation — Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests. Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow YAML changes, Python sanitizer, workflow boundary validators. Static tests cover logic but not full CI execution flow.
  • PRA-T5 Runtime validation — Add source guard before Python read test for cloud-onboard job (parity with live job). Runtime/sandbox/infrastructure paths need behavioral runtime validation: workflow YAML changes, Python sanitizer, workflow boundary validators. Static tests cover logic but not full CI execution flow.
  • PRA-T6 Missing explicit test for slowest_spans cap at 10 (MAX_SLOWEST_SPANS) — Add test case in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts with >10 valid slowest spans in trace summary; verify output contains exactly 10, sorted by duration.
  • PRA-T7 Acceptance clause — Sanitized trace artifacts for Vitest scenario suite — add test evidence or identify existing coverage. PR adds trace collection to registry-driven live matrix (registry-targets.test.ts), not legacy Vitest scenario suite (e2e-vitest-scenarios.yaml). Different target set.
  • PRA-T8 Acceptance clause — Upload under e2e-artifacts/vitest/<scenario>/traces/ — add test evidence or identify existing coverage. PR uploads to e2e-artifacts/live/${{ matrix.id }}/cloud-onboard-trace-timing-summary.json. Path structure and parent directory differ from issue expectation.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: scripts/e2e/sanitize-trace-timing.py (post-hoc trace sanitizer)

  • 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: Emission-boundary test in src/lib/trace.test.ts verifying sanitizeTraceAttributes (or equivalent) redacts sensitive keys before trace emission
  • 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: Sanitizer header comment acknowledges source but lacks three required elements. PR description does not address them. No test in src/lib/trace.test.ts for emission redaction.

PRA-2 Required — Issue #5341 acceptance clause drift — scope mismatch between issue and PR delivery

PRA-3 Required — Artifact path structure differs from issue #5341 expectation without documented justification

  • Location: .github/workflows/e2e.yaml:417
  • Category: acceptance
  • Problem: Issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 expects trace artifacts under e2e-artifacts/vitest/<scenario>/traces/. PR uploads single sanitized summary at e2e-artifacts/live/${{ matrix.id }}/cloud-onboard-trace-timing-summary.json. Path difference is real and undocumented in PR description.
  • Impact: Consumers expecting Vitest scenario artifact layout will not find trace timing summaries. Scorecard comparison remains tied to dedicated cloud-onboard artifact, so aggregation is stable, but per-target evidence location differs.
  • Required action: Document path difference in PR description or update issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341. If vitest/ prefix required by consumers, extend pattern to remaining scenario suite targets. Otherwise confirm current path is intentional for consolidated workflow.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check upload-e2e-artifacts step in .github/workflows/e2e.yaml line 417 for cloud-onboard-trace-timing-summary.json path. Compare with issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 'e2e-artifacts/vitest/...' expectation.
  • Missing regression test: N/A — path contract documentation, not testable behavior
  • Done when: The required change is committed and verification passes: Check upload-e2e-artifacts step in .github/workflows/e2e.yaml line 417 for cloud-onboard-trace-timing-summary.json path. Compare with issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341 'e2e-artifacts/vitest/...' expectation.
  • Evidence: Workflow upload step includes 'e2e-artifacts/live/${{ matrix.id }}/cloud-onboard-trace-timing-summary.json'. Issue perf(e2e): extend trace timing artifacts to Vitest scenarios #5341: 'e2e-artifacts/vitest/<scenario-or-job>/traces/'. README acknowledges mapping.

PRA-4 Required — Post-hoc trace sanitizer lacks completed source-of-truth review — 5-element analysis incomplete

  • Location: scripts/e2e/sanitize-trace-timing.py:1
  • Category: architecture
  • Problem: Sanitizer strips sensitive data after emission rather than preventing collection at source. Required 5-element source-of-truth analysis missing: (3) explicit justification why emission-side fix is out of scope, (4) regression test at emission boundary (src/lib/trace.ts TraceCollector.flush) verifying sanitizeTraceAttributes doesn't leak sensitive keys, (5) concrete removal condition (e.g., 'when TraceArtifact emits timingOnly schema').
  • Impact: Workaround may preserve invalid state indefinitely if source boundary is unclear. If onboard telemetry can be fixed to not emit sensitive data, sanitizer becomes defense-in-depth rather than primary control. No removal trigger documented.
  • Required action: Add to PR description or follow-up issue: (1) invalid state = sensitive data in raw traces, (2) source = TraceCollector.flush in src/lib/trace.ts, (3) why source fix deferred = local diagnosis needs full traces (make explicit), (4) add emission-boundary regression test for sanitizeTraceAttributes, (5) removal condition = when TraceArtifact emits timingOnly schema (define schema flag).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read sanitize-trace-timing.py header comment and PR description for the five elements. Check src/lib/trace.ts for TraceCollector.flush and any sanitizeTraceAttributes function.
  • Missing regression test: Emission-boundary test in src/lib/trace.test.ts verifying sanitizeTraceAttributes (or equivalent) redacts sensitive keys before trace emission
  • Done when: The required change is committed and verification passes: Read sanitize-trace-timing.py header comment and PR description for the five elements. Check src/lib/trace.ts for TraceCollector.flush and any sanitizeTraceAttributes function.
  • Evidence: Sanitizer header comment notes 'raw trace shape is produced by src/lib/trace.ts TraceArtifact' and 'If the producer grows a timing-only artifact, this post-run reducer can be removed' but does not specify the three missing elements. PR description does not address them.

PRA-5 Resolve/justify — Duplicate source guard ordering validation logic between workflow boundary modules

  • Location: tools/e2e/operations-workflow-boundary.mts:332
  • Category: security
  • Problem: workflow-boundary.mts has shared requireRunFragmentBefore helper (line 375); operations-workflow-boundary.mts has manual indexOf check (lines 332-339) for cloud-onboard trace sanitizer source guard ordering.
  • Impact: DRY violation increases maintenance burden and risk of divergent validation logic. If guard ordering requirement changes, both locations must be updated consistently.
  • Recommended action: Extract shared requireRunFragmentBefore helper to common module (e.g., tools/e2e/workflow-validate.mts) and import in both boundary files. Keep validation logic DRY.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare workflow-boundary.mts:375-390 (requireRunFragmentBefore) with operations-workflow-boundary.mts:332-339 (manual sourceGuardIndex check).
  • Missing regression test: N/A — code structure improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare workflow-boundary.mts:375-390 (requireRunFragmentBefore) with operations-workflow-boundary.mts:332-339 (manual sourceGuardIndex check).
  • Evidence: operations-workflow-boundary.mts uses manual indexOf comparison; workflow-boundary.mts uses requireRunFragmentBefore for identical ordering check on live job sanitizer.

PRA-6 Resolve/justify — Sanitizer output permissions set but artifact directory permissions unverified

  • Location: scripts/e2e/sanitize-trace-timing.py:130
  • Category: security
  • Problem: Sanitizer creates output directory with mode 0o700 and summary file with 0o600, but prepare-e2e action does not create E2E_ARTIFACT_DIR with restrictive permissions. Base artifact directory (e2e-artifacts/live/) may have default permissions.
  • Impact: If artifact directory has loose permissions, other jobs or processes on the runner could read/write artifacts before sanitizer runs. Defense-in-depth expects restrictive permissions at creation.
  • Recommended action: Verify prepare-e2e action creates E2E_ARTIFACT_DIR with restrictive permissions (mkdir -m 0700). If not, add it in workflow or fixture layer. Document assumption if GitHub-hosted runner isolation is deemed sufficient.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/actions/prepare-e2e/action.yaml for artifact directory creation. Check workflow steps for mkdir -m 0700 on e2e-artifacts/live/.
  • Missing regression test: Test that E2E_ARTIFACT_DIR is created with 0o700 permissions before any job writes to it
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/actions/prepare-e2e/action.yaml for artifact directory creation. Check workflow steps for mkdir -m 0700 on e2e-artifacts/live/.
  • Evidence: prepare-e2e action only runs npm ci and build:cli; no mkdir for artifact dir. Sanitizer creates its output subdir with 0o700 but parent e2e-artifacts/live/ is created implicitly by workflow.

PRA-7 Resolve/justify — Workflow boundary module growing — trace validation not extracted to dedicated module

  • Location: tools/e2e/workflow-boundary.mts:3765
  • Category: architecture
  • Problem: Workflow boundary module growing — trace validation logic (requireRunFragmentBefore, step ordering, source guard checks) mixed with general workflow validation. New e2e-workflow-trace.test.ts validates trace steps but validation helpers remain in workflow-boundary.mts.
  • Impact: Monolith growth makes boundary validation harder to maintain and review. Trace-specific validation should be separated for clarity and reusability.
  • Recommended action: Extract trace-specific validation helpers to tools/e2e/workflow-trace-boundary.mts now. Include: source guard ordering, step sequence validation, script path pinning, upload path allowlist for trace summaries. Both workflow-boundary.mts and operations-workflow-boundary.mts import from it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check tools/e2e/workflow-boundary.mts for trace-specific validation functions (requireRunFragmentBefore, trace step ordering checks). Verify no dedicated workflow-trace-boundary.mts exists.
  • Missing regression test: N/A — architecture improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check tools/e2e/workflow-boundary.mts for trace-specific validation functions (requireRunFragmentBefore, trace step ordering checks). Verify no dedicated workflow-trace-boundary.mts exists.
  • Evidence: requireRunFragmentBefore defined in workflow-boundary.mts:375; used for live job sanitizer validation. operations-workflow-boundary.mts duplicates logic manually. No separate trace boundary module exists.

PRA-8 Resolve/justify — Live job step ordering lacks explicit inline documentation for trace setup/sanitize/cleanup sequence

  • Location: .github/workflows/e2e.yaml:255
  • Category: workflow
  • Problem: Live job trace setup step runs before workspace preparation (correct ordering), but cloud-onboard job also has trace setup before workspace prep. Both validated by workflow-boundary tests. Inline comments explain ordering but not at each step.
  • Impact: Maintainers may accidentally reorder steps without understanding the security constraint (raw traces must stay under RUNNER_TEMP, never under upload roots). Explicit inline comments at each step reduce regression risk.
  • Recommended action: Add inline workflow comments at each trace-related step explaining the ordering constraint. Example: '# Trace setup before workspace prep: ensures child commands write raw traces to RUNNER_TEMP, not upload roots' before 'Configure live E2E trace directory'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/workflows/e2e.yaml lines 255-350 for live job trace steps. Check cloud-onboard job lines 2799-2895 for equivalent steps. Verify e2e-workflow-trace.test.ts validates both jobs.
  • Missing regression test: N/A — documentation improvement, not testable behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e.yaml lines 255-350 for live job trace steps. Check cloud-onboard job lines 2799-2895 for equivalent steps. Verify e2e-workflow-trace.test.ts validates both jobs.
  • Evidence: Live job has comment '# Configure NEMOCLAW_TRACE_DIR before workspace prep so every child command writes raw traces under runner temp, never under upload roots.' Cloud-onboard job has similar comment but not at each step.

PRA-9 Resolve/justify — Missing explicit test for slowest_spans cap at 10 (MAX_SLOWEST_SPANS)

  • Location: test/e2e/support/e2e-scorecard.test.ts:1
  • Category: tests
  • Problem: Sanitizer limits slowest_spans to 10 entries (MAX_SLOWEST_SPANS=10) but no test verifies this bound.
  • Impact: If cap logic regresses (e.g., constant changed, loop condition broken), sanitizer could emit unbounded slowest_spans array, bloating artifact and potentially exposing more span metadata.
  • Recommended action: Add test case in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts with >10 valid slowest spans in trace summary; verify output contains exactly 10, sorted by duration.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/e2e/support/ for MAX_SLOWEST_SPANS or slowest_spans cap test. Constant is 10 in sanitize-trace-timing.py.
  • Missing regression test: Test providing 15+ valid slowest_spans in trace summary; assert output slowest_spans.length === 10 and entries sorted by duration_ms descending
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/e2e/support/ for MAX_SLOWEST_SPANS or slowest_spans cap test. Constant is 10 in sanitize-trace-timing.py.
  • Evidence: sanitize-trace-timing.py enforces MAX_SLOWEST_SPANS=10 in extract_candidate loop. No test in sanitize-trace-timing.test.ts or e2e-scorecard.test.ts exercises this bound.

PRA-10 Improvement — Extract shared requireRunFragmentBefore helper to common module

  • Location: tools/e2e/operations-workflow-boundary.mts:332
  • Category: architecture
  • Problem: Shared requireRunFragmentBefore helper exists in workflow-boundary.mts but not extracted to common module. Can be done in follow-up PR since behavior unchanged.
  • Impact: Minor code duplication; maintenance risk if validation logic evolves.
  • Suggested action: Create tools/e2e/workflow-validate.mts with shared requireRunFragmentBefore. Import in both boundary files.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if tools/e2e/workflow-validate.mts exists and exports requireRunFragmentBefore.
  • Missing regression test: N/A — refactoring, not testable behavior
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: requireRunFragmentBefore defined in workflow-boundary.mts:375-390. operations-workflow-boundary.mts duplicates logic at lines 332-339.

PRA-11 Improvement — Doc naming consistency for trace summary files across targets

  • Location: test/e2e/docs/MIGRATION.md:30
  • Category: docs
  • Problem: OUTPUT_FILE constant is 'cloud-onboard-trace-timing-summary.json' but used for both cloud-onboard and live matrix targets.
  • Impact: Minor naming confusion; file name suggests cloud-onboard-specific but used generically.
  • Suggested action: Optional: rename output file to live-trace-timing-summary.json in sanitizer (OUTPUT_FILE constant) and update all references. Low priority, can be follow-up.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check OUTPUT_FILE constant in sanitize-trace-timing.py and all references in workflow YAML and tests.
  • Missing regression test: N/A — documentation improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: sanitize-trace-timing.py:15 OUTPUT_FILE = 'cloud-onboard-trace-timing-summary.json'. Used in live job upload (e2e.yaml:346) and cloud-onboard job (e2e.yaml:2858).

PRA-12 Resolve/justify — Cloud-onboard trace step ordering not covered by workflow trace tests

  • Location: test/e2e/support/e2e-workflow-trace.test.ts:1
  • Category: security
  • Problem: e2e-workflow-trace.test.ts validates live job trace steps (configure, sanitize, cleanup, ordering, source guard, script path). Cloud-onboard job has equivalent trace steps but no equivalent test coverage.
  • Impact: Cloud-onboard trace step ordering, source guard position, and always() guards could regress without detection. Live job is validated; cloud-onboard is not.
  • Recommended action: Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check e2e-workflow-trace.test.ts validates live job trace steps. Check if cloud-onboard trace steps have equivalent test coverage in operations-workflow-boundary tests.
  • Missing regression test: Add cloud-onboard trace step ordering test to e2e-workflow-trace.test.ts or operations-workflow-boundary tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check e2e-workflow-trace.test.ts validates live job trace steps. Check if cloud-onboard trace steps have equivalent test coverage in operations-workflow-boundary tests.
  • Evidence: e2e-workflow-trace.test.ts only validates live job (workflow.jobs.live). Cloud-onboard job is validated in operations-workflow-boundary.mts but not with the same granular step-ordering tests.

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 1, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard, ubuntu-repo-cloud-openclaw
Optional E2E: credential-sanitization

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): Required because the dedicated cloud-onboard workflow path now validates NEMOCLAW_TRACE_DIR against RUNNER_TEMP before sanitizing and deleting raw traces. This is the full hosted onboarding flow and the timing artifact used by the E2E scorecard.
  • ubuntu-repo-cloud-openclaw (high): Required as a representative registry-driven live matrix target because the live job now sets per-target trace directories, sanitizes timing summaries, deletes raw traces, and uploads the new per-target timing summary artifact. This validates the changed generic live-matrix workflow path against a real hosted OpenClaw onboarding target.

Optional E2E

  • credential-sanitization (medium): Optional adjacent confidence for secret-handling behavior because the PR changes E2E fixture environment allowlisting and raw-trace artifact sanitization, but the main changed paths are better covered by cloud-onboard and a registry-driven live target.

New E2E recommendations

  • None.

@github-code-quality

github-code-quality Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the 5341-perf-e2e-extend... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main 5341-perf-e2e-extend... c2eccd8 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/blueprint/ssrf.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 5341-perf-e2e-extend... branch is 69%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main 5341-perf-e2e-extend... c2eccd8 +/-
src/lib/actions...dbox/rebuild.ts 82%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/shields/index.ts 75%
src/lib/state/sandbox.ts 73%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/policy/index.ts 56%
src/lib/onboard.ts 20%

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

@github-actions

github-actions Bot commented Jul 1, 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: The PR changes shared E2E target workflow machinery, live matrix trace/artifact handling, the trace sanitizer used by workflow artifacts, and shared fixture environment redaction support. These surfaces affect all registry-driven live targets, so run the full e2e.yaml fan-out.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref>

Optional E2E targets

  • None.

Relevant changed files

  • .github/workflows/e2e.yaml
  • scripts/e2e/sanitize-trace-timing.py
  • test/e2e/README.md
  • test/e2e/docs/MIGRATION.md
  • test/e2e/docs/README.md
  • test/e2e/docs/RETIREMENT.md
  • test/e2e/fixtures/redaction.ts
  • test/e2e/support/e2e-fixture-context.test.ts
  • test/e2e/support/e2e-redaction-entry.test.ts
  • test/e2e/support/e2e-scorecard.test.ts
  • test/e2e/support/e2e-workflow-trace.test.ts
  • test/e2e/support/e2e-workflow.test.ts
  • test/e2e/support/sanitize-trace-timing.test.ts
  • tools/e2e/operations-workflow-boundary.mts
  • tools/e2e/upload-e2e-artifacts-workflow-boundary.mts
  • tools/e2e/workflow-boundary.mts

@github-actions

github-actions Bot commented Jul 1, 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: Validate the real live-matrix trace artifact path.
Open items: 0 required · 1 warning · 0 suggestions · 6 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Validate the real live-matrix trace artifact path in .github/workflows/e2e.yaml:318
  • 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: Runtime validation
  • PRA-T5 Add or justify test follow-up: Acceptance clause
  • PRA-T6 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify security .github/workflows/e2e.yaml:318 Add or identify current-PR read-only runtime/integration evidence for one representative registry-driven live target proving that the sanitized timing summary is created, raw trace JSON and `nemoclaw-e2e-traces` are absent from the uploaded artifact, and `${RUNNER_TEMP}/nemoclaw-e2e-traces/<target>` is deleted before the upload step. If maintainers intentionally accept static-only proof for this security-sensitive workflow boundary, record that rationale explicitly in the PR.
Review findings by urgency: 0 required fixes, 1 item 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 — Validate the real live-matrix trace artifact path

  • Location: .github/workflows/e2e.yaml:318
  • Category: security
  • Problem: The PR adds strong static controls for registry-driven live trace artifacts: workflow-owned `NEMOCLAW_TRACE_DIR`, guarded sanitizer and cleanup steps, an explicit upload allowlist, fixture env filtering, and workflow-boundary tests. The remaining unproven behavior crosses GitHub Actions `$GITHUB_ENV` propagation, the Vitest live target child-env boundary, actual trace emission from `nemoclaw onboard`, sanitizer execution, cleanup ordering, and the reusable upload action. Those are the runtime boundaries where raw traces could still be missed, uploaded, or left behind even when static contract tests pass.
  • Impact: A real workflow wiring regression could silently omit the per-target timing artifact or expose raw trace JSON containing prompts, paths, environment details, raw errors, or credential material through uploaded artifacts or later diagnostics.
  • Recommended action: Add or identify current-PR read-only runtime/integration evidence for one representative registry-driven live target proving that the sanitized timing summary is created, raw trace JSON and `nemoclaw-e2e-traces` are absent from the uploaded artifact, and `${RUNNER_TEMP}/nemoclaw-e2e-traces/<target>` is deleted before the upload step. If maintainers intentionally accept static-only proof for this security-sensitive workflow boundary, record that rationale explicitly in the PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect one representative live matrix run artifact file list for `e2e-artifacts/live/<target>/cloud-onboard-trace-timing-summary.json`, confirm no `nemoclaw-e2e-traces` directory and no raw trace `*.json` files are included, and inspect step logs showing `Delete raw live E2E traces` completed before `Upload E2E artifacts`.
  • Missing regression test: Add or document a behavior-specific runtime validation named `registry live target emits sanitized trace summary, excludes raw traces from artifact, and deletes RUNNER_TEMP trace dir before upload`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect one representative live matrix run artifact file list for `e2e-artifacts/live/<target>/cloud-onboard-trace-timing-summary.json`, confirm no `nemoclaw-e2e-traces` directory and no raw trace `*.json` files are included, and inspect step logs showing `Delete raw live E2E traces` completed before `Upload E2E artifacts`.
  • Evidence: The diff adds `Configure live E2E trace directory`, `Build trusted live E2E timing summary`, `Delete raw live E2E traces`, and an allowlisted summary upload in `.github/workflows/e2e.yaml`; sanitizer tests in `test/e2e/support/e2e-scorecard.test.ts` and `test/e2e/support/sanitize-trace-timing.test.ts`; env-boundary tests in `test/e2e/support/e2e-redaction-entry.test.ts`; and workflow-boundary tests in `test/e2e/support/e2e-workflow-trace.test.ts`. The previous Advisor finding requested runtime evidence for this same real workflow boundary, and the current diff still does not provide that evidence.

💡 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 — registry live target emits `e2e-artifacts/live/<target>/cloud-onboard-trace-timing-summary.json` from an onboard trace. Changed behavior crosses a real workflow/runtime boundary: `$GITHUB_ENV` propagation, Vitest child env filtering, actual onboard trace production, Python sanitization, cleanup ordering, and composite artifact upload. Static tests cover the contracts but do not execute those boundaries together.
  • PRA-T2 Runtime validation — registry live target artifact excludes `nemoclaw-e2e-traces` and raw `*.json` trace files while retaining `cloud-onboard-trace-timing-summary.json`. Changed behavior crosses a real workflow/runtime boundary: `$GITHUB_ENV` propagation, Vitest child env filtering, actual onboard trace production, Python sanitization, cleanup ordering, and composite artifact upload. Static tests cover the contracts but do not execute those boundaries together.
  • PRA-T3 Runtime validation — registry live target deletes `${RUNNER_TEMP}/nemoclaw-e2e-traces/<target>` before `Upload E2E artifacts`. Changed behavior crosses a real workflow/runtime boundary: `$GITHUB_ENV` propagation, Vitest child env filtering, actual onboard trace production, Python sanitization, cleanup ordering, and composite artifact upload. Static tests cover the contracts but do not execute those boundaries together.
  • PRA-T4 Runtime validation — registry live target child env passes `NEMOCLAW_TRACE_DIR` through `buildAvailabilityProbeEnv` into `nemoclaw onboard` without passing `NEMOCLAW_TRACE_FILE` or `NEMOCLAW_TRACE_EXPORTER`. Changed behavior crosses a real workflow/runtime boundary: `$GITHUB_ENV` propagation, Vitest child env filtering, actual onboard trace production, Python sanitization, cleanup ordering, and composite artifact upload. Static tests cover the contracts but do not execute those boundaries together.
  • PRA-T5 Acceptance clause — Scope: Extend trace artifact capture to the Vitest E2E scenario suite without changing onboard orchestration or timeout behavior. — add test evidence or identify existing coverage. The workflow adds trace setup, sanitization, cleanup, and upload steps around the existing `Run live E2E tests` step without changing `test/e2e/live/registry-targets.test.ts` orchestration or target timeouts. Runtime evidence that the active Vitest live target path produces the expected artifact is still recommended and covered by the finding.
  • PRA-T6 Acceptance clause — Acceptance Criteria: Vitest E2E scenario runs produce sanitized trace artifacts when onboarding/runtime trace spans are emitted. — add test evidence or identify existing coverage. Static diff evidence shows the intended path: `.github/workflows/e2e.yaml` configures `NEMOCLAW_TRACE_DIR`, runs `scripts/e2e/sanitize-trace-timing.py`, deletes raw traces, and uploads `cloud-onboard-trace-timing-summary.json`; sanitizer tests prove allowlisted timing output from the TraceArtifact shape. Runtime evidence for an actual registry live target remains recommended and is covered by the finding.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Validate the real live-matrix trace artifact path

  • Location: .github/workflows/e2e.yaml:318
  • Category: security
  • Problem: The PR adds strong static controls for registry-driven live trace artifacts: workflow-owned `NEMOCLAW_TRACE_DIR`, guarded sanitizer and cleanup steps, an explicit upload allowlist, fixture env filtering, and workflow-boundary tests. The remaining unproven behavior crosses GitHub Actions `$GITHUB_ENV` propagation, the Vitest live target child-env boundary, actual trace emission from `nemoclaw onboard`, sanitizer execution, cleanup ordering, and the reusable upload action. Those are the runtime boundaries where raw traces could still be missed, uploaded, or left behind even when static contract tests pass.
  • Impact: A real workflow wiring regression could silently omit the per-target timing artifact or expose raw trace JSON containing prompts, paths, environment details, raw errors, or credential material through uploaded artifacts or later diagnostics.
  • Recommended action: Add or identify current-PR read-only runtime/integration evidence for one representative registry-driven live target proving that the sanitized timing summary is created, raw trace JSON and `nemoclaw-e2e-traces` are absent from the uploaded artifact, and `${RUNNER_TEMP}/nemoclaw-e2e-traces/<target>` is deleted before the upload step. If maintainers intentionally accept static-only proof for this security-sensitive workflow boundary, record that rationale explicitly in the PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect one representative live matrix run artifact file list for `e2e-artifacts/live/<target>/cloud-onboard-trace-timing-summary.json`, confirm no `nemoclaw-e2e-traces` directory and no raw trace `*.json` files are included, and inspect step logs showing `Delete raw live E2E traces` completed before `Upload E2E artifacts`.
  • Missing regression test: Add or document a behavior-specific runtime validation named `registry live target emits sanitized trace summary, excludes raw traces from artifact, and deletes RUNNER_TEMP trace dir before upload`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect one representative live matrix run artifact file list for `e2e-artifacts/live/<target>/cloud-onboard-trace-timing-summary.json`, confirm no `nemoclaw-e2e-traces` directory and no raw trace `*.json` files are included, and inspect step logs showing `Delete raw live E2E traces` completed before `Upload E2E artifacts`.
  • Evidence: The diff adds `Configure live E2E trace directory`, `Build trusted live E2E timing summary`, `Delete raw live E2E traces`, and an allowlisted summary upload in `.github/workflows/e2e.yaml`; sanitizer tests in `test/e2e/support/e2e-scorecard.test.ts` and `test/e2e/support/sanitize-trace-timing.test.ts`; env-boundary tests in `test/e2e/support/e2e-redaction-entry.test.ts`; and workflow-boundary tests in `test/e2e/support/e2e-workflow-trace.test.ts`. The previous Advisor finding requested runtime evidence for this same real workflow boundary, and the current diff still does not provide that evidence.

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.

Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
…trace-timing-artifacts-to-vitest-scenarios-followup
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: docs Documentation, examples, guides, or docs build area: e2e End-to-end tests, nightly failures, or validation infrastructure area: observability Logging, metrics, tracing, diagnostics, or debug output feature PR adds or expands user-visible functionality labels Jul 2, 2026
@wscurran

wscurran commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

ericksoa added a commit that referenced this pull request Jul 2, 2026
## Summary

Routes ordinary native CDI Linux through OpenShell 0.0.71's native
`--gpu` path instead of the legacy Docker container swap. The legacy
path remains available for WSL, Jetson, and explicit
`NEMOCLAW_DOCKER_GPU_PATCH=1`, with its OpenShell supervisor command
boundary and rollback diagnostics hardened.

## Related Issue

Related to #6110

## Changes

- Use native OpenShell GPU injection by default on ordinary native CDI
Linux.
- Document the `NEMOCLAW_DOCKER_GPU_PATCH` auto, forced-legacy, and
native-routing behavior for ordinary native Linux, Docker Desktop WSL,
and Jetson/Tegra.
- Keep `NEMOCLAW_DOCKER_GPU_PATCH=1` as the explicit legacy-swap force
control and `=0` as the existing native opt-out; Docker Desktop WSL
still ignores `=0`, and Jetson keeps its compatibility default.
- Preserve OpenShell's supervisor entrypoint on the legacy swap: Docker
receives no command tail, while the workload stays in
`OPENSHELL_SANDBOX_COMMAND`.
- Validate legacy startup tokens before stopping or renaming the
original container and serialize extra-placeholder keys as one
comma-delimited token.
- Defer every legacy recreate through the same supervisor-wait/finalize
boundary so failed clones are captured before rollback on both create
timing paths.
- Persist only allowlisted/redacted failed-clone topology, state,
process, network, and log evidence, with a 10-second total / 2-second
per-call budget so diagnostics cannot materially delay rollback.
- Permit only the canonical OpenShell Docker/Podman TLS key path in the
Hermes runtime environment; arbitrary values and persisted `.env`
entries remain rejected.
- Refresh the Dockerfile integrity pin for the changed validator so
production Hermes images fail closed on any later digest drift.
- Prove native and legacy Docker command boundaries separately,
including Ready/CUDA status, supervisor PID 1, placeholder transport,
config hashes, no backup-container leak, and inference requests.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: independent exact-diff
review found no remaining substantive issue after startup-envelope
secret redaction, bounded pre-rollback capture, unified finalize
ordering, and route-specific live assertions
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Local exact-candidate verification at
`d76f1647a5f354ba04737a6a049b82bfbf6d5454`:

- CLI build and typecheck passed.
- Hermes GPU support/client/workflow coverage passed 48/48; the built
gateway-cleanup module resolves through Node, and runtime cleanup,
registration removal, and bind availability remain fail-closed.
- The shared Docker GPU diagnostic collector owns redaction for every
text/JSON artifact and returned summary; direct conventional `*_KEY` and
custom-placeholder canaries, JSON-validity, inspect-before-write,
exhausted-budget, and collector-owned top regressions pass.
- All 12 Docker GPU suites pass 126/126; the exact-head focused
E2E-support set passes 39/39, including six process-token self-match
regressions, the scrubbed integrity-proof environment, and total
forbidden-marker count.
- Conditional scan, source-shape, test-size, Biome, repository checks,
commit hooks, and push typecheck passed for the final harness
correction; only the documented macOS-invalid full CLI hook lane was
excluded.
- The forbidden-marker request sensor support suite passed 14/14 and
records counts only, never raw request bodies or marker values.
- OpenShell transport-boundary coverage passed 4/4; Docker GPU
command-envelope coverage passed 6/6; extra-placeholder parsing coverage
passed 16/16.
- Hermes validator and wrapper integrity pins match their source SHA256
digests; hadolint and diff checks pass.
- Hermes startup/boundary coverage passed 43/43 locally; the Linux-only
wrapper cases are delegated to exact-head CI.
- The full local CLI hook is not a valid gate on this macOS Node 22
host: unchanged `a1fc52c7` TypeScript entrypoints fail through the
CommonJS preload with `ERR_UNKNOWN_FILE_EXTENSION`; exact-head Linux CI
remains required.
- Hermes runtime-guard plus current-main docs regression tests: 24/24
passed.
- Hermes workflow-boundary test passed.
- Project-boundary, project-membership, test-title, source-shape,
test-size, targeted Biome, and diff checks passed.
- `npm run docs:sync-agent-variants`, `npm run
docs:check-agent-variants`, and `npm run docs` pass; Fern reports 0
errors and 2 existing warnings.

Live A/B evidence:

- Forced legacy swap at `97e3e7e1`: [run
28554699811](https://github.com/NVIDIA/NemoClaw/actions/runs/28554699811)
reproduced sustained OpenShell `Error` followed by safe rollback. It
also exposed and now closes a lifecycle instrumentation gap: the
post-create `ensureApplied()` branch bypassed pre-rollback capture.
- Native OpenShell route at signed diagnostic SHA `15f50182`: [run
28555110558](https://github.com/NVIDIA/NemoClaw/actions/runs/28555110558)
completed onboarding with exit 0, reached `Phase: Ready`, reported `CUDA
verified`, and sent authenticated Hermes chat-completions requests to
the hermetic inference endpoint. The job's only failure was the
now-fixed test regex not stripping ANSI around `Ready`.
- Prior native evidence at `7cb219d9`: [full run
28559814959](https://github.com/NVIDIA/NemoClaw/actions/runs/28559814959)
and [second pass
28559816026](https://github.com/NVIDIA/NemoClaw/actions/runs/28559816026)
both reached Ready/CUDA with clean runtime and teardown; their Hermes
proof stopped on the now-fixed ANSI matcher before downstream
assertions.
- Prior forced-legacy diagnostic on production parent `a1fc52c7` plus
workflow-only child `6d0cf6a5`: [run
28561607207](https://github.com/NVIDIA/NemoClaw/actions/runs/28561607207)
selected the legacy swap and rolled back cleanly, but failed because the
Hermes boundary rejected the driver-owned OpenShell `OPENSHELL_TLS_KEY`
path. Candidate `54cf259d` adds an exact runtime-only allowance with
negative boundary tests.
- Prior exact-head set at `a1fc52c7`: [run
28561548749](https://github.com/NVIDIA/NemoClaw/actions/runs/28561548749)
proved the GPU and security companion jobs, while Hermes GPU stopped at
a sandbox-user `/proc` permission probe after Ready/CUDA. Candidate
`54cf259d` keeps the same proof but runs it as root and restricts the
match to the exact `nemoclaw-start` process.
- Prior second native pass at `a1fc52c7`: [run
28561555945](https://github.com/NVIDIA/NemoClaw/actions/runs/28561555945)
reproduced only the same harness permission failure after Ready/CUDA,
correct PID 1 topology, authenticated inference, zero forbidden-marker
matches, and clean teardown.
- Superseded six-job set at `54cf259d`: [run
28564960504](https://github.com/NVIDIA/NemoClaw/actions/runs/28564960504)
exposed the stale Dockerfile validator digest and was canceled before
runtime proof. Candidate `970803a4` updates the integrity pin, retained
by final head `c5a67c4c`.
- Superseded second native pass at `54cf259d`: [run
28564973806](https://github.com/NVIDIA/NemoClaw/actions/runs/28564973806)
was canceled during pre-cleanup and supplies no acceptance evidence.
- Superseded forced-legacy proof on production parent `54cf259d` plus
child `69f4e1b2`: [run
28564983760](https://github.com/NVIDIA/NemoClaw/actions/runs/28564983760)
was canceled during pre-cleanup and supplies no acceptance evidence.
- Superseded six-job set at `970803a4`: [run
28565197328](https://github.com/NVIDIA/NemoClaw/actions/runs/28565197328)
was canceled before acceptance execution when the canonical
placeholder-format advisor fix advanced the head.
- Superseded second native pass at `970803a4`: [run
28565207911](https://github.com/NVIDIA/NemoClaw/actions/runs/28565207911)
was canceled before runner assignment and supplies no acceptance
evidence.
- Superseded forced-legacy proof on production parent `970803a4` plus
child `b4d5679e`: [run
28565222881](https://github.com/NVIDIA/NemoClaw/actions/runs/28565222881)
was canceled before runner assignment and supplies no acceptance
evidence.
- Superseded six-job set at `7335903b`: [run
28565576066](https://github.com/NVIDIA/NemoClaw/actions/runs/28565576066)
was intentionally canceled when the documentation gap advanced the
candidate; the root-entrypoint smoke passed, but the remaining lanes
provide no complete acceptance proof.
- Superseded second native pass at `7335903b`: [run
28565587094](https://github.com/NVIDIA/NemoClaw/actions/runs/28565587094)
was canceled before acceptance execution and supplies no acceptance
evidence.
- Superseded forced-legacy proof on production parent `7335903b` plus
child `091e16fd`: [run
28565603460](https://github.com/NVIDIA/NemoClaw/actions/runs/28565603460)
was canceled before acceptance execution and supplies no acceptance
evidence.
- Superseded six-job set at `c5a67c4c`: [run
28566083673](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083673)
reached the native GPU/runtime proofs before the obsolete raw
strict-hash assertion failed; messaging independently hit the
process-probe self-match fixed by merged #6167. GPU, root-entrypoint,
secret-boundary, and credential companion lanes passed.
- Superseded second native pass at `c5a67c4c`: [run
28566083641](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083641)
proved native routing, Ready/CUDA, `nvidia-smi`, `/proc`, `cuInit(0)=0`,
PID 1, authenticated inference, and cleanup, then failed only the
obsolete raw strict-hash assertion.
- Superseded forced-legacy proof on production parent `c5a67c4c` plus
child `48c46a7f`: [run
28566083589](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083589)
proved the same runtime boundary on the legacy route, then failed only
the obsolete raw strict-hash assertion.
- Superseded six-job set at `a04a70ac`: [run
28568069499](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069499)
exposed a pre-onboarding harness defect: direct Vitest import of the
production cleanup helper could not resolve its lazy CommonJS TypeScript
dependencies. Companion results do not count as final-head evidence.
- Superseded second native pass at `a04a70ac`: [run
28568069558](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069558)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded forced-legacy proof on production parent `a04a70ac` plus
child `085a3b7d`: [run
28568069530](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069530)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded six-job set at `6ac4ebc8`: [run
28568490864](https://github.com/NVIDIA/NemoClaw/actions/runs/28568490864)
exposed a clean-runner preinstall edge: the compiled cleanup child was
invoked before OpenShell existed and failed before onboarding. Companion
results do not count as final-head evidence.
- Superseded second native pass at `6ac4ebc8`: [run
28568494928](https://github.com/NVIDIA/NemoClaw/actions/runs/28568494928)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded forced-legacy proof on production parent `6ac4ebc8` plus
child `5d3742a7`: [run
28568501430](https://github.com/NVIDIA/NemoClaw/actions/runs/28568501430)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded six-job set at `65b06d64`: [run
28568954862](https://github.com/NVIDIA/NemoClaw/actions/runs/28568954862)
passed all six jobs, but the candidate advanced to close the
advisor-confirmed shared diagnostic-redaction boundary and two
proof-hardening review threads.
- Superseded second native pass at `65b06d64`: [run
28568959028](https://github.com/NVIDIA/NemoClaw/actions/runs/28568959028)
passed the full native runtime proof but is not final-head evidence.
- Superseded forced-legacy proof on production parent `65b06d64` plus
child `2c6dca1b`: [run
28568966557](https://github.com/NVIDIA/NemoClaw/actions/runs/28568966557)
passed but is not final-parent evidence.
- Final six-job exact-head set at `d76f1647`: [run
28601346031](https://github.com/NVIDIA/NemoClaw/actions/runs/28601346031)
passed all six requested jobs. Native GPU, Hermes startup,
root-entrypoint, secret-boundary, credential-sanitization, and messaging
proofs are green; all 21 messaging raw-token surface probes are
`ABSENT`, and every cleanup record has zero failures.
- Final second native Hermes GPU pass at `d76f1647`: [run
28601348023](https://github.com/NVIDIA/NemoClaw/actions/runs/28601348023)
passed 9/9 assertions. Artifact `8043702467`
(`sha256:0ef929fa2478f5c9579ad2f282c46de8fe4d6eefb04acb062de1af29b4eb002c`)
proves native routing, Ready/CUDA, `nvidia-smi`, `/proc`, successful
`cuInit(0)`, OpenShell PID 1, one container/no backup, authenticated
inference with zero forbidden-marker matches, and clean teardown.
- Final failed-clone rollback proof checks out exact production SHA
`d76f1647` from signed workflow-only child `4c49b5bc`: [run
28602166456](https://github.com/NVIDIA/NemoClaw/actions/runs/28602166456)
passed. Artifact `8044114375`
(`sha256:b5cff9cc7e7cf361f55e074a265c8cfefd3e6dc21ad0d300b140d2a749cde00b`)
records clone exit 137 with `failure_kind=patched_container_failed` and
`rolled_back=no` before finalize, then `rolled_back=yes`, exactly one
running original container, no backup leak, guard-observed clone
removal, clean canary scans, and clean fixture teardown.
- Final forced-legacy success proof on exact production parent
`d76f1647` plus signed workflow-only child `078a372d`: [run
28603335692](https://github.com/NVIDIA/NemoClaw/actions/runs/28603335692)
passed 9/9 assertions. Artifact `8044550700`
(`sha256:56d97c5caa8536ebff735ff600399ac7fafa2fed71206de1f5470e7d15549f6f`)
proves `gpuRoute=legacy-patch`, `--device nvidia.com/gpu=all`,
Ready/CUDA with all three GPU probes, correct OpenShell PID 1/command
envelope, one container/no backup, integrity and negative guard checks,
two authenticated inference requests with zero forbidden-marker matches,
a clean artifact canary scan, and clean teardown.

Source-of-truth review for the retained compatibility path:

- **Invalid state:** the legacy swap temporarily leaves a stopped backup
and running clone with the same OpenShell sandbox ID.
- **Source boundary:** OpenShell's Docker driver reconciles container
summaries into a map keyed only by sandbox ID; 0.0.71 can let the
stopped backup overwrite the running clone and drive the gateway into
terminal `Error`.
- **Source-fix constraint:** the NemoClaw-supported OpenShell release
does not contain deterministic active-container selection. The focused
source fix is open as
[NVIDIA/OpenShell#2116](NVIDIA/OpenShell#2116)
and passes 96/96 Docker-driver tests, strict clippy, and formatting, but
is not yet released or pinned here.
- **Regression coverage:** routing tests pin native auto / forced legacy
/ WSL / Jetson behavior; recreate tests pin capture-before-finalize on
both create timing paths; the secret canary uses the actual single
`OPENSHELL_SANDBOX_COMMAND=env ...` envelope; the live test pins native
and legacy runtime topology separately.
- **Removal condition:** remove the legacy swap and its
rollback/diagnostic modules after WSL and Jetson are proven on native
OpenShell GPU injection and the supported OpenShell floor contains
deterministic duplicate-container reconciliation.
- **WSL boundary:** Docker Desktop WSL does not expose a usable native
CDI route to this flow, so WSL retains the compatibility path and
ignores `NEMOCLAW_DOCKER_GPU_PATCH=0`; routing tests lock that behavior.
Remove it when Docker Desktop exposes usable `nvidia.com/gpu` CDI
devices to the WSL distro.
- **Jetson boundary:** Tegra `/dev/nvmap` and `/dev/nvhost-*` device
ownership requires host group propagation for the non-root sandbox user;
group-add tests lock that behavior. Remove it only when the
platform/runtime supplies equivalent access without the compatibility
recreate.

Diagnostic redaction boundary:

- **Source-of-truth invariant:** `collectDockerGpuPatchDiagnostics()`
constructs the trusted per-bundle redactor, discovers conventional and
custom-placeholder values from every known/discovered full inspect
before writing, recursively redacts JSON values, and publishes every
summary, Docker, OpenShell, and pre-rollback top artifact through that
boundary.
- **Bounded pre-rollback path:** the caller contributes only additive
values discovered from the failed clone before snapshot capture so the
shared 10-second budget cannot hide opaque values; the collector still
performs its own discovery and owns every write. Direct raw-caller and
exhausted-budget regressions scan all artifacts and returned summaries.
- **Removal condition:** remove the additive pre-rollback discovery only
when the shared collector can own snapshot capture inside the same
budget without delaying rollback.

Advisor architecture and follow-up rationale:

- `docker-gpu-patch.ts` grows 58 lines to keep token validation before
container mutation and bounded failed-clone capture before rollback.
Splitting this security-critical ordering during the release-blocker fix
would add cross-module state transfer; extract it when the legacy swap
is retired after WSL and Jetson native proof.
- `docker-gpu-local-inference.test.ts` grows 32 lines so bridge-probe
routing assertions stay beside the behavior under test. Extract a
bridge-probe module and focused test file if that surface grows again.
- `docker-gpu-local-inference.ts` grows 15 lines to keep the
bridge-probe/host-network decision beside its caller-facing contract;
extract it with the tests if that surface grows again.
- `docker-gpu-patch.test.ts` grows 13 lines and remains below 1,350
lines; split the mode-routing cases on the next growth.
- The live fixture now supplies the canonical comma-delimited
placeholder transport. Whitespace compatibility remains covered by
parser unit tests and the messaging-provider scenario; the live proof
intentionally matches the exact canonical startup environ token.
- Dedicated `OPENSHELL_TLS_KEY` tests prove exact runtime acceptance,
arbitrary/PEM/relative/near-miss rejection, persisted `.env` rejection,
and continued rejection of supervisor identity tokens. The exact allowed
path is sourced from `NVIDIA/OpenShell@v0.0.71`
(`a242f84bb367d6df7d4d133e95a93857406c67f7`), where
`driver_utils.rs::TLS_KEY_MOUNT_PATH` defines
`/etc/openshell/tls/client/tls.key` and the Docker/Podman drivers inject
it.

This PR does not claim #6110 resolved until the reporter-class DGX Spark
aarch64 or DGX Station GB300 NVIDIA Endpoints path passes. No such
runner is declared in this repository, and organization runner inventory
is not visible with the current permissions. Missing reporter hardware
is an external acceptance blocker, not a passing result.

The #6155 docs regression fix and current `main` through `9fe45362` are
integrated. A refreshed pairwise merge-tree audit at `d76f1647` is clean
with #5595 and #6153. #5876 directly conflicts in `e2e.yaml` and related
Hermes/docs/workflow-boundary files; its resolution must union
`hermes-gpu-startup` and `mcp-bridge-dev` selectors/result summaries and
recompute uploader validation. #6020 already conflicts with current
`main` and also overlaps #6142 outside `e2e.yaml`; #6053 is mergeable
with `main` but conflicts pairwise in the uploader boundary. Those later
branches must preserve #6142's explicit-only inventory and artifact
contracts during retargeting; #6142 itself remains mergeable/CLEAN.

---
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


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

* **New Features**
* Strengthened Hermes GPU startup proof and managed startup integrity
assertions, plus added a skipped-by-default GPU live E2E run for startup
readiness.
* **Bug Fixes**
* Tightened PID 1 identity validation to block runtime mutation under a
foreign PID 1.
* Improved Docker GPU/OpenShell sandbox command and placeholder
handling; enhanced GPU failure diagnostics with safer redaction.
* **Documentation**
* Refined GPU passthrough and `NEMOCLAW_DOCKER_GPU_PATCH` guidance
across native Linux, Docker Desktop WSL, and Jetson/Tegra.
* **Tests**
* Expanded unit/E2E coverage for placeholder parsing, readiness refusal,
env/secret boundary enforcement, and diagnostic redaction.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
@cv
cv merged commit 9dcdd84 into main Jul 3, 2026
34 checks passed
@cv
cv deleted the 5341-perf-e2e-extend-trace-timing-artifacts-to-vitest-scenarios branch July 3, 2026 07:30
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary

Routes ordinary native CDI Linux through OpenShell 0.0.71's native
`--gpu` path instead of the legacy Docker container swap. The legacy
path remains available for WSL, Jetson, and explicit
`NEMOCLAW_DOCKER_GPU_PATCH=1`, with its OpenShell supervisor command
boundary and rollback diagnostics hardened.

## Related Issue

Related to NVIDIA#6110

## Changes

- Use native OpenShell GPU injection by default on ordinary native CDI
Linux.
- Document the `NEMOCLAW_DOCKER_GPU_PATCH` auto, forced-legacy, and
native-routing behavior for ordinary native Linux, Docker Desktop WSL,
and Jetson/Tegra.
- Keep `NEMOCLAW_DOCKER_GPU_PATCH=1` as the explicit legacy-swap force
control and `=0` as the existing native opt-out; Docker Desktop WSL
still ignores `=0`, and Jetson keeps its compatibility default.
- Preserve OpenShell's supervisor entrypoint on the legacy swap: Docker
receives no command tail, while the workload stays in
`OPENSHELL_SANDBOX_COMMAND`.
- Validate legacy startup tokens before stopping or renaming the
original container and serialize extra-placeholder keys as one
comma-delimited token.
- Defer every legacy recreate through the same supervisor-wait/finalize
boundary so failed clones are captured before rollback on both create
timing paths.
- Persist only allowlisted/redacted failed-clone topology, state,
process, network, and log evidence, with a 10-second total / 2-second
per-call budget so diagnostics cannot materially delay rollback.
- Permit only the canonical OpenShell Docker/Podman TLS key path in the
Hermes runtime environment; arbitrary values and persisted `.env`
entries remain rejected.
- Refresh the Dockerfile integrity pin for the changed validator so
production Hermes images fail closed on any later digest drift.
- Prove native and legacy Docker command boundaries separately,
including Ready/CUDA status, supervisor PID 1, placeholder transport,
config hashes, no backup-container leak, and inference requests.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: independent exact-diff
review found no remaining substantive issue after startup-envelope
secret redaction, bounded pre-rollback capture, unified finalize
ordering, and route-specific live assertions
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Local exact-candidate verification at
`d76f1647a5f354ba04737a6a049b82bfbf6d5454`:

- CLI build and typecheck passed.
- Hermes GPU support/client/workflow coverage passed 48/48; the built
gateway-cleanup module resolves through Node, and runtime cleanup,
registration removal, and bind availability remain fail-closed.
- The shared Docker GPU diagnostic collector owns redaction for every
text/JSON artifact and returned summary; direct conventional `*_KEY` and
custom-placeholder canaries, JSON-validity, inspect-before-write,
exhausted-budget, and collector-owned top regressions pass.
- All 12 Docker GPU suites pass 126/126; the exact-head focused
E2E-support set passes 39/39, including six process-token self-match
regressions, the scrubbed integrity-proof environment, and total
forbidden-marker count.
- Conditional scan, source-shape, test-size, Biome, repository checks,
commit hooks, and push typecheck passed for the final harness
correction; only the documented macOS-invalid full CLI hook lane was
excluded.
- The forbidden-marker request sensor support suite passed 14/14 and
records counts only, never raw request bodies or marker values.
- OpenShell transport-boundary coverage passed 4/4; Docker GPU
command-envelope coverage passed 6/6; extra-placeholder parsing coverage
passed 16/16.
- Hermes validator and wrapper integrity pins match their source SHA256
digests; hadolint and diff checks pass.
- Hermes startup/boundary coverage passed 43/43 locally; the Linux-only
wrapper cases are delegated to exact-head CI.
- The full local CLI hook is not a valid gate on this macOS Node 22
host: unchanged `a1fc52c7` TypeScript entrypoints fail through the
CommonJS preload with `ERR_UNKNOWN_FILE_EXTENSION`; exact-head Linux CI
remains required.
- Hermes runtime-guard plus current-main docs regression tests: 24/24
passed.
- Hermes workflow-boundary test passed.
- Project-boundary, project-membership, test-title, source-shape,
test-size, targeted Biome, and diff checks passed.
- `npm run docs:sync-agent-variants`, `npm run
docs:check-agent-variants`, and `npm run docs` pass; Fern reports 0
errors and 2 existing warnings.

Live A/B evidence:

- Forced legacy swap at `97e3e7e1`: [run
28554699811](https://github.com/NVIDIA/NemoClaw/actions/runs/28554699811)
reproduced sustained OpenShell `Error` followed by safe rollback. It
also exposed and now closes a lifecycle instrumentation gap: the
post-create `ensureApplied()` branch bypassed pre-rollback capture.
- Native OpenShell route at signed diagnostic SHA `15f50182`: [run
28555110558](https://github.com/NVIDIA/NemoClaw/actions/runs/28555110558)
completed onboarding with exit 0, reached `Phase: Ready`, reported `CUDA
verified`, and sent authenticated Hermes chat-completions requests to
the hermetic inference endpoint. The job's only failure was the
now-fixed test regex not stripping ANSI around `Ready`.
- Prior native evidence at `7cb219d9`: [full run
28559814959](https://github.com/NVIDIA/NemoClaw/actions/runs/28559814959)
and [second pass
28559816026](https://github.com/NVIDIA/NemoClaw/actions/runs/28559816026)
both reached Ready/CUDA with clean runtime and teardown; their Hermes
proof stopped on the now-fixed ANSI matcher before downstream
assertions.
- Prior forced-legacy diagnostic on production parent `a1fc52c7` plus
workflow-only child `6d0cf6a5`: [run
28561607207](https://github.com/NVIDIA/NemoClaw/actions/runs/28561607207)
selected the legacy swap and rolled back cleanly, but failed because the
Hermes boundary rejected the driver-owned OpenShell `OPENSHELL_TLS_KEY`
path. Candidate `54cf259d` adds an exact runtime-only allowance with
negative boundary tests.
- Prior exact-head set at `a1fc52c7`: [run
28561548749](https://github.com/NVIDIA/NemoClaw/actions/runs/28561548749)
proved the GPU and security companion jobs, while Hermes GPU stopped at
a sandbox-user `/proc` permission probe after Ready/CUDA. Candidate
`54cf259d` keeps the same proof but runs it as root and restricts the
match to the exact `nemoclaw-start` process.
- Prior second native pass at `a1fc52c7`: [run
28561555945](https://github.com/NVIDIA/NemoClaw/actions/runs/28561555945)
reproduced only the same harness permission failure after Ready/CUDA,
correct PID 1 topology, authenticated inference, zero forbidden-marker
matches, and clean teardown.
- Superseded six-job set at `54cf259d`: [run
28564960504](https://github.com/NVIDIA/NemoClaw/actions/runs/28564960504)
exposed the stale Dockerfile validator digest and was canceled before
runtime proof. Candidate `970803a4` updates the integrity pin, retained
by final head `c5a67c4c`.
- Superseded second native pass at `54cf259d`: [run
28564973806](https://github.com/NVIDIA/NemoClaw/actions/runs/28564973806)
was canceled during pre-cleanup and supplies no acceptance evidence.
- Superseded forced-legacy proof on production parent `54cf259d` plus
child `69f4e1b2`: [run
28564983760](https://github.com/NVIDIA/NemoClaw/actions/runs/28564983760)
was canceled during pre-cleanup and supplies no acceptance evidence.
- Superseded six-job set at `970803a4`: [run
28565197328](https://github.com/NVIDIA/NemoClaw/actions/runs/28565197328)
was canceled before acceptance execution when the canonical
placeholder-format advisor fix advanced the head.
- Superseded second native pass at `970803a4`: [run
28565207911](https://github.com/NVIDIA/NemoClaw/actions/runs/28565207911)
was canceled before runner assignment and supplies no acceptance
evidence.
- Superseded forced-legacy proof on production parent `970803a4` plus
child `b4d5679e`: [run
28565222881](https://github.com/NVIDIA/NemoClaw/actions/runs/28565222881)
was canceled before runner assignment and supplies no acceptance
evidence.
- Superseded six-job set at `7335903b`: [run
28565576066](https://github.com/NVIDIA/NemoClaw/actions/runs/28565576066)
was intentionally canceled when the documentation gap advanced the
candidate; the root-entrypoint smoke passed, but the remaining lanes
provide no complete acceptance proof.
- Superseded second native pass at `7335903b`: [run
28565587094](https://github.com/NVIDIA/NemoClaw/actions/runs/28565587094)
was canceled before acceptance execution and supplies no acceptance
evidence.
- Superseded forced-legacy proof on production parent `7335903b` plus
child `091e16fd`: [run
28565603460](https://github.com/NVIDIA/NemoClaw/actions/runs/28565603460)
was canceled before acceptance execution and supplies no acceptance
evidence.
- Superseded six-job set at `c5a67c4c`: [run
28566083673](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083673)
reached the native GPU/runtime proofs before the obsolete raw
strict-hash assertion failed; messaging independently hit the
process-probe self-match fixed by merged NVIDIA#6167. GPU, root-entrypoint,
secret-boundary, and credential companion lanes passed.
- Superseded second native pass at `c5a67c4c`: [run
28566083641](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083641)
proved native routing, Ready/CUDA, `nvidia-smi`, `/proc`, `cuInit(0)=0`,
PID 1, authenticated inference, and cleanup, then failed only the
obsolete raw strict-hash assertion.
- Superseded forced-legacy proof on production parent `c5a67c4c` plus
child `48c46a7f`: [run
28566083589](https://github.com/NVIDIA/NemoClaw/actions/runs/28566083589)
proved the same runtime boundary on the legacy route, then failed only
the obsolete raw strict-hash assertion.
- Superseded six-job set at `a04a70ac`: [run
28568069499](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069499)
exposed a pre-onboarding harness defect: direct Vitest import of the
production cleanup helper could not resolve its lazy CommonJS TypeScript
dependencies. Companion results do not count as final-head evidence.
- Superseded second native pass at `a04a70ac`: [run
28568069558](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069558)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded forced-legacy proof on production parent `a04a70ac` plus
child `085a3b7d`: [run
28568069530](https://github.com/NVIDIA/NemoClaw/actions/runs/28568069530)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded six-job set at `6ac4ebc8`: [run
28568490864](https://github.com/NVIDIA/NemoClaw/actions/runs/28568490864)
exposed a clean-runner preinstall edge: the compiled cleanup child was
invoked before OpenShell existed and failed before onboarding. Companion
results do not count as final-head evidence.
- Superseded second native pass at `6ac4ebc8`: [run
28568494928](https://github.com/NVIDIA/NemoClaw/actions/runs/28568494928)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded forced-legacy proof on production parent `6ac4ebc8` plus
child `5d3742a7`: [run
28568501430](https://github.com/NVIDIA/NemoClaw/actions/runs/28568501430)
failed at the same pre-onboarding cleanup boundary and supplies no
runtime acceptance evidence.
- Superseded six-job set at `65b06d64`: [run
28568954862](https://github.com/NVIDIA/NemoClaw/actions/runs/28568954862)
passed all six jobs, but the candidate advanced to close the
advisor-confirmed shared diagnostic-redaction boundary and two
proof-hardening review threads.
- Superseded second native pass at `65b06d64`: [run
28568959028](https://github.com/NVIDIA/NemoClaw/actions/runs/28568959028)
passed the full native runtime proof but is not final-head evidence.
- Superseded forced-legacy proof on production parent `65b06d64` plus
child `2c6dca1b`: [run
28568966557](https://github.com/NVIDIA/NemoClaw/actions/runs/28568966557)
passed but is not final-parent evidence.
- Final six-job exact-head set at `d76f1647`: [run
28601346031](https://github.com/NVIDIA/NemoClaw/actions/runs/28601346031)
passed all six requested jobs. Native GPU, Hermes startup,
root-entrypoint, secret-boundary, credential-sanitization, and messaging
proofs are green; all 21 messaging raw-token surface probes are
`ABSENT`, and every cleanup record has zero failures.
- Final second native Hermes GPU pass at `d76f1647`: [run
28601348023](https://github.com/NVIDIA/NemoClaw/actions/runs/28601348023)
passed 9/9 assertions. Artifact `8043702467`
(`sha256:0ef929fa2478f5c9579ad2f282c46de8fe4d6eefb04acb062de1af29b4eb002c`)
proves native routing, Ready/CUDA, `nvidia-smi`, `/proc`, successful
`cuInit(0)`, OpenShell PID 1, one container/no backup, authenticated
inference with zero forbidden-marker matches, and clean teardown.
- Final failed-clone rollback proof checks out exact production SHA
`d76f1647` from signed workflow-only child `4c49b5bc`: [run
28602166456](https://github.com/NVIDIA/NemoClaw/actions/runs/28602166456)
passed. Artifact `8044114375`
(`sha256:b5cff9cc7e7cf361f55e074a265c8cfefd3e6dc21ad0d300b140d2a749cde00b`)
records clone exit 137 with `failure_kind=patched_container_failed` and
`rolled_back=no` before finalize, then `rolled_back=yes`, exactly one
running original container, no backup leak, guard-observed clone
removal, clean canary scans, and clean fixture teardown.
- Final forced-legacy success proof on exact production parent
`d76f1647` plus signed workflow-only child `078a372d`: [run
28603335692](https://github.com/NVIDIA/NemoClaw/actions/runs/28603335692)
passed 9/9 assertions. Artifact `8044550700`
(`sha256:56d97c5caa8536ebff735ff600399ac7fafa2fed71206de1f5470e7d15549f6f`)
proves `gpuRoute=legacy-patch`, `--device nvidia.com/gpu=all`,
Ready/CUDA with all three GPU probes, correct OpenShell PID 1/command
envelope, one container/no backup, integrity and negative guard checks,
two authenticated inference requests with zero forbidden-marker matches,
a clean artifact canary scan, and clean teardown.

Source-of-truth review for the retained compatibility path:

- **Invalid state:** the legacy swap temporarily leaves a stopped backup
and running clone with the same OpenShell sandbox ID.
- **Source boundary:** OpenShell's Docker driver reconciles container
summaries into a map keyed only by sandbox ID; 0.0.71 can let the
stopped backup overwrite the running clone and drive the gateway into
terminal `Error`.
- **Source-fix constraint:** the NemoClaw-supported OpenShell release
does not contain deterministic active-container selection. The focused
source fix is open as
[NVIDIA/OpenShell#2116](NVIDIA/OpenShell#2116)
and passes 96/96 Docker-driver tests, strict clippy, and formatting, but
is not yet released or pinned here.
- **Regression coverage:** routing tests pin native auto / forced legacy
/ WSL / Jetson behavior; recreate tests pin capture-before-finalize on
both create timing paths; the secret canary uses the actual single
`OPENSHELL_SANDBOX_COMMAND=env ...` envelope; the live test pins native
and legacy runtime topology separately.
- **Removal condition:** remove the legacy swap and its
rollback/diagnostic modules after WSL and Jetson are proven on native
OpenShell GPU injection and the supported OpenShell floor contains
deterministic duplicate-container reconciliation.
- **WSL boundary:** Docker Desktop WSL does not expose a usable native
CDI route to this flow, so WSL retains the compatibility path and
ignores `NEMOCLAW_DOCKER_GPU_PATCH=0`; routing tests lock that behavior.
Remove it when Docker Desktop exposes usable `nvidia.com/gpu` CDI
devices to the WSL distro.
- **Jetson boundary:** Tegra `/dev/nvmap` and `/dev/nvhost-*` device
ownership requires host group propagation for the non-root sandbox user;
group-add tests lock that behavior. Remove it only when the
platform/runtime supplies equivalent access without the compatibility
recreate.

Diagnostic redaction boundary:

- **Source-of-truth invariant:** `collectDockerGpuPatchDiagnostics()`
constructs the trusted per-bundle redactor, discovers conventional and
custom-placeholder values from every known/discovered full inspect
before writing, recursively redacts JSON values, and publishes every
summary, Docker, OpenShell, and pre-rollback top artifact through that
boundary.
- **Bounded pre-rollback path:** the caller contributes only additive
values discovered from the failed clone before snapshot capture so the
shared 10-second budget cannot hide opaque values; the collector still
performs its own discovery and owns every write. Direct raw-caller and
exhausted-budget regressions scan all artifacts and returned summaries.
- **Removal condition:** remove the additive pre-rollback discovery only
when the shared collector can own snapshot capture inside the same
budget without delaying rollback.

Advisor architecture and follow-up rationale:

- `docker-gpu-patch.ts` grows 58 lines to keep token validation before
container mutation and bounded failed-clone capture before rollback.
Splitting this security-critical ordering during the release-blocker fix
would add cross-module state transfer; extract it when the legacy swap
is retired after WSL and Jetson native proof.
- `docker-gpu-local-inference.test.ts` grows 32 lines so bridge-probe
routing assertions stay beside the behavior under test. Extract a
bridge-probe module and focused test file if that surface grows again.
- `docker-gpu-local-inference.ts` grows 15 lines to keep the
bridge-probe/host-network decision beside its caller-facing contract;
extract it with the tests if that surface grows again.
- `docker-gpu-patch.test.ts` grows 13 lines and remains below 1,350
lines; split the mode-routing cases on the next growth.
- The live fixture now supplies the canonical comma-delimited
placeholder transport. Whitespace compatibility remains covered by
parser unit tests and the messaging-provider scenario; the live proof
intentionally matches the exact canonical startup environ token.
- Dedicated `OPENSHELL_TLS_KEY` tests prove exact runtime acceptance,
arbitrary/PEM/relative/near-miss rejection, persisted `.env` rejection,
and continued rejection of supervisor identity tokens. The exact allowed
path is sourced from `NVIDIA/OpenShell@v0.0.71`
(`a242f84bb367d6df7d4d133e95a93857406c67f7`), where
`driver_utils.rs::TLS_KEY_MOUNT_PATH` defines
`/etc/openshell/tls/client/tls.key` and the Docker/Podman drivers inject
it.

This PR does not claim NVIDIA#6110 resolved until the reporter-class DGX Spark
aarch64 or DGX Station GB300 NVIDIA Endpoints path passes. No such
runner is declared in this repository, and organization runner inventory
is not visible with the current permissions. Missing reporter hardware
is an external acceptance blocker, not a passing result.

The NVIDIA#6155 docs regression fix and current `main` through `9fe45362` are
integrated. A refreshed pairwise merge-tree audit at `d76f1647` is clean
with NVIDIA#5595 and NVIDIA#6153. NVIDIA#5876 directly conflicts in `e2e.yaml` and related
Hermes/docs/workflow-boundary files; its resolution must union
`hermes-gpu-startup` and `mcp-bridge-dev` selectors/result summaries and
recompute uploader validation. NVIDIA#6020 already conflicts with current
`main` and also overlaps NVIDIA#6142 outside `e2e.yaml`; NVIDIA#6053 is mergeable
with `main` but conflicts pairwise in the uploader boundary. Those later
branches must preserve NVIDIA#6142's explicit-only inventory and artifact
contracts during retargeting; NVIDIA#6142 itself remains mergeable/CLEAN.

---
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


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

* **New Features**
* Strengthened Hermes GPU startup proof and managed startup integrity
assertions, plus added a skipped-by-default GPU live E2E run for startup
readiness.
* **Bug Fixes**
* Tightened PID 1 identity validation to block runtime mutation under a
foreign PID 1.
* Improved Docker GPU/OpenShell sandbox command and placeholder
handling; enhanced GPU failure diagnostics with safer redaction.
* **Documentation**
* Refined GPU passthrough and `NEMOCLAW_DOCKER_GPU_PATCH` guidance
across native Linux, Docker Desktop WSL, and Jetson/Tegra.
* **Tests**
* Expanded unit/E2E coverage for placeholder parsing, readiness refusal,
env/secret boundary enforcement, and diagnostic redaction.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds sanitized onboard trace timing artifacts to registry-driven Vitest
live E2E targets so per-target timing evidence is uploaded without
exposing raw traces.
Keeps Slack/GitHub timing aggregation scoped to the dedicated
`cloud-onboard` artifact.

## Related Issue
Fixes NVIDIA#5341

## Changes
- Configure `NEMOCLAW_TRACE_DIR` for each live matrix target via a
workflow step, then sanitize trace timing and delete raw traces before
upload.
- Extend the live artifact upload allowlist and reusable upload-action
contract to include only `cloud-onboard-trace-timing-summary.json` for
trace timing.
- Pass only `NEMOCLAW_TRACE_DIR` through the E2E fixture child-env
boundary, without broad `NEMOCLAW_TRACE_*` or secret passthrough.
- Add workflow, upload-contract, and fixture-env tests for trace setup,
sanitizer, cleanup, ordering, and upload path guarantees.
- Update E2E developer docs and contributor-agent PR guidance for the
new artifact path and validation expectations.
- Stabilize existing `service-env` and `state-dir-guard` tests observed
during full-suite validation.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Self-review focused on
workflow runner cleanup, artifact allowlist, and fixture env boundary.
Tests assert only `NEMOCLAW_TRACE_DIR` crosses into child commands and
raw trace paths are never uploaded.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [x] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [x] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
Signed-off-by: Angel Mata <amata@nvidia.com>

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

* **New Features**
* Added per-target live E2E onboard trace timing summaries by generating
sanitized timing evidence and uploading it per target.
* **Bug Fixes**
* Hardened the live E2E workflow with target-scoped trace directories,
forced sanitization and cleanup, and stricter per-target artifact upload
paths.
* **Documentation**
* Updated E2E CI guidance and migration/retirement docs to reflect the
new trace-evidence artifact locations and inputs for timing comparisons.
  * Updated targeted test instructions for E2E workflow-related changes.
* **Tests**
* Expanded boundary, workflow-trace, artifact allowlist, fixture
environment propagation, and trace sanitization coverage.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Angel Mata <amata@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: docs Documentation, examples, guides, or docs build area: e2e End-to-end tests, nightly failures, or validation infrastructure area: observability Logging, metrics, tracing, diagnostics, or debug output feature PR adds or expands user-visible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(e2e): extend trace timing artifacts to Vitest scenarios

3 participants