fix(cicd): make the Core gate cover every Core job - #4655
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:
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)
Summary by CodeRabbit
WalkthroughThe change adds a Core CI gate checker, tests workflow inventory and result policies, expands ChangesCore CI gate
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChangesJob
participant GateChecker
participant CoreCIPass
participant ProtoBreakingChanges
ChangesJob->>GateChecker: Validate workflow inventory
GateChecker-->>ChangesJob: Return inventory status
ChangesJob->>CoreCIPass: Set run_core_ci
CoreCIPass->>GateChecker: Evaluate NEEDS_JSON results
GateChecker-->>CoreCIPass: Return gate status
CoreCIPass->>ProtoBreakingChanges: Provide gated dependency result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/test_check_core_ci_gate.py (1)
136-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a passing-path assertion for
_check_resultsand a malformed-entry case forresult_errors.Every
_check_resultscase asserts return code 1. No test asserts that a healthy context returns 0. A regression that makes the gate always fail would still pass this suite, and the failure would surface only as a red required check on every pull request.
result_errorsalso has an untested branch. Line 208 ofscripts/check_core_ci_gate.pyreportsdid not provide a job result objectfor a non-mapping entry. No case exercises it.Both gaps are one assertion each.
💚 Proposed tests for the uncovered branches
def test_gate_result_policy(self) -> None: cases = ( { "name": "success", "result": "success", "expected": [], }, @@ for case in cases: with self.subTest(case["name"]): needs_context = {"fixture-job": {"result": case["result"]}} self.assertEqual(result_errors(needs_context), case["expected"]) + def test_non_mapping_entry_is_rejected(self) -> None: + self.assertEqual( + result_errors({"fixture-job": "success"}), + ["`fixture-job` did not provide a job result object"], + ) + + def test_result_command_accepts_healthy_context(self) -> None: + needs_json = '{"gated-job":{"result":"success"},"skipped-job":{"result":"skipped"}}' + output = io.StringIO() + with mock.patch.dict(os.environ, {"NEEDS_JSON": needs_json}): + with contextlib.redirect_stdout(output): + return_code = _check_results() + + self.assertEqual(return_code, 0) + self.assertNotIn("::error::", output.getvalue()) + def test_result_command_rejects_invalid_context(self) -> None:🤖 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 `@scripts/test_check_core_ci_gate.py` around lines 136 - 194, Add a healthy-context case to test_result_command_rejects_invalid_context or a dedicated _check_results test, asserting _check_results returns 0 for valid successful NEEDS_JSON. Add a malformed non-mapping entry case to test_gate_result_policy, such as a job value that is not an object, and assert result_errors returns the expected “did not provide a job result object” message.scripts/check_core_ci_gate.py (2)
235-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse the workflow once in
_check_inventory.
inventory_errorsalready parses the workflow at line 151. Line 240 parses the same text again only to count jobs. This duplicates work and leaves aWorkflowFormatErrorraise outside any handler. The second call cannot currently fail, becauseinventory_errorsreturns early for the same input, but the coupling is implicit and fragile.Expose the parsed inventory from a single call instead.
♻️ Proposed refactor to parse once
- errors = inventory_errors(workflow_text) - if errors: - _print_annotations(errors) - return 1 - - inventory = parse_workflow(workflow_text) + try: + inventory = parse_workflow(workflow_text) + except WorkflowFormatError as error: + _print_annotations([str(error)]) + return 1 + + errors = inventory_errors(workflow_text) + if errors: + _print_annotations(errors) + return 1 + print(🤖 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 `@scripts/check_core_ci_gate.py` around lines 235 - 244, Update _check_inventory to obtain the parsed inventory from the single workflow-parsing operation used by inventory_errors, then reuse that inventory for validation and job-count reporting instead of calling parse_workflow again. Preserve the existing error annotations and return behavior while ensuring any WorkflowFormatError remains handled through the established path.
160-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCount gate dependencies once instead of per unique job.
duplicate_needscallstuple.countfor every unique dependency. That is a quadratic scan overgate_needs. The list is small today, so this is a clarity improvement more than a performance fix.collections.Counterstates the intent directly.♻️ Proposed refactor using
collections.Counter+from collections import Counter- duplicate_needs = sorted( - job - for job in gated_jobs - if inventory.gate_needs.count(job) > 1 - ) + duplicate_needs = sorted( + job for job, count in Counter(inventory.gate_needs).items() if count > 1 + )🤖 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 `@scripts/check_core_ci_gate.py` around lines 160 - 164, Update the duplicate dependency detection around duplicate_needs to build a collections.Counter from inventory.gate_needs once, then identify gated_jobs whose counted dependency total exceeds one. Preserve the sorted duplicate_needs result and add the required Counter import.
🤖 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 @.github/workflows/ci.yaml:
- Around line 2222-2231: Add a job-level permissions block to the job containing
the “Decide pass/fail” step, granting only contents: read. Keep the existing
checkout and check_core_ci_gate.py execution unchanged.
---
Nitpick comments:
In `@scripts/check_core_ci_gate.py`:
- Around line 235-244: Update _check_inventory to obtain the parsed inventory
from the single workflow-parsing operation used by inventory_errors, then reuse
that inventory for validation and job-count reporting instead of calling
parse_workflow again. Preserve the existing error annotations and return
behavior while ensuring any WorkflowFormatError remains handled through the
established path.
- Around line 160-164: Update the duplicate dependency detection around
duplicate_needs to build a collections.Counter from inventory.gate_needs once,
then identify gated_jobs whose counted dependency total exceeds one. Preserve
the sorted duplicate_needs result and add the required Counter import.
In `@scripts/test_check_core_ci_gate.py`:
- Around line 136-194: Add a healthy-context case to
test_result_command_rejects_invalid_context or a dedicated _check_results test,
asserting _check_results returns 0 for valid successful NEEDS_JSON. Add a
malformed non-mapping entry case to test_gate_result_policy, such as a job value
that is not an object, and assert result_errors returns the expected “did not
provide a job result object” message.
🪄 Autofix
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: 9642482f-295b-47ad-aabe-f84cfea7904f
📒 Files selected for processing (3)
.github/workflows/ci.yamlscripts/check_core_ci_gate.pyscripts/test_check_core_ci_gate.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_check_core_ci_gate.py (1)
188-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the missing and malformed
NEEDS_JSONpaths.
_check_results()has separate failure branches whenNEEDS_JSONis unset or contains invalid JSON. The test cases do not execute either branch. Add cases for an unset variable withmock.patch.dict(os.environ, {}, clear=True)and malformed JSON. This keeps the CI gate error contract covered.🤖 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 `@scripts/test_check_core_ci_gate.py` around lines 188 - 207, The test_result_command_rejects_invalid_context cases do not cover unset or malformed NEEDS_JSON. Add an unset-variable case using mock.patch.dict(os.environ, {}, clear=True) and a malformed-JSON case, asserting each branch’s expected CI gate error output while preserving the existing invalid-context cases.
🤖 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 `@scripts/test_check_core_ci_gate.py`:
- Around line 188-207: The test_result_command_rejects_invalid_context cases do
not cover unset or malformed NEEDS_JSON. Add an unset-variable case using
mock.patch.dict(os.environ, {}, clear=True) and a malformed-JSON case, asserting
each branch’s expected CI gate error output while preserving the existing
invalid-context cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b4648b70-3b0c-41dc-b7b0-5e67c2260019
📒 Files selected for processing (3)
.github/workflows/ci.yamlscripts/check_core_ci_gate.pyscripts/test_check_core_ci_gate.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yaml
- scripts/check_core_ci_gate.py
|
@coderabbitai full_review, thanks! |
|
🐇🔎
|
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
chet
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
nv-dmendoza
left a comment
nv-dmendoza
left a comment
There was a problem hiding this comment.
LGTM, aligns the jobs needed for CI passing, some of which were probably not added to this list as time went on
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/ci/test_check_core_ci_gate.py (1)
296-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
inventorycommand.
_check_resultsis covered end to end, but_check_inventoryis not tested. That function owns the exit codes and annotations the workflow relies on, including the unreadable-file branch and the success summary. Add a short test with a temporary workflow file.💚 Proposed tests for `_check_inventory`
import tempfile from pathlib import Path from check_core_ci_gate import _check_inventory class InventoryCommandTests(unittest.TestCase): """Verify the exit codes and annotations of the `inventory` command.""" def _run(self, workflow_text: str) -> tuple[int, str]: output = io.StringIO() with tempfile.TemporaryDirectory() as directory: path = Path(directory, "ci.yaml") path.write_text(workflow_text, encoding="utf-8") with contextlib.redirect_stdout(output): return_code = _check_inventory(path) return return_code, output.getvalue() def test_command_accepts_complete_inventory(self) -> None: return_code, output = self._run(COMPLETE_WORKFLOW) self.assertEqual(return_code, 0) self.assertNotIn("::error::", output) def test_command_rejects_ungated_job(self) -> None: workflow = COMPLETE_WORKFLOW.replace(" - build\n", "") return_code, output = self._run(workflow) self.assertEqual(return_code, 1) self.assertIn("::error::top-level jobs are not gated or exempt: build", output) def test_command_rejects_unreadable_workflow(self) -> None: output = io.StringIO() with contextlib.redirect_stdout(output): return_code = _check_inventory(Path("does-not-exist.yaml")) self.assertEqual(return_code, 1) self.assertIn("::error::could not read", output.getvalue())Note:
test_command_accepts_complete_inventorypasses only when the fixture's exempt jobs match the productionEXEMPT_JOBS, because_check_inventoryuses the module constant. The current fixture satisfies that.🤖 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 @.github/ci/test_check_core_ci_gate.py around lines 296 - 303, Add focused tests for _check_inventory using a temporary workflow file: cover a complete inventory returning 0 without errors, an ungated job returning 1 with the expected annotation, and an unreadable path returning 1 with a “could not read” annotation. Reuse the existing fixture and production EXEMPT_JOBS expectations, and place the tests alongside the current command tests.
🤖 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 @.github/ci/test_check_core_ci_gate.py:
- Around line 296-303: Add focused tests for _check_inventory using a temporary
workflow file: cover a complete inventory returning 0 without errors, an ungated
job returning 1 with the expected annotation, and an unreadable path returning 1
with a “could not read” annotation. Reuse the existing fixture and production
EXEMPT_JOBS expectations, and place the tests alongside the current command
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df2dbaa5-94dc-43fa-9f68-638eda600d6d
📒 Files selected for processing (3)
.github/ci/check_core_ci_gate.py.github/ci/test_check_core_ci_gate.py.github/workflows/ci.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/ci/test_check_core_ci_gate.py (1)
112-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering
_check_inventoryend to end.The suite exercises
inventory_errorsand_check_resultsthoroughly._check_inventoryremains untested, so the annotation output, the unreadable-file branch, and the summary line have no regression guard. A single test that writes the fixture to a temporary path and asserts the return code and the summary text would close the gap.🤖 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 @.github/ci/test_check_core_ci_gate.py around lines 112 - 202, Add an end-to-end test for `_check_inventory` that writes a representative workflow fixture to a temporary path, invokes the checker, and asserts its return code and summary output. Cover annotation output and the unreadable-file branch so these behaviors have regression protection, using the existing `inventory_errors` and `_check_results` conventions where appropriate..github/ci/check_core_ci_gate.py (1)
303-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
inventory_errorsto remove the duplicate parse path.
_check_inventoryrepeats the parse-and-classify sequence thatinventory_errorsalready implements. The inventory count message needs the parsed object, so a small refactor keeps one code path: parse once, then report. The current form risks the two paths drifting apart.♻️ Optional consolidation
- try: - inventory = parse_workflow(workflow_text) - except WorkflowFormatError as error: - _print_annotations([str(error)]) - return 1 - - errors = _inventory_errors(inventory, EXEMPT_JOBS) - if errors: + try: + inventory = parse_workflow(workflow_text) + except WorkflowFormatError as error: + _print_annotations([str(error)]) + return 1 + + if errors := _inventory_errors(inventory, EXEMPT_JOBS):The public
inventory_errorshelper remains the entry point for tests. Consider making it return the parsed inventory alongside the errors so both callers share one implementation.🤖 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 @.github/ci/check_core_ci_gate.py around lines 303 - 309, Refactor _check_inventory to reuse the parsing and classification performed by inventory_errors instead of duplicating the parse path. Extend inventory_errors to return both the parsed inventory and its errors while preserving its public test-facing entry point, then use the returned inventory for the count message and errors for reporting.
🤖 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 @.github/ci/check_core_ci_gate.py:
- Around line 303-309: Refactor _check_inventory to reuse the parsing and
classification performed by inventory_errors instead of duplicating the parse
path. Extend inventory_errors to return both the parsed inventory and its errors
while preserving its public test-facing entry point, then use the returned
inventory for the count message and errors for reporting.
In @.github/ci/test_check_core_ci_gate.py:
- Around line 112-202: Add an end-to-end test for `_check_inventory` that writes
a representative workflow fixture to a temporary path, invokes the checker, and
asserts its return code and summary output. Cover annotation output and the
unreadable-file branch so these behaviors have regression protection, using the
existing `inventory_errors` and `_check_results` conventions where appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9048820f-61d7-4b7e-885b-e5c5e905f8d5
📒 Files selected for processing (3)
.github/ci/check_core_ci_gate.py.github/ci/test_check_core_ci_gate.py.github/workflows/ci.yaml
|
Looks great @chet Seems like the Python script could be generic enough to apply to REST gate as well. |
|
/ok to test 529db80 |
| @@ -0,0 +1,381 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Should we add these files in .github/workflows/scripts?
There was a problem hiding this comment.
@thossain-nv Yeah I actually put these in .github/ci/ intentionally! I think you can tell most of my PR(s) here are introducing some Python to bump us more into kind of an "advanced" mode away from pure workflows config, lol.
In this case, my thinking is .github/workflows/ is where we keep all of the declarative Actions YAML, while .github/ci/ is the common home for the code behind it.
And yeah, especially since the other CI PRs are adding helpers too, this would be where we'd build out tools/libs/etc.
Totally doesn't need to be, but that was my thinking!
|
@coderabbitai full_review, thanks! |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4655.docs.buildwithfern.com/infra-controller |
|
🐇🔎 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/ci/test_check_core_ci_gate.py (1)
260-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover malformed per-job result objects.
The cases do not test a job object with a missing or non-string
resultfield. Add cases such as{"fixture-job":{}}and{"fixture-job":{"result":null}}. Both must return a failure code.This protects the stated fail-closed policy for malformed results.
🤖 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 @.github/ci/test_check_core_ci_gate.py around lines 260 - 283, Extend test_result_command_rejects_invalid_context with cases for a per-job object missing the result field and one whose result is non-string/null. Assert each produces the expected malformed-result error and a nonzero failure code, preserving the fail-closed behavior already covered by the command tests.
🤖 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 @.github/ci/check_core_ci_gate.py:
- Around line 91-100: Update the jobs scan around the loop following jobs_blocks
to track BLOCK_SCALAR headers and skip their bodies while indentation remains at
least indentation + 1; add the requested BLOCK_SCALAR pattern alongside the
other grammar patterns. When scalar tracking ends, reject unexpected column-0
content with the existing format-error path instead of silently breaking, while
preserving normal job-key handling and avoiding errors for valid scalar lines
indented two spaces. Add a fixture in test_check_core_ci_gate.py with a column-0
heredoc body between two job declarations and assert the later job remains
classified.
---
Nitpick comments:
In @.github/ci/test_check_core_ci_gate.py:
- Around line 260-283: Extend test_result_command_rejects_invalid_context with
cases for a per-job object missing the result field and one whose result is
non-string/null. Assert each produces the expected malformed-result error and a
nonzero failure code, preserving the fail-closed behavior already covered by the
command tests.
🪄 Autofix
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: 62e68d02-0460-4f14-b715-9238062d560e
📒 Files selected for processing (3)
.github/ci/check_core_ci_gate.py.github/ci/test_check_core_ci_gate.py.github/workflows/ci.yaml
@thossain-nv Oh yeah totally! I wanted to keep this scoped to Core, but I generalize and extend to REST in a follow-up PR if we like it. |
`core-ci-pass` was only waiting on part of the workflow, which meant an unlisted build could fail while the protected check still reported green. This makes it wait on every current substantive Core job and checks that inventory before the expensive work starts. - **Expected green-run effect:** No speedup. The final gate may finish later because it now waits for every job it represents. - **What it really buys us:** `core-ci-pass` cannot report green while a known Core job is red, and a future top-level job cannot quietly sit outside the gate. Intentional skips remain accepted, while failures, cancellations, malformed results, and unknown inventory entries stop the gate. Tests added! This supports NVIDIA#4649 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
3 participants
core-ci-passwas only waiting on part of the workflow, which meant an unlisted build could fail while the protected check still reported green. This makes it wait on every current substantive Core job and checks that inventory before the expensive work starts.core-ci-passcannot report green while a known Core job is red, and a future top-level job cannot quietly sit outside the gate.Intentional skips remain accepted, while failures, cancellations, malformed results, and unknown inventory entries stop the gate. The inventory check also requires
if: always()on the gate, because otherwise a failing dependency could skip the protected check entirely.proto-breaking-changesnow uses the Core classifier as well, so REST-only PRs do not pick up a Core-only check just because the final gate became complete.Related issues
This supports #4649
Type of Change
Breaking Changes
Testing
Additional Notes
The gate intentionally does not exempt a job because it has been flaky or depends on the network -- a red Core job needs to make the protected Core result red. Reliability work for those jobs can stay separate from the question of whether the gate reports them.
core-ci-passinherits the workflow-widecontents: readtoken baseline from #4656. Merge #4656 first so the permission policy stays centralized and checked instead of being duplicated on this one job.This does not change the repository ruleset itself. It makes the existing
core-ci-passcheck accurately represent the workflow the ruleset already relies on; #4586 still owns the later classifier-aware selected/full policy and timing report.