fix(ci): fail required checks when dependency locks fail - #3780
fix(ci): fail required checks when dependency locks fail#3780Hughhhhcoder wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
One narrow hardening suggestion after comparing this with #3756 and reproducing the dependency-result behavior in GitHub Actions:
The current patch still addresses the original skipped-required-check failure mode; these points are about tightening failure ordering and cancellation semantics. |
|
Thanks for the focused hardening suggestions. I applied them in commit
Validation:
The full @codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd930546fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !cancelled() && | ||
| (github.event_name == 'push' || github.event_name == 'merge_group' || github.event.pull_request.head.repo.fork) |
There was a problem hiding this comment.
For any non-fork pull_request, this condition remains false even when needs.dependency-locks.result == 'failure', so the job never reaches the new failure step and GitHub reports the required lint/build/test job as skipped. This is especially consequential for the dependency job's PR-only Dependabot security-floor check: that check can fail while every required downstream context still satisfies branch protection. Include the dependency-failure case in the outer condition for all four jobs and cover a non-fork PR in the regression test.
AGENTS.md reference: AGENTS.md:L41-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 238b68e. The outer condition for all four required jobs (lint, build, test, and test-httpx2) now also includes needs.dependency-locks.result == 'failure'. This makes a same-repository pull request, where head.repo.fork is false, enter the job when dependency-lock validation fails; the first step then fails the required check before checkout. The workflow regression test now asserts this guard and the non-fork failure case. Validation passed: direct execution of test_required_ci_checks_fail_when_dependency_locks_fail, Ruff check/format, YAML parsing, and git diff --check. The full pytest command remains unavailable locally because the repository conftest.py requires the missing httpx2 module.
|
Validation note correction for clarity: I reran the workflow test suite and isolated the first failure. The failing test is unrelated to this patch: the local uv is 0.6.14, while CONTRIBUTING.md requires uv >=0.12.1, and uv 0.6.14 rejects the test command combination --frozen --dry-run before the dependency assertion runs. The xdist run had 170 tests passed before reporting that environment failure; I stopped the remaining workers. No repository files were changed by the test run. The focused regression test still passes (1 passed), and the YAML parse, Ruff check, Ruff format check, and git diff --check validations pass. I updated the PR description to record this accurately. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Follow-up hardening from the earlier review: the current head Proposed minimal alignment for each of if: >-
!cancelled() &&
(needs.dependency-locks.result != 'success' ||
github.event_name == 'push' ||
github.event_name == 'merge_group' ||
github.event.pull_request.head.repo.fork)The existing first executable guard can remain unchanged. This keeps |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
P1 remains on the current candidate: the job-level predicate for lint/build/test/test-httpx2 still excludes non-fork pull requests when dependency-locks fails. The early needs.dependency-locks.result != 'success' step cannot run if the job itself is skipped. Please include the dependency result in the job-level condition for all four required jobs, e.g. !cancelled() && (github.event_name == 'push' || github.event_name == 'merge_group' || github.event.pull_request.head.repo.fork || needs.dependency-locks.result != 'success'). Keep the early guard before checkout. This is required to actually close the same-repo failure path described by the P1 review.
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Correction to my previous CHANGES_REQUESTED review: I re-read the current head 238b68e346 and the same-repository dependency-locks == 'failure' path is already included in the job-level predicate for the four required jobs, so that P1 blocker was stale/incorrect. I withdraw it as a blocker.
The remaining != 'success' observation is only a non-blocking semantic-hardening suggestion for other non-success dependency states such as skipped; it is not required to close the reported failure path. Sorry for the noise.
|
Addressed in commit |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Thanks for the correction. I rechecked the current head ( |
2 participants
Changes being requested
When the required
lint,build,test, andtest-httpx2jobs depend on a faileddependency-locksjob, GitHub Actions skips them. If those skipped jobs are branch-protection requirements, the pull request can appear to satisfy the required checks even though dependency validation failed.Reproduction:
dependency-locksfail on a push or fork pull request.Expected behavior: Required checks remain visible and fail when dependency-lock validation fails.
Actual behavior: The downstream required checks are skipped, which can let branch rules treat the workflow as successful.
Root cause: A failed job in
needsimplicitly skips dependent jobs unless their job-level condition includes a status function such asalways().This change keeps the existing event filter, makes the four required jobs run after dependency-lock failure, and adds an early guard step that exits with failure before installing dependencies or executing the regular job. A static regression test protects both the job guards and the failure steps.
Additional context & links
Fixes #3755
Validation:
ruby -e "require %q(yaml); YAML.load_file(%q(.github/workflows/ci.yml))": passed.venv/bin/pytest tests/test_uv_workflows.py::test_required_ci_checks_fail_when_dependency_locks_fail -q: 1 passed.venv/bin/ruff check tests/test_uv_workflows.py: passed.venv/bin/ruff format --check tests/test_uv_workflows.py: passedgit diff --check origin/main...HEAD: passedtests/test_uv_workflows.py: not claimed. The first unrelated local failure istest_editable_project_sync_requires_only_the_reviewed_root_build_exemption: the installed uv is0.6.14, whileCONTRIBUTING.mdrequires uv>=0.12.1; uv 0.6.14 rejects the test command combination--frozen --dry-runbefore the dependency assertion runs.