Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues remain.
Review effort: Lite
Findings: None
What changed in this PR
Updates PR automation to compare public APIs using rustdoc artifacts generated with each revision’s locked dependencies.
Changes:
- Builds baseline and head rustdoc JSON with
cargo rustdoc --locked. - Compares artifacts using the pinned semver checker.
- Documents the locked-dependency workflow.
| File | Summary |
|---|---|
.github/workflows/pr-automation.yml |
Builds and compares locked rustdoc artifacts. |
.github/PR_AUTOMATION.md |
Documents the updated compatibility check. |
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
|
This pull request may overlap with #2103. Both target the pr-automation CI subsystem: this PR reworks the cargo-semver-checks job in .github/workflows/pr-automation.yml (and its doc), while This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
CI-only change to the pull_request_target pr-automation workflow: the semver job now builds rustdoc JSON for baseline and head with `cargo rustdoc --locked` in an inner bash script run as nobody under `env -i`, then compares the two artifacts with the existing checksum-pinned cargo-semver-checks binary. This fixes real failures caused by the comparator's placeholder crate resolving a fresh (unlocked) dependency graph, while preserving the unprivileged boundary, cleared environment, feature selection, and exit-code handling (0/100 mapping). The accompanying PR_AUTOMATION.md note documents the locked-baseline behavior. The change is correct and safe: SHA/feature values flow through env passthrough rather than interpolation into the script text, `RUSTC_BOOTSTRAP=1` is required for the unstable rustdoc flags on the stable toolchain, and per-revision `CARGO_TARGET_DIR` values keep artifacts separated with paths matching what the comparator is given. The single accepted candidate is a low-s…
2455b39 to
c6cdee2
Compare
Build each revision through a small helper called once per SHA and pass the artifact root explicitly instead of capturing CARGO_TARGET_DIR before overwriting it.
There was a problem hiding this comment.
The PR reworks the semver step of the pull_request_target PR automation workflow to build baseline and head rustdoc JSON itself via `cargo rustdoc --locked`, then compares those artifacts with the pinned cargo-semver-checks binary using --baseline-rustdoc/--current-rustdoc. This is a correct and well-motivated change: cargo-semver-checks 0.51.0 has no lockfile option, so its --baseline-rev placeholder-crate mode re-resolves dependency graphs from the registry and can fail the API check on unrelated registry patch releases (the picky-krb 0.12.5 incident). Building with --locked aligns the API check with ordinary CI's dependency versions and makes reruns reproducible. The sandbox boundary is preserved: the `sudo -u nobody env -i` invocation still lists every variable explicitly, SHAs arrive only through the environment, and the comparator's 0/100/101 exit-code handling is unchanged. The PR_AUTOMATION.md doc is updated consistently. I verified the sandboxed inner script lines against the…
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
The inner script runs under env -i, so its inputs still have to be listed explicitly, but they no longer get new aliases.
There was a problem hiding this comment.
The PR replaces cargo-semver-checks' --baseline-rev mode, which resolves a fresh dependency graph per revision and ignores Cargo.lock, with manual rustdoc JSON builds: an inner sandboxed bash script checks out BASE_SHA then HEAD_SHA and runs 'cargo rustdoc --locked' into per-revision target dirs, then passes the two JSON files to the pinned, checksummed comparator via --baseline-rustdoc/--current-rustdoc. Exit-code propagation is intact (cargo 101 and comparator 100 both reach the existing 0/100/other case mapping through sudo and bash -e), the nobody/env -i sandbox and SHA handling are unchanged, the required CARGO_SEMVER_CHECKS_DIR/CARGO_SEMVER_CHECKS_FEATURES variables are explicitly forwarded under the cleared environment, the --no-default-features --features invocation matches the old --only-explicit-features semantics, the rustdoc JSON output paths match the comparator arguments, and the PR_AUTOMATION.md addition accurately documents the new behavior. The change makes the API ch…
Reduced coverage: optional reviewer code-compressor was unavailable.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| build_rustdoc() { | ||
| git checkout --detach "$2" | ||
| CARGO_TARGET_DIR="$SEMVER_ARTIFACT_ROOT/$1" cargo rustdoc --locked \ | ||
| --package ironrdp --lib --no-default-features --features "$CARGO_SEMVER_CHECKS_FEATURES" | ||
| } | ||
| build_rustdoc baseline "$BASE_SHA" | ||
| build_rustdoc current "$HEAD_SHA" |
There was a problem hiding this comment.
[general] Shared workspace is not cleaned between baseline and head rustdoc builds — low 🟡 — build_rustdoc switches revisions with 'git checkout --detach' only, which removes/updates tracked files but leaves untracked files in place. Any artifact the baseline build writes into the source tree (possible via build scripts) survives into the head build: if the path collides with a file tracked at HEAD_SHA, the head checkout fails and the API check spuriously fails; otherwise stray files can influence the head build's inputs. Adding 'git clean -xdf' before each checkout, or using separate worktrees/copies per revision, removes the risk. Low severity: cargo normally confines outputs to CARGO_TARGET_DIR (kept outside the workspace) and the failure mode is a flaky check, not a security issue.
There was a problem hiding this comment.
Fixed in 581c546: build_rustdoc now runs git clean -xdfq before each checkout, so nothing the baseline build writes into the source tree reaches the head build. The target directories live under $SEMVER_STATE_DIR, outside the workspace, so the clean does not touch them.
Untracked files written into the source tree by the baseline build could collide with files tracked at the head revision or leak into its build.
Build baseline and head rustdoc JSON with
cargo rustdoc --locked, then compare those artifacts with the existing pinned cargo-semver-checks binary. Each revision keeps its own committed dependency versions. The unprivilegednobodyexecution, cleared environment, tool checksum, feature selection, and compatibility exit-code handling are unchanged.Why
The original trigger is gone:
picky-krb 0.12.5added aGssApiMessageErrorvariant that brokesspi 0.21.3, which made the API check fail for every open PR while ordinary CI passed on the lockfile's0.12.4. #2074 capped it on master and 0.12.5 was later yanked (re-released as 0.13.0).The cause is still there. With
--baseline-rev, cargo-semver-checks builds each revision in a placeholder crate and resolves a fresh dependency graph from the registry, ignoringCargo.lock. Any future patch release in the dependency tree that breaks the build therefore fails the API check for all PRs until someone caps or pins it on master, and because this is apull_request_targetworkflow the repair only takes effect after it lands on the base branch. Building with--lockedmakes the API check use the same dependency versions that ordinary CI builds and tests, and makes reruns of the same SHAs reproducible. cargo-semver-checks 0.51.0 has no lockfile option, so building the rustdoc JSON ourselves and passing--baseline-rustdoc/--current-rustdocis the way to get this.A stale or inconsistent lockfile now fails the job (exit 101) instead of being silently re-resolved.
Validation
actionlint1.7.7 (with shellcheck 0.10.0 onrun:blocks): no new findings; the remaining findings are identical on master (concurrency.queueandnode24are newer than actionlint knows).bash -cscript: clean.git/cargo/comparator: checks out base then head, builds into<root>/baselineand<root>/current, passes those JSON paths to the comparator, and propagates comparator 0/100 and cargo 101 exit codes.node --test .github/pr-automation/automation.test.js: 187/187 pass.cargo rustdoc --lockedbuild with the full workflow feature set passed and the pinned comparator parsed and compared the JSON; fixtures showed unchanged API exits 0, a removed root function exits 100, and a stale lockfile exits 101 without modifying the lockfile. Thenobodyboundary was inspected, not executed locally.The comparator's existing inability to detect some changes through dependency re-exports reproduces with both its placeholder mode and this JSON mode; that limitation is unchanged.