fix(ci): stop a diagnostic artifact upload from failing the gate - #104
Merged
Merged
Conversation
Two runs on main went red on 2026-09-08 with nothing wrong. Runs 34249684140
and 34250282010 passed unit tests, the coverage report, AND the
--fail-under-lines 70 gate, then failed on the Upload LCOV Artifact step:
##[error]Failed to FinalizeArtifact: Received non-retryable error:
Failed request: (403) Forbidden: Error from intermediary
The same step succeeded minutes later on run 34250438288 with the same
workflow, so this was a transient artifact-service fault, not a repo problem --
the repo holds 178 artifacts totalling well under any quota, and the names are
SHA-suffixed so they cannot collide.
The cost was not just a red badge. build-llamacpp declares `needs: rust-test`
and integration-tests declares `needs: build-llamacpp`, so both were skipped:
one flaky diagnostic upload suppressed the two most expensive real checks in
the workflow and then failed the gate on their absence.
Marked `continue-on-error: true`. Nothing downstream consumes this artifact --
it exists for humans reading coverage after the fact. Coverage enforcement is
untouched: the threshold is a separate step that still blocks.
Deliberately NOT applied to the DLL upload in the build job. That one is
load-bearing -- Integration Tests downloads it -- and it correctly carries
`if-no-files-found: error` for the same reason. The distinction is whether a
later job depends on the artifact, not whether uploads can be flaky.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t
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. |
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is narrowly scoped to a diagnostic-only artifact upload and preserves the existing coverage enforcement gate.
Pull request overview
This PR hardens the CI workflow against transient GitHub Actions artifact service failures by making the LCOV diagnostic artifact upload non-blocking, preventing unrelated upload flakiness from skipping downstream build/integration checks and failing the overall gate.
Changes:
- Mark the “Upload LCOV Artifact” step as
continue-on-error: truewhile keeping itif: always(). - Add detailed inline rationale documenting the observed 403 artifact-finalization flake and why only this upload is non-blocking.
File summaries
| File | Description |
|---|---|
| .github/workflows/ci.yml | Makes the LCOV artifact upload step non-blocking to avoid transient artifact upload failures breaking the CI gate and skipping downstream jobs. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review caught that the comment said `build/integration-test declare needs: rust-test`. Only build-llamacpp does. integration-tests declares `needs: [build-llamacpp, powershell-test]`, so it was skipped through build-llamacpp rather than directly. The conclusion is unchanged -- both jobs were skipped by the rust-test failure -- but the comment exists so someone can audit the job graph later, and a comment that misdescribes that graph fails at the one job it has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t
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
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.
Two runs on
mainwent red today with nothing actually wrong.Runs 34249684140 (#87 merge) and 34250282010 (#85 merge) passed unit tests, the coverage report, and the
--fail-under-lines 70gate — then failed here:The same step succeeded minutes later on run 34250438288 with the same workflow, so this was a transient artifact-service fault. Ruled out the repo-side explanations: 178 artifacts totalling well under any quota, and names are
${{ github.sha }}-suffixed so they cannot collide.The real cost was the cascade
needs:build-llamacpprust-testintegration-testsbuild-llamacppBoth were skipped. One flaky diagnostic upload suppressed the two most expensive real checks in the workflow, and then the gate failed on their absence. The step-level view of the failed job makes it stark:
Scope
continue-on-error: trueon the LCOV upload only. Nothing downstream consumes it — it exists for humans reading coverage after the fact. Coverage enforcement is untouched: the threshold is a separate step that still blocks.Deliberately not applied to the DLL upload in the build job. That one is load-bearing — Integration Tests downloads it — and it already carries
if-no-files-found: errorfor exactly that reason. The distinction is whether a later job depends on the artifact, not whether uploads can be flaky.The two red runs on
mainhave been re-run separately; this prevents the recurrence.🤖 Generated with Claude Code
https://claude.ai/code/session_01Mu7jB5ysKe4DN5ovjVtX1t