fix(build-cli): parallelize tarball publish preflight

[Alex Villarreal (alexvy86)]

Description

ff_publishing invokes flub publish tarballs for internal-feed publishing. Each tarball previously performed a serial registry lookup before publishing, adding registry round-trip time for every package.

Run initial version checks with a bounded concurrency of 10 while preserving serial, dependency-ordered npm publish calls. Retry attempts still re-check the registry so a successful upload with a lost response is treated as already published.

AB#21577

Reviewer Guidance

The review process is outlined in the pull request guidelines.

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (553 lines, 2 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[Copilot]

🟡 Changes recommended

Address negative retry handling and duplicate package entries before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request parallelizes tarball registry preflight checks while preserving serial, dependency-ordered publishing.

Changes:

• Adds bounded concurrency for version checks.
• Rechecks registry state before retries.
• Retains serial npm publish execution.

File summaries

File	Description
build-tools/packages/build-cli/src/commands/publish/tarballs.ts	Implements concurrent preflight checks and retry handling.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ❌ Request Changes

0 Spicy, 1 Pungent, 2 Smelly

Findings

Sev	#	Area	File	What	Fix
🧄 Pungent	H1	Correctness	build-tools/packages/build-cli/src/commands/publish/tarballs.ts:349-353	preflightConcurrency ?? publishPreflightConcurrency only falls back to the default of 10 when preflightConcurrency is null/undefined. If a caller of the exported publishTarballsInOrder passes preflightConcurrency: 0 (a legitimate-looking numeric value, e.g. from a computed config or a future CLI flag mirroring the retry flag's min: 0 pattern), 0 is not nullish and is passed straight through to async.mapLimit(tarballs, 0, ...). The async library's mapLimit never starts any worker when the limit is 0, so the returned promise never resolves — the whole publish command hangs indefinitely instead of erroring, unlike the retry option which is explicitly validated (if (retry < 0) throw ...). This is a real deadlock risk since preflightConcurrency has no validation at all despite being part of the public PublishTarballsOptions API.	Validate preflightConcurrency similarly to retry (e.g. if (preflightConcurrency !== undefined && preflightConcurrency < 1) { throw new RangeError('preflightConcurrency must be at least 1'); }), or normalize with Math.max(1, preflightConcurrency ?? publishPreflightConcurrency) before passing it to async.mapLimit.
🧅 Smelly	M1	Testing	build-tools/packages/build-cli/src/commands/publish/tarballs.ts:438	isTarballPublished() (the real implementation that calls latestVersion() and interprets its result/errors) is never exercised by any test. All tests of publishTarballsInOrder inject a hand-written isPublished stub instead, so the actual empty-string/undefined check and the try/catch that swallows lookup errors and returns false have zero coverage. A regression here (e.g. inverting the boolean, or letting the catch rethrow) would ship undetected even though the orchestration logic around it is well tested.	Add a unit test that mocks the latest-version module (e.g. via esmock/proxyquire or dependency injection) and calls isTarballPublished directly: one case where latestVersion resolves to a version string (expect true), one where it resolves to ""/undefined (expect false), and one where latestVersion throws (expect the promise to resolve to false, not reject).
🧅 Smelly	M2	Testing	build-tools/packages/build-cli/src/commands/publish/tarballs.ts:222-247	The run() method's result-handling switch over publishTarballsInOrder's output (the SuccessfullyPublished/AlreadyPublished/Error branches that call this.info/this.error and compute the countText attempt suffix) is never invoked by any test — the only command-level test only checks the static retry flag config. A bug such as the case "Error" block falling through into default (no break/return after this.error(...)), or an off-by-one in countText, would not be caught.	Add a command-level test that runs PublishTarballCommand (via oclif's test harness, stubbing publishTarball/latestVersion/execa) for at least one success path and one error path, and asserts the logged messages, e.g. that Published <file> (attempt N/retry) is logged on a retried success and that a fatal, non-retryable error causes the command to exit non-zero without continuing to publish subsequent tarballs.

---

View workflow run

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed the deep-review feedback in the latest commit:

• Extracted the ordered lookup, preflight, publish, and retry orchestration behind injectable callbacks.
• Kept the concurrent upfront preflight, but now re-checks package availability after a failed publish before treating it as fatal. This preserves the optimization while restoring the unless already published contract for stale preflight races.
• Added deterministic coverage for concurrency limits, out-of-order preflight completion with serial publish order, first-occurrence deduplication, already-published skips, missing tarball rejection, retry exhaustion, and negative retry handling.

[Copilot]

🟡 Changes recommended

Publishing currently continues to dependent tarballs after an earlier tarball exhausts its retries.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed the latest feedback and failing checks in the newest commit:

• Stopped publish orchestration after the first unrecoverable tarball error.
• Added a regression test that verifies a later tarball is not published after the first tarball exhausts retries.
• Applied Biome formatting to the new test file, which should fix the repo-policy and build-tools format failures.

[Tony Murphy (anthony-murphy)]

Deep Review

Reviewed commit 2c5dd4f on 2026-09-11.

Readiness: 10/10 — READY

Ready for sign-off. Commit 2c5dd4f stops dependency-ordered publishing after the first unrecoverable error and adds focused regression coverage for that behavior.

Context for Reviewers

• async.mapLimit bounds only registry checks; the subsequent awaited for loop preserves serial, dependency-ordered publishing established in feat(build-tools): New command 'publish:tarballs' #20934.
• A fresh registry check after a failed publish preserves the lost-response recovery behavior repaired in fix(publish:tarballs): Fix unhandled exception when publish fails #23953.
• Ordered input is validated and deduplicated before side effects, retaining only the first occurrence of each lookup key.
• Retry validation covers both the oclif flag and direct callers of the exported publishTarballsInOrder helper.
• feat(build-tools): New command 'publish:tarballs' #20934 and fix(publish:tarballs): Fix unhandled exception when publish fails #23953 define the ordering and retry contracts most relevant to this review.

For human reviewer

• Needs human judgment — Confirm that a fixed preflight concurrency of 10 is appropriate for the internal registry.
• Needs human judgment — Confirm that collapsing duplicate order entries to their first occurrence is acceptable for all callers.
• Cannot be assessed by the pipeline — Validate latency improvement, registry load, throttling, and propagation behavior in the actual ff_publishing test-branch environment.

Review history (2 prior reviews)

• a757bcf 2026-09-11 · 5/10 — Not ready for sign-off.
• 3953827 2026-09-11 · 2/10 — Not ready for sign-off.

[Copilot]

🔵 Needs a closer look

The publish callback contract permits AlreadyPublished, but that result can incorrectly trigger additional publish attempts.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

build-tools/packages/build-cli/src/commands/publish/tarballs.ts:333

• publish is declared to return any PublishStatus, including AlreadyPublished, but this loop only stops for SuccessfullyPublished. With retries enabled, a valid AlreadyPublished result causes another publish attempt whenever the follow-up check returns false. Stop retrying for every non-error status (or narrow the callback's return type to exclude AlreadyPublished).

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Alex Villarreal (alexvy86)]

[Agent-generated]

Followed up on the remaining human-judgment items from the deep review:

• The current client publish lists contain 90 public, 92 internal-build, 7 internal-dev, and 99 internal-test packages. A preflight cap of 10 reduces the initial registry phase from up to 99 serialized requests to at most 10 concurrent requests over roughly 10 batches, while bounding registry load well below the build CLI's established package-operation default concurrency of 25 and the build-performance client default of 20.
• flub list client --no-private produced no duplicate entries for any of those four feed-specific order lists. De-duplicating defensively is therefore behavior-preserving for active callers; it only prevents an invalid/repeated input from attempting the same immutable package version twice. The prior implementation would attempt the duplicate and fail after the first publish.

I consider the fixed cap of 10 a conservative, appropriate starting point. The remaining validation is operational: measure the publishing stage in an ff_publishing test-branch run for latency, throttling, and registry propagation behavior.

[Alex Villarreal (alexvy86)]

[Agent-generated]

Addressed Copilot's closer-look finding in the latest commit. publishTarballsInOrder now stops after every non-error result, including AlreadyPublished, so it cannot issue an unnecessary retry. Added a focused regression test with retry: 2 that confirms an AlreadyPublished result makes exactly one publish attempt.

[Copilot]

🟢 Approval recommended

The implementation preserves publish ordering and retry behavior with comprehensive focused tests.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Joshua Smithrud (Josmithr)]

Didn't have time for a full review, but left a couple of quick comments. I can review more next week.

[github-actions]

Bundle size comparison

Base commit: could not be determined; will be reported when the comparison runs
Head commit: b283299eb1086dcf701aedd36574f3c63e28ea19

Pending — Build - client packages is running. Results will appear here when the build completes.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🟢 Approval recommended

The implementation preserves publishing invariants and includes focused regression coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation preserves publish ordering and retry behavior with comprehensive targeted tests.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced