fix(ci): omit null conclusion from running checks#7065
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR E2E gate no longer sends ChangesE2E check-run update
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 80%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
@jyaunches @ericksoa — urgent v0.0.85 prerequisite review requested for exact commit Everything except the controller's self-bootstrap check is green: full ordinary CI, all self-hosted tests, CodeQL, macOS/WSL, DCO, Verified signature, Advisor 0/0/0, CodeRabbit clean, no review threads, and an independent nine-category security review with no findings. The only failures are Please independently review the exact diff. If it is acceptable, approve and use the documented manual merge exception for those two self-referential checks. After this lands, #7055 will be refreshed on repaired |
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head a76af0b against current base 94b5e46. No blocking findings. The patch correctly omits the final-only conclusion field from in-progress PATCH requests while preserving strict validation that GitHub persists the response as conclusion === null; identity, authorization, TOCTOU, and fail-closed behavior remain unchanged. The focused controller suite passes 149/149, automated advisors are clean, and all ordinary CI is green. I accept the two non-success E2E checks as a narrow self-bootstrap exception for this exact revision: the trusted evaluator checked out the buggy base controller, received the reproduced 422 before any child dispatch, and the observer failure is derivative. Approved.
Summary
Restore the trusted PR E2E controller after #7054 by omitting the invalid
conclusion: nullfield when updating an active GitHub check. GitHub currently rejects that request with HTTP 422 before control-plane authorization or E2E dispatch can occur.Changes
conclusionfrom in-progress check-run PATCH requests while retaining strict validation that GitHub returnsconclusion === nullfor the persisted active check.The escaped defect was caused by treating GitHub's response representation as a valid request representation. Existing mocks spread the request into a successful response,
toMatchObjectallowed the extra request key, and one test explicitly required the invalid null field. Live controller run https://github.com/NVIDIA/NemoClaw/actions/runs/29549179465 reproduced the API rejection after all ordinary CI passed.This prerequisite cannot satisfy its own required
E2E / PR Gate Coordinationcheck: trusted workflow-run evaluation checks out the currentmaincontroller, which contains the defect being repaired. After ordinary CI and automated review pass, an independent maintainer must review this exact commit and explicitly accept that one check as a manual merge exception. Subsequent release PRs will then refresh on the repaired base and produce normal exact-head E2E evidence.Type of Change
Quality Gates
a76af0b9c2d15486c0f5b6f869a802e3bb9ed631returned PASS/GO with no findings; outbound null removal does not alter strict inbound identity, authorization, TOCTOU, or fail-closed validation.E2E / PR Gate Coordinationself-bootstrap failure described above. Do not merge before ordinary CI and automated review are otherwise green.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run test/pr-e2e-gate*.test.ts test/pr-e2e-required.test.tspassed 149/149 tests across 9 files; the focused regression failed before the fix with receivedconclusion: nulland passed afterward. Source-shape, test-size, Biome, CLI build, and CLI typecheck also passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not run; the focused controller suite and normal hooks cover this three-file request-shape fix.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests