ci(e2e): enforce retained runtime budgets - #7945
Draft
cjagwani wants to merge 4 commits into
Draft
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesLive E2E runtime policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CheckRunner
participant RuntimePolicyValidator
participant RuntimePolicy
participant E2EInventory
CheckRunner->>RuntimePolicyValidator: execute runtime-policy.mts
RuntimePolicyValidator->>RuntimePolicy: load LIVE_E2E_RUNTIME_POLICY
RuntimePolicyValidator->>E2EInventory: read registry and workflow targets
E2EInventory-->>RuntimePolicyValidator: return target inventory
RuntimePolicyValidator-->>CheckRunner: report success or policy violation
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
Sensitive-path security reviewVerdict: PASS Reviewed commit
Residual operational note: the baseline and two soon-to-retire selectors are explicitly provisional, carry rationale and review conditions, and expire on 2026-08-31. That dependency is why this PR remains draft; it is not a security exception. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: Blockers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/runtime-policy.ts (1)
369-381: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist repeated
listTargets()/readFreeStandingJobsInventory()calls.
registryOwningFilesandworkflowOwningFilesare invoked once perentry(...)call, so buildingLIVE_E2E_RUNTIME_POLICY.coveragecallslistTargets()26 times andreadFreeStandingJobsInventory()69 times at module load, instead of once each. This runs on every process that imports this module (the CLI check on every PR viachecks:repository, plus any test file pulling in the policy), so redundant I/O/computation is paid repeatedly.♻️ Proposed fix: compute inventories once and reuse
+const REGISTRY_TARGETS = listTargets(); +const WORKFLOW_INVENTORY = readFreeStandingJobsInventory(); + function registryOwningFiles(id: string): string[] { - const target = listTargets().find((candidate) => candidate.id === id); + const target = REGISTRY_TARGETS.find((candidate) => candidate.id === id); if (!target?.manifestPath) return ["test/e2e/registry/definitions/baseline.ts"]; return ["test/e2e/registry/definitions/baseline.ts", target.manifestPath]; } function workflowOwningFiles(id: string): string[] { if (id === "staging-brev-launchable") return ["tools/e2e/brev-launchable-e2e.sh"]; - const inventory = readFreeStandingJobsInventory(); - return [...inventory.liveTestToJobs] + return [...WORKFLOW_INVENTORY.liveTestToJobs] .filter(([, targetIds]) => targetIds.includes(id)) .map(([file]) => file); }Also applies to: 439-446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/runtime-policy.ts` around lines 369 - 381, Hoist the results of listTargets() and readFreeStandingJobsInventory() to shared module-level values before registryOwningFiles and workflowOwningFiles are used, then have both functions reuse those values instead of recomputing inventories per entry. Preserve the existing target lookup, baseline manifest handling, staging-brev-launchable special case, and workflow filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/support/e2e-runtime-policy.test.ts`:
- Line 28: Update the top-level describe title in the live E2E runtime policy
suite to use a behavior-oriented title ending with the local issue reference
suffix "(`#7922`)".
In `@tools/e2e/runtime-policy.mts`:
- Around line 216-222: Update the policy validation around validateEntry to
enforce uniqueness of non-empty uniqueBoundary values across policy.coverage,
reusing the existing duplicates() helper as with owningFiles/string lists. Add a
validation error for duplicate boundary text while preserving the current format
checks and allowing missing boundaries to be handled by their existing
validation.
---
Nitpick comments:
In `@test/e2e/runtime-policy.ts`:
- Around line 369-381: Hoist the results of listTargets() and
readFreeStandingJobsInventory() to shared module-level values before
registryOwningFiles and workflowOwningFiles are used, then have both functions
reuse those values instead of recomputing inventories per entry. Preserve the
existing target lookup, baseline manifest handling, staging-brev-launchable
special case, and workflow filtering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b7ecf59d-12ef-4fa9-b417-9d8a57b073db
📒 Files selected for processing (8)
scripts/checks/run.mtstest/checks-runner.test.tstest/e2e/docs/README.mdtest/e2e/runtime-policy.tstest/e2e/support/e2e-runtime-policy.test.tstest/helpers/vitest-watch-triggers.tstest/vitest-watch-triggers.test.tstools/e2e/runtime-policy.mts
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit ea32b37 in the TypeScript / code-coverage/cliThe overall coverage in commit ea32b37 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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
Adds a machine-checkable runtime policy for every retained live E2E target and workflow unit. The validator separates policy failures from live-target failures and constrains the initial PR/nightly set to 15 targets and 121 planned runner-minutes while the measured baseline is refreshed after #7665 and #7915.
Related Issue
Fixes #7922
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedtest/e2e/docs/README.mdDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run checks:repositorypassed; the focused policy does not change live executionnpm run docsbuilds without warnings (doc changes only) — build passed with two non-blocking Fern warningsSigned-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests