Skip to content

feat(ci): add post-merge E2E risk shadow - #6447

Merged
cv merged 19 commits into
codex/deterministic-pr-risk-planfrom
codex/e2e-risk-gate-shadow
Jul 9, 2026
Merged

feat(ci): add post-merge E2E risk shadow#6447
cv merged 19 commits into
codex/deterministic-pr-risk-planfrom
codex/e2e-risk-gate-shadow

Conversation

@cv

@cv cv commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a trusted post-merge shadow gate that uses the deterministic risk plan from #6446 to run at most three live E2E jobs for the merged commit during the merge-to-tag window. It also makes the fast E2E support contracts part of normal PR and main CI so harness regressions are caught before QA.

This PR is stacked on #6446 and should merge after it.

Changes

  • Dispatch risk-selected live E2E only from a trusted push to main, never from unmerged PR code with repository secrets.
  • Bind controller checkout, child workflow checkout, plan hash, correlation ID, and per-job/per-shard evidence to the merged commit.
  • Validate the tested SHA against origin/main before preparation and close the post-merge shadow check as success, failure, or neutral from complete evidence.
  • Cap automatic live E2E fan-out at three jobs and retain the post-merge risk plan artifact for diagnosis.
  • Add the non-live E2E support project to PR and main CI, plus workflow, controller, signal-reporter, and failure-mode contracts.
  • Document the merge-to-tag safety window and shadow-gate operating boundaries for contributors and maintainers.

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: Internal CI/E2E behavior only; contributor and maintainer documentation was updated, with no Fern user documentation required.
  • 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: Security-focused implementation and independent final audit confirmed the trusted main-push boundary, commit identity and ancestry validation, controller-state digest verification across artifact download, bounded fan-out, complete per-shard evidence, and handled failure cleanup; unmerged code never receives repository secrets.
  • 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
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — focused controller, workflow, advisor-normalization, and signal-reporter tests passed after conflict resolution (84/84); E2E support project (770 passed); workflow contracts (28 passed); CLI and plugin typechecks passed
  • Applicable broad gate passed — GitHub CI passed 38 checks for head commit b5552e68b, with 2 expected skips and no failures; the prior local broad gate passed 1,268 files and 14,134 tests, with 39 expected skips and 1 todo
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

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

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the v0.0.77 label Jul 8, 2026
@cv cv self-assigned this Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3fffeb04-1327-4438-8f6c-9efa2a8adfd0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/e2e-risk-gate-shadow

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

@github-code-quality

github-code-quality Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the codex/deterministic-... branch.

TypeScript / code-coverage/cli

The overall coverage in the codex/e2e-risk-gate-... branch remains at 76%, unchanged from the codex/deterministic-... branch.

Show a code coverage summary of the most impacted files.
File codex/deterministic-... fed4604 codex/e2e-risk-gate-... b5552e6 +/-
src/lib/sandbox...vileged-exec.ts 89% 45% -44%
src/lib/actions...ess-recovery.ts 79% 64% -15%
src/lib/messagi...p-qr-compact.ts 92% 79% -13%
src/lib/actions...tus-snapshot.ts 91% 79% -12%
src/lib/inference/nim.ts 71% 64% -7%
src/lib/actions...dbox/connect.ts 75% 72% -3%
src/lib/inferen...board-probes.ts 89% 93% +4%
src/lib/actions...e-validation.ts 81% 88% +7%
src/lib/actions...-monkeypatch.ts 72% 81% +9%
src/lib/actions...x/mcp-bridge.ts 35% 44% +9%

Updated July 08, 2026 22:36 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@cv cv mentioned this pull request Jul 8, 2026
21 tasks
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard
Optional E2E: openshell-version-pin, docs-validation

Dispatch hint: openshell-version-pin,docs-validation

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/codex/deterministic-pr-risk-plan
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): Changed onboard, trace timing, scorecard, or E2E workflow code can affect cloud onboard wall-clock behavior and should refresh the trusted cloud-onboard trace timing signal.

Optional E2E

  • openshell-version-pin (low): Optional low-cost smoke of the modified E2E workflow path and newly wired risk-signal reporter in an existing hermetic live E2E job, without exercising product runtime infrastructure unnecessarily.
  • docs-validation (low): Optional confidence check for changed Markdown/README files and the E2E workflow's reporter integration in a checkout-local validation lane.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: openshell-version-pin,docs-validation

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

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

Dispatch required E2E targets:

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

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/codex/deterministic-pr-risk-plan
Head: HEAD
Confidence: high

Required E2E targets

  • e2e-all: The PR changes the canonical E2E workflow machinery in .github/workflows/e2e.yaml, including shared workflow_dispatch inputs, checkout behavior, matrix/job execution paths, and reporter wiring across registry and free-standing live E2E jobs. It also changes shared E2E risk-signal reporting/support files. Per policy, shared E2E target workflow machinery and shared support changes require the full E2E fan-out.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref>

Optional E2E targets

  • None.

Relevant changed files

  • .github/workflows/e2e.yaml
  • test/e2e-risk-signal-reporter.test.ts
  • test/e2e/README.md
  • test/e2e/docs/README.md
  • test/e2e/risk-signal-reporter.ts
  • test/e2e/support/e2e-operations-workflow-boundary.test.ts
  • tools/e2e/operations-workflow-boundary.mts

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-5: Shadow workflow requests excessive actions:write permission; then add or justify PRA-T1.
Open items: 5 required · 16 warnings · 10 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 17 still apply · 6 new items found

Action checklist

  • PRA-5 Fix: Shadow workflow requests excessive actions:write permission in .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12
  • PRA-6 Fix: No scheduled cleanup for stale in_progress check runs after controller crash in .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:1
  • PRA-7 Fix: Missing evidence returns neutral instead of failure when workflow succeeds in tools/e2e-advisor/post-merge-risk-gate.mts:315
  • PRA-8 Fix: Exact-commit validation runs before inventory parse; cannot validate job membership in .github/workflows/e2e.yaml:45
  • PRA-9 Fix: Workflow conclusion differentiation incomplete in classifyRiskEvidence in tools/e2e-advisor/post-merge-risk-gate.mts:285
  • PRA-1 Resolve or justify: Source-of-truth review needed: tools/e2e-advisor/private-file.ts:6
  • PRA-2 Resolve or justify: Source-of-truth review needed: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • PRA-3 Resolve or justify: Source-of-truth review needed: test/e2e/risk-signal-reporter.ts:98
  • PRA-4 Resolve or justify: Source-of-truth review needed: .github/workflows/e2e.yaml:45-80
  • PRA-10 Resolve or justify: O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable in tools/e2e-advisor/private-file.ts:6
  • PRA-11 Resolve or justify: validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning in tools/e2e-advisor/post-merge-risk-gate.mts:625
  • PRA-12 Resolve or justify: mergeSignal assumes no duplicate test names across Vitest invocations in test/e2e/risk-signal-reporter.ts:98
  • PRA-13 Resolve or justify: Validation step cannot validate job membership because inventory parsing happens later in .github/workflows/e2e.yaml:75
  • PRA-14 Resolve or justify: O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable in tools/e2e-advisor/private-file.ts:6
  • PRA-15 Resolve or justify: validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning in tools/e2e-advisor/post-merge-risk-gate.mts:625
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Missing integration tests with mocked GitHub API for controller modes
  • PRA-T7 Add or justify test follow-up: Workflow contract test asserts excessive permissions
  • PRA-T8 Add or justify test follow-up: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform
  • PRA-22 In-scope improvement: Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification in tools/e2e-advisor/private-file.ts:6
  • PRA-23 In-scope improvement: Document trust assumption for validateRiskPlan allowedJobs in tools/e2e-advisor/post-merge-risk-gate.mts:625
  • PRA-24 In-scope improvement: Add test for duplicate test names in mergeSignal in test/e2e/risk-signal-reporter.ts:98
  • PRA-25 In-scope improvement: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform in test/post-merge-e2e-risk-gate.test.ts:1
  • PRA-26 In-scope improvement: Integration test for validateRiskPlan with mocked workflow inventory in tools/e2e-advisor/post-merge-risk-gate.mts:625
  • PRA-27 In-scope improvement: Test for mergeSignal duplicate test name handling across Vitest invocations in test/e2e/risk-signal-reporter.ts:98
  • PRA-28 In-scope improvement: Concurrency group collision test for controller runs in tools/e2e-advisor/post-merge-risk-gate.mts:200
  • PRA-29 In-scope improvement: classifyRiskEvidence tests for each workflow conclusion type in tools/e2e-advisor/post-merge-risk-gate.mts:285
  • PRA-30 In-scope improvement: Consider splitting post-merge-risk-gate.mts into smaller modules in tools/e2e-advisor/post-merge-risk-gate.mts:1
  • PRA-31 In-scope improvement: Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification in tools/e2e-advisor/private-file.ts:6

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 Required security .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12 Reduce permissions to { checks: 'write', contents: 'read' }. The controller uses the GitHub REST API to dispatch the E2E workflow via POST /repos/{owner}/{repo}/actions/workflows/{workflow_id}/dispatches which only requires contents:read + repository scope in the token. If actions:write is genuinely required for the dispatch call, document the trust assumption explicitly with a prominent comment and add a security review gate for any changes to this workflow.
PRA-6 Required security .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:1 Add a separate scheduled cleanup workflow (e.g., hourly) that queries check runs with conclusion=null and status=in_progress for 'E2E / Post-merge Risk Gate (shadow)' older than a grace period (e.g., 2 hours) and marks them neutral via GitHub API. This is a standard pattern for long-running check-run controllers.
PRA-7 Required correctness tools/e2e-advisor/post-merge-risk-gate.mts:315 Change missing evidence verdict from 'neutral' to 'failure' when workflowConclusion is 'success' and expectedEvidence.length > 0. Add a distinct 'missing' verdict if differentiation is needed for diagnostics. The shadow check should only pass (success) when all expected job shards produce complete, unskipped evidence.
PRA-8 Required correctness .github/workflows/e2e.yaml:45 Move the inventory parsing into the validation step (duplicate the npx tsx tools/e2e/workflow-inventory.mts --shell call) so job membership can be validated before E2E workspace preparation. Or restructure: parse inventory first, validate, then proceed.
PRA-9 Required correctness tools/e2e-advisor/post-merge-risk-gate.mts:285 Differentiate: ['failure', 'timed_out', 'action_required', 'startup_failure'] → failure; ['cancelled', 'skipped'] → neutral with distinct titles (e.g., 'Workflow cancelled', 'Workflow skipped'); workflowConclusion === 'success' with missing evidence → failure (see PRA-7).
PRA-10 Resolve/justify architecture tools/e2e-advisor/private-file.ts:6 Add a runtime check that O_NOFOLLOW and O_NONBLOCK are effective (e.g., attempt to open a known symlink and verify ELOOP/ENXIO), or document platform requirements explicitly. Consider using fs.open with 'r'/'w' flags and manual fs.lstat verification as a portable alternative.
PRA-11 Resolve/justify architecture tools/e2e-advisor/post-merge-risk-gate.mts:625 Document this trust assumption explicitly with a prominent comment. Consider pinning a known-good allowedJobs set in the controller or validating against a maintainer-approved list stored separately (e.g., in a protected file or repository variable).
PRA-12 Resolve/justify architecture test/e2e/risk-signal-reporter.ts:98 Document the assumption that each test runs exactly once per Vitest command per job/shard. Add deduplication by test name + file, or track unique test IDs. Add a test verifying duplicate test names across invocations are handled correctly. Add JSDoc for mergeSignal explaining runReason priority.
PRA-13 Resolve/justify architecture .github/workflows/e2e.yaml:75 Duplicate the inventory fetch in the validation step (call npx tsx tools/e2e/workflow-inventory.mts --shell) and validate job membership before proceeding to Prepare E2E workspace.
PRA-14 Resolve/justify security tools/e2e-advisor/private-file.ts:6 Add a runtime check at module load that verifies O_NOFOLLOW and O_NONBLOCK are effective (e.g., create a temp symlink, attempt open with flags, expect ELOOP). If not effective, throw or fall back to portable fs.lstat verification. Document minimum Node.js version/platform requirements.
PRA-15 Resolve/justify security tools/e2e-advisor/post-merge-risk-gate.mts:625 Document this trust assumption explicitly with a prominent comment. Consider a separate pinned allowlist in a protected location (repository variable, protected file) that the controller validates against.
PRA-16 Resolve/justify correctness test/e2e/risk-signal-reporter.ts:98 Document the assumption clearly. Add a test for duplicate test names (PRA-T8) first. Only implement deduplication if the test reveals a real problem. Consider tracking test identities by (file, name) tuple.
PRA-17 Resolve/justify correctness tools/e2e-advisor/post-merge-risk-gate.mts:200 Add a comment documenting the collision mitigation rationale: correlationId is validated by child workflow, so only the controller that dispatched the run can complete its check. Consider using workflow_run event with correlation label if GitHub supports it in the future.
PRA-18 Resolve/justify tests test/post-merge-e2e-risk-gate.test.ts:1 Add integration tests using nock or msw to mock GitHub API and verify the full controller lifecycle for all modes (start, finish, abandon). Test error paths: dispatch failure, child run failure, evidence download failure, check completion failure.
PRA-19 Resolve/justify tests test/post-merge-e2e-risk-gate-workflow.test.ts:15 Update the workflow contract test to expect { checks: 'write', contents: 'read' } once PRA-5 is fixed, or update to match the documented rationale if actions:write is kept.
PRA-20 Resolve/justify security tools/e2e-advisor/post-merge-risk-gate.mts:386 Use fs.openSync with O_NOFOLLOW (when available) and verify the opened file is the expected GITHUB_OUTPUT by checking inode/device against a pre-stat, or accept this as a low-risk TOCTOU in a trusted workflow context and document it.

