Skip to content

ci(e2e): preserve cutover operations - #6012

Merged
cv merged 8 commits into
codex/phase-11-e2e-shell-retirementfrom
codex/e2e-cutover-operations
Jun 29, 2026
Merged

ci(e2e): preserve cutover operations#6012
cv merged 8 commits into
codex/phase-11-e2e-shell-retirementfrom
codex/e2e-cutover-operations

Conversation

@cv

@cv cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Preserves the operational controls that would otherwise disappear when #5756 retires the legacy nightly E2E workflow. This is intentionally stacked on #5756 so review does not activate a second schedule on main; merge it into the cutover branch before #5756 lands.

Related Issue

Contributes to #5919.

Changes

  • Restore scheduled failure routing while keeping the historical Nightly E2E failed issue thread.
  • Restore API-accurate GitHub and Slack scorecards, including matrix jobs, deterministic rerun deduplication, job links, selective-dispatch opt-in, and a needs fallback.
  • Preserve cloud-onboard timing comparisons through an allowlisted timing-only artifact; raw traces remain under runner temp storage and are deleted before upload.
  • Ratchet notify-on-failure, report-to-pr, and scorecard to the same complete execution-job set.
  • Keep the E2E advisor read-only: it recommends targets but cannot dispatch secret-bearing workflows.
  • Document the consolidated workflow's scheduled operations.

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: CI-only behavior; the internal E2E README is updated, with no user-facing product behavior change.
  • 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 verifies step-only Slack secrets, pinned actions, read-only scorecard permissions, allowlisted trace fields, symlink rejection, raw-trace cleanup, and no advisor dispatch authority; automated security review remains a merge gate.
  • 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)

Verification details:

  • make check
  • npm test — 830 files passed, 9,591 tests passed, 26 skipped
  • Focused E2E operations/workflow/scorecard/GPU support tests — 33 passed

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added consolidated E2E scorecard support for scheduled and manual runs, including Cloud Onboard phase-based trace timing comparisons and enriched Slack blocks; preview Slack posting is now opt-in via workflow dispatch.
    • Added scheduled-only “notify-on-failure” that creates/updates an open failure issue when runs fail or are cancelled.
    • Added trusted trace timing summarization and trace comparison output for scorecards.
  • Bug Fixes
    • Strengthened explicit-only job inventory parsing/validation (deduping, existence checks) and scorecard/PR reporting behavior.
  • Tests / Documentation
    • Expanded E2E and workflow-boundary validation coverage; updated E2E operations documentation.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4d7c2354-a0a9-4373-8fcb-e69f7db730cc

📥 Commits

Reviewing files that changed from the base of the PR and between fda6ade and 6bf5ed5.

📒 Files selected for processing (2)
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts

📝 Walkthrough

Walkthrough

Adds trace sanitization, scorecard generation, scheduled failure routing, explicit-only job inventory outputs, and workflow boundary validation for the E2E operations flow.

Changes

E2E scorecard and workflow operations

Layer / File(s) Summary
Trace timing sanitizer
scripts/e2e/sanitize-trace-timing.py
New CLI scans trace JSON, extracts onboard timing data, writes a trusted summary, and enforces input/output validation and filesystem restrictions.
Scorecard scripts
scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/summarize-jobs.ts
Adds Slack payload builders, job summarization, and trace-timing comparison logic for scheduled scorecards.
Workflow jobs and inventory outputs
.github/workflows/e2e.yaml, tools/e2e/workflow-inventory.mts
Updates inventory parsing and outputs, marks explicit-only jobs, adds trace handling in cloud-onboard, introduces notify-on-failure, and adds the scorecard job with Slack posting and selective preview opt-in.
Boundary validation
tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts
Adds workflow validation for the new jobs, explicit-only inventory wiring, trace-step ordering, permissions, and generate-matrix/report-to-pr output consistency.
Tests, helpers, and docs
test/e2e/README.md, test/helpers/e2e-workflow-contract.ts, test/e2e/fixtures/availability-env.ts, test/e2e/support/*
Adds workflow helpers, environment allowlist updates, e2e coverage for scorecard, sanitizer, and boundary checks, plus scheduled-operations documentation.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related issues

Suggested labels

area: ci, area: e2e

Suggested reviewers

  • jyaunches
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the PR’s main goal of preserving E2E cutover operations and operational controls during the workflow transition.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/e2e-cutover-operations

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

@github-code-quality

github-code-quality Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/e2e-cutover-op... branch is 96%. Coverage data for the codex/phase-11-e2e-s... branch is not yet available.

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

TypeScript / code-coverage/cli

The overall coverage in the codex/e2e-cutover-op... branch is 68%. Coverage data for the codex/phase-11-e2e-s... branch is not yet available.

Show a code coverage summary of the most covered files.
File codex/phase-11-e2e-s... codex/e2e-cutover-op... 6bf5ed5 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 29, 2026 20:59 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard, scorecard, report-to-pr
Optional E2E: sandbox-rlimits-connect, jetson-nvmap-gpu, full-e2e

Dispatch hint: cloud-onboard

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/codex/phase-11-e2e-shell-retirement
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): Required because the cloud-onboard live workflow steps changed: trace directory setup, trusted timing-summary generation, raw trace cleanup, artifact upload, and availability env propagation all run around the hosted onboarding flow. Running this target verifies hosted onboarding still completes and the new sanitized artifact path works in the real workflow.
  • scorecard (low): Required as part of the same E2E workflow dispatch because the PR adds the scorecard job and scorecard helper scripts. A workflow_dispatch run with cloud-onboard should exercise scorecard generation, job summarization, trusted trace artifact lookup, and branch-safe Slack skipping without exposing Slack secrets.
  • report-to-pr (low): Required as part of the same E2E workflow dispatch because PR result reporting logic changed to consume the generated explicit-only job inventory and present selector/skipped-job results. This validates the workflow_dispatch reporting path against the changed selector metadata.

Optional E2E

  • sandbox-rlimits-connect (high): Optional confidence check for the explicit-only job metadata path. The live sandbox rlimit behavior is not changed, but selecting this job validates that explicit-only inventory and jobs/targets selection still reach the destructive rlimit connect target when explicitly requested.
  • jetson-nvmap-gpu (very high): Optional confidence check for the other explicit-only target affected by E2E_DEFAULT_ENABLED metadata. Run only if a stable Jetson runner is available; the PR mainly changes default-dispatch exclusion/reporting rather than Jetson sandbox runtime behavior.
  • full-e2e (high): Optional broad hosted-inference and sandbox lifecycle smoke after workflow graph/reporting changes. Useful if maintainers want additional assurance that a representative full user flow still reports correctly under the updated aggregation jobs.

New E2E recommendations

  • ci-operations-reporting (medium): There is no dedicated live dry-run target for scheduled failure issue routing and Slack scorecard publishing with stubbed external services. Unit/support tests cover the scripts and workflow boundary, but a non-secret operations dry-run E2E would catch GitHub Actions runtime integration drift without posting to production routes.
    • Suggested test: operations-reporting-dry-run-e2e

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: cloud-onboard

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-6: Crash-mid-sanitization test missing for cloud-onboard raw trace cleanup; then add or justify PRA-T1.
Open items: 6 required · 10 warnings · 4 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 12 still apply · 4 new items found

Action checklist

  • PRA-6 Fix: Crash-mid-sanitization test missing for cloud-onboard raw trace cleanup in .github/workflows/e2e.yaml:3390
  • PRA-7 Fix: GitHub API fallback lacks source-of-truth justification comment in scripts/scorecard/summarize-jobs.ts:95
  • PRA-8 Fix: Null-return chain in trace timing comparison lacks source-of-truth justification comments in scripts/scorecard/analyze-trace-timing.ts:209
  • PRA-9 Fix: Zip path traversal in artifact extraction in scripts/scorecard/analyze-trace-timing.ts:252
  • PRA-10 Fix: Bare catch-all swallows programming errors in trace comparison in scripts/scorecard/analyze-trace-timing.ts:342
  • PRA-11 Fix: Triple-duplicated execution job list in workflow YAML in .github/workflows/e2e.yaml:5900
  • PRA-1 Resolve or justify: Source-of-truth review needed: loadWorkflowRunJobs API fallback to needs context
  • PRA-2 Resolve or justify: Source-of-truth review needed: resolvePriorReleaseTag returns null
  • PRA-3 Resolve or justify: Source-of-truth review needed: findLatestCompletedE2eRunForReleaseTag returns null
  • PRA-4 Resolve or justify: Source-of-truth review needed: readTraceSummaryFromRun returns null
  • PRA-5 Resolve or justify: Source-of-truth review needed: sanitize-trace-timing.py MAX_JSON_FILES/MAX_JSON_BYTES bounds
  • PRA-12 Resolve or justify: Sanitizer MAX_JSON_FILES/MAX_JSON_BYTES limits lack threat model documentation in scripts/e2e/sanitize-trace-timing.py:55
  • PRA-13 Resolve or justify: formatTraceDelta division-by-zero branch untested in scripts/scorecard/analyze-trace-timing.ts:96
  • PRA-14 Resolve or justify: Overbroad fallback catches non-transient API errors in scripts/scorecard/summarize-jobs.ts:117
  • PRA-15 Resolve or justify: selectOnboardTrace non-deterministic on equal totalMs in scripts/scorecard/analyze-trace-timing.ts:135
  • PRA-16 Resolve or justify: Predictable temp directory prefix on shared runners in scripts/scorecard/analyze-trace-timing.ts:248
  • 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: loadWorkflowRunJobs API fallback to needs context
  • PRA-T7 Add or justify test follow-up: resolvePriorReleaseTag returns null
  • PRA-T8 Add or justify test follow-up: findLatestCompletedE2eRunForReleaseTag returns null
  • PRA-17 In-scope improvement: selectOnboardTrace selection criterion lacks explanatory comment in scripts/scorecard/analyze-trace-timing.ts:135
  • PRA-18 In-scope improvement: Repeated path.join in scorecard generator step in .github/workflows/e2e.yaml:6447
  • PRA-19 In-scope improvement: Single-use DEFAULT_* path constants can be inlined in tools/e2e/operations-workflow-boundary.mts:11
  • PRA-20 In-scope improvement: Slack payload builder lacks input validation on ScorecardData in scripts/scorecard/build-slack-blocks.ts:1

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-5 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-6 Required architecture .github/workflows/e2e.yaml:3390 Add a test simulating sanitizer crash (SIGKILL) mid-run verifying raw traces are not uploaded and runner.temp isolation contains leakage, or document infeasibility with GitHub Actions runner.temp isolation guarantees.
PRA-7 Required architecture scripts/scorecard/summarize-jobs.ts:95 Add a source-of-truth justification comment above loadWorkflowRunJobs explaining: (1) invalid state = GitHub Jobs API unavailable, (2) source boundary = GitHub Actions API, (3) why source can't be fixed here = transient network/API outages are external, (4) regression test = test mocking API failure, (5) removal condition = when API reliability is guaranteed.
PRA-8 Required architecture scripts/scorecard/analyze-trace-timing.ts:209 Add source-of-truth justification comments for each null-return function (resolvePriorReleaseTag line 233, findLatestCompletedE2eRunForReleaseTag line 263, readTraceSummaryFromRun line 277) and the catch-all fallback in buildTraceTimingResult line 342.
PRA-9 Required security scripts/scorecard/analyze-trace-timing.ts:252 Replace unzip -p with a pure-JS zip library (e.g., adm-zip, yauzl) that validates entry paths, or add validation that the zip entry name matches exactly before extraction. At minimum, add a test creating a zip with entry `../../../etc/passwd` verifying it is not extracted.
PRA-10 Required architecture scripts/scorecard/analyze-trace-timing.ts:342 Narrow the catch to expected error types (network errors, missing artifact) or add core.warning/error logging before returning the fallback. Avoid swallowing programming errors.
PRA-11 Required architecture .github/workflows/e2e.yaml:5900 Extract the execution job list to a shared generator (e.g., tools/e2e/generate-needs.mts) that emits the array for all three jobs, or use YAML anchors/aliases to deduplicate.
PRA-12 Resolve/justify security scripts/e2e/sanitize-trace-timing.py:55 Document the threat model and rationale for MAX_JSON_FILES=100 and MAX_JSON_BYTES=2MB in code comments. Consider making them configurable or deriving from runner specs.
PRA-13 Resolve/justify correctness scripts/scorecard/analyze-trace-timing.ts:96 Add a direct unit test: formatTraceDelta(1000, 0) returns expected string per behavior; verify the zero-division branch produces correct output.
PRA-14 Resolve/justify correctness scripts/scorecard/summarize-jobs.ts:117 Only fall back on truly transient errors (5xx, network timeout, ECONNRESET). For 4xx errors (especially 401, 403), fail the step instead of falling back silently.
PRA-15 Resolve/justify architecture scripts/scorecard/analyze-trace-timing.ts:135 Add a tiebreaker (e.g., prefer trace with most phases, or earliest start time, or stable sort by trace_id). Document the tiebreaker rationale.
PRA-16 Resolve/justify security scripts/scorecard/analyze-trace-timing.ts:248 Consider using a more unique temp directory name (include process.pid and random suffix) or use the runner's dedicated temp space. The finally block cleanup mitigates but doesn't eliminate the window.
PRA-17 Improvement correctness scripts/scorecard/analyze-trace-timing.ts:135 Add comment explaining why highest totalMs is the correct selection criterion (e.g., captures full onboard including retries). Test already verifies behavior.
PRA-18 Improvement architecture .github/workflows/e2e.yaml:6447 Extract to a variable: const SCORECARD_DIR = path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard'); then require(SCORECARD_DIR + '/analyze-trace-timing.ts') etc.
PRA-19 Improvement architecture tools/e2e/operations-workflow-boundary.mts:11 Inline the path strings directly in the function signatures; delete the constants.
PRA-20 Improvement correctness scripts/scorecard/build-slack-blocks.ts:1 Add runtime validation or TypeScript strict types for ScorecardData; consider using a schema validator (zod, valibot) for the precomputed slackData payload.

🚨 Required before merge

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

PRA-6 Required — Crash-mid-sanitization test missing for cloud-onboard raw trace cleanup

  • Location: .github/workflows/e2e.yaml:3390
  • Category: architecture
  • Problem: Cloud-onboard job uses separate always() steps for trace sanitization and raw trace cleanup with path validation, but no regression test exercises a sanitizer crash (SIGKILL) mid-run to verify raw traces are not uploaded and runner.temp isolation contains leakage.
  • Impact: Raw traces with API keys, prompts, and environment data could persist on runner disk if sanitization crashes mid-run, defeating the security purpose of the sanitizer.
  • Required action: Add a test simulating sanitizer crash (SIGKILL) mid-run verifying raw traces are not uploaded and runner.temp isolation contains leakage, or document infeasibility with GitHub Actions runner.temp isolation guarantees.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check test/e2e/support/e2e-scorecard.test.ts for crash-mid-sanitization test; inspect .github/workflows/e2e.yaml cloud-onboard steps for trap/cleanup comments
  • Missing regression test: Test simulating sanitizer crash (SIGKILL) mid-run verifying raw traces are not uploaded and runner.temp isolation contains leakage
  • Done when: The required change is committed and verification passes: Check test/e2e/support/e2e-scorecard.test.ts for crash-mid-sanitization test; inspect .github/workflows/e2e.yaml cloud-onboard steps for trap/cleanup comments.
  • Evidence: Workflow comment documents invalid state=raw traces with secrets, source boundary=OpenClaw agent writing full traces, source-fix constraint=agent needs full traces for debugging, removal condition=when agent emits timing-only schema; no test exercises crash during sanitization

PRA-7 Required — GitHub API fallback lacks source-of-truth justification comment

  • Location: scripts/scorecard/summarize-jobs.ts:95
  • Category: architecture
  • Problem: loadWorkflowRunJobs catches GitHub API errors and falls back to needs context with a core.warning. This fallback is a localized workaround for transient GitHub API failures but has no production code comment explaining the five required source-of-truth elements.
  • Impact: Future maintainers cannot evaluate whether the fallback is still needed or can be removed; the workaround may mask API reliability issues.
  • Required action: Add a source-of-truth justification comment above loadWorkflowRunJobs explaining: (1) invalid state = GitHub Jobs API unavailable, (2) source boundary = GitHub Actions API, (3) why source can't be fixed here = transient network/API outages are external, (4) regression test = test mocking API failure, (5) removal condition = when API reliability is guaranteed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs function for justification comment; check test/e2e/support/e2e-scorecard.test.ts for fallback test
  • Missing regression test: Test already exists mocking github.paginate to throw and verifying fallback to needs context
  • Done when: The required change is committed and verification passes: Read scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs function for justification comment; check test/e2e/support/e2e-scorecard.test.ts for fallback test.
  • Evidence: loadWorkflowRunJobs catch block at line 117 returns null on any error; test 'falls back to needs when the GitHub jobs API is unavailable' mocks 503

PRA-8 Required — Null-return chain in trace timing comparison lacks source-of-truth justification comments

  • Location: scripts/scorecard/analyze-trace-timing.ts:209
  • Category: architecture
  • Problem: Null-return functions resolvePriorReleaseTag (no tags), findLatestCompletedE2eRunForReleaseTag (no runs for tag), readTraceSummaryFromRun (artifact missing or download fails), and buildTraceTimingResult catch-all fallback lack source-of-truth justification comments explaining the invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Impact: Future maintainers cannot evaluate whether these fallbacks are still needed or can be removed; workarounds may mask reliability issues in GitHub API, artifact storage, or release tagging.
  • Required action: Add source-of-truth justification comments for each null-return function (resolvePriorReleaseTag line 233, findLatestCompletedE2eRunForReleaseTag line 263, readTraceSummaryFromRun line 277) and the catch-all fallback in buildTraceTimingResult line 342.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/scorecard/analyze-trace-timing.ts lines 220-346 for justification comments on resolvePriorReleaseTag, findLatestCompletedE2eRunForReleaseTag, readTraceSummaryFromRun, and buildTraceTimingResult catch block
  • Missing regression test: Tests exist for null returns (see e2e-scorecard.test.ts 'returns null at missing release-run and trace-artifact boundaries' and 'keeps trace comparison fallbacks explicit and non-fatal')
  • Done when: The required change is committed and verification passes: Read scripts/scorecard/analyze-trace-timing.ts lines 220-346 for justification comments on resolvePriorReleaseTag, findLatestCompletedE2eRunForReleaseTag, readTraceSummaryFromRun, and buildTraceTimingResult catch block.
  • Evidence: Four fallback surfaces with null returns or catch-all, each handling a distinct external dependency failure mode, all without production-code justification comments

PRA-9 Required — Zip path traversal in artifact extraction

  • Location: scripts/scorecard/analyze-trace-timing.ts:252
  • Category: security
  • Problem: readTraceSummaryFromRun uses `execFileSync('unzip', ['-p', zipPath, TRACE_SUMMARY_FILE])` without validating zip entry paths. A malicious artifact zip could contain entries like `../../../etc/passwd` leading to path traversal during extraction.
  • Impact: Zip slip vulnerability: attacker-controlled artifact could overwrite arbitrary files on the runner filesystem during scorecard generation.
  • Required action: Replace unzip -p with a pure-JS zip library (e.g., adm-zip, yauzl) that validates entry paths, or add validation that the zip entry name matches exactly before extraction. At minimum, add a test creating a zip with entry `../../../etc/passwd` verifying it is not extracted.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun function; check test/e2e/support/e2e-scorecard.test.ts for zip path traversal test
  • Missing regression test: Test creating zip with entry `../../../etc/passwd` verifying it is not extracted or does not escape the temp directory
  • Done when: The required change is committed and verification passes: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun function; check test/e2e/support/e2e-scorecard.test.ts for zip path traversal test.
  • Evidence: Line 252: execFileSync('unzip', ['-p', zipPath, TRACE_SUMMARY_FILE]) with no entry validation; artifact downloaded from GitHub Actions which could be compromised in supply chain

PRA-10 Required — Bare catch-all swallows programming errors in trace comparison

  • Location: scripts/scorecard/analyze-trace-timing.ts:342
  • Category: architecture
  • Problem: buildTraceTimingResult has a bare `catch { return traceTimingResult('Trace: ⊘ comparison unavailable'); }` that swallows all errors including programming errors (TypeError, ReferenceError, etc.), masking bugs.
  • Impact: Programming errors in trace comparison logic (e.g., typos, null dereferences) are silently converted to 'comparison unavailable' instead of failing loudly, reducing observability.
  • Required action: Narrow the catch to expected error types (network errors, missing artifact) or add core.warning/error logging before returning the fallback. Avoid swallowing programming errors.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts line 342 catch block
  • Missing regression test: Test that programming errors (e.g., TypeError from null deref) are not swallowed but propagate or are logged
  • Done when: The required change is committed and verification passes: Inspect scripts/scorecard/analyze-trace-timing.ts line 342 catch block.
  • Evidence: Line 342: `} catch { return traceTimingResult('Trace: ⊘ comparison unavailable'); }` with no error type filtering or logging

PRA-11 Required — Triple-duplicated execution job list in workflow YAML

  • Location: .github/workflows/e2e.yaml:5900
  • Category: architecture
  • Problem: The notify-on-failure, report-to-pr, and scorecard jobs each duplicate an identical 60+ entry needs list. This violates DRY and creates maintenance burden where adding a new execution job requires updating three places.
  • Impact: High risk of drift where one aggregation job misses a dependency, leading to incomplete failure reporting or scorecard generation.
  • Required action: Extract the execution job list to a shared generator (e.g., tools/e2e/generate-needs.mts) that emits the array for all three jobs, or use YAML anchors/aliases to deduplicate.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Compare needs arrays in notify-on-failure, report-to-pr, and scorecard jobs in .github/workflows/e2e.yaml
  • Missing regression test: Workflow boundary test validating all three aggregation jobs have identical needs (already covered by validateE2eOperationsWorkflowBoundary)
  • Done when: The required change is committed and verification passes: Compare needs arrays in notify-on-failure, report-to-pr, and scorecard jobs in .github/workflows/e2e.yaml.
  • Evidence: Three jobs each list ~60 execution jobs in needs; validateE2eOperationsWorkflowBoundary enforces equality but duplication remains
Review findings by urgency: 6 required fixes, 10 items to resolve/justify, 4 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: loadWorkflowRunJobs API fallback to needs context

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test mocks github.paginate to throw 503 and verifies fallback to needs context
  • 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: scripts/scorecard/summarize-jobs.ts lines 117-126; test 'falls back to needs when the GitHub jobs API is unavailable'

PRA-2 Resolve/justify — Source-of-truth review needed: resolvePriorReleaseTag returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 233: if (semverTags.length === 0) return null;

PRA-3 Resolve/justify — Source-of-truth review needed: findLatestCompletedE2eRunForReleaseTag returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 263: return null after paging through runs

PRA-4 Resolve/justify — Source-of-truth review needed: readTraceSummaryFromRun returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' and 'keeps trace comparison fallbacks explicit and non-fatal' verify null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 277: if (!artifact) return null;

PRA-5 Resolve/justify — Source-of-truth review needed: sanitize-trace-timing.py MAX_JSON_FILES/MAX_JSON_BYTES bounds

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'bounds trace input count and file size before parsing' verifies limits enforced
  • 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: scripts/e2e/sanitize-trace-timing.py lines 15-17: constants with no threat model comment; test verifies enforcement

PRA-12 Resolve/justify — Sanitizer MAX_JSON_FILES/MAX_JSON_BYTES limits lack threat model documentation

  • Location: scripts/e2e/sanitize-trace-timing.py:55
  • Category: security
  • Problem: MAX_JSON_FILES=100 and MAX_JSON_BYTES=2MB constants lack threat model documentation explaining the rationale for these specific limits.
  • Impact: Future maintainers cannot evaluate whether limits are still appropriate for runner specs or threat model; arbitrary limits may be too restrictive (causing false negatives) or too permissive (allowing DoS via large trace uploads).
  • Recommended action: Document the threat model and rationale for MAX_JSON_FILES=100 and MAX_JSON_BYTES=2MB in code comments. Consider making them configurable or deriving from runner specs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read scripts/e2e/sanitize-trace-timing.py lines 15-17 for threat model comments
  • Missing regression test: None needed; documentation-only fix
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read scripts/e2e/sanitize-trace-timing.py lines 15-17 for threat model comments.
  • Evidence: Lines 15-17 define constants with no comment explaining threat model rationale

PRA-13 Resolve/justify — formatTraceDelta division-by-zero branch untested

  • Location: scripts/scorecard/analyze-trace-timing.ts:96
  • Category: correctness
  • Problem: formatTraceDelta has a branch for priorMs <= 0 returning '(n/a)' for percentage, but no direct unit test exercises this zero-division branch.
  • Impact: The division-by-zero guard is untested; a regression could change the behavior silently.
  • Recommended action: Add a direct unit test: formatTraceDelta(1000, 0) returns expected string per behavior; verify the zero-division branch produces correct output.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/e2e/support/e2e-scorecard.test.ts for formatTraceDelta test with priorMs=0
  • Missing regression test: Unit test calling formatTraceDelta(1000, 0) and asserting output contains '(n/a)'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/e2e/support/e2e-scorecard.test.ts for formatTraceDelta test with priorMs=0.
  • Evidence: Line 96-98: `if (priorMs <= 0) { return `${direction} ${sign}${formatDuration(Math.abs(deltaMs))} (n/a)`; }` with no corresponding test

PRA-14 Resolve/justify — Overbroad fallback catches non-transient API errors

  • Location: scripts/scorecard/summarize-jobs.ts:117
  • Category: correctness
  • Problem: loadWorkflowRunJobs catches all errors and falls back to needs context, but the error status check only looks for 'status' property. GitHub API errors may have different shapes; the fallback could trigger on non-transient errors (e.g., 401 auth failure, 403 permission denied) where falling back to stale needs data is misleading.
  • Impact: Non-transient API errors (auth failures, permission issues) would silently fall back to needs context, potentially reporting stale/wrong job results instead of failing visibly.
  • Recommended action: Only fall back on truly transient errors (5xx, network timeout, ECONNRESET). For 4xx errors (especially 401, 403), fail the step instead of falling back silently.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs catch block error handling logic
  • Missing regression test: Test that 401/403 errors do not trigger fallback but fail the step
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs catch block error handling logic.
  • Evidence: Catch block at line 117 checks for `error.status` but treats all errors the same; test only mocks 503

PRA-15 Resolve/justify — selectOnboardTrace non-deterministic on equal totalMs

  • Location: scripts/scorecard/analyze-trace-timing.ts:135
  • Category: architecture
  • Problem: selectOnboardTrace sorts candidates by totalMs descending and picks the first, but if multiple traces have identical totalMs, the selection is non-deterministic (depends on array sort stability).
  • Impact: Non-deterministic selection could cause flaky scorecard comparisons when multiple valid traces have identical total duration.
  • Recommended action: Add a tiebreaker (e.g., prefer trace with most phases, or earliest start time, or stable sort by trace_id). Document the tiebreaker rationale.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace sort logic
  • Missing regression test: Test with two traces having identical totalMs verifying deterministic selection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace sort logic.
  • Evidence: Line 151: `candidates.sort((a, b) => b.totalMs - a.totalMs);` with no secondary sort key

PRA-16 Resolve/justify — Predictable temp directory prefix on shared runners

  • Location: scripts/scorecard/analyze-trace-timing.ts:248
  • Category: security
  • Problem: readTraceSummaryFromRun downloads artifact to a temp directory using fs.mkdtempSync with a predictable prefix 'nemoclaw-trace-artifact-'. On self-hosted runners with shared /tmp, there's a theoretical race condition where another process could interfere with the temp directory between mkdtempSync and cleanup.
  • Impact: Low risk on GitHub-hosted runners (ephemeral), but on self-hosted runners with shared /tmp, theoretical race condition for temp directory interference.
  • Recommended action: Consider using a more unique temp directory name (include process.pid and random suffix) or use the runner's dedicated temp space. The finally block cleanup mitigates but doesn't eliminate the window.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun temp directory creation
  • Missing regression test: None; low-risk hardening
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun temp directory creation.
  • Evidence: Line 248: `fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-trace-artifact-'))` with predictable prefix

💡 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-17 Improvement — selectOnboardTrace selection criterion lacks explanatory comment

  • Location: scripts/scorecard/analyze-trace-timing.ts:135
  • Category: correctness
  • Problem: selectOnboardTrace selects the candidate with highest totalMs but lacks an explanatory comment for why this criterion is correct (e.g., captures full onboard including retries).
  • Impact: Future maintainers may change the selection logic incorrectly without understanding the intent.
  • Suggested action: Add comment explaining why highest totalMs is the correct selection criterion (e.g., captures full onboard including retries). Test already verifies behavior.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace function for explanatory comment
  • Missing regression test: Test already exists verifying highest totalMs wins
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 135-151: selection logic with no comment explaining rationale

PRA-18 Improvement — Repeated path.join in scorecard generator step

  • Location: .github/workflows/e2e.yaml:6447
  • Category: architecture
  • Problem: Three repeated path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/...') calls in the scorecard generator step can be extracted to a variable.
  • Impact: Minor code duplication; increases risk of typos and makes path changes harder.
  • Suggested action: Extract to a variable: const SCORECARD_DIR = path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard'); then require(SCORECARD_DIR + '/analyze-trace-timing.ts') etc.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Inspect .github/workflows/e2e.yaml scorecard generator step for repeated path.join calls
  • Missing regression test: None needed; refactoring-only fix
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 6447, 6450, 6453 each repeat path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/...')

PRA-19 Improvement — Single-use DEFAULT_* path constants can be inlined

  • Location: tools/e2e/operations-workflow-boundary.mts:11
  • Category: architecture
  • Problem: DEFAULT_WORKFLOW_PATH and DEFAULT_ADVISOR_PATH constants are each used only once in their respective function default parameters.
  • Impact: Minor indirection; constants add no value when used exactly once.
  • Suggested action: Inline the path strings directly in the function signatures; delete the constants.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check tools/e2e/operations-workflow-boundary.mts lines 11-12 and usages at lines 47, 316, 328-329
  • Missing regression test: None needed; refactoring-only fix
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 11-12 define constants used once each at lines 47, 316, 328-329

PRA-20 Improvement — Slack payload builder lacks input validation on ScorecardData

  • Location: scripts/scorecard/build-slack-blocks.ts:1
  • Category: correctness
  • Problem: The Slack payload builder uses 'any' type for several internal types and has no input validation on ScorecardData fields before rendering.
  • Impact: If upstream scorecard data is malformed (missing fields, wrong types), the Slack payload generation could throw or produce malformed blocks.
  • Suggested action: Add runtime validation or TypeScript strict types for ScorecardData; consider using a schema validator (zod, valibot) for the precomputed slackData payload.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read scripts/scorecard/build-slack-blocks.ts for input validation on ScorecardData
  • Missing regression test: Test with malformed ScorecardData verifying graceful handling or clear error
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Types use 'any' for context/github; no validation before accessing scorecardData fields
Simplification opportunities: 3 possible cuts, net -3 lines possible

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

  • PRA-17 shrink (scripts/scorecard/analyze-trace-timing.ts:135): No code to cut; add comment only
    • Replacement: Add comment: // Pick the trace with highest total duration — this captures the full onboard flow including any retries, which is the signal we want to compare across runs
    • Net: 1 lines
    • Safety boundary: Selection behavior must remain: highest totalMs wins
  • PRA-18 shrink (.github/workflows/e2e.yaml:6447): Three repeated path.join calls
    • Replacement: const SCORECARD_DIR = path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard'); then require(SCORECARD_DIR + '/analyze-trace-timing.ts') etc.
    • Net: -2 lines
    • Safety boundary: Must still load from checked-out workflow ref (GITHUB_WORKSPACE)
  • PRA-19 shrink (tools/e2e/operations-workflow-boundary.mts:11): const DEFAULT_WORKFLOW_PATH = ...; const DEFAULT_ADVISOR_PATH = ...;
    • Replacement: Inline join(REPO_ROOT, '.github', 'workflows', 'e2e.yaml') and join(REPO_ROOT, '.github', 'workflows', 'e2e-advisor.yaml') in function signatures
    • Net: -2 lines
    • Safety boundary: Paths must remain identical
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 — Crash-mid-sanitization test: SIGKILL during sanitize-trace-timing.py execution verifying raw traces not uploaded and runner.temp isolation contains leakage. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, scripts/e2e/sanitize-trace-timing.py, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/summarize-jobs.ts, tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts, tools/e2e/workflow-inventory.mts.
  • PRA-T2 Runtime validation — formatTraceDelta zero-division test: formatTraceDelta(1000, 0) returns expected string with '(n/a)'. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, scripts/e2e/sanitize-trace-timing.py, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/summarize-jobs.ts, tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts, tools/e2e/workflow-inventory.mts.
  • PRA-T3 Runtime validation — Zip path traversal test: artifact zip with entry '../../../etc/passwd' not extracted. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, scripts/e2e/sanitize-trace-timing.py, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/summarize-jobs.ts, tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts, tools/e2e/workflow-inventory.mts.
  • PRA-T4 Runtime validation — Non-transient API error test: 401/403 from github.paginate fails step without fallback to needs. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, scripts/e2e/sanitize-trace-timing.py, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/summarize-jobs.ts, tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts, tools/e2e/workflow-inventory.mts.
  • PRA-T5 Runtime validation — selectOnboardTrace tiebreaker test: two traces with identical totalMs produce deterministic selection. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, scripts/e2e/sanitize-trace-timing.py, scripts/scorecard/analyze-trace-timing.ts, scripts/scorecard/build-slack-blocks.ts, scripts/scorecard/summarize-jobs.ts, tools/e2e/operations-workflow-boundary.mts, tools/e2e/workflow-boundary.mts, tools/e2e/workflow-inventory.mts.
  • PRA-T6 loadWorkflowRunJobs API fallback to needs context — Test mocks github.paginate to throw 503 and verifies fallback to needs context. scripts/scorecard/summarize-jobs.ts lines 117-126; test 'falls back to needs when the GitHub jobs API is unavailable'
  • PRA-T7 resolvePriorReleaseTag returns null — Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return. scripts/scorecard/analyze-trace-timing.ts line 233: if (semverTags.length === 0) return null;
  • PRA-T8 findLatestCompletedE2eRunForReleaseTag returns null — Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return. scripts/scorecard/analyze-trace-timing.ts line 263: return null after paging through runs
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: loadWorkflowRunJobs API fallback to needs context

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test mocks github.paginate to throw 503 and verifies fallback to needs context
  • 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: scripts/scorecard/summarize-jobs.ts lines 117-126; test 'falls back to needs when the GitHub jobs API is unavailable'

PRA-2 Resolve/justify — Source-of-truth review needed: resolvePriorReleaseTag returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 233: if (semverTags.length === 0) return null;

PRA-3 Resolve/justify — Source-of-truth review needed: findLatestCompletedE2eRunForReleaseTag returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' verifies null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 263: return null after paging through runs

PRA-4 Resolve/justify — Source-of-truth review needed: readTraceSummaryFromRun returns null

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'returns null at missing release-run and trace-artifact boundaries' and 'keeps trace comparison fallbacks explicit and non-fatal' verify null return
  • 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: scripts/scorecard/analyze-trace-timing.ts line 277: if (!artifact) return null;

PRA-5 Resolve/justify — Source-of-truth review needed: sanitize-trace-timing.py MAX_JSON_FILES/MAX_JSON_BYTES bounds

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • 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: Test 'bounds trace input count and file size before parsing' verifies limits enforced
  • 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: scripts/e2e/sanitize-trace-timing.py lines 15-17: constants with no threat model comment; test verifies enforcement

PRA-6 Required — Crash-mid-sanitization test missing for cloud-onboard raw trace cleanup

  • Location: .github/workflows/e2e.yaml:3390
  • Category: architecture
  • Problem: Cloud-onboard job uses separate always() steps for trace sanitization and raw trace cleanup with path validation, but no regression test exercises a sanitizer crash (SIGKILL) mid-run to verify raw traces are not uploaded and runner.temp isolation contains leakage.
  • Impact: Raw traces with API keys, prompts, and environment data could persist on runner disk if sanitization crashes mid-run, defeating the security purpose of the sanitizer.
  • Required action: Add a test simulating sanitizer crash (SIGKILL) mid-run verifying raw traces are not uploaded and runner.temp isolation contains leakage, or document infeasibility with GitHub Actions runner.temp isolation guarantees.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check test/e2e/support/e2e-scorecard.test.ts for crash-mid-sanitization test; inspect .github/workflows/e2e.yaml cloud-onboard steps for trap/cleanup comments
  • Missing regression test: Test simulating sanitizer crash (SIGKILL) mid-run verifying raw traces are not uploaded and runner.temp isolation contains leakage
  • Done when: The required change is committed and verification passes: Check test/e2e/support/e2e-scorecard.test.ts for crash-mid-sanitization test; inspect .github/workflows/e2e.yaml cloud-onboard steps for trap/cleanup comments.
  • Evidence: Workflow comment documents invalid state=raw traces with secrets, source boundary=OpenClaw agent writing full traces, source-fix constraint=agent needs full traces for debugging, removal condition=when agent emits timing-only schema; no test exercises crash during sanitization

PRA-7 Required — GitHub API fallback lacks source-of-truth justification comment

  • Location: scripts/scorecard/summarize-jobs.ts:95
  • Category: architecture
  • Problem: loadWorkflowRunJobs catches GitHub API errors and falls back to needs context with a core.warning. This fallback is a localized workaround for transient GitHub API failures but has no production code comment explaining the five required source-of-truth elements.
  • Impact: Future maintainers cannot evaluate whether the fallback is still needed or can be removed; the workaround may mask API reliability issues.
  • Required action: Add a source-of-truth justification comment above loadWorkflowRunJobs explaining: (1) invalid state = GitHub Jobs API unavailable, (2) source boundary = GitHub Actions API, (3) why source can't be fixed here = transient network/API outages are external, (4) regression test = test mocking API failure, (5) removal condition = when API reliability is guaranteed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs function for justification comment; check test/e2e/support/e2e-scorecard.test.ts for fallback test
  • Missing regression test: Test already exists mocking github.paginate to throw and verifying fallback to needs context
  • Done when: The required change is committed and verification passes: Read scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs function for justification comment; check test/e2e/support/e2e-scorecard.test.ts for fallback test.
  • Evidence: loadWorkflowRunJobs catch block at line 117 returns null on any error; test 'falls back to needs when the GitHub jobs API is unavailable' mocks 503

PRA-8 Required — Null-return chain in trace timing comparison lacks source-of-truth justification comments

  • Location: scripts/scorecard/analyze-trace-timing.ts:209
  • Category: architecture
  • Problem: Null-return functions resolvePriorReleaseTag (no tags), findLatestCompletedE2eRunForReleaseTag (no runs for tag), readTraceSummaryFromRun (artifact missing or download fails), and buildTraceTimingResult catch-all fallback lack source-of-truth justification comments explaining the invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Impact: Future maintainers cannot evaluate whether these fallbacks are still needed or can be removed; workarounds may mask reliability issues in GitHub API, artifact storage, or release tagging.
  • Required action: Add source-of-truth justification comments for each null-return function (resolvePriorReleaseTag line 233, findLatestCompletedE2eRunForReleaseTag line 263, readTraceSummaryFromRun line 277) and the catch-all fallback in buildTraceTimingResult line 342.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/scorecard/analyze-trace-timing.ts lines 220-346 for justification comments on resolvePriorReleaseTag, findLatestCompletedE2eRunForReleaseTag, readTraceSummaryFromRun, and buildTraceTimingResult catch block
  • Missing regression test: Tests exist for null returns (see e2e-scorecard.test.ts 'returns null at missing release-run and trace-artifact boundaries' and 'keeps trace comparison fallbacks explicit and non-fatal')
  • Done when: The required change is committed and verification passes: Read scripts/scorecard/analyze-trace-timing.ts lines 220-346 for justification comments on resolvePriorReleaseTag, findLatestCompletedE2eRunForReleaseTag, readTraceSummaryFromRun, and buildTraceTimingResult catch block.
  • Evidence: Four fallback surfaces with null returns or catch-all, each handling a distinct external dependency failure mode, all without production-code justification comments

PRA-9 Required — Zip path traversal in artifact extraction

  • Location: scripts/scorecard/analyze-trace-timing.ts:252
  • Category: security
  • Problem: readTraceSummaryFromRun uses `execFileSync('unzip', ['-p', zipPath, TRACE_SUMMARY_FILE])` without validating zip entry paths. A malicious artifact zip could contain entries like `../../../etc/passwd` leading to path traversal during extraction.
  • Impact: Zip slip vulnerability: attacker-controlled artifact could overwrite arbitrary files on the runner filesystem during scorecard generation.
  • Required action: Replace unzip -p with a pure-JS zip library (e.g., adm-zip, yauzl) that validates entry paths, or add validation that the zip entry name matches exactly before extraction. At minimum, add a test creating a zip with entry `../../../etc/passwd` verifying it is not extracted.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun function; check test/e2e/support/e2e-scorecard.test.ts for zip path traversal test
  • Missing regression test: Test creating zip with entry `../../../etc/passwd` verifying it is not extracted or does not escape the temp directory
  • Done when: The required change is committed and verification passes: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun function; check test/e2e/support/e2e-scorecard.test.ts for zip path traversal test.
  • Evidence: Line 252: execFileSync('unzip', ['-p', zipPath, TRACE_SUMMARY_FILE]) with no entry validation; artifact downloaded from GitHub Actions which could be compromised in supply chain

PRA-10 Required — Bare catch-all swallows programming errors in trace comparison

  • Location: scripts/scorecard/analyze-trace-timing.ts:342
  • Category: architecture
  • Problem: buildTraceTimingResult has a bare `catch { return traceTimingResult('Trace: ⊘ comparison unavailable'); }` that swallows all errors including programming errors (TypeError, ReferenceError, etc.), masking bugs.
  • Impact: Programming errors in trace comparison logic (e.g., typos, null dereferences) are silently converted to 'comparison unavailable' instead of failing loudly, reducing observability.
  • Required action: Narrow the catch to expected error types (network errors, missing artifact) or add core.warning/error logging before returning the fallback. Avoid swallowing programming errors.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts line 342 catch block
  • Missing regression test: Test that programming errors (e.g., TypeError from null deref) are not swallowed but propagate or are logged
  • Done when: The required change is committed and verification passes: Inspect scripts/scorecard/analyze-trace-timing.ts line 342 catch block.
  • Evidence: Line 342: `} catch { return traceTimingResult('Trace: ⊘ comparison unavailable'); }` with no error type filtering or logging

PRA-11 Required — Triple-duplicated execution job list in workflow YAML

  • Location: .github/workflows/e2e.yaml:5900
  • Category: architecture
  • Problem: The notify-on-failure, report-to-pr, and scorecard jobs each duplicate an identical 60+ entry needs list. This violates DRY and creates maintenance burden where adding a new execution job requires updating three places.
  • Impact: High risk of drift where one aggregation job misses a dependency, leading to incomplete failure reporting or scorecard generation.
  • Required action: Extract the execution job list to a shared generator (e.g., tools/e2e/generate-needs.mts) that emits the array for all three jobs, or use YAML anchors/aliases to deduplicate.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Compare needs arrays in notify-on-failure, report-to-pr, and scorecard jobs in .github/workflows/e2e.yaml
  • Missing regression test: Workflow boundary test validating all three aggregation jobs have identical needs (already covered by validateE2eOperationsWorkflowBoundary)
  • Done when: The required change is committed and verification passes: Compare needs arrays in notify-on-failure, report-to-pr, and scorecard jobs in .github/workflows/e2e.yaml.
  • Evidence: Three jobs each list ~60 execution jobs in needs; validateE2eOperationsWorkflowBoundary enforces equality but duplication remains

PRA-12 Resolve/justify — Sanitizer MAX_JSON_FILES/MAX_JSON_BYTES limits lack threat model documentation

  • Location: scripts/e2e/sanitize-trace-timing.py:55
  • Category: security
  • Problem: MAX_JSON_FILES=100 and MAX_JSON_BYTES=2MB constants lack threat model documentation explaining the rationale for these specific limits.
  • Impact: Future maintainers cannot evaluate whether limits are still appropriate for runner specs or threat model; arbitrary limits may be too restrictive (causing false negatives) or too permissive (allowing DoS via large trace uploads).
  • Recommended action: Document the threat model and rationale for MAX_JSON_FILES=100 and MAX_JSON_BYTES=2MB in code comments. Consider making them configurable or deriving from runner specs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read scripts/e2e/sanitize-trace-timing.py lines 15-17 for threat model comments
  • Missing regression test: None needed; documentation-only fix
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read scripts/e2e/sanitize-trace-timing.py lines 15-17 for threat model comments.
  • Evidence: Lines 15-17 define constants with no comment explaining threat model rationale

PRA-13 Resolve/justify — formatTraceDelta division-by-zero branch untested

  • Location: scripts/scorecard/analyze-trace-timing.ts:96
  • Category: correctness
  • Problem: formatTraceDelta has a branch for priorMs <= 0 returning '(n/a)' for percentage, but no direct unit test exercises this zero-division branch.
  • Impact: The division-by-zero guard is untested; a regression could change the behavior silently.
  • Recommended action: Add a direct unit test: formatTraceDelta(1000, 0) returns expected string per behavior; verify the zero-division branch produces correct output.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/e2e/support/e2e-scorecard.test.ts for formatTraceDelta test with priorMs=0
  • Missing regression test: Unit test calling formatTraceDelta(1000, 0) and asserting output contains '(n/a)'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/e2e/support/e2e-scorecard.test.ts for formatTraceDelta test with priorMs=0.
  • Evidence: Line 96-98: `if (priorMs <= 0) { return `${direction} ${sign}${formatDuration(Math.abs(deltaMs))} (n/a)`; }` with no corresponding test

PRA-14 Resolve/justify — Overbroad fallback catches non-transient API errors

  • Location: scripts/scorecard/summarize-jobs.ts:117
  • Category: correctness
  • Problem: loadWorkflowRunJobs catches all errors and falls back to needs context, but the error status check only looks for 'status' property. GitHub API errors may have different shapes; the fallback could trigger on non-transient errors (e.g., 401 auth failure, 403 permission denied) where falling back to stale needs data is misleading.
  • Impact: Non-transient API errors (auth failures, permission issues) would silently fall back to needs context, potentially reporting stale/wrong job results instead of failing visibly.
  • Recommended action: Only fall back on truly transient errors (5xx, network timeout, ECONNRESET). For 4xx errors (especially 401, 403), fail the step instead of falling back silently.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs catch block error handling logic
  • Missing regression test: Test that 401/403 errors do not trigger fallback but fail the step
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/summarize-jobs.ts loadWorkflowRunJobs catch block error handling logic.
  • Evidence: Catch block at line 117 checks for `error.status` but treats all errors the same; test only mocks 503

PRA-15 Resolve/justify — selectOnboardTrace non-deterministic on equal totalMs

  • Location: scripts/scorecard/analyze-trace-timing.ts:135
  • Category: architecture
  • Problem: selectOnboardTrace sorts candidates by totalMs descending and picks the first, but if multiple traces have identical totalMs, the selection is non-deterministic (depends on array sort stability).
  • Impact: Non-deterministic selection could cause flaky scorecard comparisons when multiple valid traces have identical total duration.
  • Recommended action: Add a tiebreaker (e.g., prefer trace with most phases, or earliest start time, or stable sort by trace_id). Document the tiebreaker rationale.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace sort logic
  • Missing regression test: Test with two traces having identical totalMs verifying deterministic selection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace sort logic.
  • Evidence: Line 151: `candidates.sort((a, b) => b.totalMs - a.totalMs);` with no secondary sort key

PRA-16 Resolve/justify — Predictable temp directory prefix on shared runners

  • Location: scripts/scorecard/analyze-trace-timing.ts:248
  • Category: security
  • Problem: readTraceSummaryFromRun downloads artifact to a temp directory using fs.mkdtempSync with a predictable prefix 'nemoclaw-trace-artifact-'. On self-hosted runners with shared /tmp, there's a theoretical race condition where another process could interfere with the temp directory between mkdtempSync and cleanup.
  • Impact: Low risk on GitHub-hosted runners (ephemeral), but on self-hosted runners with shared /tmp, theoretical race condition for temp directory interference.
  • Recommended action: Consider using a more unique temp directory name (include process.pid and random suffix) or use the runner's dedicated temp space. The finally block cleanup mitigates but doesn't eliminate the window.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun temp directory creation
  • Missing regression test: None; low-risk hardening
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect scripts/scorecard/analyze-trace-timing.ts readTraceSummaryFromRun temp directory creation.
  • Evidence: Line 248: `fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-trace-artifact-'))` with predictable prefix

PRA-17 Improvement — selectOnboardTrace selection criterion lacks explanatory comment

  • Location: scripts/scorecard/analyze-trace-timing.ts:135
  • Category: correctness
  • Problem: selectOnboardTrace selects the candidate with highest totalMs but lacks an explanatory comment for why this criterion is correct (e.g., captures full onboard including retries).
  • Impact: Future maintainers may change the selection logic incorrectly without understanding the intent.
  • Suggested action: Add comment explaining why highest totalMs is the correct selection criterion (e.g., captures full onboard including retries). Test already verifies behavior.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read scripts/scorecard/analyze-trace-timing.ts selectOnboardTrace function for explanatory comment
  • Missing regression test: Test already exists verifying highest totalMs wins
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 135-151: selection logic with no comment explaining rationale

PRA-18 Improvement — Repeated path.join in scorecard generator step

  • Location: .github/workflows/e2e.yaml:6447
  • Category: architecture
  • Problem: Three repeated path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/...') calls in the scorecard generator step can be extracted to a variable.
  • Impact: Minor code duplication; increases risk of typos and makes path changes harder.
  • Suggested action: Extract to a variable: const SCORECARD_DIR = path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard'); then require(SCORECARD_DIR + '/analyze-trace-timing.ts') etc.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Inspect .github/workflows/e2e.yaml scorecard generator step for repeated path.join calls
  • Missing regression test: None needed; refactoring-only fix
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 6447, 6450, 6453 each repeat path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/...')

PRA-19 Improvement — Single-use DEFAULT_* path constants can be inlined

  • Location: tools/e2e/operations-workflow-boundary.mts:11
  • Category: architecture
  • Problem: DEFAULT_WORKFLOW_PATH and DEFAULT_ADVISOR_PATH constants are each used only once in their respective function default parameters.
  • Impact: Minor indirection; constants add no value when used exactly once.
  • Suggested action: Inline the path strings directly in the function signatures; delete the constants.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check tools/e2e/operations-workflow-boundary.mts lines 11-12 and usages at lines 47, 316, 328-329
  • Missing regression test: None needed; refactoring-only fix
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 11-12 define constants used once each at lines 47, 316, 328-329

PRA-20 Improvement — Slack payload builder lacks input validation on ScorecardData

  • Location: scripts/scorecard/build-slack-blocks.ts:1
  • Category: correctness
  • Problem: The Slack payload builder uses 'any' type for several internal types and has no input validation on ScorecardData fields before rendering.
  • Impact: If upstream scorecard data is malformed (missing fields, wrong types), the Slack payload generation could throw or produce malformed blocks.
  • Suggested action: Add runtime validation or TypeScript strict types for ScorecardData; consider using a schema validator (zod, valibot) for the precomputed slackData payload.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read scripts/scorecard/build-slack-blocks.ts for input validation on ScorecardData
  • Missing regression test: Test with malformed ScorecardData verifying graceful handling or clear error
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Types use 'any' for context/github; no validation before accessing scorecardData fields

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 Jun 29, 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: Scorecard helper loading still relies on test-only TypeScript strip-types.
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: Scorecard helper loading still relies on test-only TypeScript strip-types in .github/workflows/e2e.yaml:6447
  • 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: Acceptance clause
  • 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 correctness .github/workflows/e2e.yaml:6447 Either convert the three scorecard helpers to plain CommonJS JavaScript that vanilla `require()` can load, or make the workflow and test use the same explicit loader/runtime contract and prove it under the pinned `actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3` Node 24 path.
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 — Scorecard helper loading still relies on test-only TypeScript strip-types

  • Location: .github/workflows/e2e.yaml:6447
  • Category: correctness
  • Problem: The `Generate E2E scorecard` github-script step directly `require()`s `scripts/scorecard/analyze-trace-timing.ts`, `scripts/scorecard/summarize-jobs.ts`, and `scripts/scorecard/build-slack-blocks.ts` from `GITHUB_WORKSPACE`. Those helpers contain TypeScript-only syntax such as `as typeof import(...)`, `as const`, type declarations, and `export type`. The regression test named `loads typed scorecard helpers through the native github-script require boundary` still launches Node with `--experimental-strip-types`, while the workflow step does not set that flag or install a loader. This is the prior advisor finding and still applies.
  • Impact: Scheduled or manual E2E runs can complete their target jobs but fail in the operational scorecard job, preventing the GitHub summary and Slack payload from being generated or posted.
  • Recommended action: Either convert the three scorecard helpers to plain CommonJS JavaScript that vanilla `require()` can load, or make the workflow and test use the same explicit loader/runtime contract and prove it under the pinned `actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3` Node 24 path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `.github/workflows/e2e.yaml` around the `Generate E2E scorecard` step and `test/e2e/support/e2e-scorecard.test.ts` around `loads typed scorecard helpers through the native github-script require boundary`; confirm whether both paths use identical loader assumptions and whether the helpers contain TS-only syntax.
  • Missing regression test: Add or adjust a test named `scorecard helpers load under the exact actions/github-script Node 24 require path without --experimental-strip-types`, or replace it with a plain `node -e require(...)` test after converting the helpers to JavaScript.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `.github/workflows/e2e.yaml` around the `Generate E2E scorecard` step and `test/e2e/support/e2e-scorecard.test.ts` around `loads typed scorecard helpers through the native github-script require boundary`; confirm whether both paths use identical loader assumptions and whether the helpers contain TS-only syntax.
  • Evidence: The workflow calls `require(path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/*.ts'))`, while the helper-load test invokes `process.execPath` with `--experimental-strip-types` before requiring the same helpers.

💡 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.
Simplification opportunities: 1 possible cut

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

  • PRA-1 shrink (.github/workflows/e2e.yaml:6447): TypeScript-only syntax from the three runtime-required scorecard helpers, or the implicit reliance on Node strip-types behavior.
    • Replacement: Plain CommonJS `.js` helpers loaded by vanilla `require()`, or one explicitly configured loader path shared by the workflow and regression test.
    • Safety boundary: Preserve the no-secret scorecard generation step, read-only permissions, and the separate Slack publisher step that does not load checked-out workflow-ref code.
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 — scorecard helpers load under the pinned actions/github-script Node 24 CommonJS require path without --experimental-strip-types. Static and Vitest coverage is broad for workflow inventory, scorecard summarization, Slack routing, sanitizer behavior, permissions, and boundary drift. Remaining confidence risk is concentrated at GitHub Actions/runtime boundaries that unit tests approximate, especially the helper loader path, existing issue update behavior, and artifact zip extraction.
  • PRA-T2 Runtime validation — notify-on-failure comments on an existing open Nightly E2E failed CI/CD issue instead of creating a duplicate issue. Static and Vitest coverage is broad for workflow inventory, scorecard summarization, Slack routing, sanitizer behavior, permissions, and boundary drift. Remaining confidence risk is concentrated at GitHub Actions/runtime boundaries that unit tests approximate, especially the helper loader path, existing issue update behavior, and artifact zip extraction.
  • PRA-T3 Runtime validation — readTraceSummaryFromRun extracts only cloud-onboard-trace-timing-summary.json from a real e2e-cloud-onboard artifact zip fixture. Static and Vitest coverage is broad for workflow inventory, scorecard summarization, Slack routing, sanitizer behavior, permissions, and boundary drift. Remaining confidence risk is concentrated at GitHub Actions/runtime boundaries that unit tests approximate, especially the helper loader path, existing issue update behavior, and artifact zip extraction.
  • PRA-T4 Acceptance clause — Contributes to Complete the Vitest E2E cutover with job-variant parity evidence #5919. — add test evidence or identify existing coverage. The deterministic review context did not include linked issue Complete the Vitest E2E cutover with job-variant parity evidence #5919 body or comments, so literal issue acceptance clauses could not be verified from trusted context.
  • PRA-T5 Acceptance clause — Restore scheduled failure routing while keeping the historical `Nightly E2E failed` issue thread. — add test evidence or identify existing coverage. `.github/workflows/e2e.yaml` adds scheduled-only `notify-on-failure` with `issues: write`, searches open `CI/CD` issues by `Nightly E2E failed`, comments when found, and creates otherwise. `test/e2e/support/e2e-operations-workflow-boundary.test.ts` covers the no-existing-thread create path; the existing-thread comment path remains a recommended runtime/static follow-up.
  • PRA-T6 Acceptance clause — Restore API-accurate GitHub and Slack scorecards, including matrix jobs, deterministic rerun deduplication, job links, selective-dispatch opt-in, and a `needs` fallback. — add test evidence or identify existing coverage. `scripts/scorecard/summarize-jobs.ts` uses the Actions jobs API, normalizes matrix job names, dedupes by `run_attempt` and `completed_at`, preserves job URLs, filters explicit-only jobs, and falls back to `needs`; `scripts/scorecard/build-slack-blocks.ts` routes Slack payloads; tests cover API summaries, reruns, links, Slack routing, selective preview opt-in, and needs fallback. The remaining gap is the unproven github-script runtime contract for requiring the `.ts` helpers.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Scorecard helper loading still relies on test-only TypeScript strip-types

  • Location: .github/workflows/e2e.yaml:6447
  • Category: correctness
  • Problem: The `Generate E2E scorecard` github-script step directly `require()`s `scripts/scorecard/analyze-trace-timing.ts`, `scripts/scorecard/summarize-jobs.ts`, and `scripts/scorecard/build-slack-blocks.ts` from `GITHUB_WORKSPACE`. Those helpers contain TypeScript-only syntax such as `as typeof import(...)`, `as const`, type declarations, and `export type`. The regression test named `loads typed scorecard helpers through the native github-script require boundary` still launches Node with `--experimental-strip-types`, while the workflow step does not set that flag or install a loader. This is the prior advisor finding and still applies.
  • Impact: Scheduled or manual E2E runs can complete their target jobs but fail in the operational scorecard job, preventing the GitHub summary and Slack payload from being generated or posted.
  • Recommended action: Either convert the three scorecard helpers to plain CommonJS JavaScript that vanilla `require()` can load, or make the workflow and test use the same explicit loader/runtime contract and prove it under the pinned `actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3` Node 24 path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `.github/workflows/e2e.yaml` around the `Generate E2E scorecard` step and `test/e2e/support/e2e-scorecard.test.ts` around `loads typed scorecard helpers through the native github-script require boundary`; confirm whether both paths use identical loader assumptions and whether the helpers contain TS-only syntax.
  • Missing regression test: Add or adjust a test named `scorecard helpers load under the exact actions/github-script Node 24 require path without --experimental-strip-types`, or replace it with a plain `node -e require(...)` test after converting the helpers to JavaScript.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `.github/workflows/e2e.yaml` around the `Generate E2E scorecard` step and `test/e2e/support/e2e-scorecard.test.ts` around `loads typed scorecard helpers through the native github-script require boundary`; confirm whether both paths use identical loader assumptions and whether the helpers contain TS-only syntax.
  • Evidence: The workflow calls `require(path.join(process.env.GITHUB_WORKSPACE, 'scripts/scorecard/*.ts'))`, while the helper-load test invokes `process.execPath` with `--experimental-strip-types` before requiring the same helpers.

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: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/e2e.yaml:
- Around line 6394-6402: The selective dispatch check in the workflow script is
using filtered selector arrays, so invalid but non-empty JOBS/TARGETS input can
be misclassified as a full manual run. Update the logic around parseSelectors,
requestedJobs/requestedTargets, and isSelectiveDispatch so selectiveness is
determined from the raw environment inputs before filtering, while the parsed
selectors are only used for aggregation/routing. Keep the workflow behavior
aligned with the rule that selective dispatch stays silent unless post_to_slack
is true.
- Line 3352: Move NEMOCLAW_TRACE_DIR out of the job-level env block in the e2e
workflow because runner.temp is not resolved there; instead set it after the
runner is available by writing to GITHUB_ENV in a step, or switch the
cloud-onboard step logic to read RUNNER_TEMP directly. Update the workflow
section that defines NEMOCLAW_TRACE_DIR so the cloud-onboard steps can start
without early expansion failure.
- Around line 6386-6392: The github-script step is requiring scorecard helper
files as if they were plain CommonJS modules, but analyze-trace-timing,
summarize-jobs, and build-slack-blocks still contain TypeScript-only syntax.
Update those scorecard helpers to be loadable at runtime by either converting
them to plain JavaScript or introducing a TS-aware load path/compile step, and
make sure the workflow’s require calls in the github-script block resolve
through the chosen approach.

In `@scripts/scorecard/analyze-trace-timing.ts`:
- Around line 87-93: The formatTraceDelta helper currently treats a zero priorMs
like a normal baseline, which can render non-zero deltas as +0.0%. Update
formatTraceDelta to handle priorMs === 0 as its own case, using the current
delta direction and duration but omitting the percentage or showing n/a. Keep
the existing unchanged path for tiny deltas, and make the change inside
formatTraceDelta so the scorecard output is accurate.

In `@test/helpers/e2e-workflow-contract.ts`:
- Around line 57-69: The removeJobNeed helper is searching and replacing the
dependency across ownerJob and all following jobs instead of only within
ownerJob’s block. Update removeJobNeed in e2e-workflow-contract to isolate the
ownerJob section before checking/removing the dependency, so includes/replace
only act inside that job’s needs list and don’t mutate later jobs. Keep the
existing function name and error behavior, but scope the needle match to
ownerJob’s block so the test fixture reflects the claimed negative case.

In `@tools/e2e/operations-workflow-boundary.mts`:
- Around line 220-227: The permission check in validateAdvisorRetirement is
relying on regexes against raw YAML text, which can miss equivalent permission
encodings. Update the advisor validation to parse the workflow structure in
validateAdvisorRetirement and inspect the permissions object/fields directly,
then use that parsed data to enforce that actions is not write and that workflow
dispatch is not enabled. Keep the existing symbols validateAdvisorRetirement and
advisorPath as the entry point for the fix.
- Around line 129-131: The scorecard permissions check only verifies the actions
and contents values, so it can still pass if extra permission scopes are added
later. Update the validation in the scorecard permission block to mirror the
exact-key enforcement used for notify-on-failure, using the job.permissions
object to confirm the only allowed keys are actions and contents and that both
are read. Keep the existing error path in operations-workflow-boundary.mts so
this secret-bearing job remains least-privileged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 80fea691-ae95-4ba9-a9fb-e2481c36de52

📥 Commits

Reviewing files that changed from the base of the PR and between 955479c and 299d1dd.

📒 Files selected for processing (14)
  • .github/workflows/e2e.yaml
  • scripts/e2e/sanitize-trace-timing.py
  • scripts/scorecard/analyze-trace-timing.ts
  • scripts/scorecard/build-slack-blocks.ts
  • scripts/scorecard/summarize-jobs.ts
  • test/e2e/README.md
  • test/e2e/fixtures/availability-env.ts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/support/e2e-scorecard.test.ts
  • test/e2e/support/e2e-workflow.test.ts
  • test/e2e/support/gpu-e2e-helpers.test.ts
  • test/helpers/e2e-workflow-contract.ts
  • tools/e2e/operations-workflow-boundary.mts
  • tools/e2e/workflow-boundary.mts

Comment thread .github/workflows/e2e.yaml Outdated
Comment thread .github/workflows/e2e.yaml
Comment thread .github/workflows/e2e.yaml
Comment thread scripts/scorecard/analyze-trace-timing.ts
Comment thread test/helpers/e2e-workflow-contract.ts
Comment thread tools/e2e/operations-workflow-boundary.mts Outdated
Comment thread tools/e2e/operations-workflow-boundary.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Resolved the fresh exact-head advisor findings in 1cab7f42e.

Source boundaries and behavior coverage

  • Raw traces (PRA-1/PRA-6/PRA-T8): the target process must write diagnostic traces that can contain prompts, environment data, and credentials. The sanitizer remains the trust boundary and uploads only an exact timing schema. Raw deletion is now a separate always() step after sanitization, with an exact ${RUNNER_TEMP}/nemoclaw-cloud-onboard-traces path guard, before artifact upload. This survives a sanitizer process failure; runner-level termination remains contained by GitHub-hosted ephemeral runner.temp. The workflow documents the removal condition: the trace producer itself emits only the allowlisted timing schema. Mutation tests pin cleanup, ordering, and the path guard.
  • Jobs API fallback (PRA-2/PRA-7/PRA-15): GitHub's Jobs API is the only source that preserves matrix entries, rerun attempts, and job URLs; needs.live collapses them. API unavailability is external to this workflow, so the degraded needs summary is intentional. Fetch/fallback/warning behavior now lives in the typed scorecard module and is directly tested with a 503 response plus fallback result counts.
  • Historical trace fallbacks (PRA-3/PRA-8): no prior semver tag, no completed run at that tag, and no timing artifact are valid repository-history states, not corrupt local state. buildTraceTimingResult now has an injectable service boundary and direct tests for each state, an artifact failure, and a successful phase comparison. The underlying no-tag/no-run/no-artifact functions are also directly exercised.
  • Explicit-only jobs (PRA-5/PRA-9): removed the scorecard/report hardcoding. E2E_DEFAULT_ENABLED: "0" is now canonical workflow job metadata; the inventory derives and validates it, matrix generation emits it once, and both report/scorecard consume that output. Mutation tests reject invalid metadata and selector drift.

Additional review gaps closed

  • The exact actions/github-script v9 SHA is ratcheted for scorecard steps because it provides Node 24's native type stripping. A plain native-Node require() contract test loads all three typed CommonJS helpers through the same workspace paths used by the workflow.
  • Trace selection now proves the highest valid candidate wins. Sanitizer tests cover the 100-file and 2 MiB limits.
  • The actual YAML scripts are executed in tests to prove first-run failure-issue creation and that selective scorecards make no network call without post_to_slack=true.
  • The three static needs lists remain necessarily repeated because GitHub Actions resolves job dependencies before any repository code runs. The exact-membership validator compares all three sets and rejects missing or extra jobs; a runtime generator cannot populate needs.
  • unzip -p <archive> <exact-summary-name> streams one named entry and never extracts archive paths to disk. The downloaded ZIP is mode 0600 in a fresh temp directory that is removed in finally; path traversal cannot escape through this read shape.

Verification on the pushed head:

  • full npm test: 830 files passed, 2 skipped; 9,605 tests passed, 26 skipped
  • make check: passed, including source-shape and 1,500-line test-file ratchets
  • npm run typecheck:cli: passed
  • focused E2E support tests: passed
  • normal commit and push hooks: passed; commit is signed and DCO-signed-off

No user-facing product behavior changed, so no docs-page update is needed. The E2E README already documents the surviving operational behavior.

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/e2e/support/jetson-workflow-boundary.test.ts (1)

25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid locking this test to CSV order.

This assertion couples the test to the current workflow declaration order instead of the explicit-only behavior. Parse the explicit_only_jobs_csv line and assert membership instead. As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”

Proposed change
-    expect(formatFreeStandingJobsInventoryForShell(inventory)).toContain(
-      "explicit_only_jobs_csv=sandbox-rlimits-connect,jetson-nvmap-gpu",
-    );
+    const explicitOnlyLine = formatFreeStandingJobsInventoryForShell(inventory)
+      .split("\n")
+      .find((line) => line.startsWith("explicit_only_jobs_csv="));
+    expect(explicitOnlyLine).toBeDefined();
+    expect(explicitOnlyLine!.replace("explicit_only_jobs_csv=", "").split(",")).toEqual(
+      expect.arrayContaining(["sandbox-rlimits-connect", "jetson-nvmap-gpu"]),
+    );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/support/jetson-workflow-boundary.test.ts` around lines 25 - 27, The
test is asserting the exact CSV ordering in
formatFreeStandingJobsInventoryForShell, which makes it depend on workflow
declaration order instead of the explicit-only behavior. Update the assertion in
jetson-workflow-boundary.test.ts to parse the explicit_only_jobs_csv output and
verify that the expected job names are present as members, rather than matching
the full comma-separated string. Use the observable boundary from
formatFreeStandingJobsInventoryForShell and keep the check focused on membership
for the explicit_only_jobs_csv field.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/e2e/support/jetson-workflow-boundary.test.ts`:
- Around line 25-27: The test is asserting the exact CSV ordering in
formatFreeStandingJobsInventoryForShell, which makes it depend on workflow
declaration order instead of the explicit-only behavior. Update the assertion in
jetson-workflow-boundary.test.ts to parse the explicit_only_jobs_csv output and
verify that the expected job names are present as members, rather than matching
the full comma-separated string. Use the observable boundary from
formatFreeStandingJobsInventoryForShell and keep the check focused on membership
for the explicit_only_jobs_csv field.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 62c3a8b2-d664-4518-a817-b2adb787dedf

📥 Commits

Reviewing files that changed from the base of the PR and between 299d1dd and 1cab7f4.

📒 Files selected for processing (10)
  • .github/workflows/e2e.yaml
  • scripts/scorecard/analyze-trace-timing.ts
  • scripts/scorecard/summarize-jobs.ts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/support/e2e-scorecard.test.ts
  • test/e2e/support/jetson-workflow-boundary.test.ts
  • test/e2e/support/rlimit-connect-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts
  • tools/e2e/workflow-boundary.mts
  • tools/e2e/workflow-inventory.mts
🚧 Files skipped from review as they are similar to previous changes (2)
  • tools/e2e/operations-workflow-boundary.mts
  • scripts/scorecard/analyze-trace-timing.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Closed the Slack secret boundary in 2c0b457df.

  • Slack payload construction now happens in the no-secret scorecard generation step.
  • The secret-bearing publisher consumes only the precomputed JSON payload and executes no checked-out workspace module.
  • Its job condition requires github.ref == 'refs/heads/main'; branch-dispatched runs still get GitHub summaries but never receive Slack webhook secrets.
  • The operations validator rejects removal of the main-ref gate and rejects GITHUB_WORKSPACE, require(, or scripts/scorecard/ in the secret-bearing script. Mutation tests cover those failures.
  • The E2E README now states the main-only Slack contract.

For the remaining TypeScript loader warning: the workflow is pinned to actions/github-script@3a2844… v9, whose immutable action.yml declares runs.using: node24. Exact Node v24.18.0 loads all three .ts CommonJS helpers through plain require() with no flag; the repository test uses --experimental-strip-types only because the repository's ordinary test runtime is Node 22.16, where the same built-in capability is not yet default. The validator pins the exact Node 24 action SHA, so downgrading that runtime fails the current-state ratchet.

Verification: focused operations/scorecard tests, CLI typecheck, make check, commit hooks, and push hooks all pass.

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{}

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
.github/workflows/e2e.yaml (1)

6521-6541: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Classify selective dispatch from raw inputs before choosing the Slack route.

slackBlocks.getSlackChannel(scorecardData) depends on isSelectiveDispatch, but that flag is still computed from filtered selector arrays. A dispatch with non-empty rejected jobs/targets can be treated as a manual full run and route away from the preview/silent path. This matches the prior selector-classification review, but it still affects the newly precomputed Slack payload. As per path instructions, “selective dispatch” must stay silent unless on main with post_to_slack=true.

Proposed fix
             const safeSelector = /^[A-Za-z0-9_-]+$/;
             const parseSelectors = (value) =>
               value.split(',').map((name) => name.trim()).filter((name) => safeSelector.test(name));
-            const requestedJobs = parseSelectors(process.env.JOBS || '');
-            const requestedTargets = parseSelectors(process.env.TARGETS || '');
+            const rawRequestedJobs = process.env.JOBS || '';
+            const rawRequestedTargets = process.env.TARGETS || '';
+            const requestedJobs = parseSelectors(rawRequestedJobs);
+            const requestedTargets = parseSelectors(rawRequestedTargets);
             const isDispatch = context.eventName === 'workflow_dispatch';
             const isSelectiveDispatch =
-              isDispatch && (requestedJobs.length > 0 || requestedTargets.length > 0);
+              isDispatch && (rawRequestedJobs.trim() !== '' || rawRequestedTargets.trim() !== '');
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/e2e.yaml around lines 6521 - 6541, Classify selective
dispatch from the raw workflow inputs before building the Slack payload so
`slackBlocks.getSlackChannel(scorecardData)` sees the correct route. Update the
`scorecardData` preparation around `isSelectiveDispatch` to derive that flag
from the original dispatch inputs, not from filtered selector arrays, and keep
the selective path silent unless the run is on `main` with `post_to_slack=true`.
Use the `scorecardData` object and `slackBlocks.getSlackChannel` call as the
main touchpoints.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In @.github/workflows/e2e.yaml:
- Around line 6521-6541: Classify selective dispatch from the raw workflow
inputs before building the Slack payload so
`slackBlocks.getSlackChannel(scorecardData)` sees the correct route. Update the
`scorecardData` preparation around `isSelectiveDispatch` to derive that flag
from the original dispatch inputs, not from filtered selector arrays, and keep
the selective path silent unless the run is on `main` with `post_to_slack=true`.
Use the `scorecardData` object and `slackBlocks.getSlackChannel` call as the
main touchpoints.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fb6f720b-e26d-4055-b483-f881af3790bb

📥 Commits

Reviewing files that changed from the base of the PR and between 1cab7f4 and 2c0b457.

📒 Files selected for processing (4)
  • .github/workflows/e2e.yaml
  • test/e2e/README.md
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts
✅ Files skipped from review due to trivial changes (1)
  • test/e2e/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • tools/e2e/operations-workflow-boundary.mts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the current automated-review findings in bb1bd930a:

  • initialize NEMOCLAW_TRACE_DIR from RUNNER_TEMP in a runner step instead of using the unavailable job-level runner context
  • render a zero timing baseline as n/a rather than the misleading +0.0%
  • scope removeJobNeed mutations to the named job block, with a focused regression test
  • require exactly actions: read and contents: read on the scorecard job
  • parse advisor permission blocks structurally, including quoted YAML values

Validation:

  • 41 focused E2E-support tests passed before the helper regression was split out
  • 22 focused E2E-support tests passed after the split
  • npm run typecheck:cli
  • make check
  • commit and push hooks, including CLI type-checking

The helper regression lives in its own focused test file so the existing 1,500-line test-size ratchet remains unchanged.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

One additional valid item from the earlier consolidated review was not represented by a remaining open thread. Fixed in fda6adeaa:

  • scorecard selectiveness is now derived from raw jobs / targets input before safe-selector filtering
  • malformed-but-nonempty workflow-dispatch selectors therefore remain on the selective/preview route instead of being mislabeled as a manual full run
  • the classification is a typed helper with direct coverage for scheduled, empty, malformed, and target-only inputs
  • the workflow boundary ratchets the helper call

Focused E2E-support tests, YAML validation, the operations boundary, CLI type-checking, test-size budget, and normal commit/push hooks passed.

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cv
cv requested review from ericksoa and jason-ma-nv June 29, 2026 20:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tools/e2e/operations-workflow-boundary.mts`:
- Around line 288-293: The retirement gate in the workflow permissions check
only looks at permissions.actions, so it misses the GitHub Actions scalar
shortcut write-all. Update the logic in the YAML.parse-based permissions
handling for OperationsWorkflow to normalize scalar permissions values before
evaluating them, and treat write-all the same as write access for both the
top-level advisor.permissions and each job.permissions entry. Make the check in
the permissionBlocks flow recognize both object and scalar forms consistently so
workflows using write-all cannot bypass the gate.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0edf8e68-76f1-46eb-ab8e-d31c6e74aee8

📥 Commits

Reviewing files that changed from the base of the PR and between 2c0b457 and fda6ade.

📒 Files selected for processing (8)
  • .github/workflows/e2e.yaml
  • scripts/scorecard/analyze-trace-timing.ts
  • scripts/scorecard/summarize-jobs.ts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/support/e2e-scorecard.test.ts
  • test/e2e/support/e2e-workflow-contract.test.ts
  • test/helpers/e2e-workflow-contract.ts
  • tools/e2e/operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (6)
  • test/helpers/e2e-workflow-contract.ts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • test/e2e/support/e2e-scorecard.test.ts
  • scripts/scorecard/summarize-jobs.ts
  • .github/workflows/e2e.yaml
  • scripts/scorecard/analyze-trace-timing.ts

Comment thread tools/e2e/operations-workflow-boundary.mts Outdated
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the exact-head CodeRabbit finding in 6bf5ed5b9: the advisor retirement gate now normalizes both scalar and mapping permission forms and rejects write-all at workflow or job scope, in addition to explicit actions: write.

Regression coverage exercises workflow-level permissions: write-all and job-level quoted actions: "write". Focused tests, the live boundary validator, CLI type-checking, test-size budget, and normal commit/push hooks passed.

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All requested jobs passed

Run: 28402270047
Workflow ref: codex/e2e-cutover-operations
Requested targets: (default — all supported)
Requested jobs: openshell-version-pin
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
openshell-version-pin ✅ success

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Exact-head runtime proof and advisor disposition

Run 28402270047 executed the actual branch e2e.yaml at 6bf5ed5b9 with the hermetic openshell-version-pin selector:

  • generate-matrix: passed
  • openshell-version-pin: passed
  • report-to-pr: passed and posted the one-job result above
  • scorecard: passed under the pinned actions/github-script Node 24 runtime
  • Post scorecard to Slack: correctly skipped because the ref was not main

That resolves GPT’s sole “resolve or justify” item: the three TypeScript scorecard helpers loaded and executed successfully through the exact production github-script path, without a test-only loader.

Nemotron findings were reviewed individually:

  • Sanitizer termination: ordinary sanitizer failure cannot bypass the separate always() cleanup step; runner-level termination is contained by ephemeral runner.temp. The boundary test mutation-proves sanitize → cleanup → trusted upload ordering. A SIGKILL unit test cannot prove runner destruction semantics.
  • Zip traversal: unzip -p archive exact-entry streams only cloud-onboard-trace-timing-summary.json to stdout; it does not extract archive paths to disk, so a ../../../... member cannot overwrite a path.
  • Fallbacks: GitHub jobs/tags/runs/artifacts are external availability boundaries. The scorecard intentionally remains useful in degraded mode, emits explicit fallback text, and warns when the jobs API is unavailable. Tests cover the API failure, null boundaries, and comparison failure. This preserves scheduled observability instead of turning an auxiliary comparison into a pipeline failure.
  • Catch-all: the comparison is intentionally non-fatal; its observable result is Trace: ⊘ comparison unavailable, which is asserted. It must not break an otherwise valid nightly scorecard.
  • Repeated needs: GitHub Actions has no shared job-dependency primitive. Exact set equality among report-to-pr, notify-on-failure, and scorecard is derived and mutation-tested; adding a generator or migration ledger would increase complexity without removing the YAML requirement.
  • Zero baseline: already covered through buildTraceTimingResult, asserting increased +2.0s (n/a).
  • Equal timings: modern JavaScript sort is stable, so equal totals retain deterministic input order.
  • Temporary directory: mkdtempSync atomically creates a directory with a random suffix; the visible string is only its prefix.
  • Input bounds: file-count/size limits are security resource bounds and have direct regression coverage; they are not compatibility fallbacks requiring a removal condition.

No advisor item remains actionable. Ordinary PR CI, both advisors, CodeRabbit, and the exact workflow runtime are green. The remaining gate is independent human review of the operations end state.

@cv
cv merged commit d6c1dfa into codex/phase-11-e2e-shell-retirement Jun 29, 2026
117 checks passed
@cv
cv deleted the codex/e2e-cutover-operations branch June 29, 2026 21:16
@cv cv mentioned this pull request Jun 29, 2026
21 tasks
cv added a commit that referenced this pull request Jun 30, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Retires the remaining legacy shell-driven E2E lanes for #5098 Phase 11
and makes the surviving E2E surface a single target/live Vitest
workflow. This terminal cleanup deletes the old runner paths,
consolidates fixtures and workflow controls, and preserves the
operational, docs-validation, and two-agent security-posture coverage
landed while the migration was in flight.

## Related Issues
- Parent migration: #5098
- Cutover acceptance and post-merge burn-in: #5919

## Changes
- Replace `.github/workflows/e2e-vitest-scenarios.yaml` with
`.github/workflows/e2e.yaml`; remove the legacy nightly/script workflows
and shared script action.
- Move E2E fixtures, live tests, registry, manifests, support tests, and
migration docs under `test/e2e/` with target/live naming.
- Delete converted `test/e2e/test-*.sh` entrypoints and the retired
shell-runner test/support code while retaining implementation shell
fixtures used by Vitest.
- Rename target advisor files/schema under `tools/e2e-advisor/`, remove
auto-dispatch, and align PR Review Advisor terminology.
- Preserve #6012 controls: inventory-derived selection, fail-closed
selectors, complete aggregation, PR reporting, scheduled failure issues,
timing sanitization, and scorecards.
- Preserve #6013 behavior: multiline sandbox scripts plus
default-enabled `docs-validation` and two-agent `security-posture` jobs
in the final workflow/path/project model.
- Keep parity decisions and execution evidence in #5919; exact-head
full, explicit-only, malformed-selector, and selective-dispatch runs are
linked there.

## 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
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [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)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: exact-head CodeQL/GHAS
and #5919 domain approvals are pending
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue: CodeRabbit could not
perform a line review because the 409-file cutover exceeds its 300-file
service limit; exact-head GPT-5.5 and Nemotron findings are
dispositioned in PR comments, while human #5919 domain review remains
pending

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] 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)
- [ ] 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) — passed
with 0 errors and 2 existing Fern warnings
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 10, 2026
21 tasks
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Retires the remaining legacy shell-driven E2E lanes for NVIDIA#5098 Phase 11
and makes the surviving E2E surface a single target/live Vitest
workflow. This terminal cleanup deletes the old runner paths,
consolidates fixtures and workflow controls, and preserves the
operational, docs-validation, and two-agent security-posture coverage
landed while the migration was in flight.

## Related Issues
- Parent migration: NVIDIA#5098
- Cutover acceptance and post-merge burn-in: NVIDIA#5919

## Changes
- Replace `.github/workflows/e2e-vitest-scenarios.yaml` with
`.github/workflows/e2e.yaml`; remove the legacy nightly/script workflows
and shared script action.
- Move E2E fixtures, live tests, registry, manifests, support tests, and
migration docs under `test/e2e/` with target/live naming.
- Delete converted `test/e2e/test-*.sh` entrypoints and the retired
shell-runner test/support code while retaining implementation shell
fixtures used by Vitest.
- Rename target advisor files/schema under `tools/e2e-advisor/`, remove
auto-dispatch, and align PR Review Advisor terminology.
- Preserve NVIDIA#6012 controls: inventory-derived selection, fail-closed
selectors, complete aggregation, PR reporting, scheduled failure issues,
timing sanitization, and scorecards.
- Preserve NVIDIA#6013 behavior: multiline sandbox scripts plus
default-enabled `docs-validation` and two-agent `security-posture` jobs
in the final workflow/path/project model.
- Keep parity decisions and execution evidence in NVIDIA#5919; exact-head
full, explicit-only, malformed-selector, and selective-dispatch runs are
linked there.

## 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
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [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)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: exact-head CodeQL/GHAS
and NVIDIA#5919 domain approvals are pending
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue: CodeRabbit could not
perform a line review because the 409-file cutover exceeds its 300-file
service limit; exact-head GPT-5.5 and Nemotron findings are
dispositioned in PR comments, while human NVIDIA#5919 domain review remains
pending

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] 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)
- [ ] 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) — passed
with 0 errors and 2 existing Fern warnings
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure chore Build, CI, dependency, or tooling maintenance labels Aug 1, 2026
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: e2e End-to-end tests, nightly failures, or validation infrastructure chore Build, CI, dependency, or tooling maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants