Repository navigation
ci: check transitive crate edges in check-crate-edges.sh (#407) - #408
Conversation
The gate inspected only direct normal dependencies (`cargo tree --depth 1`), so a forbidden crate reached through an intermediate one passed: hwp-render depending on hwpx would pull in hwp-convert via hwpx -> hwp-convert. Walk the full normal-dependency tree (`-e normal`, no depth limit) and dedupe the names before matching. The self-test now builds a throwaway three-crate workspace (edge-a -> edge-b -> edge-c) and requires the gate to fail `edge-a` against `edge-c`, since no internal crate reaches another only transitively today. AGENTS.md invariant 1 and REVIEW.md now say the edges are direct or transitive.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…407) The self-test's real-workspace check could never fire: under pipefail, `deps hwp-cli | grep -Ecx hwp-render | grep -qx 0` is false both when the count is 0 (grep -c exits 1) and when it is not (grep -qx 0 exits 1). deps() also discarded cargo's stderr and exit status, so a broken `cargo tree` (for example a missing manifest) produced an empty list and every edge read as "ok". deps() now returns cargo's failure with the error left on stderr, and a new has_dep() reports match, no match, or cargo failure separately. forbid() fails the gate on a cargo failure, and both self-test cases go through has_dep(), matching on a here-string so pipefail cannot flip the result.
The self-test checked has_dep() only, so forbid() setting fail=0 on a forbidden edge or on a cargo tree failure still passed. Run forbid() against the fixture for both cases and require fail=1. The hit case runs after has_dep() has shown cargo works on the fixture, so a cargo failure cannot satisfy it vacuously.
Summary
scripts/check-crate-edges.shnow walks the full normal-dependency tree instead of direct edges only, so a forbidden crate reached through an intermediate crate fails the gate (for example hwp-render -> hwpx -> hwp-convert).--self-testgains a transitive case, and AGENTS.md invariant 1 and REVIEW.md "Crate direction" now say the edges are direct or transitive.Closes #407
Changes
deps(): drop--depth 1, keep-e normal, dedupe names withsort -u; matching stays anchored on the dependency name. Amanifestvariable (defaultCargo.toml) lets the self-test point the same code at a fixture.cargo treefailure now fails the gate. Before,deps()discarded cargo's stderr and exit status, so a brokencargo tree(for example a missing manifest) gave an empty list and every edge read as "ok".deps()now returns cargo's status with the error left on stderr, andhas_dep()reports match / no match / cargo failure separately;forbid()printsFAIL: cargo tree failed for <crate>and fails the gate on the third.pipefail,deps hwp-cli | grep -Ecx hwp-render | grep -qx 0is false both when the count is 0 (grep -cexits 1) and when it is not (grep -qx 0exits 1). Both self-test cases now go throughhas_dep(), which matches on a here-string sopipefailcannot flip the result.forbid()against the fixture and requiresfail=1both for the forbidden edge (forbid edge-a edge-c) and for a cargo failure (forbid no-such-crate edge-c), so neither verdict arm can silently stop failing the gate. The edge case runs only afterhas_dephas shown cargo works on the fixture, so a cargo failure cannot satisfy it vacuously.--self-testtransitive case: builds a throwaway path-only workspace inmktemp -d(edge-a -> edge-b -> edge-c, no direct edge-a -> edge-c) and requireshas_dep edge-a edge-cto hit. No internal crate reaches another only transitively today, so a synthetic fixture is the only way to cover the case.Cargo.toml.Verification
bash scripts/check-crate-edges.shon this branch (main's crate graph):bash scripts/check-crate-edges.sh --self-test:shellcheck scripts/check-crate-edges.sh: clean.Mutants (each run on a copy of the script or a temporary edit, then reverted); every one must fail, and does:
no-such-crateinstead ofhwp-render--self-testFAIL: self-test could not see hwp-cli -> hwp-render, so the matcher is broken, exit 1manifest=Missing.tomlerror: manifest path ... does not existthenFAIL: cargo tree failed for <crate>, so its edges were not checkedfor all 5 crates,== crate-edges: FAILED ==, exit 1manifest=Missing.toml--self-testFAIL: self-test could not see hwp-cli -> hwp-render ..., exit 1--self-testFAIL: self-test missed the transitive edge edge-a -> edge-b -> edge-c, exit 1forbid()forbidden-edge arm setsfail=0--self-testFAIL: self-test: forbid() did not fail on a forbidden edge, exit 1forbid()cargo-failure arm setsfail=0--self-testFAIL: self-test: forbid() did not fail when cargo tree failed, exit 1--depth 1put back intodeps()--self-testFAIL: self-test missed the transitive edge edge-a -> edge-b -> edge-c, exit 1hwpx = { path = "../hwpx" }added tocrates/hwp-render/Cargo.toml(the #407 example)FAIL: hwp-render depends on hwp-convert, directly or transitively (AGENTS.md invariant 1),== crate-edges: FAILED ==, exit 1Against main's script, the same two breakages (sanity check pointed at
no-such-crate;--manifest-path Missing.tomlinjected into itscargo treecall) both exit 0 with== crate-edges self-test: OK ==and== crate-edges: OK ==respectively.bash scripts/check.sh(tail), run on the first commit; the follow-up commits change onlyscripts/check-crate-edges.sh, verified with the gate,--self-test, shellcheck and the mutants above:The 47 skips are local-only fixtures absent from a fresh worktree; this change touches no Rust code or fixture-backed test.
Checklist
scripts/check.shpasses (fmt -> clippy --all-targets -D warnings -> test; the same gates as CI)--self-testtransitive case)NAME.mdandNAME.ko.md) in the same commit: not applicable, no user-facing docs changeNotes
cargo treestill resolves the host target and default features, so an internal dependency behind acfg(target_os = ...)or a non-default feature would not be seen. Raise a follow-up if that should be covered (--target all,--all-features).🤖 Generated with Claude Code