Skip to content

fix(ci): publish in topological order so a mid-run failure cannot strand a package ahead of its deps - #47

Open
yakimoto wants to merge 1 commit into
fix/publish-version-guardfrom
fix/publish-topological-order
Open

fix(ci): publish in topological order so a mid-run failure cannot strand a package ahead of its deps#47
yakimoto wants to merge 1 commit into
fix/publish-version-guardfrom
fix/publish-topological-order

Conversation

@yakimoto

@yakimoto yakimoto commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #46. Stacked on #45 (base is fix/publish-version-guard, not main) — both change the same publish step, so basing this on main would conflict.

The defect

The publish loop iterates packages/*/, which globs alphabetically. That is not dependency order, and the loop runs under set -euo pipefail — so a failure partway through leaves every package before the failure point already live on npm.

Measured on this workspace:

workspace packages: 49
dependency cycles:  0
ALPHABETICAL VIOLATIONS (package sorts before its own dependency): 23
  @wave-av/adk (0) before dep @wave-av/kernel (22)
  @wave-av/audience (1) before dep @wave-av/core (12)
  … 12 packages sort ahead of @wave-av/core

npm does not validate that a dependency exists at publish time, and a published version can never be replaced. So an interrupted alphabetical run can leave a permanently-broken package sitting on latest.

The fix

Sort the already-validated publishable set topologically before executing it. Only edges to packages in this release constrain the order — a dependency already on the registry is satisfied regardless of when we publish, so it imposes nothing.

Nothing else changes: auth (OIDC, no token), permissions, the publish command, and all three existing guards from #45 (backwards-version, unresolved-dependency, tarball-size) run unmodified in the earlier validation pass. This only reorders a set that has already been approved.

Three deliberate hardening choices, since this is the last thing standing between a tag push and npm:

  • Fail closed on a cycle. No publish order is safe if one exists, so it errors and publishes nothing rather than picking arbitrarily.
  • Re-validate each path against ^packages/[a-z0-9][a-z0-9._-]*/$. The dirs come from the workflow's own glob, but this script builds a filesystem path from each one, and provenance is not a substitute for validation.
  • Null-prototype maps. Package names are used as object keys; a package named __proto__ would corrupt the lookup rather than merely being wrong.

The node program is a fixed single-quoted argv with input on stdin — no ${{ }} interpolation, and no package-controlled data is ever assembled into code.

Verification

CI cannot run — the whole org is under a GitHub billing lock ("The job was not started because your account is locked due to a billing issue."), which affects public repos too. So this was verified locally against the exact bytes extracted from the workflow file, not a copy:

TEST 1 — real workspace (all 49 packages)
  PASS  all 49 packages present (none dropped)
  PASS  0 ordering violations (was 23 alphabetically)
        first 6 published: kernel adk core audience autopilot billing
TEST 2 — dependency cycle must fail closed
  PASS  exit 1 — cycle rejected
  PASS  names the cycle: @wave-av/alpha -> @wave-av/beta -> @wave-av/alpha
TEST 3 — path outside packages/ must fail closed
  PASS  exit 1 — traversal path rejected
TEST 4 — partial release (core, adk, kernel only)
  PASS  kernel(1) before adk(2)

RESULT: 6 passed, 0 failed

This should not be merged until CI can actually run it. Branch protection is currently unenforced org-wide as a side effect of the same billing lock, so a green tick here would mean the gate vanished, not that the change passed.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Note

Medium Risk
Changes the publish gate ordering logic for all tag releases; mistakes could block publishes (cycles/empty sort) or still mis-order if dependency edges are wrong, but it does not weaken existing validation or auth.

Overview
The npm publish step now topologically sorts the already-validated publishable set before pnpm publish, instead of iterating packages/*/ in alphabetical order.

That matters because the loop runs under set -euo pipefail: a mid-run failure leaves earlier packages live on npm, and npm does not require dependencies to exist at publish time—so publishing a dependent before its workspace dep can strand a broken latest. Only @wave-av/ edges to packages in the same release constrain ordering; registry-only deps do not.

The sort is a fixed stdin-fed node -e block that re-validates paths (^packages/...), uses null-prototype maps, fails closed on dependency cycles, and rejects an empty sort when publishable is non-empty. Validation guards from the prior pass are unchanged; only the publish loop input switches from publishable to topo_sorted.

Reviewed by Cursor Bugbot for commit 1d1a4fe. Configure here.


Summary by cubic

Publish step now sorts packages topologically so dependencies are published first. This prevents a mid-run failure from leaving a package live ahead of its deps. No other behavior changes.

  • Bug Fixes
    • Compute a safe publish order from the publishable set; only edges to packages in this release constrain the order.
    • Fail closed on dependency cycles and print the cycle to the logs.
    • Re-validate package directory paths and use null‑prototype maps to avoid key pollution (e.g., __proto__).

Written for commit 1d1a4fe. Summary will update on new commits.

Review in cubic

@changeset-bot

changeset-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 1d1a4fe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_74b087d7-a5b8-4494-b0ea-39a67ad9cdf0)

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3de612fa-be84-4cf2-9772-a4f39cae8eaf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/publish-topological-order
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/publish-topological-order

Comment @coderabbitai help to get the list of available commands.

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.

1 participant