Skip to content

Keep npm installs of kcap complete and within the server's version - #892

Merged
alexeyzimarev merged 4 commits into
mainfrom
fix/npm-install-race
Sep 11, 2026
Merged

alexeyzimarev merged 4 commits into
mainfrom
fix/npm-install-race

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Closes #891 — AI-2708

What & why

Release 1.0.2's desktop publish failed, and kcap update to 1.0.x left macOS installs without their binary. npm publish returns while the registry is still processing a large tarball (darwin-arm64 took ~10 min), so the wrapper became installable first and npm silently skipped the unresolvable optional platform package. The release now waits for every platform package before publishing the wrapper, and every npm poll and install passes --prefer-online to bypass npm's 5-minute packument cache. The update hint now always says kcap update: --check pins install_tag to a trailing server's version.

Where to look

kcap update on a CLI already at its server's version reports "Already up to date" even when npm has newer — deliberate, matching the hint's cap.

Verification

  • bash scripts/run-shell-tests.sh → all ok; a copy of the wait script without --prefer-online fails all 4 cases
  • live wait-npm-available.sh 1.0.2 <six packages> → all available; 99.0.0 → npm error 404 No match found for version 99.0.0
  • CLI unit suite: 4042 passed, 19 skipped, 0 failed; UpdateNoticeDeliveryTests 12/12
  • dotnet publish -c Release: no IL2xxx/IL3xxx warnings

🤖 Generated with Claude Code

npm publish returns while the registry still processes the tarball, and npm skips an optional dependency it cannot resolve, so the wrapper must never be installable first. kcap update now pins install_tag to a trailing server's version, which lets the hint name it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-11T14:37:45.638946Z 9ed4ce4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Prevent incomplete npm installs and cap updates to server versions

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Waits for every platform package before publishing the npm wrapper.
• Bypasses npm cache when polling or installing recently published packages.
• Caps stable updates and hints to the connected server's supported version.
Diagram

graph TD
  Release["Release Workflow"] --> Platform["Platform Publish"] --> Registry[("npm Registry")] --> Wait["Availability Poll"] --> Wrapper["Wrapper Publish"]
  CLI["Update Check"] --> Advisory["Server-Capped Advisory"] --> Launcher["npm Launcher"] --> Registry
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Verify binaries after installation
  • ➕ Detects missing optional platform packages on affected installations
  • ➕ Could retry only when an incomplete installation occurs
  • ➖ Fails after npm has already produced a broken global installation
  • ➖ Would require reliable install-time repair despite blocked lifecycle scripts
  • ➖ Does not protect downstream consumers that invoke npm directly
2. Poll the registry HTTP API directly
  • ➕ Avoids dependence on npm CLI cache semantics
  • ➕ Could provide precise control over retry and timeout behavior
  • ➖ Duplicates npm registry authentication and response handling
  • ➖ Must distinguish metadata visibility from actual npm installability
  • ➖ Adds custom networking logic to release automation
3. Publish wrapper after a fixed delay
  • ➕ Simpler release workflow
  • ➕ Requires no polling utility
  • ➖ Cannot account for variable registry processing times
  • ➖ Either remains race-prone or adds excessive release latency
  • ➖ Provides poor failure diagnostics

Recommendation: Keep the PR's availability polling and --prefer-online strategy. It tests package resolution through the same npm tooling consumers use, waits only as long as registry processing requires, and fails releases before an incomplete wrapper becomes installable. Server-derived exact-version pins are also preferable to duplicating cap logic in the JavaScript launcher.

Files changed (14) +249 / -61

Bug fix (3) +40 / -39
kcap.jsRevalidate npm metadata for installs +12/-6

Revalidate npm metadata for installs

• Centralizes npm install arguments with '--prefer-online' for updates and recovery guidance. Supports exact versions supplied by server-capped update checks.

npm/kcap/bin/kcap.js

UpdateCommand.csApply server caps to update checks and installs +23/-22

Apply server caps to update checks and installs

• Resolves update checks through the shared advisory, emits exact server versions as 'install_tag' when capped, and uses the effective target for status and direct-install guidance.

src/Capacitor.Cli/Commands/UpdateCommand.cs

UpdateNotice.csAlways recommend kcap update in notices +5/-11

Always recommend kcap update in notices

• Simplifies capped notices to retain the server-version marker while consistently recommending 'kcap update', which now honors the same cap.

src/Capacitor.Cli/UpdateNotice.cs

Tests (4) +139 / -0
kcap.test.jsTest pinned and online npm install specifications +9/-0

Test pinned and online npm install specifications

• Covers exact server-version install specs, verifies '--prefer-online', and guards update execution through the centralized argument builder.

npm/kcap/bin/kcap.test.js

wait-npm-available.test.shTest npm availability polling behavior +48/-0

Test npm availability polling behavior

• Uses a fake npm executable to cover immediate availability, retries, multiple packages, cache bypassing, and deadline diagnostics.

scripts/wait-npm-available.test.sh

UpdateNoticeDeliveryTests.csVerify capped notices recommend kcap update +24/-0

Verify capped notices recommend kcap update

• Adds integration coverage proving a trailing server's version appears in the notice without exposing a direct npm install command.

test/Capacitor.Cli.Tests.Integration/UpdateNoticeDeliveryTests.cs

UpdateCommandCheckJsonTests.csTest server-capped update-check JSON +58/-0

Test server-capped update-check JSON

• Covers exact server pins, already-current CLIs, uncapped unknown-server and beta cases, and nullable 'newer' results when npm data is unavailable.

test/Capacitor.Cli.Tests.Unit/UpdateCommandCheckJsonTests.cs

Documentation (4) +32 / -11
README.mdDocument stable update server-version limits +3/-1

Document stable update server-version limits

• Explains that stable-channel updates stop at the connected server's version when npm is ahead, preventing unsupported CLI upgrades.

README.md

CHANGES.mdRecord npm availability and update-cap decisions +19/-0

Record npm availability and update-cap decisions

• Documents the npm publication race, online cache revalidation, shared availability polling, and server-capped update contract.

docs/CHANGES.md

help-update.txtAlign update help with server-capped behavior +7/-8

Align update help with server-capped behavior

• Clarifies that stable update commands, notices, and status output all target the connected server's supported version while beta remains uncapped.

src/Capacitor.Cli.Core/Resources/help-update.txt

UpdateAdvisory.csExtend advisory contract to active updates +3/-2

Extend advisory contract to active updates

• Updates the advisory documentation to establish that 'kcap update', notices, and status all share the server-capped target.

src/Capacitor.Cli/UpdateAdvisory.cs

Other (3) +38 / -11
release.ymlGate wrapper publication on platform package availability +7/-0

Gate wrapper publication on platform package availability

• Adds a release step that extracts every optional platform dependency and waits until its release version resolves from npm. This prevents publishing an installable wrapper while native packages are still processing.

.github/workflows/release.yml

verify-npm-trio.shReuse availability polling before npm verification +2/-11

Reuse availability polling before npm verification

• Replaces fixed retry attempts with the shared npm availability wait and packs the platform package using online metadata revalidation.

scripts/verify-npm-trio.sh

wait-npm-available.shAdd npm package availability polling utility +29/-0

Add npm package availability polling utility

• Introduces a configurable deadline-based poll for multiple package versions. Every query uses '--prefer-online' and reports the package and npm error when timing out.

scripts/wait-npm-available.sh

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ed4ce454e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


var checkResult = await CheckForUpdateAsync(forceCheck: true, channel, root, npm);
var (latest, current) = (checkResult.Latest, checkResult.Current);
var advisory = UpdateAdvisoryResolver.Resolve(checkResult, channel, profiles.Resolution.ServerUrl, root);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh the server version before suppressing updates

When the cached header predates a server rollout, this resolves the advisory against the old version and can emit newer: false; the Node launcher then exits before invoking npm. ServerVersionStore.Get ignores the stored seen_at, and kcap update makes no request to the connected server, so repeated update attempts cannot refresh the value and remain pinned until an unrelated server-backed command runs. Refresh or expire the server-version cache before treating it as authoritative for an active update.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 692d935: on the stable channel, kcap update (and --check, which the launcher runs) now sends one authenticated read-only probe to /api/me/notification-prefs alongside the npm lookup. The response refreshes the cached server version before the cap is applied. Without a credential, or if the probe fails within its 5 s budget, the cached value still applies. Covered by UpdateCommandServerRefreshTests.

@qodo-code-review

qodo-code-review Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. A stalled registry leaves releases stuck ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new wait-npm-available.sh gate runs only after the workflow has published every platform
package, and exits nonzero when a package remains unavailable at its deadline. A registry delay or
wrapper-publish failure then leaves immutable platform versions published without a wrapper, while a
rerun attempts to publish those platform versions again because the release guard checks only the
wrapper package.
Code

.github/workflows/release.yml[R624-627]

+      - name: Wait for the platform packages to become installable
+        run: |
+          read -ra pkgs <<< "$(node -p "Object.keys(require('./npm/kcap/package.json').optionalDependencies).join(' ')")"
+          bash scripts/wait-npm-available.sh "${{ steps.version.outputs.VERSION }}" "${pkgs[@]}"
Relevance

●●● Strong

Recent release-workflow precedent accepts fail-closed handling for immutable npm publication states.

PR-#363

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed gate is positioned after the platform publish loop and before wrapper publication, and
its script exits at a deadline. The workflow itself documents npm versions as non-republishable, but
its preflight registry query is hard-coded to the wrapper package, so it does not prevent or
reconcile a partial platform-only release on rerun.

.github/workflows/release.yml[32-38]
.github/workflows/release.yml[55-77]
.github/workflows/release.yml[561-605]
.github/workflows/release.yml[624-642]
scripts/wait-npm-available.sh[15-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A timeout while waiting for npm registry visibility occurs after platform packages have been published but before the wrapper is published. Since npm versions cannot be republished, rerunning the workflow attempts to republish those platform versions; the existing immutable-release guard checks only the wrapper package and cannot safely recover this partial release.

## Fix Focus Areas
- .github/workflows/release.yml[55-77]
- .github/workflows/release.yml[561-605]
- .github/workflows/release.yml[624-642]

## Recommended Fix
Make platform publication idempotent for a retry: before publishing each platform package, query its exact version and release-commit metadata. If that exact package version already exists and carries the current tag commit, skip publishing it; if it exists with a different or missing commit, fail safely. Extend the release-state validation to account for the platform packages so a retry can wait again and publish the missing wrapper without republishing immutable artifacts.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Platform tarballs can remain unavailable ✓ Resolved 🔗 Cross-repo conflict ☼ Reliability
Description
wait-npm-available.sh treats a matching npm view response as installability even though that
command reads package metadata without downloading the platform tarball. If metadata becomes visible
before its tarball, the wrapper's optional dependency can still be skipped and cloud-meta's global
install reaches kcap --version without a native executable.
Code

scripts/wait-npm-available.sh[18]

+    if [ "$(npm view "$pkg@$version" version --prefer-online --registry https://registry.npmjs.org 2>"$err")" = "$version" ]; then
Relevance

●● Moderate

The concern is technically plausible, but no close historical precedent establishes the team’s
decision on metadata versus tarball validation.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new release gate only queries each package's version, while the wrapper declares native packages
as optional dependencies and its launcher fails when the selected package is absent. cloud-meta
installs this wrapper globally and immediately executes kcap --version, so an unavailable tarball
breaks its image build when it consumes the affected release.

scripts/wait-npm-available.sh[15-20]
npm/kcap/package.json[16-22]
npm/kcap/bin/kcap.js[201-210]
External repo: kurrent-io/cloud-meta, .devcontainer/Dockerfile [407-427]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The release wait checks package metadata with `npm view`, which does not prove that each platform tarball can be downloaded before the optional-dependency wrapper is published.

## Fix Focus Areas
- scripts/wait-npm-available.sh[15-28]
- .github/workflows/release.yml[624-627]

## Recommended Fix
Poll each platform package with `npm pack --prefer-online` into a temporary directory and consider it available only after the tarball downloads successfully. Remove each downloaded tarball between attempts and retain the existing shared timeout and error reporting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. One test comment repeats the case ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
resolveInstallSpec's new inline // server-capped pin comment merely labels the input and repeats
what the version-valued install_tag assertion already shows. When this case changes, the label
adds a second description to maintain without documenting any hidden constraint or failure mode.
Code

npm/kcap/bin/kcap.test.js[21]

+assert.strictEqual(resolveInstallSpec({ install_tag: "1.0.1" }), "@kurrent/kcap@1.0.1"); // server-capped pin
Relevance

●●● Strong

Recent precedent accepts removing or minimizing redundant explanatory comments in tests and
production code.

PR-#525

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2762993 prohibits comments that merely restate what nearby code clearly expresses.
The cited assertion passes 1.0.1 as install_tag, expects the same version in the package
specification, and then redundantly labels that behavior as a server-capped pin.

Rule 2762993: Restrict comments to documenting non-obvious, behavior‑critical constraints
npm/kcap/bin/kcap.test.js[21-21]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inline `server-capped pin` comment only restates the test input and does not document a non-obvious behavioral constraint.

## Fix Focus Areas
- npm/kcap/bin/kcap.test.js[21-21]

## Recommended Fix
Remove the inline comment while retaining the assertion unchanged; the test expression already communicates that the server-supplied version becomes the install specification.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/cloud-meta (sha: 15782900)
  Explored: repo: kurrent-io/kcap-server (sha: d2657546)
Review mode: 🧠 Deep: This spans npm release automation, shell polling, launcher behavior, and C# update/version-capping logic across multiple independent paths, creating a high density of subtle regressions despite reported tests.

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread npm/kcap/bin/kcap.test.js Resolved
Comment thread .github/workflows/release.yml Resolved
Comment thread scripts/wait-npm-available.sh Resolved
alexeyzimarev and others added 3 commits September 11, 2026 17:26
…#891)

npm skips an optional dependency whose tarball fails to download, not only one it cannot resolve, so a listed version alone does not prove the wrapper installs complete.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#891)

npm refuses to publish a version twice, so without the skip a release whose later step failed could not be finished by re-running the failed jobs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cached version changes only on an authenticated response, so a server upgraded since the last one would pin the update to its old version.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit 6b2ce89 into main Sep 11, 2026
8 checks passed
@alexeyzimarev
alexeyzimarev deleted the fix/npm-install-race branch September 11, 2026 15:58
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.

kcap installs can miss their platform binary, and the update hint suggests npm install

1 participant