Skip to content

fix(release): validate crate publication order - #5306

Merged
Hmbown merged 2 commits into
mainfrom
codex/5298-crate-publish-order
Aug 9, 2026
Merged

fix(release): validate crate publication order#5306
Hmbown merged 2 commits into
mainfrom
codex/5298-crate-publish-order

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • validate the maintained 20-crate publication order against locked Cargo metadata before any registry operation
  • move codewhale-core before codewhale-tui and fail closed on duplicates, missing or extra crates, mixed versions, and normal/build dependency inversions
  • exercise the exact v0.9.5 regression plus the live workspace graph in CI
  • correct the release runbook inventory and dry-run contract

Verification

  • bash scripts/release/validate-crate-publish-order.test.sh
  • ./scripts/release/publish-crates.sh dry-run (20/20 crates)
  • cargo fmt --all -- --check
  • bash scripts/release/check-versions.sh
  • python3 scripts/check-source-structure-budget.py
  • shellcheck and bash syntax checks
  • git diff --check

Closes #5298

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@cursor

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@Hmbown Hmbown added this to the v0.9.6 milestone Aug 9, 2026
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 57s —— View job


Claude's Review

Todo list

  • Gather context (diff, changed files, repo guidance)
  • Review scripts/release/validate-crate-publish-order.py (new validator)
  • Review scripts/release/validate-crate-publish-order.test.sh (new tests)
  • Review scripts/release/publish-crates.sh changes
  • Review scripts/release/crates.sh, docs/RELEASE_RUNBOOK.md, .github/workflows/ci.yml
  • Post final review

Summary

This is a solid, well-scoped release-tooling fix: it replaces the inline bash heredoc-embedded Python (previously piped via process substitution, whose failures set -e could silently swallow) with a standalone, testable validator that raises a single ValidationError type and fails closed on duplicates/missing/extra crates/mixed versions/dependency-order inversions. Moving codewhale-core before codewhale-tui in crates.sh and adding a regression test that reproduces the exact v0.9.5 ordering bug is a good, targeted fix. Docs and CI wiring are consistent (crate count 18→20, codewhale-telemetry bullet added, new test added to ci.yml).

Findings

1. Correctness — latent unhandled KeyError (inline comment on validate-crate-publish-order.py:130-139)
workspace_by_name includes all workspace packages, but positions is keyed only by the codewhale--prefixed release_names. The dependency-name filter at line 131 only skips deps entirely outside the workspace, not in-workspace-but-non-codewhale--prefixed ones. If a release crate ever path-depends on such a crate (e.g. a future internal/xtask helper), positions[dependency] in the violations check raises an uncaught KeyError/traceback instead of the clean fail-closed ValidationError this script is built around. Not triggerable today — every current workspace member is codewhale--prefixed (confirmed against the workspace Cargo.toml) — but it undermines the "fail closed" contract for future workspace changes, and isn't covered by the test script.

2. Tests — coverage narrower than the PR description claims
The PR summary states the validator "fail[s] closed on duplicates, missing or extra crates, mixed versions, and normal/build dependency inversions," but validate-crate-publish-order.test.sh only exercises the ordering-inversion case (bad order / good order) plus a run against the live workspace graph. There's no dedicated test asserting the duplicate-crate, missing-crate, extra-crate, or mixed-version ValidationError paths, so regressions in those branches wouldn't be caught by CI.

Good choices

  • Consolidating the duplicated bash missing/extra-crate loops into the Python validator (removes ~40 lines of O(n²) bash logic) is a clean simplification.
  • Using a real command-substitution assignment (metadata_inventory="$(...)") instead of done < <(...) process substitution fixes a real gap: previously a validator failure inside the process substitution wasn't guaranteed to trip set -e.
  • --metadata-file for test injection avoids invoking cargo metadata in unit tests while still running the real thing against the live graph in the third test stage.

No provider/model/route-selection code is touched by this PR, so that invariant doesn't apply here.

Comment thread scripts/release/validate-crate-publish-order.py
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 37s —— View job


Claude's Review

Todo list

  • Gather context (new commit since last review, diff vs origin/main)
  • Check whether the previously flagged KeyError issue was addressed
  • Review new test coverage commit (test(release): cover invalid crate inventories)
  • Post final review

Summary

The new commit (5763000, test(release): cover invalid crate inventories) directly resolves both findings from the prior review pass. Diffing it against the previous commit (4a69337) shows a 5-line fix plus ~100 lines of new test coverage, touching only validate-crate-publish-order.py and validate-crate-publish-order.test.sh.

Previous findings — now resolved

1. Latent KeyError (previously flagged at validate-crate-publish-order.py:144) — fixed.
scripts/release/validate-crate-publish-order.py:139-143 now checks if dependency_name not in positions before the publish_edges.add(...) / later positions[dependency] lookup, and raises a proper ValidationError:

if dependency_name not in positions:
    raise ValidationError(
        f"{dependent} depends on workspace crate {dependency_name} "
        f"[{kind}], which is not in the codewhale-* release inventory"
    )

This closes the gap between workspace_by_name (all workspace packages) and positions (only codewhale-* release crates) — exactly the scenario described in the inline comment. Traced through by hand against the new internal-helper fixture: codewhale-core depending on a non-codewhale--prefixed workspace crate now raises a clean, fail-closed ValidationError instead of an uncaught traceback.

2. Narrow test coverage — fixed.
validate-crate-publish-order.test.sh now adds dedicated expect_validation_failure cases for:

  • duplicate crate in the publish list (duplicate-crate)
  • missing workspace crate (missing-crate)
  • extra/non-workspace crate (extra-crate)
  • mixed package versions (mixed-versions)
  • a release crate depending on a non-release workspace crate (nonrelease-workspace-dependency — the regression test for finding Fix cargo fmt formatting issues #1 above)

Each case also asserts no Traceback appears in output, which is a nice touch for asserting fail-closed behavior specifically (not just "some error occurred").

I traced the fixtures against validate_order() by hand (order of checks: duplicates → name sanity → mixed versions → missing/extra → dependency-order/positions) and each new case exercises the intended branch without being masked by an earlier check — e.g. the mixed-versions fixture uses an otherwise-valid crate list so it isn't caught by the missing/extra check first, and nonrelease-workspace-dependency correctly reaches the new guard since internal-helper isn't codewhale--prefixed. I wasn't able to execute the test scripts directly in this environment (Bash execution requires approval that isn't available here), so this is a static/manual trace rather than an actual run — worth a final CI check before merge, though the logic reads correct.

No new issues found in this delta. The rest of the PR (crates.sh reorder, CI wiring, runbook docs, publish-crates.sh simplification) is unchanged since the previous review pass and remains solid.
· branch codex/5298-crate-publish-order

@Hmbown
Hmbown merged commit fde3cb0 into main Aug 9, 2026
35 of 36 checks passed
@Hmbown
Hmbown deleted the codex/5298-crate-publish-order branch August 9, 2026 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

release: topologically validate Cargo crate publication order

1 participant