Guard npm update readiness#19389
Conversation
…ness + thread-store harness - anomalyco/opencode#24179: session-scoped permission bridge for external providers (merge-after-nits) - anomalyco/opencode#24162: desktop health-check exponential backoff (request-changes; loopback gate dropped, no_proxy widened) - openai/codex#19389: npm update prompt readiness gate (merge-after-nits) - openai/codex#19266: non-local thread-store regression harness (merge-after-nits)
shijie-oai
force-pushed
the
codex/npm-update-readiness
branch
from
92a9708 to
4ec099d
Compare
shijie-oai
force-pushed
the
codex/npm-update-readiness
branch
from
4ec099d to
90e50c3
Compare
shijie-oai
changed the title
shijie-oai
force-pushed
the
codex/npm-update-readiness
branch
2 times, most recently
from
12092c7 to
c4a3343
Compare
shijie-oai
force-pushed
the
codex/npm-update-readiness
branch
from
c4a3343 to
6a66b76
Compare
| let latest_version = fetch_latest_github_release_version().await?; | ||
| let package_info = create_client() | ||
| .get(npm_registry::PACKAGE_URL) | ||
| .send() | ||
| .await? | ||
| .error_for_status()? | ||
| .json::<ReleaseInfo>() | ||
| .json::<NpmPackageInfo>() | ||
| .await?; | ||
| extract_version_from_latest_tag(&latest_tag_name)? | ||
| npm_registry::ensure_version_ready(&package_info, &latest_version)?; | ||
| latest_version | ||
| } |
There was a problem hiding this comment.
Previouly we only rely on github latest tag but given recent issue there is a clear lag between github package readiness and npm status. Here we are adding a cross validation to make sure both are ready.
| tarballs=( | ||
| "${platform_tarballs[@]}" | ||
| "${other_tarballs[@]}" | ||
| "${root_tarball}" |
There was a problem hiding this comment.
Root tarball is what determines when npm says a version is ready. So even with the cross validation logic below against github latest, we can still run into user wants to upgrade yet it is actually no 100% ready.
| } | ||
| } | ||
|
|
||
| fn extract_version_from_latest_tag(latest_tag_name: &str) -> anyhow::Result<String> { |
There was a problem hiding this comment.
refactored into its own file.
| Ok(()) | ||
| } | ||
|
|
||
| fn parse_version(v: &str) -> Option<(u64, u64, u64)> { |
There was a problem hiding this comment.
Refactored out into update_version
| if [[ ! -f "${required_tarball}" ]]; then | ||
| echo "Missing npm tarball: ${required_tarball}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
So hardcoding the platform is a bit ugly but we can make sure that all tarballs are covered. Alternatively we can use a sort given that we didn't really care about existence prviously.
shijie-oai
force-pushed
the
codex/npm-update-readiness
branch
from
6a66b76 to
5f9e8c9
Compare
etraut-openai
left a comment
etraut-openai
left a comment
There was a problem hiding this comment.
Thanks for doing this!
This PR includes two pieces: a build script change and a client-side change. Of the two, I think the build script change is much more important.
I have a small concern about the client change. It hardcodes the address of the public npm registry. Most of our users will go through the public registry, but some enterprises may use internal npm registries for supply chain controls. I guess that these enterprises will typically disable our auto-update check using check_for_update_on_startup = false.
Hm that is a good point with the internal npm registries but there should not be any behavioral difference. Really the only case is check_for_update_on_startup == true (or unset) and distributed through internal registry. With previous approach of only checking against github latest release we would still show upgrade disregarding internal registry status. With the adding cross valiation against public registry that behavior does not change - not ideal but no regression. |
2 participants
Why
For npm/Bun-managed installs, the update prompt was treating the latest GitHub release as ready to install. During the
0.124.0release, GitHub and npm visibility were not atomic: the root npm wrapper could become visible before the npm registry marked that version as the packagelatest. That left a window where users could be prompted to upgrade before npm was ready for the release.What changed
version.jsoncache after npm registry metadata proves that same root version is ready.codex-rs/tui/src/npm_registry.rsto validate npm readiness by checkingdist-tags.latestand root packagedistmetadata for the GitHub candidate version.codex-rs/tui/src/update_versions.rsso that logic can be tested without compiling the release-onlyupdates.rsmodule under tests..github/workflows/rust-release.ymlso the six known platform tarballs publish before the root@openai/codexwrapper. Other npm tarballs publish before the root wrapper, and the SDK publishes after the root package it depends on.