feat(ci): add onboard performance budget signal - #5686
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an advisory onboard performance budget ( ChangesOnboard Performance Budget
E2E Advisor Deterministic cloud-onboard-e2e Rule
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
amata-human
added
area: ci
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/scorecard/analyze-trace-timing.ts`:
- Around line 327-330: The phaseWarnings filter sorts by deltaMs (signed delta)
while formatTopPhaseChanges at line 276 sorts by deltaAbsMs (absolute delta
magnitude). This inconsistency means phaseWarnings prioritizes increases over
decreases, whereas formatTopPhaseChanges ranks by magnitude of change. Resolve
this by either: updating the sort comparator in the phaseWarnings filter to use
deltaAbsMs instead of deltaMs to match the behavior of formatTopPhaseChanges, or
adding a clear comment explaining why phaseWarnings intentionally uses signed
delta to prioritize regressions over improvements.
In `@tools/e2e-advisor/analyze.mts`:
- Around line 359-363: The deduplication check in the
applyDeterministicRecommendations function only compares by test.id, but since
AdvisorTest.id is optional, this misses cases where the cloud-onboard-e2e test
is already required via workflow and job combination. Update the condition that
checks `result.requiredTests.some((test) => test.id ===
CLOUD_ONBOARD_E2E_RECOMMENDATION.id)` to also verify the workflow and job
properties match CLOUD_ONBOARD_E2E_RECOMMENDATION in addition to the id check,
ensuring proper deduplication by both id and workflow+job combination.
🪄 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: 7af3df22-9c87-4531-9251-9bbe129477b8
📒 Files selected for processing (8)
.github/workflows/nightly-e2e.yamlci/onboard-performance-budget.jsonscripts/scorecard/analyze-trace-timing.tstest/e2e-advisor-dispatch.test.tstest/e2e-script-workflow.test.tstest/e2e/README.mdtools/e2e-advisor/README.mdtools/e2e-advisor/analyze.mts
…i-regression-signal
…i-regression-signal
…i-regression-signal
…i-regression-signal
…i-regression-signal
…i-regression-signal
…i-regression-signal
…i-regression-signal
…erformance-budget-and-ci-regression-signal
|
🌿 Preview your docs: https://nvidia-preview-pr-5686.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Angel Mata <amata@nvidia.com>
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
…i-regression-signal
…i-regression-signal
…i-regression-signal
Resolve the release-base conflicts without changing the reviewed feature tree. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Parse production-shaped multi-entry trace artifacts and reject ambiguous summaries. Emit sanitized warnings, pass core to the analyzer, and lock phase names behaviorally. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
the
area: performance
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed exact head 38ea887 after the signed main merge and hardening commit. The final tree exactly matches the independently reviewed replay; the real 62-entry artifact parses byte-for-byte, focused validation passes 226/226, and build/typecheck/config/security gates are clean. No blocking code findings. Approval remains contingent on required exact-head CI, advisors, and the full default E2E run completing.
Bring in the validated Hermes base-image resolution fix from #6254 before rerunning E2E. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 28691778854
|
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Refreshed approval for exact head ed6eb53 after merging current main/#6254. The new head is signed, the PR delta remains independently reviewed, Hermes base-resolution tests now pass, focused validation is 258/258, and the production 62-entry artifact still parses byte-for-byte. Approval remains contingent on fresh exact-head CI/advisors/full E2E.
E2E Target Results — ❌ Some jobs failedRun: 28692152602
|
E2E Target Results — ✅ All default jobs passedRun: 28692152602
|
<!-- markdownlint-disable MD041 --> ## Summary This PR adds a data-backed advisory performance budget for warm-system cloud onboarding and surfaces regressions in the unified E2E scorecard without making timing itself merge-blocking. It also makes malformed or ambiguous timing evidence explicit and routes relevant changes through deterministic E2E review. ## Related Issue Fixes NVIDIA#3776 ## Changes - Add `ci/onboard-performance-budget.json` with a `390000 ms` warm-system total budget, calibrated from ten successful `main` samples using p95 plus a 25% buffer rounded to 30 seconds. - Evaluate total duration and phase diagnostics in the scorecard, emitting advisory GitHub Actions warnings instead of failing solely on timing variance. - Parse the production-shaped multi-entry GitHub artifact without extraction; require exactly one root timing summary and validate its ZIP metadata, size, compression, local header, inflated size, and CRC. - Emit only a fixed sanitized warning when timing-artifact validation fails, and pass `core` through the workflow analyzer so the warning is visible. - Wire the budget through the current `.github/workflows/e2e.yaml` scorecard path and keep E2E Advisor routing deterministic for onboarding, timing, scorecard, workflow, and budget changes. - Add schema/runtime validation, an emitter/sanitizer/scorecard phase contract, focused scorecard/workflow/advisor tests, and E2E maintainer documentation. - Preserve `ci/onboard-performance-budget.json` and `test/e2e/README.md` as the durable record for the budget and advisory policy; closed parent issue NVIDIA#2001 remains historical context. - Merge current `main` with signed commits, including NVIDIA#6254's validated Hermes base-image resolution fix, and apply a signed hardening commit on top. The protected contributor branch forbids history rewrites, so its historical commits remain intact. ### Calibration rationale The initial cap is intentionally based on the ten durable successful `main` samples available on 2026-06-23; the checked-in `$comment` records every sample, the p95 interpolation, the 25% buffer, and why the original NVIDIA#3769 traces were unavailable. That is a limited baseline, so this PR keeps the signal advisory: distribution drift can warn maintainers but cannot block a merge. We accept that bounded calibration risk to establish a measured signal now and will rebaseline from a wider durable sample set as it accumulates rather than delay the non-blocking instrumentation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: focused tests were added or updated instead - [ ] Tests not applicable — justification: tests are applicable and included - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: E2E maintainer documentation changed - [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: [exact-head maintainer approval](NVIDIA#5686 (review)) on `ed6eb53ccc3ebef49c7ec31798fa29150985528a` - [x] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: [full default E2E](NVIDIA#5686 (comment)) passed all 69 default jobs; the five skips are the workflow's documented explicit-only jobs (`openshell-gateway-auth-contract`, `mcp-bridge-dev`, `hermes-gpu-startup`, `sandbox-rlimits-connect`, and `jetson-nvmap-gpu`) ## Verification - [ ] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub — new commits `e09e88445`, `38ea88760`, and `ed6eb53cc` are Verified; the protected branch retains eight historical unverified commits that cannot be rewritten - [ ] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes — all applicable hooks pass; the local monolithic CLI coverage hook was skipped after unrelated tests timed out under concurrent full-suite load, while the exact-head GitHub aggregate and all five CLI shards passed - [x] Targeted tests pass for changed behavior — 258 focused tests, all three TypeScript checks, config validation, source-shape budget, CLI build, and production-artifact parsing pass on exact final tree `d79bc43cc45b6665dd93b7b5c11c16eb3efe8b66` - [x] Full default E2E recommendation satisfied — [attempt 2](https://github.com/NVIDIA/NemoClaw/actions/runs/28692152602/attempts/2) passed all 69 default jobs on the exact head with only five documented explicit-only skips; `cloud-onboard`, all three MCP agent scenarios, scorecard, and PR reporting passed, and the scorecard emitted zero annotations. The fresh cloud trace was `156944 ms` against the advisory `390000 ms` cap. - [ ] Full `npm test` passes (broad runtime changes only) — not run as one local monolith; the exact-head GitHub aggregate, CLI shards 1-5, plugin tests, and full default E2E all passed - [x] Quality Gates section completed with required justifications or waivers — [required CI](https://github.com/NVIDIA/NemoClaw/actions/runs/28692118362/job/85095763350), [GPT advisor](NVIDIA#5686 (comment)), documented Nemotron calibration rationale, exact-head approval, and full default E2E are complete - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — not applicable; this is not doc-only - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — not applicable; no doc page changed - [ ] New doc pages include SPDX header and frontmatter (new pages only) — not applicable; no new doc page --- Signed-off-by: Angel Mata <amata@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Angel Mata <amata@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
5 participants
Summary
This PR adds a data-backed advisory performance budget for warm-system cloud onboarding and surfaces regressions in the unified E2E scorecard without making timing itself merge-blocking. It also makes malformed or ambiguous timing evidence explicit and routes relevant changes through deterministic E2E review.
Related Issue
Fixes #3776
Changes
ci/onboard-performance-budget.jsonwith a390000 mswarm-system total budget, calibrated from ten successfulmainsamples using p95 plus a 25% buffer rounded to 30 seconds.corethrough the workflow analyzer so the warning is visible..github/workflows/e2e.yamlscorecard path and keep E2E Advisor routing deterministic for onboarding, timing, scorecard, workflow, and budget changes.ci/onboard-performance-budget.jsonandtest/e2e/README.mdas the durable record for the budget and advisory policy; closed parent issue perf: investigate and reduce networking latency during onboard and validation #2001 remains historical context.mainwith signed commits, including perf(cli): reuse validated sandbox base images #6254's validated Hermes base-image resolution fix, and apply a signed hardening commit on top. The protected contributor branch forbids history rewrites, so its historical commits remain intact.Calibration rationale
The initial cap is intentionally based on the ten durable successful
mainsamples available on 2026-06-23; the checked-in$commentrecords every sample, the p95 interpolation, the 25% buffer, and why the original #3769 traces were unavailable. That is a limited baseline, so this PR keeps the signal advisory: distribution drift can warn maintainers but cannot block a merge. We accept that bounded calibration risk to establish a measured signal now and will rebaseline from a wider durable sample set as it accumulates rather than delay the non-blocking instrumentation.Type of Change
Quality Gates
ed6eb53ccc3ebef49c7ec31798fa29150985528aopenshell-gateway-auth-contract,mcp-bridge-dev,hermes-gpu-startup,sandbox-rlimits-connect, andjetson-nvmap-gpu)Verification
Verifiedin GitHub — new commitse09e88445,38ea88760, anded6eb53ccare Verified; the protected branch retains eight historical unverified commits that cannot be rewrittennpx prek run --from-ref main --to-ref HEADpasses — all applicable hooks pass; the local monolithic CLI coverage hook was skipped after unrelated tests timed out under concurrent full-suite load, while the exact-head GitHub aggregate and all five CLI shards passedd79bc43cc45b6665dd93b7b5c11c16eb3efe8b66cloud-onboard, all three MCP agent scenarios, scorecard, and PR reporting passed, and the scorecard emitted zero annotations. The fresh cloud trace was156944 msagainst the advisory390000 mscap.npm testpasses (broad runtime changes only) — not run as one local monolith; the exact-head GitHub aggregate, CLI shards 1-5, plugin tests, and full default E2E all passednpm run docsbuilds without warnings (doc changes only) — not applicable; this is not doc-onlySigned-off-by: Angel Mata amata@nvidia.com
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Signed-off-by: Aaron Erickson aerickson@nvidia.com