fix(ci): handle cargo-public-api JSON format compatibility#74
Open
EffortlessSteven wants to merge 1 commit intomainfrom
Open
fix(ci): handle cargo-public-api JSON format compatibility#74EffortlessSteven wants to merge 1 commit intomainfrom
EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
The Public API Guard CI step fails with 'invalid type: integer' when cargo-public-api can't parse newer nightly rustdoc JSON format. Changes: - Remove pinned cargo-public-api v0.38.0; install latest version instead - Use nightly toolchain for rustdoc JSON generation (required by cargo-public-api) - Emit GitHub Actions warnings instead of hard failures on format mismatches - Remove redundant retry steps (nightly is now used by default) - Make xtask verify_public_api detect JSON format errors and treat as warnings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
Review Summary by QodoHandle cargo-public-api JSON format compatibility gracefully
WalkthroughsDescription• Handle rustdoc JSON format incompatibility gracefully in public API checks • Remove pinned cargo-public-api version; install latest instead • Use nightly toolchain for rustdoc JSON generation by default • Detect and treat JSON format errors as warnings, not failures • Simplify CI workflow by eliminating redundant retry logic Diagramflowchart LR
A["CI Workflow"] -->|"Install latest cargo-public-api"| B["cargo-public-api"]
A -->|"Use nightly toolchain"| C["rustdoc JSON generation"]
B -->|"Detect format errors"| D["xtask validate"]
D -->|"Format mismatch"| E["Emit warning, skip check"]
D -->|"API change"| F["Hard failure"]
E -->|"Continue CI"| G["Build succeeds"]
F -->|"Block merge"| H["Build fails"]
File Changes1. xtask/src/validate.rs
|
Code Review by Qodo
1. API guard errors swallowed
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
552
to
+574
| - name: Check flight-core public API | ||
| if: needs.path-filter.outputs.core == 'true' | ||
| id: check-core-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-core diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-core API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.core == 'true' && steps.check-core-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-core public API check failed (possible rustdoc JSON format mismatch)" | ||
| } | ||
|
|
||
| - name: Check flight-ipc public API | ||
| if: needs.path-filter.outputs.ipc == 'true' | ||
| id: check-ipc-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-ipc diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-ipc API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.ipc == 'true' && steps.check-ipc-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-ipc diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-ipc diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-ipc public API check failed (possible rustdoc JSON format mismatch)" | ||
| } | ||
|
|
||
| - name: Check flight-hid public API | ||
| if: needs.path-filter.outputs.hid == 'true' | ||
| id: check-hid-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-hid diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-hid API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.hid == 'true' && steps.check-hid-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-hid diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-hid diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-hid public API check failed (possible rustdoc JSON format mismatch)" | ||
| } |
There was a problem hiding this comment.
1. Api guard errors swallowed 🐞 Bug ✓ Correctness
The CI public API steps unconditionally convert any cargo public-api ... diff failure into a success, so real API drift (or other errors) won’t be reflected as a failing step/check. This undermines the job’s stated purpose of detecting unintended public API changes.
Agent Prompt
### Issue description
CI’s `public-api-check` currently masks *all* failures from `cargo public-api ... diff` as a successful step by using `|| { echo "::warning::..." }`. This makes genuine API drift indistinguishable from rustdoc JSON format incompatibilities.
### Issue Context
The PR intent is to treat known rustdoc JSON format mismatches as warnings, not to suppress true API-diff failures.
### Fix Focus Areas
- .github/workflows/ci.yml[552-574]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+545
to
567
| - name: Install cargo-public-api (latest) | ||
| run: | | ||
| if ! command -v cargo-public-api &> /dev/null; then | ||
| cargo install --locked cargo-public-api@=0.38.0 | ||
| else | ||
| echo "cargo-public-api already installed" | ||
| fi | ||
| cargo install --locked cargo-public-api || { | ||
| echo "::warning::Failed to install cargo-public-api — skipping public API checks" | ||
| exit 0 | ||
| } | ||
|
|
||
| - name: Check flight-core public API | ||
| if: needs.path-filter.outputs.core == 'true' | ||
| id: check-core-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-core diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-core API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.core == 'true' && steps.check-core-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-core public API check failed (possible rustdoc JSON format mismatch)" | ||
| } | ||
|
|
||
| - name: Check flight-ipc public API | ||
| if: needs.path-filter.outputs.ipc == 'true' | ||
| id: check-ipc-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-ipc diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-ipc API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.ipc == 'true' && steps.check-ipc-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-ipc diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-ipc diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-ipc public API check failed (possible rustdoc JSON format mismatch)" | ||
| } | ||
|
|
There was a problem hiding this comment.
2. Install failure not gated 🐞 Bug ⛯ Reliability
If cargo-public-api fails to install, the workflow exits 0 from the install step but still runs the subsequent checks, which then fail for the wrong reason and emit misleading “format mismatch” warnings. This can fully bypass the guard while producing confusing output.
Agent Prompt
### Issue description
When `cargo install cargo-public-api` fails, later public API steps still run and produce misleading warnings.
### Issue Context
The PR intends to “gracefully install failure — emit warning and skip”. Currently it only exits 0 from the install step, not from the rest of the job.
### Fix Focus Areas
- .github/workflows/ci.yml[545-550]
- .github/workflows/ci.yml[552-574]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Updates the Public API Guard CI and xtask validation to be resilient to rustdoc JSON format changes that can break cargo-public-api, degrading to warnings/skips instead of failing CI.
Changes:
- CI: install latest
cargo-public-api, use nightly for checks, and emit GitHub Actions warnings instead of hard failures. - xtask: detect rustdoc JSON format mismatch errors in
cargo public-apistderr and treat them as warnings (Ok(false)). - CI: remove stable→nightly retry logic by defaulting checks to nightly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
xtask/src/validate.rs |
Detects rustdoc JSON incompatibility in cargo-public-api runs and downgrades those failures to warnings/skips. |
.github/workflows/ci.yml |
Switches Public API Guard to nightly + unpinned cargo-public-api and warns instead of failing on incompatibilities/installation issues. |
Comment on lines
+545
to
+558
| - name: Install cargo-public-api (latest) | ||
| run: | | ||
| if ! command -v cargo-public-api &> /dev/null; then | ||
| cargo install --locked cargo-public-api@=0.38.0 | ||
| else | ||
| echo "cargo-public-api already installed" | ||
| fi | ||
| cargo install --locked cargo-public-api || { | ||
| echo "::warning::Failed to install cargo-public-api — skipping public API checks" | ||
| exit 0 | ||
| } | ||
|
|
||
| - name: Check flight-core public API | ||
| if: needs.path-filter.outputs.core == 'true' | ||
| id: check-core-api | ||
| continue-on-error: true | ||
| run: cargo public-api -p flight-core diff origin/main..HEAD | ||
|
|
||
| - name: Retry flight-core API check with nightly (if needed) | ||
| if: needs.path-filter.outputs.core == 'true' && steps.check-core-api.outcome == 'failure' | ||
| run: | | ||
| echo "Retrying with nightly toolchain..." | ||
| rustup toolchain install nightly | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD | ||
| cargo +nightly public-api -p flight-core diff origin/main..HEAD 2>&1 || { | ||
| echo "::warning::flight-core public API check failed (possible rustdoc JSON format mismatch)" | ||
| } |
There was a problem hiding this comment.
If cargo install fails, the job continues but the subsequent check steps still run and will likely fail with no such command: public-api (or similar). That failure then gets reported as a possible rustdoc JSON mismatch, which is misleading. Consider setting a step output (e.g., installed=true/false) or adding a command -v cargo-public-api guard and using it in if: to skip the check steps when the install did not succeed.
Comment on lines
+545
to
+550
| - name: Install cargo-public-api (latest) | ||
| run: | | ||
| if ! command -v cargo-public-api &> /dev/null; then | ||
| cargo install --locked cargo-public-api@=0.38.0 | ||
| else | ||
| echo "cargo-public-api already installed" | ||
| fi | ||
| cargo install --locked cargo-public-api || { | ||
| echo "::warning::Failed to install cargo-public-api — skipping public API checks" | ||
| exit 0 | ||
| } |
There was a problem hiding this comment.
Installing the latest cargo-public-api on every CI run without a version pin reduces reproducibility and increases supply-chain exposure (the behavior of this job can change without any repo change). If possible, pin to a known-good version (and bump intentionally), or at least centralize the version in an env var so updates are explicit and reviewable.
Comment on lines
+494
to
500
| let output = Command::new("cargo") | ||
| .args(["public-api", "-p", crate_name]) | ||
| .status() | ||
| .output() | ||
| .context(format!( | ||
| "Failed to execute cargo public-api for {}", | ||
| crate_name | ||
| ))?; |
There was a problem hiding this comment.
Switching from .status() to .output() captures both stdout and stderr into memory. cargo public-api output can be large, so this can increase peak memory usage unnecessarily (especially in workspaces). Since you only need stderr for format detection, consider discarding stdout (e.g., configure stdout to null) and only piping/capturing stderr.
Comment on lines
+504
to
+507
| if stderr.contains("invalid type: integer") | ||
| || stderr.contains("rustdoc JSON") | ||
| || stderr.contains("format version") | ||
| { |
There was a problem hiding this comment.
The matching for format incompatibility is quite broad (\"rustdoc JSON\" / \"format version\") and may accidentally classify unrelated failures as a format mismatch, which would downgrade real failures into warnings. Consider tightening the match to more specific phrases produced by cargo-public-api/rustdoc JSON parsing errors (or using a small regex set that targets known error lines), to reduce the chance of false positives.
Suggested change
| if stderr.contains("invalid type: integer") | |
| || stderr.contains("rustdoc JSON") | |
| || stderr.contains("format version") | |
| { | |
| let is_format_mismatch = | |
| stderr.contains("invalid type: integer") | |
| || stderr.contains("unsupported rustdoc JSON format version") | |
| || stderr.contains("unsupported rustdoc JSON format") | |
| || stderr.contains("failed to parse rustdoc JSON"); | |
| if is_format_mismatch { |
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.
Problem
The Public API Guard CI job fails with \invalid type: integer 4370\ when \cargo-public-api\ v0.38.0 cannot parse newer nightly rustdoc JSON format versions.
Changes
CI workflow (.github/workflows/ci.yml)
xtask (\xtask/src/validate.rs)
ustdoc JSON, \ ormat version)