ci: reshape PR and main CI around composable slices, prepping for Depot workers - #1244
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR replaces monolithic CI workflows with a planner-driven controller, reusable lanes and slices, validated manifests, immutable artifacts, centralized runner policies, and stable required CI summaries. It also updates CI documentation, migration policy, tests, and workflow consistency checks. ChangesCI planner and orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟠 High · up to This PR substantially rewires repository CI, but unresolved workflow trust-boundary and privileged-execution issues could allow pull-request code to run with elevated permissions, while an input-handling defect may break main-branch CI. Additional documentation, cache-isolation, and release-validation issues remain; the PR should not merge until the security and CI execution risks are fixed or explicitly accepted. Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ndizazzo
changed the title
ndizazzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (10)
ci/ci-plan.schema.json (1)
90-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd semantic validation for standalone plan consumers.
build_planrejects duplicate matrix IDs and constructs all slice maps fromrequired_slices. The schema alone still accepts duplicate matrix IDs and arbitrary keys inreasons,dependencies,runner_roles, andcache_modes. Add standalone semantic validation or document that consumers must usescripts/plan-ci.py.🤖 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 `@ci/ci-plan.schema.json` around lines 90 - 93, Extend validation around the matrix schema and related plan fields so standalone consumers reject duplicate matrix IDs and enforce the same semantic constraints as build_plan, including keys in reasons, dependencies, runner_roles, and cache_modes being limited to required_slices. Alternatively, explicitly document that consumers must invoke scripts/plan-ci.py, but preserve consistent validation behavior for all plan consumers.scripts/tests/test_plan_ci.py (1)
157-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the failure reason, not only the exception type.
Each of these tests contains two independent fail-closed contracts, and both raise the same
PlanError. If one contract stops being enforced while the other still raises, the test still passes. Bind the exception and assert on its message so each contract fails independently.assertRaisesRegexorsubTestalso makes the failing case identifiable.♻️ Suggested change
def test_unknown_paths_and_unknown_input_fields_fail_closed(self) -> None: payload = fixture("docs-only.json") payload["changed_files"] = ["new-owner/unknown.dat"] - with self.assertRaises(PLANNER.PlanError): - PLANNER.build_plan(payload, root=ROOT) + with self.assertRaisesRegex(PLANNER.PlanError, "new-owner/unknown.dat"): + PLANNER.build_plan(payload, root=ROOT) invalid = fixture("docs-only.json") invalid["unexpected"] = True - with self.assertRaises(PLANNER.PlanError): - PLANNER.build_plan(invalid, root=ROOT) + with self.assertRaisesRegex(PLANNER.PlanError, "unexpected"): + PLANNER.build_plan(invalid, root=ROOT)Apply the same pattern to
test_profile_event_contract_is_strictfor the profile mismatch and the malformedsource_sha.🤖 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/tests/test_plan_ci.py` around lines 157 - 177, Update test_unknown_paths_and_unknown_input_fields_fail_closed and test_profile_event_contract_is_strict to bind each raised PLANNER.PlanError and assert its message identifies the specific invalid condition. Use assertRaisesRegex or separate subTest cases so unknown paths, unexpected fields, profile mismatch, and malformed source_sha are independently validated.scripts/tests/test_pr_workflow_artifacts.py (1)
55-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnbounded job sectioning in two workflow contract tests. Both tests isolate a job with
workflow[workflow.index(" <job>:") :], which keeps every job declared after the target job. The shared root cause is a missing end boundary at the next top-level job header. Bound the slice, or parse the workflow YAML and index intojobs[<job>].
scripts/tests/test_pr_workflow_artifacts.py#L55-L57: end thelinux_productslice at the next job header so thecargo buildandpackage-native-runtime.shbans do not apply to the macOS and Windows runtime jobs.scripts/tests/test_pr_builds_summary.py#L15-L32: end thesummaryslice at the next job header, or assert on the parsedjobs.summary.needslist, so a- <name>item in a later job cannot satisfy the required-job assertions.🤖 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/tests/test_pr_workflow_artifacts.py` around lines 55 - 57, Bound the workflow job sections in both affected tests: in scripts/tests/test_pr_workflow_artifacts.py lines 55-57, end the linux_product slice at the next top-level job header so its prohibited-command assertions only inspect that job; in scripts/tests/test_pr_builds_summary.py lines 15-32, end the summary slice at the next job header or inspect the parsed jobs.summary.needs list so required-job assertions cannot match later jobs.scripts/tests/test_ci_workflow_artifacts.py (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse
ci/slices.ymlwithjsonand assert themainprofile.The manifest uses JSON-compatible YAML, and
scripts/plan-ci.pyuses the standard-libraryjsonparser. Avoid adding PyYAML. Readprofiles["main"]before assertingall_rowsand its budgets; the current substring checks can pass becausemanual-fullhas the same values.🤖 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/tests/test_ci_workflow_artifacts.py` around lines 19 - 24, Update test_main_plan_uses_all_rows_and_bounded_budgets to parse SLICES.read_text() with the standard-library json parser, select profiles["main"], and assert all_rows, total_max_workers, linux_max_parallel, and windows_max_parallel on that profile. Remove the substring-based manifest checks and do not add PyYAML.scripts/plan-ci.py (2)
544-548: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive
docs_onlyfrom the matched domains.The signal inspects raw paths.
docs/index.html,docs/assets/**, anddocs/catalog/**belong to thewebsitedomain inci/ownership.yml, but they start withdocs/, sodocs_onlybecomesTruewhile thewebslice is selected. A consumer that skips work ondocs_onlythen contradicts the plan.♻️ Proposed refactor
if name == "docs_only": - return bool(changed_files) and all( - path.endswith(".md") or path.startswith("docs/") - for path in changed_files - ) + return bool(changed_files) and domains == ["docs"]🤖 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/plan-ci.py` around lines 544 - 548, Update the docs_only logic in the plan classification function to derive the result from matched ownership domains rather than raw changed-file paths. Ensure files assigned to the website domain, including docs/index.html, docs/assets/**, and docs/catalog/**, do not mark docs_only true when the web slice is selected, while preserving true only for plans containing exclusively documentation changes.
583-588: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConfirm the cost of the control-plane fail-open path.
Any change under
.github/**,.omo/**, orci/**maps to theci-controldomain. This block then setsforce_all_rowsand adds every control slice, including onpr-draft. Documentation-only edits such as.github/AGENTS.mdtherefore start every runtime, platform, smoke, and SDK row. This conflicts with the stated goal of bounded pull request matrices.Consider excluding pure Markdown paths under
.github/and.omo/fromci-control, or limitingforce_all_rowsto themainandmanual-fullprofiles.🤖 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/plan-ci.py` around lines 583 - 588, Adjust the ci-control domain handling before the force_all_rows logic in scripts/plan-ci.py so pure Markdown-only changes under .github/ and .omo/ do not trigger the control-plane fail-open path, or restrict force_all_rows and control-slice expansion to main and manual-full profiles. Preserve control-plane coverage for non-documentation changes and existing profile behavior..github/workflows/ci-sdk-slice.yml (1)
37-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMatch the SDK rows by value, not by JSON substring.
contains(inputs.sdk_matrix, '"id":"rust"')depends on the exact serialization. It works only while the producer emits compact JSON without spaces. Use the parsed array instead.♻️ Proposed refactor
- if: ${{ contains(inputs.sdk_matrix, '"id":"rust"') }} + if: ${{ contains(fromJSON(inputs.sdk_matrix).*.id, 'rust') }}Apply the same change for
kotlinat Lines 50 and 65, and forswiftat Lines 82 and 93.Also applies to: 50-50, 65-65, 82-82, 93-93
🤖 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/workflows/ci-sdk-slice.yml at line 37, Replace the JSON substring checks in the workflow conditions for rust, kotlin, and swift with value-based matching against the parsed sdk_matrix array. Update all affected conditions, including the duplicate kotlin and swift checks, so they select rows by each row’s id regardless of JSON whitespace or serialization formatting..github/workflows/ci-runtime-product-slice.yml (1)
90-91: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePass the matrix value through
env.
verify-runner-image ${{ matrix.runtime.verify_backend }}splices plan data into the script body. Read it from an environment variable to keep the value as a single argument and to avoid template expansion inrun.♻️ Proposed refactor
- name: Verify prebuilt native environment - run: verify-runner-image ${{ matrix.runtime.verify_backend }} + env: + VERIFY_BACKEND: ${{ matrix.runtime.verify_backend }} + run: verify-runner-image "$VERIFY_BACKEND"🤖 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/workflows/ci-runtime-product-slice.yml around lines 90 - 91, Update the “Verify prebuilt native environment” step to pass matrix.runtime.verify_backend through the step’s env mapping, then invoke verify-runner-image using that environment variable rather than interpolating the matrix expression in run. Preserve the value as a single argument..github/workflows/ci-orchestrator.yml (1)
93-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
runner_sizeinput.
ci-quality-slice.ymldeclaresrunner_sizebut never reads it. The slice selects runners fromrunner_policyoutputs. Drop the input here and in the slice.As per coding guidelines: "Do not leave warnings, unused code, or dead code introduced by a change."
🤖 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/workflows/ci-orchestrator.yml at line 93, Remove the unused runner_size input from the workflow interface and from ci-quality-slice.yml, while preserving runner selection through the runner_policy outputs. Ensure no references or declarations to runner_size remain.Source: Coding guidelines
.github/workflows/ci-platform-checks-slice.yml (1)
68-76: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass
inputs.profilethroughenvinstead of expanding it into the pwsh script.Template expansion inside
runsplices the value into the script body. Static analysis flags this as template injection. Read the value from an environment variable.🛡️ Proposed fix
- name: Check neutral host and Node SDK on Windows if: ${{ matrix.check.platform == 'windows' && matrix.check.kind != 'unit' }} shell: pwsh + env: + PROFILE: ${{ inputs.profile }} run: | cargo check --locked -p mesh-llm --bin mesh-llm --no-default-features --features web-ui,dynamic-native-runtime cargo check --locked -p mesh-llm-nodejs - if ('${{ inputs.profile }}' -eq 'main' -or '${{ inputs.profile }}' -eq 'manual-full') { + if ($env:PROFILE -eq 'main' -or $env:PROFILE -eq 'manual-full') { cargo build --release --locked -p mesh-llm-nodejs }🤖 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/workflows/ci-platform-checks-slice.yml around lines 68 - 76, Update the “Check neutral host and Node SDK on Windows” step to pass inputs.profile through the step’s env configuration, then read that environment variable inside the PowerShell condition instead of embedding the GitHub expression directly in the run script. Preserve the existing main/manual-full release-build behavior.Source: Linters/SAST tools
🤖 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/actions/plan-ci/action.yml:
- Around line 183-191: Update the jq invocations in the matrix extraction and
signal loop to remove the unnecessary backslashes before embedded quotes,
including platform selectors and the --arg signal value. Ensure jq receives
valid filters and signal names so the existing echo outputs contain the actual
matrices and signal values.
In @.github/workflows/ci-orchestrator.yml:
- Around line 251-254: Update the jq gate around NEEDS_RESULTS so the plan entry
is accepted only when its result is "success", rather than exempting plan from
result validation. Preserve the existing success-or-skipped requirement for all
other entries.
- Around line 129-135: Update the rust_tests, hosts, and runtime_product job
conditions in ci-orchestrator.yml to require successful completion of their
required producer jobs, including static_abi for rust_tests and ui-artifact for
runtime_product, while preserving existing cancellation and planner-output
checks. Update sdk so static_abi success is required only when its matrix
includes Kotlin; non-Kotlin SDK runs should retain their existing prerequisite
behavior.
In @.github/workflows/ci-quality-slice.yml:
- Around line 44-61: Update the runner_policy jobs in
.github/workflows/ci-quality-slice.yml#L44-L61,
.github/workflows/ci-host-slice.yml#L42-L58, and
.github/workflows/ci-runtime-product-slice.yml#L39-L55: change permissions to
contents: read and add an actions/checkout step before the local
select-ci-runners action step.
In @.github/workflows/ci-runner-contract-slice.yml:
- Around line 36-39: Update the jq validation in the runner-role guard so
malformed plans, including a missing runner_roles key or non-string role values,
cause the workflow to fail closed rather than be treated as no Depot role.
Ensure the jq expression explicitly validates the expected structure and
propagates validation errors while still rejecting any role matching “depot”;
preserve the existing error message and exit behavior for rejected plans.
- Around line 44-46: Update the assertions in the CI runner contract step to
verify that .github/workflows/ci-orchestrator.yml and the ci-*-slice.yml
workflow set resolve to existing files before checking their contents. Keep the
pull_request_target prohibition grep, but prevent missing or renamed targets
from being converted into a passing result by ! and stderr suppression.
In @.github/workflows/ci-ui-artifact-slice.yml:
- Around line 43-49: Save the pnpm store after dependency installation in both
workflow sites: add a matching actions/cache/save step after pnpm i
--frozen-lockfile in .github/workflows/ci-ui-artifact-slice.yml:43-49 and after
Install UI dependencies in .github/workflows/ci-web-slice.yml:40-45. Reuse the
identical cache key, keep the key prefix aligned with the UI artifact slice’s
CACHE_NAMESPACE, and verify the referenced crates/mesh-llm-ui/pnpm-lock.yaml
exists; otherwise use the correct lockfile path.
In @.github/workflows/ci-web-slice.yml:
- Around line 60-69: Add a pinned taiki-e/install-action step for just before
the Build public website step, then add a website dependency installation step
running npm ci with website as its working directory. Ensure both setup steps
execute before just website-build, which must continue using npm-installed local
binaries.
In @.github/workflows/pr_builds.yml:
- Around line 19-21: Update the PR CI workflow entry that invokes
ci-orchestrator.yml so product-smoke does not depend on HF_TOKEN: use a PR-safe
model cache or fallback, or skip model-dependent smoke rows when the token is
unavailable. Ensure fork PRs never receive the repository secret while the
remaining smoke checks continue to run.
In `@ci/ci.md`:
- Around line 39-45: Add the missing direct dependency edges from UI, ABI, HOST,
and RUNTIME to the CI Required node in the Mermaid graph, alongside the existing
QUALITY, WEB, TESTS, SMOKE, SDK, PLATFORM, and RUNNER edges. Ensure the topology
represents every top-level slice consumed by the required summary.
- Around line 72-74: Update the PR/main parity statement near “The selected PR
row” to remove the claim that only row selection and bounded parallelism differ.
Limit the comparison to shared build semantics, or explicitly include the
permitted trust-derived differences: provider placement, cache mode, artifact
namespace, and optional credentials.
In `@ci/ownership.yml`:
- Around line 26-39: Add ownership rules in ci/ownership.yml for every currently
unmatched tracked path, covering scripts/**, evals/**, repository configuration
files, and other top-level directories with their correct domains. Preserve the
existing ci-control patterns, then validate that all tracked paths match at
least one ownership rule so unknown_path_policy: fail succeeds.
In `@ci/slices.yml`:
- Around line 8-13: Connect the budget fields from ci/slices.yml through
plan_json into ci-orchestrator.yml instead of using hard-coded workflow limits.
Pass each slice’s linux_max_parallel, macos_max_parallel, and
windows_max_parallel to its jobs, add the macOS max-parallel constraint, and
enforce total_max_workers; alternatively, add equivalent validation in
_validate_plan if workflow enforcement is not possible.
In `@scripts/plan-ci.py`:
- Around line 315-348: Empty affected_crates currently suppresses fallback
computation and produces an empty Rust test matrix. In scripts/plan-ci.py lines
315-348, update _affected_crates to treat only truthy raw values as explicit
input so empty arrays invoke affected-crates.sh; in
.github/actions/plan-ci/action.yml lines 153-163, omit affected_crates from the
jq payload when AFFECTED_CRATES is empty or [] so the planner can distinguish an
unset value from an explicit non-empty list.
In `@scripts/tests/test_reusable_workflow_runner_trust.py`:
- Around line 86-103: Add "static-abi-artifact.yml" to the names list in
test_pr_facing_checkouts_disable_persisted_credentials so its checkout and
persist-credentials settings are included in the existing parity assertions.
In `@scripts/tests/test_sccache_evidence.py`:
- Around line 341-358: Add an explicit INSTRUMENTED workflow-name set near
test_instrumented_workflows_use_unique_evidence_artifacts, excluding ci.yml, and
iterate over that set to assert each workflow contains at least one
capture-sccache-stats step. Preserve the existing artifact-name uniqueness and
github.run_attempt assertions, while retaining WORKFLOWS coverage for the
current checks.
In `@tools/xtask/src/workflow_checks.rs`:
- Around line 735-771: Remove the unused _compute_changes_action parameter from
check_ci_crate_test_coverage and delete the corresponding compute-changes action
read and argument in check_ci_crate_test_coverage_files. Ensure
check_test_batch_planner_covers_workspace runs only once per repo-consistency
invocation by removing its duplicate call from check_current_ci_invariants while
retaining the call in check_ci_crate_test_coverage_files.
- Around line 13-46: Split check_current_ci_invariants into named
domain-specific helpers for documentation, workflow, producer, and orchestrator
checks, keeping the top-level function under the 200-line Clippy threshold. Move
each related invariant check into its corresponding helper and pass only the
already-read file contents that helper uses, preserving existing validation
behavior.
- Around line 9-11: Restore the release invariant calls in
check_current_ci_invariants, including
check_release_dispatch_version_preparation, check_release_container_contracts,
and check_windows_dynamic_runtime_contract, and ensure
release_container_job_names is reached by the active production path; remove any
obsolete checks instead. Mark test-only helpers with #[cfg(test)], remove
#[allow(dead_code)], and simplify check_docs_and_workflow_invariants to directly
return check_current_ci_invariants(repo_root).
---
Nitpick comments:
In @.github/workflows/ci-orchestrator.yml:
- Line 93: Remove the unused runner_size input from the workflow interface and
from ci-quality-slice.yml, while preserving runner selection through the
runner_policy outputs. Ensure no references or declarations to runner_size
remain.
In @.github/workflows/ci-platform-checks-slice.yml:
- Around line 68-76: Update the “Check neutral host and Node SDK on Windows”
step to pass inputs.profile through the step’s env configuration, then read that
environment variable inside the PowerShell condition instead of embedding the
GitHub expression directly in the run script. Preserve the existing
main/manual-full release-build behavior.
In @.github/workflows/ci-runtime-product-slice.yml:
- Around line 90-91: Update the “Verify prebuilt native environment” step to
pass matrix.runtime.verify_backend through the step’s env mapping, then invoke
verify-runner-image using that environment variable rather than interpolating
the matrix expression in run. Preserve the value as a single argument.
In @.github/workflows/ci-sdk-slice.yml:
- Line 37: Replace the JSON substring checks in the workflow conditions for
rust, kotlin, and swift with value-based matching against the parsed sdk_matrix
array. Update all affected conditions, including the duplicate kotlin and swift
checks, so they select rows by each row’s id regardless of JSON whitespace or
serialization formatting.
In `@ci/ci-plan.schema.json`:
- Around line 90-93: Extend validation around the matrix schema and related plan
fields so standalone consumers reject duplicate matrix IDs and enforce the same
semantic constraints as build_plan, including keys in reasons, dependencies,
runner_roles, and cache_modes being limited to required_slices. Alternatively,
explicitly document that consumers must invoke scripts/plan-ci.py, but preserve
consistent validation behavior for all plan consumers.
In `@scripts/plan-ci.py`:
- Around line 544-548: Update the docs_only logic in the plan classification
function to derive the result from matched ownership domains rather than raw
changed-file paths. Ensure files assigned to the website domain, including
docs/index.html, docs/assets/**, and docs/catalog/**, do not mark docs_only true
when the web slice is selected, while preserving true only for plans containing
exclusively documentation changes.
- Around line 583-588: Adjust the ci-control domain handling before the
force_all_rows logic in scripts/plan-ci.py so pure Markdown-only changes under
.github/ and .omo/ do not trigger the control-plane fail-open path, or restrict
force_all_rows and control-slice expansion to main and manual-full profiles.
Preserve control-plane coverage for non-documentation changes and existing
profile behavior.
In `@scripts/tests/test_ci_workflow_artifacts.py`:
- Around line 19-24: Update test_main_plan_uses_all_rows_and_bounded_budgets to
parse SLICES.read_text() with the standard-library json parser, select
profiles["main"], and assert all_rows, total_max_workers, linux_max_parallel,
and windows_max_parallel on that profile. Remove the substring-based manifest
checks and do not add PyYAML.
In `@scripts/tests/test_plan_ci.py`:
- Around line 157-177: Update
test_unknown_paths_and_unknown_input_fields_fail_closed and
test_profile_event_contract_is_strict to bind each raised PLANNER.PlanError and
assert its message identifies the specific invalid condition. Use
assertRaisesRegex or separate subTest cases so unknown paths, unexpected fields,
profile mismatch, and malformed source_sha are independently validated.
In `@scripts/tests/test_pr_workflow_artifacts.py`:
- Around line 55-57: Bound the workflow job sections in both affected tests: in
scripts/tests/test_pr_workflow_artifacts.py lines 55-57, end the linux_product
slice at the next top-level job header so its prohibited-command assertions only
inspect that job; in scripts/tests/test_pr_builds_summary.py lines 15-32, end
the summary slice at the next job header or inspect the parsed
jobs.summary.needs list so required-job assertions cannot match later jobs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 432b54cf-48c1-4879-962f-0b72df45e830
📒 Files selected for processing (49)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/AGENTS.md.github/actions/compute-changes/action.yml.github/actions/plan-ci/action.yml.github/actions/prepare-windows-host-input/action.yml.github/workflows/ci-host-slice.yml.github/workflows/ci-orchestrator.yml.github/workflows/ci-platform-checks-slice.yml.github/workflows/ci-product-smoke-slice.yml.github/workflows/ci-quality-slice.yml.github/workflows/ci-runner-contract-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-rust-tests-slice.yml.github/workflows/ci-sdk-slice.yml.github/workflows/ci-ui-artifact-slice.yml.github/workflows/ci-web-slice.yml.github/workflows/ci.yml.github/workflows/pr_builds.yml.github/workflows/pr_quality.yml.github/workflows/pr_website.yml.github/workflows/smoke.yml.gitignore.omo/specs/pr-ci-optimization.mdCONTRIBUTING.mdREADME.mdci/DEPOT_MIGRATION.mdci/METRICS.mdci/ci-plan.schema.jsonci/ci.mdci/metrics/2026-07-29-pr-builds-baseline.jsonci/ownership.ymlci/slices.ymlscripts/plan-ci.pyscripts/plan-pr-build-jobs.pyscripts/tests/fixtures/ci-plan/docs-only.jsonscripts/tests/fixtures/ci-plan/main.jsonscripts/tests/fixtures/ci-plan/runtime.jsonscripts/tests/test_build_windows.pyscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_plan_ci.pyscripts/tests/test_plan_pr_build_jobs.pyscripts/tests/test_pr_builds_summary.pyscripts/tests/test_pr_workflow_artifacts.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_sccache_evidence.pytools/xtask/src/publish_consistency.rstools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (5)
- ci/metrics/2026-07-29-pr-builds-baseline.json
- .github/workflows/pr_website.yml
- scripts/plan-pr-build-jobs.py
- scripts/tests/test_plan_pr_build_jobs.py
- .github/workflows/pr_quality.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/ci.md (1)
160-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the complete validation contract.
The “Minimum validation” section lists actionlint,
git diff --check, Python tests, shellcheck, planner fixtures, and applicablextask repo-consistencychecks. The PR validation contract also includes Rust formatting, Rust tests, and worktree verification. If contributors use this section as the required checklist, they can omit those checks. Add the repository’s canonicaljustvalidation targets, or state that this section is only a partial checklist and link to the full contract.As per coding guidelines, do not commit until formatting and required local validation are complete.
🤖 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 `@ci/ci.md` around lines 160 - 169, Update the “Minimum validation” section to document the complete PR validation contract, including the canonical just targets for Rust formatting, Rust tests, and worktree verification alongside the existing checks. Alternatively, explicitly label it as a partial checklist and link to the repository’s full validation contract, using the canonical validation target names already defined by the project.Source: Coding guidelines
🧹 Nitpick comments (2)
scripts/tests/test_ci_workflow_artifacts.py (1)
48-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the budget consumption, not only the output declaration.
The asserted string
"<budget>: ${{ steps.plan.outputs.<budget> }}"matches theoutputs:block of theplanjob. The slice calls consume budgets throughfromJson(needs.plan.outputs.<budget>). The test therefore passes even if a slice call stops passing a budget, which contradicts the test nametest_manifest_budgets_drive_orchestrator_parallelism. Add assertions for the consumer form.♻️ Proposed additional assertions
self.assertIn( f"{budget}: ${{{{ steps.plan.outputs.{budget} }}}}", workflow, ) + for budget in ("linux_max_parallel", "macos_max_parallel", "windows_max_parallel"): + self.assertIn( + f"{budget}: ${{{{ fromJson(needs.plan.outputs.{budget}) }}}}", + workflow, + ) self.assertNotIn("contains(needs.plan.outputs.profile, 'pr-') &&", workflow)🤖 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/tests/test_ci_workflow_artifacts.py` around lines 48 - 60, Update test_manifest_budgets_drive_orchestrator_parallelism to assert each budget is consumed by orchestrator slice calls via the fromJson(needs.plan.outputs.<budget>) form, in addition to validating the plan output declarations. Keep the existing profile-condition assertion unchanged.tools/xtask/src/workflow_checks.rs (1)
273-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the invariants that
check_windows_dynamic_runtime_contractalready enforces.
check_current_ci_invariantscallscheck_producer_invariantsat Line 76 andcheck_windows_dynamic_runtime_contractat Line 80 in the same run. Three assertions are duplicated between them with only the context string different:
prepare_windows_hostmust not containpackage-native-runtime.sh(Lines 273-277 and Lines 715-719).prepare_runtimemust not containbuild-windows.ps1(Lines 283-287 and Lines 725-729).compose_productmust containscripts/ci-compose-product-input.sh(Lines 288-292 and Lines 730-734).Keep each assertion in one function. The Windows contract function is the more specific owner of the shared-action checks.
🤖 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 `@tools/xtask/src/workflow_checks.rs` around lines 273 - 297, Remove the three duplicated assertions from check_producer_invariants: the prepare_windows_host exclusion of package-native-runtime.sh, the prepare_runtime exclusion of build-windows.ps1, and the compose_product inclusion of scripts/ci-compose-product-input.sh. Keep these checks in check_windows_dynamic_runtime_contract as the single owner of the Windows-specific invariants.
🤖 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.
Outside diff comments:
In `@ci/ci.md`:
- Around line 160-169: Update the “Minimum validation” section to document the
complete PR validation contract, including the canonical just targets for Rust
formatting, Rust tests, and worktree verification alongside the existing checks.
Alternatively, explicitly label it as a partial checklist and link to the
repository’s full validation contract, using the canonical validation target
names already defined by the project.
---
Nitpick comments:
In `@scripts/tests/test_ci_workflow_artifacts.py`:
- Around line 48-60: Update test_manifest_budgets_drive_orchestrator_parallelism
to assert each budget is consumed by orchestrator slice calls via the
fromJson(needs.plan.outputs.<budget>) form, in addition to validating the plan
output declarations. Keep the existing profile-condition assertion unchanged.
In `@tools/xtask/src/workflow_checks.rs`:
- Around line 273-297: Remove the three duplicated assertions from
check_producer_invariants: the prepare_windows_host exclusion of
package-native-runtime.sh, the prepare_runtime exclusion of build-windows.ps1,
and the compose_product inclusion of scripts/ci-compose-product-input.sh. Keep
these checks in check_windows_dynamic_runtime_contract as the single owner of
the Windows-specific invariants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae88f543-80a2-4621-a008-b75a5180bd85
📒 Files selected for processing (22)
.github/actions/plan-ci/action.yml.github/workflows/ci-host-slice.yml.github/workflows/ci-orchestrator.yml.github/workflows/ci-platform-checks-slice.yml.github/workflows/ci-quality-slice.yml.github/workflows/ci-runner-contract-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-sdk-slice.yml.github/workflows/ci-ui-artifact-slice.yml.github/workflows/ci-web-slice.ymlci/ci-plan.schema.jsonci/ci.mdci/ownership.ymlci/slices.ymlscripts/plan-ci.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_plan_ci.pyscripts/tests/test_pr_builds_summary.pyscripts/tests/test_pr_workflow_artifacts.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_sccache_evidence.pytools/xtask/src/workflow_checks.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/ci-runner-contract-slice.yml
- .github/workflows/ci-sdk-slice.yml
- .github/workflows/ci-ui-artifact-slice.yml
- .github/workflows/ci-quality-slice.yml
- .github/workflows/ci-orchestrator.yml
- .github/workflows/ci-web-slice.yml
- ci/ci-plan.schema.json
- .github/actions/plan-ci/action.yml
- ci/slices.yml
- scripts/tests/test_sccache_evidence.py
- scripts/tests/test_reusable_workflow_runner_trust.py
- .github/workflows/ci-platform-checks-slice.yml
- scripts/plan-ci.py
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
from
August 11, 2026 22:14
831ab05 to
448a134
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci-runner-contract-slice.yml (1)
63-73: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winExclude this workflow from its own
pull_request_targetscan.The glob includes
.github/workflows/ci-runner-contract-slice.yml. That file containspull_request_targetin Lines 61 and 73. Therefore,grepalways finds a match, and the final! grepcheck fails on every run.Exclude this workflow from the text scan, or restrict the check to parsed trigger declarations.
🐛 Proposed fix
+ policy_targets=() + for workflow in "${workflow_targets[@]}"; do + if [[ "$workflow" != ".github/workflows/ci-runner-contract-slice.yml" ]]; then + policy_targets+=("$workflow") + fi + done - ! grep -n 'pull_request_target' "${workflow_targets[@]}" + ! grep -n 'pull_request_target' "${policy_targets[@]}"🤖 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/workflows/ci-runner-contract-slice.yml around lines 63 - 73, Update the workflow scan around workflow_targets and the final grep so ci-runner-contract-slice.yml is excluded from the pull_request_target check while remaining validated as an existing required workflow. Preserve the scan for all other matching CI slice and orchestrator workflows.
🤖 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.
Outside diff comments:
In @.github/workflows/ci-runner-contract-slice.yml:
- Around line 63-73: Update the workflow scan around workflow_targets and the
final grep so ci-runner-contract-slice.yml is excluded from the
pull_request_target check while remaining validated as an existing required
workflow. Preserve the scan for all other matching CI slice and orchestrator
workflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecc63f95-d936-45a2-9979-ae5952c43109
📒 Files selected for processing (5)
.github/workflows/ci-orchestrator.yml.github/workflows/ci-runner-contract-slice.ymlci/ci.mdscripts/tests/test_ci_workflow_artifacts.pytools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (1)
- tools/xtask/src/workflow_checks.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci-orchestrator.yml
- ci/ci.md
- scripts/tests/test_ci_workflow_artifacts.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_reusable_workflow_runner_trust.py (1)
103-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject direct secret-context references.
assertNotIn("secrets:", workflow)detects a YAMLsecrets:mapping, but it does not detect${{ secrets.SOME_TOKEN }}or${{ secrets["SOME_TOKEN"] }}. A future change topr_builds.ymlcould then access a repository secret while this test still passes.Replace the substring check with a regex that rejects
secrets:,secrets., andsecrets[forms.Suggested fix
workflow = self.workflow("pr_builds.yml") - self.assertNotIn("secrets:", workflow) + self.assertNotRegex(workflow, r"\bsecrets\s*(?:[:.\[])") self.assertNotIn("HF_TOKEN", workflow)🤖 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/tests/test_reusable_workflow_runner_trust.py` around lines 103 - 106, Update test_pr_entrypoint_maps_no_repository_secret to replace the literal secrets: assertion with a regex-based assertion that rejects secrets:, secrets., and secrets[ references anywhere in workflow, while preserving the existing HF_TOKEN check.
🤖 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.
Outside diff comments:
In `@scripts/tests/test_reusable_workflow_runner_trust.py`:
- Around line 103-106: Update test_pr_entrypoint_maps_no_repository_secret to
replace the literal secrets: assertion with a regex-based assertion that rejects
secrets:, secrets., and secrets[ references anywhere in workflow, while
preserving the existing HF_TOKEN check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d35dedcd-d790-41d4-8d7c-2de6e565a7e4
📒 Files selected for processing (2)
.github/workflows/ci-runner-contract-slice.ymlscripts/tests/test_reusable_workflow_runner_trust.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-runner-contract-slice.yml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_reusable_workflow_runner_trust.py (1)
25-36: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRequire checkout parity in each credential-smoke workflow.
The test only checks that
persist-credentials: falseoccurs once. It passes if anotheractions/checkoutstep uses the default persisted credentials. Compare the checkout count with the disabled-credential count for each workflow.Proposed fix
workflow = self.workflow(name) self.assertIn("ubuntu-24.04", workflow) self.assertNotIn("depot-ubuntu", workflow) - self.assertIn("persist-credentials: false", workflow) + self.assertEqual( + workflow.count("uses: actions/checkout@"), + workflow.count("persist-credentials: false"), + )🤖 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/tests/test_reusable_workflow_runner_trust.py` around lines 25 - 36, Update test_credential_smokes_remain_fixed_github_hosted to count actions/checkout steps and occurrences of “persist-credentials: false” for each workflow, then assert the counts are equal. Keep the existing runner and depot-ubuntu assertions, ensuring every checkout step explicitly disables credential persistence.
🤖 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-product-smoke-slice.yml:
- Around line 49-52: Update the core-cuda job configuration around the runner
and secrets block to comply with the credential policy: remove the HF_TOKEN
forwarding from the gpu-nvidia lane, unless the repository inventory and
smoke.yml credential-isolation comment are explicitly updated to document this
GPU runner as an approved isolated exception.
In `@scripts/tests/test_pr_builds_summary.py`:
- Line 39: Update the required-job assertions in the test covering native
runtimes to enumerate the complete static superset of platform-specific runtime
job names, including macOS and Windows, instead of using the generic
"native_runtimes" name; preserve the existing assertion structure for other
required jobs.
---
Outside diff comments:
In `@scripts/tests/test_reusable_workflow_runner_trust.py`:
- Around line 25-36: Update test_credential_smokes_remain_fixed_github_hosted to
count actions/checkout steps and occurrences of “persist-credentials: false” for
each workflow, then assert the counts are equal. Keep the existing runner and
depot-ubuntu assertions, ensuring every checkout step explicitly disables
credential persistence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ffff4ca-95a7-4457-a19b-b0cd3efdfbc0
📒 Files selected for processing (18)
.agents/skills/manage-ci/references/current-inventory.md.github/workflows/ci-orchestrator.yml.github/workflows/ci-product-smoke-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-rust-tests-slice.yml.github/workflows/ci-sdk-slice.yml.github/workflows/smoke.yml.github/workflows/static-abi-artifact.yml.omo/specs/pr-ci-optimization.mdci/ci.mdci/slices.ymlscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_plan_ci.pyscripts/tests/test_pr_builds_summary.pyscripts/tests/test_pr_workflow_artifacts.pyscripts/tests/test_release_workflow_artifacts.pyscripts/tests/test_reusable_workflow_runner_trust.py
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/tests/test_pr_workflow_artifacts.py
- scripts/tests/test_plan_ci.py
- scripts/tests/test_ci_artifact_actions.py
- ci/ci.md
- .omo/specs/pr-ci-optimization.md
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
2 times, most recently
from
August 12, 2026 18:00
d0f3bf4 to
efb3d1e
Compare
|
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.
Actionable comments posted: 5
🤖 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 @.agents/skills/manage-ci/SKILL.md:
- Around line 84-86: Document the documentation-only ci-control exception in
both .agents/skills/manage-ci/SKILL.md lines 84-86 and CONTRIBUTING.md lines
178-181: clarify that paths mapping to both docs and ci-control retain limited
documentation routing rather than unconditionally forcing all product rows,
while preserving fail-open behavior for other CI control-plane changes.
In @.github/actions/prepare-windows-host-input/action.yml:
- Around line 74-78: Update the fallback condition around $uiDist to
specifically test whether its index.html entry point exists as a file, using
Test-Path with the joined index.html path and Leaf path type. Preserve the
existing directory creation and placeholder file generation when index.html is
absent, regardless of other files in the directory.
In `@ci/DEPOT_MIGRATION.md`:
- Around line 36-47: Complete the current main allowlist in the Depot migration
documentation by adding exact `@refs/heads/main` entries for
.github/workflows/depot-canary.yml and .github/workflows/release.yml. Do not add
the listed CI slice workflows solely because they use non-Depot runners.
In `@scripts/plan-ci.py`:
- Around line 196-203: Update _validate_manifests to verify every row ID in
domain_rows, smoke_domain_rows, platform_domain_rows, and sdk_domain_rows exists
in its corresponding row catalog, while preserving the existing domain and
string-list validation. Raise PlanError for unknown mapped IDs and add coverage
for that validation failure.
- Around line 185-188: Update the _validate_rows call for runtime_rows to
require a non-empty runner_role before _select_rows accesses row["runner_role"].
Preserve the existing validation for platform and architecture, and add a
malformed-manifest test asserting that a missing runner_role raises PlanError
rather than KeyError.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab7d98a3-2799-48ee-8512-50fc81051c0b
📒 Files selected for processing (51)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/AGENTS.md.github/actions/compute-changes/action.yml.github/actions/plan-ci/action.yml.github/actions/prepare-windows-host-input/action.yml.github/workflows/ci-host-slice.yml.github/workflows/ci-orchestrator.yml.github/workflows/ci-platform-checks-slice.yml.github/workflows/ci-product-smoke-slice.yml.github/workflows/ci-quality-slice.yml.github/workflows/ci-runner-contract-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-rust-tests-slice.yml.github/workflows/ci-sdk-slice.yml.github/workflows/ci-ui-artifact-slice.yml.github/workflows/ci-web-slice.yml.github/workflows/ci.yml.github/workflows/pr_builds.yml.github/workflows/pr_quality.yml.github/workflows/pr_website.yml.github/workflows/smoke.yml.github/workflows/static-abi-artifact.yml.gitignore.omo/specs/pr-ci-optimization.mdCONTRIBUTING.mdREADME.mdci/DEPOT_MIGRATION.mdci/METRICS.mdci/ci-plan.schema.jsonci/ci.mdci/metrics/2026-07-29-pr-builds-baseline.jsonci/ownership.ymlci/slices.ymlscripts/plan-ci.pyscripts/plan-pr-build-jobs.pyscripts/tests/fixtures/ci-plan/docs-only.jsonscripts/tests/fixtures/ci-plan/main.jsonscripts/tests/fixtures/ci-plan/runtime.jsonscripts/tests/test_build_windows.pyscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_plan_ci.pyscripts/tests/test_plan_pr_build_jobs.pyscripts/tests/test_pr_builds_summary.pyscripts/tests/test_pr_workflow_artifacts.pyscripts/tests/test_release_workflow_artifacts.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_sccache_evidence.pytools/xtask/src/publish_consistency.rstools/xtask/src/workflow_checks.rs
💤 Files with no reviewable changes (5)
- ci/metrics/2026-07-29-pr-builds-baseline.json
- .github/workflows/pr_website.yml
- .github/workflows/pr_quality.yml
- scripts/plan-pr-build-jobs.py
- scripts/tests/test_plan_pr_build_jobs.py
🚧 Files skipped from review as they are similar to previous changes (38)
- scripts/tests/test_release_workflow_artifacts.py
- .github/workflows/static-abi-artifact.yml
- .gitignore
- tools/xtask/src/publish_consistency.rs
- .github/actions/compute-changes/action.yml
- scripts/tests/fixtures/ci-plan/main.json
- .github/workflows/ci-ui-artifact-slice.yml
- .github/workflows/ci-platform-checks-slice.yml
- scripts/tests/fixtures/ci-plan/docs-only.json
- .github/actions/plan-ci/action.yml
- .github/workflows/ci-runner-contract-slice.yml
- .github/workflows/ci-rust-tests-slice.yml
- .github/workflows/ci.yml
- ci/slices.yml
- .github/workflows/ci-web-slice.yml
- .github/workflows/ci-sdk-slice.yml
- ci/ci-plan.schema.json
- .github/workflows/pr_builds.yml
- scripts/tests/test_reusable_workflow_runner_trust.py
- ci/ci.md
- .github/workflows/ci-product-smoke-slice.yml
- .github/workflows/ci-orchestrator.yml
- scripts/tests/test_ci_artifact_actions.py
- .github/workflows/ci-runtime-product-slice.yml
- .github/workflows/ci-host-slice.yml
- .github/workflows/ci-quality-slice.yml
- ci/ownership.yml
- tools/xtask/src/workflow_checks.rs
- scripts/tests/test_ci_workflow_artifacts.py
- scripts/tests/fixtures/ci-plan/runtime.json
- .omo/specs/pr-ci-optimization.md
- scripts/tests/test_pr_workflow_artifacts.py
- scripts/tests/test_build_windows.py
- .github/workflows/smoke.yml
- README.md
- .agents/skills/manage-ci/references/current-inventory.md
- scripts/tests/test_pr_builds_summary.py
- scripts/tests/test_sccache_evidence.py
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
from
August 12, 2026 18:28
efb3d1e to
85cd1ca
Compare
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 @.agents/skills/manage-ci/SKILL.md:
- Around line 254-258: Replace the three direct cargo xtask commands in
.agents/skills/manage-ci/SKILL.md lines 254-258 with the documented just recipes
for ci-crate-lists, release-targets, and publish-crates. Update CONTRIBUTING.md
lines 196-199 to use the same just-based repo-consistency recipe family instead
of the generic Cargo instruction.
In `@scripts/plan-ci.py`:
- Around line 355-359: Update the affected_crates selection logic near
_string_list so only None and the intentional empty-list value [] use fallback
behavior; pass every other non-None value, including false, "", 0, and {},
through _string_list for validation. Add regression coverage for
affected_crates: false and affected_crates: "" while preserving existing
profile-based fallback behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b2b685a-9cec-458e-801f-8ec850659db2
📒 Files selected for processing (8)
.agents/skills/manage-ci/SKILL.md.github/actions/prepare-windows-host-input/action.ymlCONTRIBUTING.mdci/DEPOT_MIGRATION.mdscripts/plan-ci.pyscripts/tests/test_build_windows.pyscripts/tests/test_plan_ci.pyscripts/tests/test_reusable_workflow_runner_trust.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/prepare-windows-host-input/action.yml
- scripts/tests/test_reusable_workflow_runner_trust.py
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
3 times, most recently
from
August 12, 2026 20:06
af963ef to
8070216
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
CONTRIBUTING.md (1)
189-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the workflow validation checklist.
This section lists actionlint,
git diff --check, Python tests, and conditional repository-consistency checks. The PR validation contract also includes Rust formatting, Rust tests, and worktree verification. Add the existing validation commands or link to the canonical full suite so contributors do not treat this list as complete.🤖 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 `@CONTRIBUTING.md` around lines 189 - 209, Update the “Local validation and extensions” section to include the existing Rust formatting, Rust test, and worktree verification commands, or link to the canonical full validation suite. Keep the current targeted checks and conditional repository-consistency guidance intact, while making clear that the checklist is complete.scripts/tests/test_validate_ci_lane_results.py (1)
53-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the macOS and Windows lane branches.
The tests cover the
linuxandqualitybranches only. Themacosbranch couples thesdkjob to aswiftrow, and thewindowsbranch expectsruntime_productwithout checking the host matrix. Tests for those two branches would pin the expected job sets and catch a regression in either mapping.🤖 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/tests/test_validate_ci_lane_results.py` around lines 53 - 80, Extend the validator tests with macOS and Windows plans that exercise their lane-specific job mappings. In the macOS case, verify the sdk job is coupled to a swift row; in the Windows case, verify runtime_product jobs are expected without requiring host-matrix entries. Assert the resulting job sets and validation behavior using the existing VALIDATOR helpers..github/workflows/ci-control.yml (1)
210-215: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPreserve the same-repository trust invariant.
SOURCE_REFselects the lane workflow and local actions from the pull-request head. Keep the existinghead_repository.full_name == github.repositoryguard. Document inci/ci.mdthat protected dispatch depends on this guard, least-privilege lane permissions, and no secrets for pull-request runs.🤖 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/workflows/ci-control.yml around lines 210 - 215, Preserve the existing head_repository.full_name == github.repository guard around the workflow dispatch in the lane-dispatch logic, ensuring SOURCE_REF is only used for trusted same-repository pull requests. Update ci/ci.md to document that protected dispatch requires this guard, least-privilege lane permissions, and no secrets on pull-request runs.Source: Linters/SAST tools
.github/workflows/ci.yml (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPropagate
use_depotthrough the reusable workflow chain. Add the input toci-orchestrator.yml, pass it from.github/workflows/ci.yml, and forward it to affected slice workflows. Read it throughinputs.use_depot. Remove the unused input from.github/workflows/pr_builds.yml, because PR CI must remain GitHub-hosted.🤖 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/workflows/ci.yml around lines 4 - 10, Propagate the workflow_dispatch input use_depot through ci.yml into ci-orchestrator.yml and then into every affected slice workflow, reading it via inputs.use_depot. Add the corresponding reusable-workflow declarations and forwarding mappings; remove the unused use_depot input from .github/workflows/pr_builds.yml so PR CI remains GitHub-hosted. Affected sites: .github/workflows/ci.yml lines 4-10 must pass the input; .github/workflows/pr_builds.yml lines 4-7 requires removal of its input declaration.
🤖 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/actions/report-ci-lane/action.yml:
- Around line 80-93: Guard the overallCheck lookup in the action’s JavaScript
before calling github.rest.checks.get: only resolve the aggregate check when
process.env.OVERALL_CHECK_ID is non-empty, and avoid converting an empty value
to 0. Preserve the existing lane-check identity validation and later empty-value
handling while allowing lane updates to proceed without an aggregate check.
- Around line 109-141: Update the aggregation loop around listForRef so
incomplete correlated lanes return normally after polling instead of throwing an
error. Paginate the check-run listing across all pages before filtering by
expected names and CORRELATION_ID, ensuring hidden runs beyond the first 100 are
included while preserving the existing aggregate update when all lanes complete.
In @.github/actions/select-ci-runners/action.yml:
- Around line 79-85: Update the effective_event_name normalization before the
case statement to also map DISPATCH_ORIGINAL_EVENT_NAME="pull_request_target" to
"pull_request", preserving the untrusted event branch and preventing Depot
activation on the main branch. Add a regression test covering this original
event value.
In @.github/workflows/ci-control.yml:
- Around line 52-56: Update the bootstrapped detection around
listJobsForWorkflowRun to recognize reusable-workflow job names beginning with
“Bootstrap PR CI / ” or “Bootstrap main CI / ”, while retaining the existing
exact-name checks. Ensure should_dispatch remains false and the early return
prevents duplicate dispatches for these prefixed jobs.
In @.github/workflows/pr_builds.yml:
- Around line 44-45: Update the sameRepository calculation in the pull request
routing step to guard against a missing pull.head.repo before accessing
full_name. Treat a null head repository as not originating from the same
repository, allowing the existing routing logic to fall back to bootstrap.
In `@scripts/tests/test_ci_lane_workflows.py`:
- Around line 15-26: Update the workflow dispatch logic in ci-control.yml to
pass SOURCE_SHA as each lane’s ref instead of the moving source_ref value, while
preserving the existing lane dispatches. Extend
test_controller_plans_once_and_dispatches_native_lane_inputs to assert that
SOURCE_SHA is used as the dispatch ref.
In `@scripts/validate-ci-lane-results.py`:
- Around line 111-114: Update the planned-job validation loop around expected
and needs to defensively read each needs entry only when it is a dictionary,
matching the handling at line 117. Ensure malformed or missing entries produce
result=None and follow the existing LaneResultError path with status 2 instead
of raising AttributeError.
- Around line 85-86: Update the macOS SDK handling in the lane validation logic
around the existing `if "swift" in sdk` branch so any non-empty SDK matrix adds
both `sdk_sdk_input` and `sdk` to the expected jobs, regardless of the SDK row
ID; preserve the existing behavior for empty SDK matrices.
---
Nitpick comments:
In @.github/workflows/ci-control.yml:
- Around line 210-215: Preserve the existing head_repository.full_name ==
github.repository guard around the workflow dispatch in the lane-dispatch logic,
ensuring SOURCE_REF is only used for trusted same-repository pull requests.
Update ci/ci.md to document that protected dispatch requires this guard,
least-privilege lane permissions, and no secrets on pull-request runs.
In @.github/workflows/ci.yml:
- Around line 4-10: Propagate the workflow_dispatch input use_depot through
ci.yml into ci-orchestrator.yml and then into every affected slice workflow,
reading it via inputs.use_depot. Add the corresponding reusable-workflow
declarations and forwarding mappings; remove the unused use_depot input from
.github/workflows/pr_builds.yml so PR CI remains GitHub-hosted. Affected sites:
.github/workflows/ci.yml lines 4-10 must pass the input;
.github/workflows/pr_builds.yml lines 4-7 requires removal of its input
declaration.
In `@CONTRIBUTING.md`:
- Around line 189-209: Update the “Local validation and extensions” section to
include the existing Rust formatting, Rust test, and worktree verification
commands, or link to the canonical full validation suite. Keep the current
targeted checks and conditional repository-consistency guidance intact, while
making clear that the checklist is complete.
In `@scripts/tests/test_validate_ci_lane_results.py`:
- Around line 53-80: Extend the validator tests with macOS and Windows plans
that exercise their lane-specific job mappings. In the macOS case, verify the
sdk job is coupled to a swift row; in the Windows case, verify runtime_product
jobs are expected without requiring host-matrix entries. Assert the resulting
job sets and validation behavior using the existing VALIDATOR helpers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11365ef9-9f69-4631-af31-bd0cc0f5e288
📒 Files selected for processing (26)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/actions/plan-ci/action.yml.github/actions/report-ci-lane/action.yml.github/actions/select-ci-runners/action.yml.github/workflows/ci-control.yml.github/workflows/ci-linux-lane.yml.github/workflows/ci-macos-lane.yml.github/workflows/ci-quality-lane.yml.github/workflows/ci-website-lane.yml.github/workflows/ci-windows-lane.yml.github/workflows/ci.yml.github/workflows/native-sdk-artifact.yml.github/workflows/pr_builds.yml.github/workflows/static-abi-artifact.yml.omo/specs/pr-ci-optimization.mdCONTRIBUTING.mdci/DEPOT_MIGRATION.mdci/ci.mdscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_lane_workflows.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_validate_ci_lane_results.pyscripts/validate-ci-lane-results.pytools/xtask/src/workflow_checks.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/static-abi-artifact.yml
- ci/ci.md
- scripts/tests/test_reusable_workflow_runner_trust.py
- .omo/specs/pr-ci-optimization.md
- .agents/skills/manage-ci/references/current-inventory.md
- scripts/tests/test_ci_artifact_actions.py
- scripts/tests/test_ci_workflow_artifacts.py
- tools/xtask/src/workflow_checks.rs
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
2 times, most recently
from
August 12, 2026 22:12
39b9b2c to
63da089
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tools/xtask/src/workflow_checks.rs (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck each lane workflow separately.
The five lane workflows are joined into one string.
check_orchestrator_invariantsthen assertsuses: ./.github/workflows/ci-runtime-product-slice.ymlagainst that combined text at Line 335. One lane satisfies the assertion for all five. If the macOS or Windows lane loses its runtime-slice call, this check still passes.Pass the lanes as named pairs and assert the expected call per lane.
♻️ Proposed refactor
- let lane_workflows = ["quality", "website", "linux", "macos", "windows"] - .into_iter() - .map(|lane| { - fs::read_to_string(repo_root.join(format!(".github/workflows/ci-{lane}-lane.yml"))) - }) - .collect::<Result<Vec<_>, _>>()? - .join("\n"); + let lane_workflows = ["quality", "website", "linux", "macos", "windows"] + .into_iter() + .map(|lane| { + let text = + fs::read_to_string(repo_root.join(format!(".github/workflows/ci-{lane}-lane.yml")))?; + Ok((lane, text)) + }) + .collect::<DynResult<Vec<(&str, String)>>>()?;Then in
check_orchestrator_invariants, takelanes: &[(&str, String)]and require the quality-slice call in thequalitylane and the runtime-slice call in each oflinux,macos, andwindows.🤖 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 `@tools/xtask/src/workflow_checks.rs` around lines 23 - 29, Update the workflow collection and check_orchestrator_invariants to preserve each lane name with its workflow content instead of joining all workflows into one string. Change the function input to accept named lane pairs, then require the quality-slice call specifically in the quality lane and the runtime-slice call independently in each linux, macos, and windows lane.scripts/tests/test_ci_artifact_actions.py (1)
1824-1843: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two cache-contract loops.
The second loop repeats the file reads of the first loop. Its
Swatinem/rust-cachebranch is also unreachable as a distinct contract: the first loop already assertssave-iffor every workflow that usesSwatinem/rust-cache, so the extraactions/cache@condition never adds coverage. Only theconfigure-sccache-ghaassertion is new.Move that assertion into the first loop and delete the second loop.
♻️ Proposed refactor
if "uses: Swatinem/rust-cache@" in workflow: self.assertIn( "save-if: ${{ github.ref == 'refs/heads/main' }}", workflow, ) - - for workflow_name in workflow_names: - workflow = ( - ROOT / ".github" / "workflows" / workflow_name - ).read_text(encoding="utf-8") - if ( - "uses: Swatinem/rust-cache@" in workflow - and "uses: actions/cache@" not in workflow - ): - self.assertIn( - "save-if: ${{ github.ref == 'refs/heads/main' }}", - workflow, - ) - if "uses: ./.github/actions/configure-sccache-gha" in workflow: - self.assertIn("allow_depot_remote_cache", workflow) + if "uses: ./.github/actions/configure-sccache-gha" in workflow: + self.assertIn("allow_depot_remote_cache", workflow)🤖 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/tests/test_ci_artifact_actions.py` around lines 1824 - 1843, Merge the two workflow-validation loops by moving the configure-sccache-gha assertion into the first loop alongside the existing rust-cache checks. Remove the second loop and its repeated workflow reads and redundant Swatinem/rust-cache condition, preserving all existing assertions.
🤖 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-control.yml:
- Around line 89-106: The ci-control workflow currently runs local orchestration
actions and dispatched lane workflows from the PR-controlled source_sha. Update
the controller checkout and createWorkflowDispatch ref to
github.event.repository.default_branch, while passing source_sha explicitly only
to product-code checkout steps in lane and slice workflows; keep local
reporting, planning, and orchestration actions on the protected default-branch
ref.
In @.github/workflows/ci.yml:
- Around line 51-57: Update the bootstrap job’s use_depot expression to pass
true only when github.event_name is workflow_dispatch and inputs.use_depot is
true, yielding false for push events; update
test_manual_main_depot_input_is_explicitly_forwarded to assert the new
expression.
In `@ci/ci.md`:
- Around line 118-123: Resolve the contradictory PR runner policy in the
ci-product-smoke-slice.yml documentation: align the CUDA PR behavior with the
policy described in the pull-request runner section, either by defining the
trusted boundary and central policy for gpu-nvidia execution or by restricting
GPU smoke to trusted profiles and documenting the PR fallback. Update both the
CUDA description and the pull-request rules consistently.
- Around line 200-210: Update the validation commands in the manage-ci
validation contract around the shown actionlint, diff, unittest, shellcheck,
planner-fixture, and repo-consistency checks to use existing just targets or add
a single just target wrapping them. Ensure scope-specific validation follows the
referenced contract and uses just test-all when full repository validation is
required; remove direct tool invocations from the contributor instructions.
In `@tools/xtask/src/installer_fixtures.rs`:
- Around line 162-171: Validate that linux_arm64_cuda_asset ends with
"-cuda.tar.gz" before performing the replacement, and fail immediately with a
clear assertion if it does not. Then retain the existing replacement and
ensure_eq comparison so the test explicitly verifies CUDA major suffix
substitution.
---
Nitpick comments:
In `@scripts/tests/test_ci_artifact_actions.py`:
- Around line 1824-1843: Merge the two workflow-validation loops by moving the
configure-sccache-gha assertion into the first loop alongside the existing
rust-cache checks. Remove the second loop and its repeated workflow reads and
redundant Swatinem/rust-cache condition, preserving all existing assertions.
In `@tools/xtask/src/workflow_checks.rs`:
- Around line 23-29: Update the workflow collection and
check_orchestrator_invariants to preserve each lane name with its workflow
content instead of joining all workflows into one string. Change the function
input to accept named lane pairs, then require the quality-slice call
specifically in the quality lane and the runtime-slice call independently in
each linux, macos, and windows lane.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37206d42-3897-4535-bd7b-115e7bd5efd6
📒 Files selected for processing (33)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/actions/plan-ci/action.yml.github/actions/report-ci-lane/action.yml.github/actions/select-ci-runners/action.yml.github/workflows/ci-control.yml.github/workflows/ci-host-slice.yml.github/workflows/ci-linux-lane.yml.github/workflows/ci-macos-lane.yml.github/workflows/ci-orchestrator.yml.github/workflows/ci-quality-lane.yml.github/workflows/ci-quality-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-website-lane.yml.github/workflows/ci-windows-lane.yml.github/workflows/ci.yml.github/workflows/native-sdk-artifact.yml.github/workflows/pr_builds.yml.github/workflows/static-abi-artifact.yml.omo/specs/pr-ci-optimization.mdCONTRIBUTING.mdci/DEPOT_MIGRATION.mdci/ci.mdscripts/plan-ci.pyscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_lane_workflows.pyscripts/tests/test_ci_workflow_artifacts.pyscripts/tests/test_plan_ci.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_validate_ci_lane_results.pyscripts/validate-ci-lane-results.pytools/xtask/src/installer_fixtures.rstools/xtask/src/workflow_checks.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- .github/workflows/ci-macos-lane.yml
- .agents/skills/manage-ci/references/current-inventory.md
- .github/workflows/ci-host-slice.yml
- .github/workflows/ci-quality-slice.yml
- .github/workflows/pr_builds.yml
- .github/workflows/ci-windows-lane.yml
- .github/actions/select-ci-runners/action.yml
- .github/workflows/ci-runtime-product-slice.yml
- .github/actions/plan-ci/action.yml
- scripts/validate-ci-lane-results.py
- .github/workflows/ci-orchestrator.yml
- .github/workflows/ci-quality-lane.yml
- scripts/tests/test_ci_workflow_artifacts.py
- .omo/specs/pr-ci-optimization.md
- .github/actions/report-ci-lane/action.yml
- .github/workflows/ci-linux-lane.yml
- .github/workflows/ci-website-lane.yml
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
2 times, most recently
from
August 12, 2026 22:46
c0a98e3 to
eaaf33e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/hf-download-smoke.yml (1)
61-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBlock cache writes for direct
pull_request_targetevents.
github.event.inputs.original_event_nameis empty for a directpull_request_targetrun. Its base ref can berefs/heads/main, so these conditions permit untrusted PR code to populate caches later restored by trusted runs. Exclude direct pull request event names at every cache-write site.
.github/workflows/hf-download-smoke.yml#L61-L75: Add directgithub.event_nameexclusions to both Rust and model-cache save conditions..github/workflows/scripted-binary-smoke.yml#L87-L87: Add directgithub.event_nameexclusions tosave_model_cache..github/workflows/sdk-smoke.yml#L258-L258: Add directgithub.event_nameexclusions tosave_model_cache.Proposed condition update
- github.ref == 'refs/heads/main' && github.event.inputs.original_event_name != 'pull_request' && github.event.inputs.original_event_name != 'pull_request_target' + github.ref == 'refs/heads/main' && github.event_name != 'pull_request' && github.event_name != 'pull_request_target' && github.event.inputs.original_event_name != 'pull_request' && github.event.inputs.original_event_name != 'pull_request_target'🤖 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/workflows/hf-download-smoke.yml around lines 61 - 75, Update the cache-save conditions to exclude direct pull_request_target events by checking github.event_name at every cache-write site. In .github/workflows/hf-download-smoke.yml lines 61-75, apply the exclusion to both Rust and model-cache save conditions; in .github/workflows/scripted-binary-smoke.yml line 87 and .github/workflows/sdk-smoke.yml line 258, apply it to save_model_cache while preserving the existing trusted-main and cache-hit checks.
🤖 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.
Outside diff comments:
In @.github/workflows/hf-download-smoke.yml:
- Around line 61-75: Update the cache-save conditions to exclude direct
pull_request_target events by checking github.event_name at every cache-write
site. In .github/workflows/hf-download-smoke.yml lines 61-75, apply the
exclusion to both Rust and model-cache save conditions; in
.github/workflows/scripted-binary-smoke.yml line 87 and
.github/workflows/sdk-smoke.yml line 258, apply it to save_model_cache while
preserving the existing trusted-main and cache-hit checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ec9b950-aa07-40ad-bb27-b9fdcea843ed
📒 Files selected for processing (38)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.md.github/actions/configure-sccache-gha/action.yml.github/workflows/ci-control.yml.github/workflows/ci-host-slice.yml.github/workflows/ci-linux-lane.yml.github/workflows/ci-macos-lane.yml.github/workflows/ci-platform-checks-slice.yml.github/workflows/ci-product-smoke-slice.yml.github/workflows/ci-quality-lane.yml.github/workflows/ci-quality-slice.yml.github/workflows/ci-runner-contract-slice.yml.github/workflows/ci-runtime-product-slice.yml.github/workflows/ci-rust-tests-slice.yml.github/workflows/ci-sdk-slice.yml.github/workflows/ci-ui-artifact-slice.yml.github/workflows/ci-web-slice.yml.github/workflows/ci-website-lane.yml.github/workflows/ci-windows-lane.yml.github/workflows/ci.yml.github/workflows/hf-download-smoke.yml.github/workflows/native-sdk-artifact.yml.github/workflows/pr_builds.yml.github/workflows/scripted-binary-smoke.yml.github/workflows/sdk-smoke.yml.github/workflows/smoke.yml.github/workflows/static-abi-artifact.yml.github/workflows/swift-sdk-artifact.yml.omo/specs/pr-ci-optimization.mdCONTRIBUTING.mdJustfileci/ci.mdscripts/tests/test_ci_artifact_actions.pyscripts/tests/test_ci_lane_workflows.pyscripts/tests/test_reusable_workflow_runner_trust.pyscripts/tests/test_sccache_evidence.pytools/xtask/src/installer_fixtures.rstools/xtask/src/workflow_checks.rs
🚧 Files skipped from review as they are similar to previous changes (21)
- tools/xtask/src/installer_fixtures.rs
- .github/workflows/ci-runner-contract-slice.yml
- .github/workflows/ci-macos-lane.yml
- .github/workflows/ci.yml
- .github/workflows/ci-website-lane.yml
- .github/workflows/ci-quality-lane.yml
- .github/workflows/pr_builds.yml
- .github/workflows/ci-quality-slice.yml
- .github/workflows/ci-linux-lane.yml
- .agents/skills/manage-ci/references/current-inventory.md
- .github/workflows/ci-sdk-slice.yml
- ci/ci.md
- .github/workflows/ci-runtime-product-slice.yml
- .github/workflows/ci-windows-lane.yml
- .github/workflows/ci-platform-checks-slice.yml
- tools/xtask/src/workflow_checks.rs
- scripts/tests/test_sccache_evidence.py
- .omo/specs/pr-ci-optimization.md
- scripts/tests/test_reusable_workflow_runner_trust.py
- scripts/tests/test_ci_artifact_actions.py
- scripts/tests/test_ci_lane_workflows.py
ndizazzo
force-pushed
the
codex/pr-ci-optimization
branch
from
August 13, 2026 00:14
eaaf33e to
bc0d28a
Compare
This was referenced Aug 13, 2026
This was referenced Aug 13, 2026
This was referenced Aug 20, 2026
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
1 participant
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
CI Requiredsignaling, and clean contributor/agent documentation.Why
The analyzed PR #1176 run demonstrated the current process problem: the old PR Builds run took 46m12s across 27 jobs, with queue p95 of 26m09s on arm64 and 16m23s on amd64. Several independent jobs spent 8–25 minutes waiting while duplicated workflows rebuilt overlapping inputs.
This change addresses the process before adding capacity: precise path/crate routing, bounded matrices, staged artifacts, dependency-aware slices, and one required summary.
Design
scripts/plan-ci.py+ci/ownership.yml+ci/slices.ymlproduce a versioned, schema-validated plan forpr-draft,pr-ready,main, andmanual-full..github/workflows/ci-orchestrator.ymlassembles the same typed slices for PRs and main builds.Depot boundary
PR jobs remain GitHub-hosted. No Depot runner, cache, ruleset, runner-group, or repository-setting change is included. The future migration gates are documented in
ci/DEPOT_MIGRATION.md, with cache isolation and poison-cache prevention as prerequisites.Validation
cargo fmt --all -- --checkcargo test -p xtaskrepo-consistencychecks for crate lists, publish crates, and release targetsorigin/mainSummary by CodeRabbit
New Features
Bug Fixes
Documentation