🚨 Required before merge

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

PRA-5 Required — Shadow workflow requests excessive actions:write permission

  • Location: .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12
  • Category: security
  • Problem: Shadow workflow declares permissions { actions: write, checks: write, contents: read }. Only checks:write (for check run creation/completion) and contents:read (for checkout) are strictly required. The actions:write permission expands attack surface allowing creation of misleading workflow dispatches if the controller is compromised.
  • Impact: If the controller is compromised, an attacker with actions:write can create arbitrary workflow dispatches, modify workflow files, or manipulate Actions state beyond the intended check-run lifecycle.
  • Required action: Reduce permissions to { checks: 'write', contents: 'read' }. The controller uses the GitHub REST API to dispatch the E2E workflow via POST /repos/{owner}/{repo}/actions/workflows/{workflow_id}/dispatches which only requires contents:read + repository scope in the token. If actions:write is genuinely required for the dispatch call, document the trust assumption explicitly with a prominent comment and add a security review gate for any changes to this workflow.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12 permissions block
  • Missing regression test: Workflow contract test in test/post-merge-e2e-risk-gate-workflow.test.ts should assert permissions equal { checks: 'write', contents: 'read' }
  • Done when: The required change is committed and verification passes: Check .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12 permissions block.
  • Evidence: Line 12 of .github/workflows/post-merge-e2e-risk-gate-shadow.yaml shows: permissions: { actions: write, checks: write, contents: read }

PRA-6 Required — No scheduled cleanup for stale in_progress check runs after controller crash

  • Location: .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:1
  • Category: security
  • Problem: If the shadow controller starts (creates check run with status=in_progress) but crashes before completing or abandoning, the check run remains in_progress indefinitely, blocking merge queues and giving false confidence.
  • Impact: Stale in_progress check runs block merge queues indefinitely and give false confidence that risk gate is still evaluating. No automated recovery exists.
  • Required action: Add a separate scheduled cleanup workflow (e.g., hourly) that queries check runs with conclusion=null and status=in_progress for 'E2E / Post-merge Risk Gate (shadow)' older than a grace period (e.g., 2 hours) and marks them neutral via GitHub API. This is a standard pattern for long-running check-run controllers.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search repository for any scheduled workflow that cleans up 'E2E / Post-merge Risk Gate (shadow)' check runs
  • Missing regression test: Integration test simulating controller crash mid-run and verifying scheduled cleanup marks the check neutral
  • Done when: The required change is committed and verification passes: Search repository for any scheduled workflow that cleans up 'E2E / Post-merge Risk Gate (shadow)' check runs.
  • Evidence: No scheduled workflow exists in .github/workflows/ that cleans up stale shadow check runs

PRA-7 Required — Missing evidence returns neutral instead of failure when workflow succeeds

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:315
  • Category: correctness
  • Problem: classifyRiskEvidence returns neutral for missing evidence when workflowConclusion is 'success'. This allows incomplete evidence (e.g., only 1 of 3 expected job shards uploaded) to produce a neutral (not red) check, which masks infrastructure failures where evidence upload silently fails.
  • Impact: Infrastructure failures (evidence upload broken, network issues, runner crashes) that prevent signal upload are masked as neutral instead of failing the risk gate, giving false confidence.
  • Required action: Change missing evidence verdict from 'neutral' to 'failure' when workflowConclusion is 'success' and expectedEvidence.length > 0. Add a distinct 'missing' verdict if differentiation is needed for diagnostics. The shadow check should only pass (success) when all expected job shards produce complete, unskipped evidence.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts lines 285-330 classifyRiskEvidence function
  • Missing regression test: Test case in test/post-merge-e2e-risk-gate.test.ts: workflowConclusion='success' with partial signals → expects conclusion='failure'
  • Done when: The required change is committed and verification passes: Read tools/e2e-advisor/post-merge-risk-gate.mts lines 285-330 classifyRiskEvidence function.
  • Evidence: Line 315-320 in classifyRiskEvidence: missing evidence returns { conclusion: 'neutral', title: 'Selected E2E jobs are missing test evidence', summary: 'At least one expected job shard did not upload a bound risk signal.' }

PRA-8 Required — Exact-commit validation runs before inventory parse; cannot validate job membership

  • Location: .github/workflows/e2e.yaml:45
  • Category: correctness
  • Problem: The 'Validate exact-commit dispatch' step (lines 45-80) runs before the inventory parsing in 'Generate E2E target matrix' step. The validation step cannot validate job membership against allowedJobs because the inventory hasn't been parsed yet. An attacker who can dispatch the workflow with crafted inputs could select arbitrary jobs.
  • Impact: A malicious workflow_dispatch with crafted jobs input could bypass job allowlist validation and execute arbitrary free-standing E2E jobs, potentially including jobs that access secrets or modify infrastructure.
  • Required action: Move the inventory parsing into the validation step (duplicate the npx tsx tools/e2e/workflow-inventory.mts --shell call) so job membership can be validated before E2E workspace preparation. Or restructure: parse inventory first, validate, then proceed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check .github/workflows/e2e.yaml generate-matrix job steps order: 'Validate exact-commit dispatch' vs 'Generate E2E target matrix' (id: matrix)
  • Missing regression test: Workflow contract test asserting validation step can reject unknown job IDs before workspace prep
  • Done when: The required change is committed and verification passes: Check .github/workflows/e2e.yaml generate-matrix job steps order: 'Validate exact-commit dispatch' vs 'Generate E2E target matrix' (id: matrix).
  • Evidence: In .github/workflows/e2e.yaml, 'Validate exact-commit dispatch' step (lines 45-80) precedes 'Generate E2E target matrix' step (id: matrix) which parses inventory via npx tsx tools/e2e/workflow-inventory.mts --shell

PRA-9 Required — Workflow conclusion differentiation incomplete in classifyRiskEvidence

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:285
  • Category: correctness
  • Problem: classifyRiskEvidence treats all non-success conclusions (cancelled, skipped, etc.) as neutral with identical title/summary. This loses diagnostic signal and treats cancelled runs the same as infrastructure failures.
  • Impact: Cancelled workflows (user action) and infrastructure failures (timed_out, action_required) produce identical neutral verdicts, making debugging impossible and hiding real failure modes.
  • Required action: Differentiate: ['failure', 'timed_out', 'action_required', 'startup_failure'] → failure; ['cancelled', 'skipped'] → neutral with distinct titles (e.g., 'Workflow cancelled', 'Workflow skipped'); workflowConclusion === 'success' with missing evidence → failure (see PRA-7).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read classifyRiskEvidence function in tools/e2e-advisor/post-merge-risk-gate.mts lines 285-350
  • Missing regression test: Parameterized tests for each workflow conclusion type in test/post-merge-e2e-risk-gate.test.ts
  • Done when: The required change is committed and verification passes: Read classifyRiskEvidence function in tools/e2e-advisor/post-merge-risk-gate.mts lines 285-350.
  • Evidence: Lines 295-305 in classifyRiskEvidence: if workflowConclusion !== 'success' returns generic neutral without distinguishing cancelled/skipped from failure/timed_out
Review findings by urgency: 5 required fixes, 16 items to resolve/justify, 10 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: tools/e2e-advisor/private-file.ts:6

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Runtime check that flags are effective (or portable fs.lstat fallback works)
  • 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: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-2 Resolve/justify — Source-of-truth review needed: tools/e2e-advisor/post-merge-risk-gate.mts:625

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: validateRiskPlan rejects jobs not in pinned allowlist
  • 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: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from checked-out workflow

PRA-3 Resolve/justify — Source-of-truth review needed: test/e2e/risk-signal-reporter.ts:98

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Test for duplicate test name handling across invocations
  • 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: Line 98: mergeSignal sums counts across invocations without deduplication by test identity

PRA-4 Resolve/justify — Source-of-truth review needed: .github/workflows/e2e.yaml:45-80

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Workflow contract: validation step validates job membership
  • 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: Validation step (lines 45-80) runs before 'Generate E2E target matrix' step (id: matrix) which parses inventory

PRA-10 Resolve/justify — O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: architecture
  • Problem: Source-of-truth review: O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable (Node.js on some platforms). This silently weakens symlink/hardlink/FIFO protection. The invalid state is a platform without these constants; the source boundary is Node's fs.constants; the source cannot be fixed in this PR because it's a platform limitation; regression test: runtime check that O_NOFOLLOW/O_NONBLOCK are effective; removal condition: when all supported platforms guarantee these constants.
  • Impact: On platforms where these constants are not defined (older Node, non-Linux), the flags become no-ops, allowing symlink following and potential blocking on FIFOs. This is a silent security weakening.
  • Recommended action: Add a runtime check that O_NOFOLLOW and O_NONBLOCK are effective (e.g., attempt to open a known symlink and verify ELOOP/ENXIO), or document platform requirements explicitly. Consider using fs.open with 'r'/'w' flags and manual fs.lstat verification as a portable alternative.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/private-file.ts lines 1-10 constants
  • Missing regression test: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform (or portable fs.lstat fallback works)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/private-file.ts lines 1-10 constants.
  • Evidence: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-11 Resolve/justify — validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: architecture
  • Problem: Source-of-truth review: validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning. If an attacker compromises main and modifies e2e.yaml to add malicious jobs to allowedJobs, the risk plan will accept them. The invalid state is a compromised e2e.yaml on main; the source boundary is the trusted main branch; the source cannot be fixed in this PR because the controller runs on main; regression test: validateRiskPlan rejects jobs not in a pinned allowlist; removal condition: when a separate pinned allowlist in a protected location (repository variable, protected file) is implemented.
  • Impact: If main branch is compromised, an attacker can inject malicious jobs into the risk plan's automaticJobs selection, causing the shadow gate to run attacker-controlled E2E jobs with repository secrets.
  • Recommended action: Document this trust assumption explicitly with a prominent comment. Consider pinning a known-good allowedJobs set in the controller or validating against a maintainer-approved list stored separately (e.g., in a protected file or repository variable).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function around line 625
  • Missing regression test: Integration test for validateRiskPlan with mocked workflow inventory showing rejection of injected jobs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function around line 625.
  • Evidence: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from the checked-out workflow without separate pinning

PRA-12 Resolve/justify — mergeSignal assumes no duplicate test names across Vitest invocations

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: architecture
  • Problem: Source-of-truth review: mergeSignal assumes no duplicate test names across Vitest invocations in the same job/shard. If a test is retried or run in multiple Vitest commands, summing counts may double-count. The invalid state is duplicate test execution; the source boundary is Vitest's test execution model; the source cannot be fixed in this PR because Vitest doesn't expose unique test IDs across runs; regression test: test for duplicate test names across invocations; removal condition: when Vitest provides stable test identities or deduplication is proven unnecessary.
  • Impact: If a test appears in multiple Vitest invocations (retries, sharding), passed/failed/skipped counts are inflated, potentially masking failures or creating false passes.
  • Recommended action: Document the assumption that each test runs exactly once per Vitest command per job/shard. Add deduplication by test name + file, or track unique test IDs. Add a test verifying duplicate test names across invocations are handled correctly. Add JSDoc for mergeSignal explaining runReason priority.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function lines 85-105
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function lines 85-105.
  • Evidence: Line 98: mergeSignal sums counts across invocations without deduplication by test identity

PRA-13 Resolve/justify — Validation step cannot validate job membership because inventory parsing happens later

  • Location: .github/workflows/e2e.yaml:75
  • Category: architecture
  • Problem: Source-of-truth review: The validation step cannot validate job membership because inventory parsing happens later. This is the same root cause as PRA-8 but framed as a localized workaround. The invalid state is a dispatch with unknown jobs; the source boundary is the generate-matrix job; the source cannot be fixed in this PR because inventory parsing is coupled to matrix generation; regression test: workflow contract test asserting validation step validates job membership; removal condition: when inventory parsing is moved before validation.
  • Impact: Same as PRA-8: unknown job IDs can be dispatched and executed before validation can reject them.
  • Recommended action: Duplicate the inventory fetch in the validation step (call npx tsx tools/e2e/workflow-inventory.mts --shell) and validate job membership before proceeding to Prepare E2E workspace.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/workflows/e2e.yaml generate-matrix job step order
  • Missing regression test: Workflow contract test asserting validation step validates job membership against inventory
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e.yaml generate-matrix job step order.
  • Evidence: Validation step at line 75 runs before inventory parsing in matrix step

PRA-14 Resolve/justify — O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: security
  • Problem: On platforms where these constants are not defined (older Node, non-Linux), the flags become no-ops, allowing symlink following and potential blocking on FIFOs. This is a silent security weakening.
  • Impact: Symlink/FIFO protection silently disabled on unsupported platforms, allowing potential file system race conditions or blocking attacks.
  • Recommended action: Add a runtime check at module load that verifies O_NOFOLLOW and O_NONBLOCK are effective (e.g., create a temp symlink, attempt open with flags, expect ELOOP). If not effective, throw or fall back to portable fs.lstat verification. Document minimum Node.js version/platform requirements.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/private-file.ts lines 1-10
  • Missing regression test: Runtime test that O_NOFOLLOW/O_NONBLOCK are effective or portable fallback is used
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/private-file.ts lines 1-10.
  • Evidence: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-15 Resolve/justify — validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: security
  • Problem: Same as PRA-2 but framed as security: a compromised main branch could inject malicious jobs into the risk plan's automaticJobs selection.
  • Impact: Compromised main branch allows injection of malicious jobs into risk plan, causing shadow gate to run attacker-controlled E2E jobs.
  • Recommended action: Document this trust assumption explicitly with a prominent comment. Consider a separate pinned allowlist in a protected location (repository variable, protected file) that the controller validates against.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function
  • Missing regression test: Integration test for validateRiskPlan with mocked workflow inventory showing rejection of injected jobs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function.
  • Evidence: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from the checked-out workflow

PRA-16 Resolve/justify — mergeSignal sums counts across Vitest invocations assuming no duplicate test names

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: correctness
  • Problem: mergeSignal sums counts across Vitest invocations assuming no duplicate test names. If a test appears in multiple invocations (e.g., retries, sharding), passed/failed/skipped counts are inflated, potentially masking failures or creating false passes.
  • Impact: Test count inflation can mask failures (failed + passed = higher passed) or create false confidence in pass rates.
  • Recommended action: Document the assumption clearly. Add a test for duplicate test names (PRA-T8) first. Only implement deduplication if the test reveals a real problem. Consider tracking test identities by (file, name) tuple.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function.
  • Evidence: Line 98: mergeSignal sums passed/failed/skipped/pending/unhandledErrors across invocations without deduplication

PRA-17 Resolve/justify — Concurrency group mitigation undocumented; correlationId collision probability not addressed

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:200
  • Category: correctness
  • Problem: The shadow workflow uses concurrency group 'e2e-post-merge-risk-gate-${{ github.sha }}' which prevents concurrent runs for the same commit, but if two pushes to main happen in rapid succession, the correlationId (randomUUID) could theoretically collide (122 bits of entropy, negligible but undocumented).
  • Impact: Theoretical correlationId collision could allow cross-run interference, though 122-bit entropy makes this negligible. Lack of documentation makes security review harder.
  • Recommended action: Add a comment documenting the collision mitigation rationale: correlationId is validated by child workflow, so only the controller that dispatched the run can complete its check. Consider using workflow_run event with correlation label if GitHub supports it in the future.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts around line 200 where correlationId is generated
  • Missing regression test: Concurrency group collision test for controller runs (theoretical, document rationale)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts around line 200 where correlationId is generated.
  • Evidence: Line 200: const correlationId = randomUUID(); // no collision documentation

PRA-18 Resolve/justify — Missing integration tests with mocked GitHub API for controller modes

  • Location: test/post-merge-e2e-risk-gate.test.ts:1
  • Category: tests
  • Problem: Missing integration tests with mocked GitHub API for controller modes (start, finish, abandon). Current tests mock fetch but don't exercise the full controller lifecycle end-to-end with error paths.
  • Impact: Error paths (dispatch failure, child run failure, evidence download failure, check completion failure) are not tested, leaving gaps in failure-mode coverage.
  • Recommended action: Add integration tests using nock or msw to mock GitHub API and verify the full controller lifecycle for all modes (start, finish, abandon). Test error paths: dispatch failure, child run failure, evidence download failure, check completion failure.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Review test/post-merge-e2e-risk-gate.test.ts for integration test coverage
  • Missing regression test: Integration tests for all controller modes with mocked GitHub API including error paths
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Review test/post-merge-e2e-risk-gate.test.ts for integration test coverage.
  • Evidence: Test file has unit tests for individual functions but no end-to-end integration tests mocking GitHub API for full controller lifecycle

PRA-19 Resolve/justify — Workflow contract test asserts excessive permissions

  • Location: test/post-merge-e2e-risk-gate-workflow.test.ts:15
  • Category: tests
  • Problem: Workflow contract test asserts excessive permissions (actions:write, checks:write, contents:read) matching the current flawed workflow. Once PRA-5 is fixed, this test must be updated to expect { checks: 'write', contents: 'read' }.
  • Impact: Test will pass with excessive permissions, failing to catch the security issue. After fix, test will fail until updated.
  • Recommended action: Update the workflow contract test to expect { checks: 'write', contents: 'read' } once PRA-5 is fixed, or update to match the documented rationale if actions:write is kept.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/post-merge-e2e-risk-gate-workflow.test.ts line 15 permissions assertion
  • Missing regression test: Workflow contract test asserting minimal permissions
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/post-merge-e2e-risk-gate-workflow.test.ts line 15 permissions assertion.
  • Evidence: Line 15: expect(workflow.permissions).toEqual({ actions: 'write', checks: 'write', contents: 'read' })

PRA-20 Resolve/justify — TOCTOU in GITHUB_OUTPUT write

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:386
  • Category: security
  • Problem: appendOutput uses fs.openSync with O_NOFOLLOW (when available) but does not verify the opened file is the expected GITHUB_OUTPUT by checking inode/device against a pre-stat. An attacker who can replace GITHUB_OUTPUT between stat and open could redirect output.
  • Impact: In a compromised runner environment, an attacker could replace GITHUB_OUTPUT between the stat check and open, redirecting controller outputs (check_id, run_id, state_hash) to a malicious file.
  • Recommended action: Use fs.openSync with O_NOFOLLOW (when available) and verify the opened file is the expected GITHUB_OUTPUT by checking inode/device against a pre-stat, or accept this as a low-risk TOCTOU in a trusted workflow context and document it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts appendOutput function around line 386
  • Missing regression test: Test demonstrating TOCTOU is mitigated or documented as accepted risk
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts appendOutput function around line 386.
  • Evidence: Line 386: fs.openSync(output, fs.constants.O_WRONLY | fs.constants.O_APPEND | (fs.constants.O_NOFOLLOW ?? 0)) without inode/device verification

PRA-21 Resolve/justify — Duplicate inventory fetch needed in validation step

  • Location: .github/workflows/e2e.yaml:75
  • Category: architecture
  • Problem: Same as PRA-4/PRA-8: the validation step should call the inventory script to validate job membership before workspace preparation.
  • Impact: Unknown job IDs can be dispatched and executed before validation can reject them.
  • Recommended action: Duplicate the inventory fetch in the validation step (call npx tsx tools/e2e/workflow-inventory.mts --shell) and validate job membership before proceeding to Prepare E2E workspace.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/workflows/e2e.yaml generate-matrix job step order
  • Missing regression test: Workflow contract test asserting validation step validates job membership against inventory
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e.yaml generate-matrix job step order.
  • Evidence: Validation step at line 75 runs before inventory parsing in matrix step

💡 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-22 Improvement — Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: architecture
  • Problem: Implement portable symlink/hardlink/FIFO detection using fs.lstat + manual checks as fallback, or add runtime verification that O_NOFOLLOW/O_NONBLOCK are effective. This would eliminate the platform dependency.
  • Impact: Platform-dependent constants create silent security weakening on unsupported platforms.
  • Suggested action: Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification: before open, lstat the path, verify it's a regular file with nlink=1, not a symlink, then open. This works on all platforms.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read tools/e2e-advisor/private-file.ts readPrivateRegularFile and writePrivateRegularFile
  • Missing regression test: Test that portable fs.lstat verification rejects symlinks/hardlinks/FIFOs on all platforms
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 1-10: O_NOFOLLOW/O_NONBLOCK constants with fallback to 0

PRA-23 Improvement — Document trust assumption for validateRiskPlan allowedJobs

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: architecture
  • Problem: Document the trust assumption explicitly. Consider a separate pinned allowlist in a protected location (repository variable, protected file) that the controller validates against.
  • Impact: Without documentation, future maintainers may not understand the trust boundary. Without separate pinning, compromised main = compromised risk plan.
  • Suggested action: Add a prominent comment in validateRiskPlan explaining the trust assumption. File a follow-up issue to implement a pinned allowlist in a protected location.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function for trust assumption comment
  • Missing regression test: None — documentation improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 625: validateRiskPlan trusts readFreeStandingJobsInventory().allowedJobs without separate pinning

PRA-24 Improvement — Add test for duplicate test names in mergeSignal

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: correctness
  • Problem: Add test for duplicate test names (PRA-T8) first. Only implement deduplication if the test reveals a real problem. Document the assumption clearly.
  • Impact: Without test, unknown if duplicate test names cause double-counting in practice.
  • Suggested action: Add test for mergeSignal duplicate test name handling. If it reveals a problem, implement deduplication by (file, name) tuple.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/e2e-risk-signal-reporter.test.ts for duplicate test name test
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: mergeSignal at line 98 sums counts without deduplication

PRA-25 Improvement — Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform

  • Location: test/post-merge-e2e-risk-gate.test.ts:1
  • Category: tests
  • Problem: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform.
  • Impact: Without runtime validation, the private-file protections may be silently disabled on the CI platform.
  • Suggested action: Add a test that verifies the private-file.ts protections work on the CI platform (ubuntu-latest). This can be a simple test that attempts to open a symlink with the private-file functions and expects rejection.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/post-merge-e2e-risk-gate.test.ts for O_NOFOLLOW/O_NONBLOCK effectiveness test
  • Missing regression test: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: No test currently verifies O_NOFOLLOW/O_NONBLOCK effectiveness on ubuntu-latest

PRA-26 Improvement — Integration test for validateRiskPlan with mocked workflow inventory

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: tests
  • Problem: Integration test for validateRiskPlan with mocked workflow inventory.
  • Impact: Without test, the trust assumption in validateRiskPlan is not verified against injected jobs.
  • Suggested action: Add integration test that mocks readFreeStandingJobsInventory to return a controlled allowedJobs set, then verifies validateRiskPlan rejects jobs not in that set.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/post-merge-e2e-risk-gate.test.ts for validateRiskPlan inventory mocking test
  • Missing regression test: Integration test for validateRiskPlan with mocked workflow inventory
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: No test mocks readFreeStandingJobsInventory to test validateRiskPlan rejection of unknown jobs

PRA-27 Improvement — Test for mergeSignal duplicate test name handling across Vitest invocations

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: tests
  • Problem: Test for mergeSignal duplicate test name handling across Vitest invocations.
  • Impact: Without test, unknown if duplicate test names cause double-counting in practice.
  • Suggested action: Add a test that calls writeRiskSignal twice with overlapping test names and verifies counts are not double-counted (or documents the current summing behavior).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/e2e-risk-signal-reporter.test.ts for duplicate test name test
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: mergeSignal at line 98 sums counts without deduplication

PRA-28 Improvement — Concurrency group collision test for controller runs

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:200
  • Category: tests
  • Problem: Concurrency group collision test for controller runs.
  • Impact: Theoretical collision risk undocumented and untested.
  • Suggested action: Add a test (or document) that the concurrency group + correlationId validation prevents cross-run interference. This is largely theoretical given 122-bit UUIDv4 entropy.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/post-merge-e2e-risk-gate.test.ts for concurrency collision test
  • Missing regression test: Concurrency group collision test for controller runs (or documentation of rationale)
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 200: correlationId = randomUUID() with no collision documentation

PRA-29 Improvement — classifyRiskEvidence tests for each workflow conclusion type

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:285
  • Category: tests
  • Problem: classifyRiskEvidence tests for each workflow conclusion type.
  • Impact: Without parameterized tests for each conclusion type, differentiation logic (PRA-9) cannot be verified.
  • Suggested action: Add parameterized tests covering: success (complete evidence → success), success (missing evidence → failure per PRA-7), failure → failure, timed_out → failure, action_required → failure, startup_failure → failure, cancelled → neutral (distinct title), skipped → neutral (distinct title).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check test/post-merge-e2e-risk-gate.test.ts classifyRiskEvidence test coverage
  • Missing regression test: Parameterized tests for each workflow conclusion type in classifyRiskEvidence
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Current tests cover success/failure/neutral but not cancelled/skipped/timed_out/action_required/startup_failure distinctly

PRA-30 Improvement — Consider splitting post-merge-risk-gate.mts into smaller modules

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:1
  • Category: scope
  • Problem: Consider splitting post-merge-risk-gate.mts into smaller modules (start, finish, abandon, validation, evidence, github-api). The file is 877 lines and handles multiple distinct responsibilities.
  • Impact: Large single file reduces reviewability, testability, and increases cognitive load for security audits.
  • Suggested action: Split into: controller-start.mts, controller-finish.mts, controller-abandon.mts, risk-evidence.mts, github-api.mts, risk-plan-validation.mts. Keep a thin main entry point. This improves testability and reviewability.
  • 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-advisor/post-merge-risk-gate.mts line count and responsibility boundaries
  • Missing regression test: None — refactoring; existing tests should cover behavior
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: File is 877 lines handling start/finish/abandon modes, GitHub API, risk plan validation, evidence classification, state management

PRA-31 Improvement — Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: scope
  • Problem: Replace O_NOFOLLOW/O_NONBLOCK fallback with portable fs.lstat verification.
  • Impact: Platform-dependent constants create silent security weakening on unsupported platforms.
  • Suggested action: Implement portable symlink/hardlink/FIFO detection using fs.lstat: before open, lstat the path, verify it's a regular file (isFile), nlink === 1, not a symlink. This works on all platforms without O_NOFOLLOW.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read tools/e2e-advisor/private-file.ts for current implementation
  • Missing regression test: Test that portable fs.lstat verification rejects symlinks/hardlinks/FIFOs on all platforms
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lines 1-10: O_NOFOLLOW/O_NONBLOCK constants with fallback to 0
Simplification opportunities: 3 possible cuts

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

  • PRA-22 native (tools/e2e-advisor/private-file.ts:6): O_NOFOLLOW/O_NONBLOCK constants with fallback to 0
    • Replacement: fs.lstatSync before open: verify isFile(), nlink === 1, not a symlink
    • Net: 5 lines
    • Safety boundary: Must preserve private regular file guarantee (no symlinks, no hardlinks, no FIFOs, nlink=1)
  • PRA-30 shrink (tools/e2e-advisor/post-merge-risk-gate.mts:1): Single 877-line file with multiple responsibilities
    • Replacement: 6 focused modules + thin main entry point
    • Net: 0 lines
    • Safety boundary: All existing tests must pass; no behavior change
  • PRA-31 native (tools/e2e-advisor/private-file.ts:6): O_NOFOLLOW/O_NONBLOCK constants with fallback to 0
    • Replacement: fs.lstatSync before open: verify isFile(), nlink === 1, not a symlink
    • Net: 5 lines
    • Safety boundary: Must preserve private regular file guarantee (no symlinks, no hardlinks, no FIFOs, nlink=1)
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 — classifyRiskEvidence: workflowConclusion='success' with partial signals → failure (PRA-7 fix). Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, .github/workflows/main.yaml, .github/workflows/post-merge-e2e-risk-gate-shadow.yaml, .github/workflows/pr.yaml, AGENTS.md, CONTRIBUTING.md, tools/e2e-advisor/README.md, tools/e2e-advisor/post-merge-risk-gate.mts.
  • PRA-T2 Runtime validation — classifyRiskEvidence: each workflow conclusion type (cancelled, skipped, timed_out, action_required, startup_failure) (PRA-9). Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, .github/workflows/main.yaml, .github/workflows/post-merge-e2e-risk-gate-shadow.yaml, .github/workflows/pr.yaml, AGENTS.md, CONTRIBUTING.md, tools/e2e-advisor/README.md, tools/e2e-advisor/post-merge-risk-gate.mts.
  • PRA-T3 Runtime validation — validateRiskPlan with mocked inventory rejecting injected jobs (PRA-2/11). Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, .github/workflows/main.yaml, .github/workflows/post-merge-e2e-risk-gate-shadow.yaml, .github/workflows/pr.yaml, AGENTS.md, CONTRIBUTING.md, tools/e2e-advisor/README.md, tools/e2e-advisor/post-merge-risk-gate.mts.
  • PRA-T4 Runtime validation — mergeSignal duplicate test names across invocations (PRA-3/12). Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, .github/workflows/main.yaml, .github/workflows/post-merge-e2e-risk-gate-shadow.yaml, .github/workflows/pr.yaml, AGENTS.md, CONTRIBUTING.md, tools/e2e-advisor/README.md, tools/e2e-advisor/post-merge-risk-gate.mts.
  • PRA-T5 Runtime validation — Integration tests: controller start/finish/abandon with mocked GitHub API (PRA-14). Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/e2e.yaml, .github/workflows/main.yaml, .github/workflows/post-merge-e2e-risk-gate-shadow.yaml, .github/workflows/pr.yaml, AGENTS.md, CONTRIBUTING.md, tools/e2e-advisor/README.md, tools/e2e-advisor/post-merge-risk-gate.mts.
  • PRA-T6 Missing integration tests with mocked GitHub API for controller modes — Add integration tests using nock or msw to mock GitHub API and verify the full controller lifecycle for all modes (start, finish, abandon). Test error paths: dispatch failure, child run failure, evidence download failure, check completion failure.
  • PRA-T7 Workflow contract test asserts excessive permissions — Update the workflow contract test to expect { checks: 'write', contents: 'read' } once PRA-5 is fixed, or update to match the documented rationale if actions:write is kept.
  • PRA-T8 Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform — Add a test that verifies the private-file.ts protections work on the CI platform (ubuntu-latest). This can be a simple test that attempts to open a symlink with the private-file functions and expects rejection.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: tools/e2e-advisor/private-file.ts:6

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Runtime check that flags are effective (or portable fs.lstat fallback works)
  • 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: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-2 Resolve/justify — Source-of-truth review needed: tools/e2e-advisor/post-merge-risk-gate.mts:625

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: validateRiskPlan rejects jobs not in pinned allowlist
  • 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: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from checked-out workflow

PRA-3 Resolve/justify — Source-of-truth review needed: test/e2e/risk-signal-reporter.ts:98

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Test for duplicate test name handling across invocations
  • 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: Line 98: mergeSignal sums counts across invocations without deduplication by test identity

PRA-4 Resolve/justify — Source-of-truth review needed: .github/workflows/e2e.yaml:45-80

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Workflow contract: validation step validates job membership
  • 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: Validation step (lines 45-80) runs before 'Generate E2E target matrix' step (id: matrix) which parses inventory

PRA-5 Required — Shadow workflow requests excessive actions:write permission

  • Location: .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12
  • Category: security
  • Problem: Shadow workflow declares permissions { actions: write, checks: write, contents: read }. Only checks:write (for check run creation/completion) and contents:read (for checkout) are strictly required. The actions:write permission expands attack surface allowing creation of misleading workflow dispatches if the controller is compromised.
  • Impact: If the controller is compromised, an attacker with actions:write can create arbitrary workflow dispatches, modify workflow files, or manipulate Actions state beyond the intended check-run lifecycle.
  • Required action: Reduce permissions to { checks: 'write', contents: 'read' }. The controller uses the GitHub REST API to dispatch the E2E workflow via POST /repos/{owner}/{repo}/actions/workflows/{workflow_id}/dispatches which only requires contents:read + repository scope in the token. If actions:write is genuinely required for the dispatch call, document the trust assumption explicitly with a prominent comment and add a security review gate for any changes to this workflow.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12 permissions block
  • Missing regression test: Workflow contract test in test/post-merge-e2e-risk-gate-workflow.test.ts should assert permissions equal { checks: 'write', contents: 'read' }
  • Done when: The required change is committed and verification passes: Check .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:12 permissions block.
  • Evidence: Line 12 of .github/workflows/post-merge-e2e-risk-gate-shadow.yaml shows: permissions: { actions: write, checks: write, contents: read }

PRA-6 Required — No scheduled cleanup for stale in_progress check runs after controller crash

  • Location: .github/workflows/post-merge-e2e-risk-gate-shadow.yaml:1
  • Category: security
  • Problem: If the shadow controller starts (creates check run with status=in_progress) but crashes before completing or abandoning, the check run remains in_progress indefinitely, blocking merge queues and giving false confidence.
  • Impact: Stale in_progress check runs block merge queues indefinitely and give false confidence that risk gate is still evaluating. No automated recovery exists.
  • Required action: Add a separate scheduled cleanup workflow (e.g., hourly) that queries check runs with conclusion=null and status=in_progress for 'E2E / Post-merge Risk Gate (shadow)' older than a grace period (e.g., 2 hours) and marks them neutral via GitHub API. This is a standard pattern for long-running check-run controllers.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search repository for any scheduled workflow that cleans up 'E2E / Post-merge Risk Gate (shadow)' check runs
  • Missing regression test: Integration test simulating controller crash mid-run and verifying scheduled cleanup marks the check neutral
  • Done when: The required change is committed and verification passes: Search repository for any scheduled workflow that cleans up 'E2E / Post-merge Risk Gate (shadow)' check runs.
  • Evidence: No scheduled workflow exists in .github/workflows/ that cleans up stale shadow check runs

PRA-7 Required — Missing evidence returns neutral instead of failure when workflow succeeds

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:315
  • Category: correctness
  • Problem: classifyRiskEvidence returns neutral for missing evidence when workflowConclusion is 'success'. This allows incomplete evidence (e.g., only 1 of 3 expected job shards uploaded) to produce a neutral (not red) check, which masks infrastructure failures where evidence upload silently fails.
  • Impact: Infrastructure failures (evidence upload broken, network issues, runner crashes) that prevent signal upload are masked as neutral instead of failing the risk gate, giving false confidence.
  • Required action: Change missing evidence verdict from 'neutral' to 'failure' when workflowConclusion is 'success' and expectedEvidence.length > 0. Add a distinct 'missing' verdict if differentiation is needed for diagnostics. The shadow check should only pass (success) when all expected job shards produce complete, unskipped evidence.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts lines 285-330 classifyRiskEvidence function
  • Missing regression test: Test case in test/post-merge-e2e-risk-gate.test.ts: workflowConclusion='success' with partial signals → expects conclusion='failure'
  • Done when: The required change is committed and verification passes: Read tools/e2e-advisor/post-merge-risk-gate.mts lines 285-330 classifyRiskEvidence function.
  • Evidence: Line 315-320 in classifyRiskEvidence: missing evidence returns { conclusion: 'neutral', title: 'Selected E2E jobs are missing test evidence', summary: 'At least one expected job shard did not upload a bound risk signal.' }

PRA-8 Required — Exact-commit validation runs before inventory parse; cannot validate job membership

  • Location: .github/workflows/e2e.yaml:45
  • Category: correctness
  • Problem: The 'Validate exact-commit dispatch' step (lines 45-80) runs before the inventory parsing in 'Generate E2E target matrix' step. The validation step cannot validate job membership against allowedJobs because the inventory hasn't been parsed yet. An attacker who can dispatch the workflow with crafted inputs could select arbitrary jobs.
  • Impact: A malicious workflow_dispatch with crafted jobs input could bypass job allowlist validation and execute arbitrary free-standing E2E jobs, potentially including jobs that access secrets or modify infrastructure.
  • Required action: Move the inventory parsing into the validation step (duplicate the npx tsx tools/e2e/workflow-inventory.mts --shell call) so job membership can be validated before E2E workspace preparation. Or restructure: parse inventory first, validate, then proceed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check .github/workflows/e2e.yaml generate-matrix job steps order: 'Validate exact-commit dispatch' vs 'Generate E2E target matrix' (id: matrix)
  • Missing regression test: Workflow contract test asserting validation step can reject unknown job IDs before workspace prep
  • Done when: The required change is committed and verification passes: Check .github/workflows/e2e.yaml generate-matrix job steps order: 'Validate exact-commit dispatch' vs 'Generate E2E target matrix' (id: matrix).
  • Evidence: In .github/workflows/e2e.yaml, 'Validate exact-commit dispatch' step (lines 45-80) precedes 'Generate E2E target matrix' step (id: matrix) which parses inventory via npx tsx tools/e2e/workflow-inventory.mts --shell

PRA-9 Required — Workflow conclusion differentiation incomplete in classifyRiskEvidence

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:285
  • Category: correctness
  • Problem: classifyRiskEvidence treats all non-success conclusions (cancelled, skipped, etc.) as neutral with identical title/summary. This loses diagnostic signal and treats cancelled runs the same as infrastructure failures.
  • Impact: Cancelled workflows (user action) and infrastructure failures (timed_out, action_required) produce identical neutral verdicts, making debugging impossible and hiding real failure modes.
  • Required action: Differentiate: ['failure', 'timed_out', 'action_required', 'startup_failure'] → failure; ['cancelled', 'skipped'] → neutral with distinct titles (e.g., 'Workflow cancelled', 'Workflow skipped'); workflowConclusion === 'success' with missing evidence → failure (see PRA-7).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read classifyRiskEvidence function in tools/e2e-advisor/post-merge-risk-gate.mts lines 285-350
  • Missing regression test: Parameterized tests for each workflow conclusion type in test/post-merge-e2e-risk-gate.test.ts
  • Done when: The required change is committed and verification passes: Read classifyRiskEvidence function in tools/e2e-advisor/post-merge-risk-gate.mts lines 285-350.
  • Evidence: Lines 295-305 in classifyRiskEvidence: if workflowConclusion !== 'success' returns generic neutral without distinguishing cancelled/skipped from failure/timed_out

PRA-10 Resolve/justify — O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: architecture
  • Problem: Source-of-truth review: O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable (Node.js on some platforms). This silently weakens symlink/hardlink/FIFO protection. The invalid state is a platform without these constants; the source boundary is Node's fs.constants; the source cannot be fixed in this PR because it's a platform limitation; regression test: runtime check that O_NOFOLLOW/O_NONBLOCK are effective; removal condition: when all supported platforms guarantee these constants.
  • Impact: On platforms where these constants are not defined (older Node, non-Linux), the flags become no-ops, allowing symlink following and potential blocking on FIFOs. This is a silent security weakening.
  • Recommended action: Add a runtime check that O_NOFOLLOW and O_NONBLOCK are effective (e.g., attempt to open a known symlink and verify ELOOP/ENXIO), or document platform requirements explicitly. Consider using fs.open with 'r'/'w' flags and manual fs.lstat verification as a portable alternative.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/private-file.ts lines 1-10 constants
  • Missing regression test: Runtime validation test that O_NOFOLLOW/O_NONBLOCK are effective on the platform (or portable fs.lstat fallback works)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/private-file.ts lines 1-10 constants.
  • Evidence: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-11 Resolve/justify — validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: architecture
  • Problem: Source-of-truth review: validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning. If an attacker compromises main and modifies e2e.yaml to add malicious jobs to allowedJobs, the risk plan will accept them. The invalid state is a compromised e2e.yaml on main; the source boundary is the trusted main branch; the source cannot be fixed in this PR because the controller runs on main; regression test: validateRiskPlan rejects jobs not in a pinned allowlist; removal condition: when a separate pinned allowlist in a protected location (repository variable, protected file) is implemented.
  • Impact: If main branch is compromised, an attacker can inject malicious jobs into the risk plan's automaticJobs selection, causing the shadow gate to run attacker-controlled E2E jobs with repository secrets.
  • Recommended action: Document this trust assumption explicitly with a prominent comment. Consider pinning a known-good allowedJobs set in the controller or validating against a maintainer-approved list stored separately (e.g., in a protected file or repository variable).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function around line 625
  • Missing regression test: Integration test for validateRiskPlan with mocked workflow inventory showing rejection of injected jobs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function around line 625.
  • Evidence: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from the checked-out workflow without separate pinning

PRA-12 Resolve/justify — mergeSignal assumes no duplicate test names across Vitest invocations

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: architecture
  • Problem: Source-of-truth review: mergeSignal assumes no duplicate test names across Vitest invocations in the same job/shard. If a test is retried or run in multiple Vitest commands, summing counts may double-count. The invalid state is duplicate test execution; the source boundary is Vitest's test execution model; the source cannot be fixed in this PR because Vitest doesn't expose unique test IDs across runs; regression test: test for duplicate test names across invocations; removal condition: when Vitest provides stable test identities or deduplication is proven unnecessary.
  • Impact: If a test appears in multiple Vitest invocations (retries, sharding), passed/failed/skipped counts are inflated, potentially masking failures or creating false passes.
  • Recommended action: Document the assumption that each test runs exactly once per Vitest command per job/shard. Add deduplication by test name + file, or track unique test IDs. Add a test verifying duplicate test names across invocations are handled correctly. Add JSDoc for mergeSignal explaining runReason priority.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function lines 85-105
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function lines 85-105.
  • Evidence: Line 98: mergeSignal sums counts across invocations without deduplication by test identity

PRA-13 Resolve/justify — Validation step cannot validate job membership because inventory parsing happens later

  • Location: .github/workflows/e2e.yaml:75
  • Category: architecture
  • Problem: Source-of-truth review: The validation step cannot validate job membership because inventory parsing happens later. This is the same root cause as PRA-8 but framed as a localized workaround. The invalid state is a dispatch with unknown jobs; the source boundary is the generate-matrix job; the source cannot be fixed in this PR because inventory parsing is coupled to matrix generation; regression test: workflow contract test asserting validation step validates job membership; removal condition: when inventory parsing is moved before validation.
  • Impact: Same as PRA-8: unknown job IDs can be dispatched and executed before validation can reject them.
  • Recommended action: Duplicate the inventory fetch in the validation step (call npx tsx tools/e2e/workflow-inventory.mts --shell) and validate job membership before proceeding to Prepare E2E workspace.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check .github/workflows/e2e.yaml generate-matrix job step order
  • Missing regression test: Workflow contract test asserting validation step validates job membership against inventory
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check .github/workflows/e2e.yaml generate-matrix job step order.
  • Evidence: Validation step at line 75 runs before inventory parsing in matrix step

PRA-14 Resolve/justify — O_NOFOLLOW/O_NONBLOCK fallback defaults to 0 if constants unavailable

  • Location: tools/e2e-advisor/private-file.ts:6
  • Category: security
  • Problem: On platforms where these constants are not defined (older Node, non-Linux), the flags become no-ops, allowing symlink following and potential blocking on FIFOs. This is a silent security weakening.
  • Impact: Symlink/FIFO protection silently disabled on unsupported platforms, allowing potential file system race conditions or blocking attacks.
  • Recommended action: Add a runtime check at module load that verifies O_NOFOLLOW and O_NONBLOCK are effective (e.g., create a temp symlink, attempt open with flags, expect ELOOP). If not effective, throw or fall back to portable fs.lstat verification. Document minimum Node.js version/platform requirements.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/private-file.ts lines 1-10
  • Missing regression test: Runtime test that O_NOFOLLOW/O_NONBLOCK are effective or portable fallback is used
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/private-file.ts lines 1-10.
  • Evidence: Line 6: const NO_FOLLOW = fs.constants.O_NOFOLLOW ?? 0; const NON_BLOCK = fs.constants.O_NONBLOCK ?? 0;

PRA-15 Resolve/justify — validateRiskPlan trusts allowedJobs from checked-out e2e.yaml without separate pinning

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:625
  • Category: security
  • Problem: Same as PRA-2 but framed as security: a compromised main branch could inject malicious jobs into the risk plan's automaticJobs selection.
  • Impact: Compromised main branch allows injection of malicious jobs into risk plan, causing shadow gate to run attacker-controlled E2E jobs.
  • Recommended action: Document this trust assumption explicitly with a prominent comment. Consider a separate pinned allowlist in a protected location (repository variable, protected file) that the controller validates against.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function
  • Missing regression test: Integration test for validateRiskPlan with mocked workflow inventory showing rejection of injected jobs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts validateRiskPlan function.
  • Evidence: Line 625: validateRiskPlan calls readFreeStandingJobsInventory().allowedJobs directly from the checked-out workflow

PRA-16 Resolve/justify — mergeSignal sums counts across Vitest invocations assuming no duplicate test names

  • Location: test/e2e/risk-signal-reporter.ts:98
  • Category: correctness
  • Problem: mergeSignal sums counts across Vitest invocations assuming no duplicate test names. If a test appears in multiple invocations (e.g., retries, sharding), passed/failed/skipped counts are inflated, potentially masking failures or creating false passes.
  • Impact: Test count inflation can mask failures (failed + passed = higher passed) or create false confidence in pass rates.
  • Recommended action: Document the assumption clearly. Add a test for duplicate test names (PRA-T8) first. Only implement deduplication if the test reveals a real problem. Consider tracking test identities by (file, name) tuple.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function
  • Missing regression test: Test for mergeSignal duplicate test name handling across Vitest invocations
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e/risk-signal-reporter.ts mergeSignal function.
  • Evidence: Line 98: mergeSignal sums passed/failed/skipped/pending/unhandledErrors across invocations without deduplication

PRA-17 Resolve/justify — Concurrency group mitigation undocumented; correlationId collision probability not addressed

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:200
  • Category: correctness
  • Problem: The shadow workflow uses concurrency group 'e2e-post-merge-risk-gate-${{ github.sha }}' which prevents concurrent runs for the same commit, but if two pushes to main happen in rapid succession, the correlationId (randomUUID) could theoretically collide (122 bits of entropy, negligible but undocumented).
  • Impact: Theoretical correlationId collision could allow cross-run interference, though 122-bit entropy makes this negligible. Lack of documentation makes security review harder.
  • Recommended action: Add a comment documenting the collision mitigation rationale: correlationId is validated by child workflow, so only the controller that dispatched the run can complete its check. Consider using workflow_run event with correlation label if GitHub supports it in the future.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts around line 200 where correlationId is generated
  • Missing regression test: Concurrency group collision test for controller runs (theoretical, document rationale)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts around line 200 where correlationId is generated.
  • Evidence: Line 200: const correlationId = randomUUID(); // no collision documentation

PRA-18 Resolve/justify — Missing integration tests with mocked GitHub API for controller modes

  • Location: test/post-merge-e2e-risk-gate.test.ts:1
  • Category: tests
  • Problem: Missing integration tests with mocked GitHub API for controller modes (start, finish, abandon). Current tests mock fetch but don't exercise the full controller lifecycle end-to-end with error paths.
  • Impact: Error paths (dispatch failure, child run failure, evidence download failure, check completion failure) are not tested, leaving gaps in failure-mode coverage.
  • Recommended action: Add integration tests using nock or msw to mock GitHub API and verify the full controller lifecycle for all modes (start, finish, abandon). Test error paths: dispatch failure, child run failure, evidence download failure, check completion failure.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Review test/post-merge-e2e-risk-gate.test.ts for integration test coverage
  • Missing regression test: Integration tests for all controller modes with mocked GitHub API including error paths
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Review test/post-merge-e2e-risk-gate.test.ts for integration test coverage.
  • Evidence: Test file has unit tests for individual functions but no end-to-end integration tests mocking GitHub API for full controller lifecycle

PRA-19 Resolve/justify — Workflow contract test asserts excessive permissions

  • Location: test/post-merge-e2e-risk-gate-workflow.test.ts:15
  • Category: tests
  • Problem: Workflow contract test asserts excessive permissions (actions:write, checks:write, contents:read) matching the current flawed workflow. Once PRA-5 is fixed, this test must be updated to expect { checks: 'write', contents: 'read' }.
  • Impact: Test will pass with excessive permissions, failing to catch the security issue. After fix, test will fail until updated.
  • Recommended action: Update the workflow contract test to expect { checks: 'write', contents: 'read' } once PRA-5 is fixed, or update to match the documented rationale if actions:write is kept.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/post-merge-e2e-risk-gate-workflow.test.ts line 15 permissions assertion
  • Missing regression test: Workflow contract test asserting minimal permissions
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/post-merge-e2e-risk-gate-workflow.test.ts line 15 permissions assertion.
  • Evidence: Line 15: expect(workflow.permissions).toEqual({ actions: 'write', checks: 'write', contents: 'read' })

PRA-20 Resolve/justify — TOCTOU in GITHUB_OUTPUT write

  • Location: tools/e2e-advisor/post-merge-risk-gate.mts:386
  • Category: security
  • Problem: appendOutput uses fs.openSync with O_NOFOLLOW (when available) but does not verify the opened file is the expected GITHUB_OUTPUT by checking inode/device against a pre-stat. An attacker who can replace GITHUB_OUTPUT between stat and open could redirect output.
  • Impact: In a compromised runner environment, an attacker could replace GITHUB_OUTPUT between the stat check and open, redirecting controller outputs (check_id, run_id, state_hash) to a malicious file.
  • Recommended action: Use fs.openSync with O_NOFOLLOW (when available) and verify the opened file is the expected GITHUB_OUTPUT by checking inode/device against a pre-stat, or accept this as a low-risk TOCTOU in a trusted workflow context and document it.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts appendOutput function around line 386
  • Missing regression test: Test demonstrating TOCTOU is mitigated or documented as accepted risk
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/e2e-advisor/post-merge-risk-gate.mts appendOutput function around line 386.
  • Evidence: Line 386: fs.openSync(output, fs.constants.O_WRONLY | fs.constants.O_APPEND | (fs.constants.O_NOFOLLOW ?? 0)) without inode/device verification

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>
Comment thread test/e2e/risk-signal-reporter.ts Fixed
Comment thread tools/e2e-advisor/post-merge-risk-gate.mts Fixed
Comment thread tools/e2e-advisor/post-merge-risk-gate.mts Fixed
Comment thread tools/e2e-advisor/post-merge-risk-gate.mts Fixed
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 5 test follow-ups
Since last review: 2 prior items resolved · 0 still apply · 0 new items found

Action checklist

  • 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
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 — post-merge shadow dispatch records the returned workflow_run_id and downloads artifacts only from that run id. Static and unit coverage is strong for the controller, workflow contracts, signal reporter, private files, and aggregate CI gates. The changed behavior still crosses live GitHub workflow dispatch, returned run details, `gh run watch/download`, artifact extraction, check-run updates, and secret-bearing E2E job boundaries that are best validated with targeted runtime exercises.
  • PRA-T2 Runtime validation — risk-shadow workflow_dispatch with a historical main ancestor checkout_sha fails before Prepare E2E workspace. Static and unit coverage is strong for the controller, workflow contracts, signal reporter, private files, and aggregate CI gates. The changed behavior still crosses live GitHub workflow dispatch, returned run details, `gh run watch/download`, artifact extraction, check-run updates, and secret-bearing E2E job boundaries that are best validated with targeted runtime exercises.
  • PRA-T3 Runtime validation — successful child run with no risk-signal.json completes the shadow check as neutral. Static and unit coverage is strong for the controller, workflow contracts, signal reporter, private files, and aggregate CI gates. The changed behavior still crosses live GitHub workflow dispatch, returned run details, `gh run watch/download`, artifact extraction, check-run updates, and secret-bearing E2E job boundaries that are best validated with targeted runtime exercises.
  • PRA-T4 Runtime validation — complete matrix shard evidence from the returned child run classifies success while unrelated artifact trees are ignored or rejected. Static and unit coverage is strong for the controller, workflow contracts, signal reporter, private files, and aggregate CI gates. The changed behavior still crosses live GitHub workflow dispatch, returned run details, `gh run watch/download`, artifact extraction, check-run updates, and secret-bearing E2E job boundaries that are best validated with targeted runtime exercises.
  • PRA-T5 Runtime validation — state digest mismatch after evidence download closes neutral before signal classification. Static and unit coverage is strong for the controller, workflow contracts, signal reporter, private files, and aggregate CI gates. The changed behavior still crosses live GitHub workflow dispatch, returned run details, `gh run watch/download`, artifact extraction, check-run updates, and secret-bearing E2E job boundaries that are best validated with targeted runtime exercises.

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.

cv added 3 commits July 8, 2026 00:34
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Comment thread tools/e2e-advisor/private-file.ts Fixed
Comment thread tools/advisors/github.mts Fixed
Comment thread tools/e2e-advisor/private-file.ts Fixed
cv added 2 commits July 8, 2026 00:50
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Comment thread tools/advisors/github.mts Fixed
Comment thread tools/advisors/github.mts Fixed
Comment thread tools/e2e-advisor/private-file.ts Fixed
cv added 2 commits July 8, 2026 01:05
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Automated-review follow-up on head 945808241:

Resolved findings

  • GPT PRA-1 is fixed structurally in 945808241: the controller opts into GitHub REST 2022-11-28 return_run_details, validates the returned positive run ID and repository-bound API/HTML URLs, and uses that ID as the sole selector for wait, download, and finish. Correlation polling and its lookalike-run race are gone. The GPT advisor now reports no blocking findings and marks the prior item resolved.
  • The direct returned ID preserves the fail-closed main-advance case: a C1 controller that dispatches after main reaches C2 receives the actual C2 child ID; the child rejects checkout_sha=C1 before preparation, and finish records that direct child's failure.
  • CodeQL's temporary-file finding remains fixed structurally: the controller uses an owned 0700 mktemp workspace, private descriptor-safe files, bounded evidence traversal, and numeric-only GitHub outputs. Exact-merge CodeQL reports 0 JavaScript/TypeScript and 0 Python results.

Advisor dispositions

  • GPT test follow-ups are covered or intentionally bounded: mocked dispatch proves the exact endpoint, method, response validation, and returned ID; workflow contracts prove that ID feeds wait/download/finish and that historical checkout_sha is rejected before preparation; evidence tests cover missing signals, per-shard completeness, and bounded unrelated artifact traversal. A live dispatch was not run against unmerged secret-bearing workflow code; the trusted post-merge path is the acceptance boundary.
  • Nemotron PRA-6 is not applicable: actions:write is required by GitHub's workflow-dispatch endpoint; checks:write is used only for the shadow check. Removing it would disable the feature.
  • Nemotron PRA-7 is operational cleanup debt, not a merge/security blocker for a non-required post-merge shadow check. Normal start/finish exceptions are already closed neutral by controller catches and workflow abandonment steps; a hard runner/process death may require later stale-check cleanup, but adding a scheduled mutating workflow is outside this immediate safety PR.
  • Nemotron PRA-8/PRA-16 invert the documented evidence policy: missing, skipped, partial, ambiguous, or cap-limited evidence is neutral, never green. Success requires complete, unskipped, exact-SHA evidence from every expected shard; product/test failures remain red.
  • Nemotron PRA-1/PRA-2/PRA-11 are covered by the Linux-only runner boundary plus descriptor checks, O_NOFOLLOW/O_NONBLOCK, owned private directories, hardlink/symlink/FIFO rejection, and regression tests.
  • Nemotron PRA-3/PRA-5/PRA-9/PRA-10 are deliberate trust ordering: the controller, inventory, and E2E workflow all come from trusted current main; exact-current-main rejection runs before preparation, and the parsed inventory rejects unknown jobs before selected jobs execute. A duplicated allowlist would not create an independent trust boundary.
  • Nemotron PRA-4/PRA-12/PRA-20 misread intentional aggregation: each reporter call is a separate Vitest command, retries are already collapsed by Vitest, counts accumulate across commands, and failure/interruption stays sticky. Test names are not used as identities.
  • Nemotron PRA-13 is superseded for run selection by the authoritative returned run ID; UUIDv4 remains only as evidence metadata. PRA-14 shard IDs and matrix shapes are already validated. PRA-15/PRA-T6 are covered by the catch-all src/lib/actions/sandbox/**/*.ts lifecycle rule and representative nested-path tests. PRA-17 depends on the invalid permission/missing-evidence premises. PRA-18 is covered by the new mocked GitHub dispatch/finish tests. PRA-19 is intentionally explicit per job as a defense-in-depth opt-in.

Verification

  • 44 focused controller/workflow/PR-contract tests passed.
  • All 770 e2e-support tests passed.
  • CLI type-check, Biome, git diff --check, diff-scoped checks, normal commit/push hooks, and docs validation passed (0 errors; 2 pre-existing hidden Fern warnings).
  • GitHub CI: 37 passed, 2 intentional skips, 0 failed/pending.
  • Every commit is DCO-signed and GitHub Verified.

cv added 2 commits July 8, 2026 01:27
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@ericksoa ericksoa added v0.0.78 and removed v0.0.77 labels Jul 8, 2026
@cjagwani cjagwani added v0.0.79 and removed v0.0.78 labels Jul 8, 2026
cv added 2 commits July 8, 2026 15:17
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Comment thread tools/e2e-advisor/post-merge-risk-gate.mts Dismissed
@cv

cv commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Automated-review follow-up for b5552e68b

The GPT-5.5 advisor now reports merge_as_is: both controller-state/evidence findings are resolved, with 0 required items, 0 warnings, 0 suggestions, and no new findings.

  • PRA-T5 is covered deterministically by test/post-merge-e2e-risk-gate.test.ts: the test hashes the original state, changes the state file, and proves finish rejects before any child-run lookup or evidence classification while issuing only the neutral check-run update.
  • PRA-T1PRA-T4 remain optional live confidence exercises. Returned-run selection, historical-commit rejection, missing-signal neutrality, shard completeness, and unrelated-artifact handling already have controller and workflow contract coverage. Running the secret-bearing post-merge path from unmerged code would violate the trust boundary this PR establishes; the trusted main-push workflow is the runtime acceptance boundary.
  • The latest Nemotron lane repeats the previously dispositioned permission, stale-check, neutral-evidence, inventory, private-file, and signal-aggregation concerns from the earlier audit. The new state-digest change does not alter those decisions, and Nemotron reported no defect in the digest binding itself.

Verification after the fix: 84 focused tests passed; Biome, title validation, normal commit hooks, CLI pre-push typechecking, DCO, commit lint, static checks, E2E support, and the primary advisor all pass.

@cv
cv merged commit 9f03fe0 into codex/deterministic-pr-risk-plan Jul 9, 2026
45 of 48 checks passed
@cv
cv deleted the codex/e2e-risk-gate-shadow branch July 9, 2026 04:39
cv added a commit that referenced this pull request Jul 9, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a shared deterministic regression risk plan for the PR head commit
and changed-file set to PR Review Advisor and E2E Advisor. Known
high-risk runtime surfaces now restore a reviewed floor of required E2E
jobs even when a model omits or downgrades them.

Execution is intentionally separated from plan generation: stacked
follow-up #6447 consumes this contract in a trusted post-merge shadow
gate without exposing repository secrets to unmerged code.

PR Review Advisor analysis is split into seven bounded, evidence-focused
turns with completion logs and per-turn response artifacts, improving
review visibility and traceability while preserving one read-only
session and the existing final JSON/comment contract.

The seven stages now live in one ordered array. Stage names, evidence,
prompt text, runtime numbering, and artifact ordering derive from that
single declaration instead of parallel hard-coded topology.

## Changes

- Add a stable risk-plan contract with lifecycle, upgrade, agent,
inference, messaging, platform, credential, and security invariants.
- Keep the canonical cloud-onboard live target in the platform-install
floor through an explicit test-file allowlist, while ordinary tests
remain excluded.
- Feed the plan into PR Review Advisor scope/risk, security/trust, and
tests/regressions contexts and both E2E Advisor normalizers.\n-
Deduplicate optional recommendations against deterministic required jobs
by both selector ID and workflow job.
- Clarify commit references in advisor, E2E, and security-review
documentation.
- Emit `risk-plan.json` from both E2E Advisor lanes with a plan hash,
required jobs, capped automatic candidates, and manual-expansion state;
PR Review Advisor exposes the same plan through its scoped context
artifacts.
- Replace four uneven advisor turns with seven bounded stages:
scope/risk, correctness/state, security/trust, tests/regressions,
CI/operations, reconciliation, and final synthesis.
- Derive turn numbering and prompt-artifact ordering from the ordered
stage declaration, and remove five duplicate standalone context JSONs
while retaining the same evidence in per-stage synthetic-tool artifacts.
- Log every turn boundary and persist each settled response immediately,
including partial output and failure/timeout status, while treating
missing trace artifacts as fatal.
- Leave live enforcement to stacked follow-up #6447 so this PR remains a
reviewable deterministic planning primitive.
- Remove unused PR-advisor check-wait configuration and align
contributor-facing documentation with actual workflow behavior.
- Add focused normalization, prompt, risk-mapping, workflow-inventory,
and review-depth regression tests.

## 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 exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: Internal CI/advisor behavior
only; contributor-facing tool and E2E documentation was updated, with no
Fern user documentation required.
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Automated and local
security review confirmed the deterministic floor is additive, bound to
the PR head commit and changed-file set, capped, and does not itself
execute secret-bearing live E2E work; enforcement is isolated in #6447.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — 112 focused risk-plan, PR-advisor,
E2E-advisor, workflow-boundary, and maintainer-policy tests passed; the
E2E-advisor suite was rerun after the final normalization fix (7/7); CLI
typechecking, Biome, test-size, and Vitest project-membership checks
passed
- [ ] Applicable broad gate passed — CI for PR head commit `9700a6ba`
has 39 successful checks and 2 expected skips. `static-checks` fails
only because merged #6441 left
`test/langchain-deepagents-code-image.test.ts` at 1501 lines against the
1500-line mainline ceiling; aggregate `checks` mirrors that failure. The
repair is tracked in #6493.
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [x] `npm run docs` builds without warnings (doc changes only)
- [ ] 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>
@jyaunches jyaunches mentioned this pull request Jul 9, 2026
21 tasks
cv pushed a commit that referenced this pull request Jul 9, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds the pre-tag v0.0.79 release notes entry to
`docs/about/release-notes.mdx` so the release plan can be generated
after docs merge.
The entry summarizes the merged v0.0.79 release train across inference,
diagnostics, runtime hardening, policies, onboarding recovery, and
release validation.

## Changes
- Added the v0.0.79 release notes section with linked follow-up
documentation for OpenRouter onboarding, managed vLLM changes,
completion and logging, Deep Agents runtime limits, policy updates,
onboarding recovery, and release validation.
- Source summary:
- #6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter
onboarding support and links to inference/provider references.
- #6271 and #6272 -> `docs/about/release-notes.mdx`: Documents shell
completion and structured logging highlights.
- #6465, #6539, #6570, and #6528 -> `docs/about/release-notes.mdx`:
Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX
Spark express-install diagnostics.
- #6523, #6551, #6484, #6488, #6324, and #6542 ->
`docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool
parser, compaction, and timeout/readiness improvements.
- #6559, #6538, #6560, #6568, #6552, #6567, and #6587 ->
`docs/about/release-notes.mdx`: Documents runtime, credential, proxy,
PID namespace, TOML, and provider-state hardening.
- #6541, #5415, #6246, #6496, and #6573 ->
`docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy,
MCP allowlist, WhatsApp, and messaging-variant updates.
- #6253, #6572, #6444, #6536, and #5860 ->
`docs/about/release-notes.mdx`: Documents onboarding resume and
create-step recovery improvements.
- #6508, #6527, #5506, #6588, #6446, #6447, #6582, #6296, #6367, #6397,
and #6505 -> `docs/about/release-notes.mdx`: Documents docs,
release-risk, and E2E validation updates.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: Release-note prose only.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: Tests
not applicable, release-note prose only.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Docs validation note: `npm run docs:check-agent-variants && npm run
docs:check-routes && git diff --check` passed. Full `npm run docs` is
currently blocked before Fern validation because the pinned
`fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching
version found`).

---
<!-- 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: Julie Yaunches <jyaunches@nvidia.com>


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

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.79 with a new summary of recent
improvements, including onboarding and inference options, operator/CLI
diagnostics, sandbox recovery hardening, runtime limits, network policy
behavior, and release validation updates.
  * Added updated references and links for the latest release.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure feature PR adds or expands user-visible functionality labels Jul 11, 2026
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a shared deterministic regression risk plan for the PR head commit
and changed-file set to PR Review Advisor and E2E Advisor. Known
high-risk runtime surfaces now restore a reviewed floor of required E2E
jobs even when a model omits or downgrades them.

Execution is intentionally separated from plan generation: stacked
follow-up NVIDIA#6447 consumes this contract in a trusted post-merge shadow
gate without exposing repository secrets to unmerged code.

PR Review Advisor analysis is split into seven bounded, evidence-focused
turns with completion logs and per-turn response artifacts, improving
review visibility and traceability while preserving one read-only
session and the existing final JSON/comment contract.

The seven stages now live in one ordered array. Stage names, evidence,
prompt text, runtime numbering, and artifact ordering derive from that
single declaration instead of parallel hard-coded topology.

## Changes

- Add a stable risk-plan contract with lifecycle, upgrade, agent,
inference, messaging, platform, credential, and security invariants.
- Keep the canonical cloud-onboard live target in the platform-install
floor through an explicit test-file allowlist, while ordinary tests
remain excluded.
- Feed the plan into PR Review Advisor scope/risk, security/trust, and
tests/regressions contexts and both E2E Advisor normalizers.\n-
Deduplicate optional recommendations against deterministic required jobs
by both selector ID and workflow job.
- Clarify commit references in advisor, E2E, and security-review
documentation.
- Emit `risk-plan.json` from both E2E Advisor lanes with a plan hash,
required jobs, capped automatic candidates, and manual-expansion state;
PR Review Advisor exposes the same plan through its scoped context
artifacts.
- Replace four uneven advisor turns with seven bounded stages:
scope/risk, correctness/state, security/trust, tests/regressions,
CI/operations, reconciliation, and final synthesis.
- Derive turn numbering and prompt-artifact ordering from the ordered
stage declaration, and remove five duplicate standalone context JSONs
while retaining the same evidence in per-stage synthetic-tool artifacts.
- Log every turn boundary and persist each settled response immediately,
including partial output and failure/timeout status, while treating
missing trace artifacts as fatal.
- Leave live enforcement to stacked follow-up NVIDIA#6447 so this PR remains a
reviewable deterministic planning primitive.
- Remove unused PR-advisor check-wait configuration and align
contributor-facing documentation with actual workflow behavior.
- Add focused normalization, prompt, risk-mapping, workflow-inventory,
and review-depth regression tests.

## 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 exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: Internal CI/advisor behavior
only; contributor-facing tool and E2E documentation was updated, with no
Fern user documentation required.
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Automated and local
security review confirmed the deterministic floor is additive, bound to
the PR head commit and changed-file set, capped, and does not itself
execute secret-bearing live E2E work; enforcement is isolated in NVIDIA#6447.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — 112 focused risk-plan, PR-advisor,
E2E-advisor, workflow-boundary, and maintainer-policy tests passed; the
E2E-advisor suite was rerun after the final normalization fix (7/7); CLI
typechecking, Biome, test-size, and Vitest project-membership checks
passed
- [ ] Applicable broad gate passed — CI for PR head commit `9700a6ba`
has 39 successful checks and 2 expected skips. `static-checks` fails
only because merged NVIDIA#6441 left
`test/langchain-deepagents-code-image.test.ts` at 1501 lines against the
1500-line mainline ceiling; aggregate `checks` mirrors that failure. The
repair is tracked in NVIDIA#6493.
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [x] `npm run docs` builds without warnings (doc changes only)
- [ ] 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>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds the pre-tag v0.0.79 release notes entry to
`docs/about/release-notes.mdx` so the release plan can be generated
after docs merge.
The entry summarizes the merged v0.0.79 release train across inference,
diagnostics, runtime hardening, policies, onboarding recovery, and
release validation.

## Changes
- Added the v0.0.79 release notes section with linked follow-up
documentation for OpenRouter onboarding, managed vLLM changes,
completion and logging, Deep Agents runtime limits, policy updates,
onboarding recovery, and release validation.
- Source summary:
- NVIDIA#6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter
onboarding support and links to inference/provider references.
- NVIDIA#6271 and NVIDIA#6272 -> `docs/about/release-notes.mdx`: Documents shell
completion and structured logging highlights.
- NVIDIA#6465, NVIDIA#6539, NVIDIA#6570, and NVIDIA#6528 -> `docs/about/release-notes.mdx`:
Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX
Spark express-install diagnostics.
- NVIDIA#6523, NVIDIA#6551, NVIDIA#6484, NVIDIA#6488, NVIDIA#6324, and NVIDIA#6542 ->
`docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool
parser, compaction, and timeout/readiness improvements.
- NVIDIA#6559, NVIDIA#6538, NVIDIA#6560, NVIDIA#6568, NVIDIA#6552, NVIDIA#6567, and NVIDIA#6587 ->
`docs/about/release-notes.mdx`: Documents runtime, credential, proxy,
PID namespace, TOML, and provider-state hardening.
- NVIDIA#6541, NVIDIA#5415, NVIDIA#6246, NVIDIA#6496, and NVIDIA#6573 ->
`docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy,
MCP allowlist, WhatsApp, and messaging-variant updates.
- NVIDIA#6253, NVIDIA#6572, NVIDIA#6444, NVIDIA#6536, and NVIDIA#5860 ->
`docs/about/release-notes.mdx`: Documents onboarding resume and
create-step recovery improvements.
- NVIDIA#6508, NVIDIA#6527, NVIDIA#5506, NVIDIA#6588, NVIDIA#6446, NVIDIA#6447, NVIDIA#6582, NVIDIA#6296, NVIDIA#6367, NVIDIA#6397,
and NVIDIA#6505 -> `docs/about/release-notes.mdx`: Documents docs,
release-risk, and E2E validation updates.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: Release-note prose only.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: Tests
not applicable, release-note prose only.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Docs validation note: `npm run docs:check-agent-variants && npm run
docs:check-routes && git diff --check` passed. Full `npm run docs` is
currently blocked before Fern validation because the pinned
`fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching
version found`).

---
<!-- 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: Julie Yaunches <jyaunches@nvidia.com>


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

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.79 with a new summary of recent
improvements, including onboarding and inference options, operator/CLI
diagnostics, sandbox recovery hardening, runtime limits, network policy
behavior, and release validation updates.
  * Added updated references and links for the latest release.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 feature PR adds or expands user-visible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants