fix(ci): bound shadow E2E wait logs - #6648
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe shadow workflow replaces ChangesE2E risk gate wait
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ShadowWorkflow
participant gh_run_view
participant ChildE2ERun
ShadowWorkflow->>gh_run_view: query child-run status and conclusion
gh_run_view->>ChildE2ERun: retrieve run state
ChildE2ERun-->>gh_run_view: return state
gh_run_view-->>ShadowWorkflow: return JSON state
ShadowWorkflow->>ShadowWorkflow: sleep 10 and repeat until completion or timeout
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
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. |
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/post-merge-e2e-risk-gate-workflow.test.ts (1)
43-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fake commands enforce the external contract.
The fake
ghaccepts any command, while the timeout scenario returns124without validating the 105-minute bound. Consequently, incorrect CLI arguments can pass, and the exactwait.runstring assertions lock tests to shell spelling.Have the fakes reject unexpected subcommands, JSON fields, and timeout options, then rely on subprocess outcomes instead of positive source-text assertions.
As per path instructions, prefer observable outcomes and flag broad mocks that bypass the behavior under test.
Also applies to: 150-161, 192-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/post-merge-e2e-risk-gate-workflow.test.ts` around lines 43 - 99, Strengthen runWaitStep and its related tests so the fake gh validates the expected subcommand and JSON fields, while the fake timeout validates the required 105-minute option before simulating outcomes. Update scenarios to fail on unexpected arguments or options, and replace exact wait.run source-text assertions with subprocess outcome assertions that exercise the workflow behavior. Apply the same contract-focused changes to the additional test sections referenced in the review, avoiding broad mocks that bypass the commands under test.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/post-merge-e2e-risk-gate-workflow.test.ts`:
- Around line 43-99: Strengthen runWaitStep and its related tests so the fake gh
validates the expected subcommand and JSON fields, while the fake timeout
validates the required 105-minute option before simulating outcomes. Update
scenarios to fail on unexpected arguments or options, and replace exact wait.run
source-text assertions with subprocess outcome assertions that exercise the
workflow behavior. Apply the same contract-focused changes to the additional
test sections referenced in the review, avoiding broad mocks that bypass the
commands under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d31b57fb-5c83-4aa5-a26d-0b2890987597
📒 Files selected for processing (2)
.github/workflows/post-merge-e2e-risk-gate-shadow.yamltest/post-merge-e2e-risk-gate-workflow.test.ts
Collaborator
Author
|
Automated-review follow-up rationale:
|
cv
enabled auto-merge (squash)
July 10, 2026 17:49
cv
disabled auto-merge
July 10, 2026 17:49
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Replace the post-merge shadow controller's repeated child-job rendering with bounded status polling. The wait step now logs only child-run state transitions while preserving failure propagation, evidence collection, and the 105-minute hard bound. ## Changes - Poll only the correlated child's `status` and `conclusion` instead of repeatedly rendering its full job matrix. - Emit concise transition, query-failure, terminal-failure, unsupported-state, and timeout diagnostics. - Add executable workflow tests for duplicate-state suppression, success, terminal failure, GitHub query failure, and timeout handling. ## Type of Change - [x] 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 <!-- 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: this changes only internal post-merge GitHub Actions polling and diagnostics; no user command, configuration, API, policy, or runtime behavior changes - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent shell and trust-boundary review confirmed positive numeric run-ID validation, fixed GitHub status fields, controlled annotation values, timeout/cancellation separation, and preserved always-run evidence/finalization behavior - [ ] 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: `npx vitest run --project integration test/post-merge-e2e-risk-gate-workflow.test.ts` — 11 tests passed; the exact embedded wait script also returned success for live run `29110351531` and a concise failure annotation for live run `29110867027` - [ ] 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) - [ ] 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> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved automated end-to-end validation monitoring, with deterministic polling of correlated run status/conclusion. * Added robust handling for invalid run identifiers, query failures, non-success completions, and explicit timeout reporting. * **Tests** * Expanded end-to-end risk gate workflow tests with a local harness that simulates CLI behavior across success, failure, query-failure, and timeout scenarios. * Added assertions to ensure the wait logic avoids unintended commands and handles timeout exit codes correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
wscurran
added
area: ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the post-merge shadow controller's repeated child-job rendering with bounded status polling. The wait step now logs only child-run state transitions while preserving failure propagation, evidence collection, and the 105-minute hard bound.
Changes
statusandconclusioninstead of repeatedly rendering its full job matrix.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/post-merge-e2e-risk-gate-workflow.test.ts— 11 tests passed; the exact embedded wait script also returned success for live run29110351531and a concise failure annotation for live run29110867027npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests