Repository navigation
ci(e2e): refresh OpenClaw budget after gateway cleanup - #12840
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe PR ChangesAssertion growth exception
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This updates a CI assertion-budget exception and does not change runtime product behavior. Merging it poses no actionable risk. The exception still has to be adopted on main before it can be used. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 71535b2 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
rsliter
left a comment
rsliter
left a comment
There was a problem hiding this comment.
Reviewed commit 3a31a7e against base c72fb58. No code-level blocking finding. The refreshed SHA-256 values exactly match the base budget and PR #12382 candidate budget. The canonical consumer accepts only PR #12382 with those exact base and head bytes, while wrong-PR, changed-base, changed-head, and candidate-only-policy cases all reject the exception. The focused guardrail suite passed 58/58, the PR #12382 assertion census passed, CodeRabbit found no actionable comments, the exact-head self-hosted qualification run passed, and all nine security categories pass for this policy-only change. I am holding approval because the trusted gate still reports 14 failed core-CI checks, and main has advanced to 5f765a1. The representative shard failed during APT mirrorlist setup before tests, so the failure is not attributable to this JSON diff. Please refresh the branch and rerun required CI; approval can follow once the exact-head gate passes.
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Review of commit 6b5b867b3cffc7dfff908d24514f42c0f83d4522 against base 59c3cef9c7233bd6457098b831d82d4c2105daef.
Product scope: blocked. Review verdict: CHANGES_REQUESTED.
Blocking decision — ci/e2e-assertion-growth-exceptions.json:27-28: The replacement SHA-256 pair would let the independent growth check accept PR #12382's new budget transition after this policy reaches main. The recorded approval for prerequisite PR #12718 covered the prior pair (fdc4a78b… to f6b26905…). PR #12840 says that no prior approval for the replacement pair is claimed. The current PR discussion contains no attributable maintainer acceptance of 53c51c42… to 81624083…. Please record a maintainer decision that accepts these exact digests and the resulting one additional unique assertion point and generated probe condition, or remove the refreshed exception. I can re-review the same commit after that decision; a code change is not required if the decision accepts this pair. The current Advisor's customer-value specialist reports the same P1 blocker.
Security review: PASS in all nine categories. (1) Secrets and credentials: no credential data or flow changes. (2) Input validation: the existing parser validates positive PR numbers and lowercase SHA-256 digests. (3) Authentication and authorization: independent enforcement reads trusted-base policy and the event PR number. (4) Dependencies: none change. (5) Error handling and logging: malformed policy throws, and unmatched growth remains a violation. (6) Cryptography and data protection: SHA-256 binds exact budget bytes; no protected data flow changes. (7) Configuration and security headers: only this bounded CI policy entry changes. (8) Security testing: wrong PR, changed bytes, candidate-only policy, and malformed policy are covered. (9) System security: PR code cannot authorize its own independent growth check. No security finding remains.
Validation and feedback: The one-file diff preserves every other exception. Both replacement digests match the actual base budget and PR #12382 candidate budget. The focused parser suite passed 58/58. The canonical consumer accepted the exact transition and rejected a different PR, changed base bytes, changed candidate bytes, and candidate-only policy. CodeRabbit found no actionable comment on this commit. All nine current Advisor artifacts were read; eight are clear and one reports the scope blocker. The cross-issue scan found no candidate issue. All three commits are GitHub Verified, and the PR body carries a DCO sign-off. Collection reached terminal pages: five issue comments, one submitted review, zero inline comments, zero review threads, three commits, and 76 check runs.
Required CI: The five live ruleset contexts have acceptable results on this commit: checks, commit-lint, dco-check, and check-hash passed; changes passed in the core run and was also skipped in other runs. The Advisor blocker gate failed separately. GitHub reports the PR as mergeable but BLOCKED pending review; auto-merge is off.
|
Maintainer decision: Accept. I accept PR #12840's exact refreshed assertion-budget transition for PR #12382, including the resulting one additional unique assertion point and one generated probe condition:
This decision applies to the pair in commit This is acceptance of the replacement transition identified in Senthil's review. Independent enforcement remains unchanged: the exception must be adopted on main before PR #12382 can use it, a different PR or either different digest remains rejected, and the entry must be removed after #12382 merges. This decision does not waive required checks or authorize merging either PR. Recorded at my direction by Codex. — Prekshi Vyas (@prekshivyas), repository maintainer |
Link the maintainer decision for the exact replacement budget digests. Preserve every exception value and independent enforcement requirement. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Approved for commit 6b5b867b3cffc7dfff908d24514f42c0f83d4522.
The replacement digests match the current PR #12382 base and candidate budgets. The policy still accepts only PR #12382 with this exact pair and rejects changed budget bytes. The transition retains the concurrent-gateway assertion reduction, and the focused policy tests pass.
The maintainer accepted this exact transition, including the one additional unique assertion point and generated probe condition. That resolves my earlier change request. The latest PR Review Advisor finding says the decision is missing from its supplied context; the live GitHub comment records it. I found no remaining blocking issue in this PR.
## Outcome Pinned Ubuntu tools install when `RUNNER_TEMP` has private parent directories. APT package lists use a unique directory under `/tmp`, which its `_apt` sandbox can traverse without changing runner-directory permissions. ## Reason Changing only `RUNNER_TEMP` permissions cannot grant access through a private ancestor. A disposable Ubuntu AMD64 container reproduced the resulting mirror-list failure with the original helper. This change retains the Ubuntu mirrorlist resolution merged in #12837. That merge addresses the shared CI installation failure; this PR preserves directory access and private runner permissions alongside it. ### Related issues Refs #12382, #12840, #12778, #12837. ## Changes - Create the package-list directory under `/tmp` and retain its permissions, ownership, and cleanup. - Remove changes to `RUNNER_TEMP` permissions. Logs and the isolated Ubuntu source remain in the runner directory. - Make the workflow test fixture remove the temporary package-list directory. Assert that APT uses `/tmp`, runner permissions are not widened, and the directory is removed. Package pins, source restrictions, timeouts, retries, and failure propagation remain unchanged from main. ## Verification - `vitest run --project integration test/automation/pull-requests/pr-workflow-contract.test.ts`: all 69 tests passed after integrating #12837. - Real Ubuntu 24.04 AMD64 installation with the combined helper passed under a private parent directory. Installed `fd-find 9.0.0-1` and `ripgrep 14.1.0-1`. - Verified private directories retain mode `0700`, the system Ubuntu source is unchanged, temporary files are removed, and the disposable container is removed. - Regression mutations: moving lists back under `RUNNER_TEMP`, adding privileged permission widening, and omitting cleanup each fail the strengthened test. The complete 69-test workflow suite passes with the correct helper. - Earlier reproduction: the original helper exited 100; the directory repair passed under the same private ancestor. - Signed commit and normal publication hooks passed, including CLI type checking. - Reviewed the complete two-file diff. No secrets, API keys, or credentials. ## Review notes Self-review of commit `29e5dc95280829f591a38f42f8f707c7e0b41d30` covered the complete NVIDIA/NemoClaw diff against main `5f765a1318a2338114a1f6865de3113d9083b405`, including the sensitive `.github/actions/ci-install-pinned-ubuntu-packages.sh` path. The combined helper preserves #12837's source restrictions and passed the real Linux installation and cleanup checks. Advisor identified a regression-test gap; this commit repairs it without changing the installer. All current checks pass, including [core CI](https://github.com/NVIDIA/NemoClaw/actions/runs/37815724516), [image qualification](https://github.com/NVIDIA/NemoClaw/actions/runs/37815730289), and [all nine Advisor reviews](https://github.com/NVIDIA/NemoClaw/actions/runs/37820691834). The first CI attempt hit an inherited five-second vLLM unit-test timeout; one maintainer-approved failed-job rerun passed on the unchanged commit. All nine Advisor reports are clear and recommend no additional live E2E for this CI-helper change. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated CI package setup to store temporary Ubuntu package data under `/tmp` and clean it up, including when setup fails. The process no longer changes permissions on the runner’s temporary directory. * **Tests** * Added checks for temporary package-data cleanup after setup failures and verified that required paths and permissions are accessible during installation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
wscurran
added
area: ci
4 participants
Outcome
Refresh the exception for #12382 so its reviewed runtime assertions can pass the independent growth check after #12608 reduced main's gateway assertions.
Reason
#12608 merged before #12718 and changed the base budget bytes. The exception adopted by #12718 therefore matches neither the new base budget nor #12382's prepared integration.
Related issues
Refs #12382, #12718, #12608.
Changes
Replace only the two budget digests for #12382 and add a scoped explanation. Preserve every other exception and all enforcement code.
53c51c4229d387633165771730f8f1a8373f84e9f3303b2144280027a56e34418162408384d7a6b97ef0062042c73d4bc9486f54d2dfa6c0c5732e42d1060e2aThe runtime transition retains native admin approval before survival/rebuild mutations and the plugin-install success check. It adds one unique assertion point and one generated probe condition over the reduced main budget. It adds no live scenario or matrix lane.
Verification
e2eAssertionBudgetGrowthViolationsconsumer: accepts the proposed trusted-base transition; rejects a different PR, changed base bytes, changed candidate bytes, and candidate-only policy.Review notes
The branch incorporates #12837, which repairs the inherited Ubuntu mirrorlist installation failure. The branch also incorporates #12833 and preserves its #12767 exception. The #12382 budget digests are unchanged. The policy comment now cites the maintainer acceptance recorded below; no exception value or enforcement code changed. Signed commit
71535b2passed normal commit and publication hooks. Fresh CI and Advisor checks for this comment-only update are pending. Senthil approved the PR, confirming that the recorded acceptance resolves his earlier change request.Maintainer Prekshi Vyas (@prekshivyas) accepted the exact replacement budget pair, including one additional unique assertion point and one generated probe condition. Independent enforcement is not waived: adoption on main remains required before #12382 can use this exception. The approval records the exact replacement pair and its bounded assertion increase.
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit