Repository navigation
feat(staged): honor npm config for managed installs and surface missing managed bridges in Doctor - #998
Conversation
…stead of hardcoding Block URLs
Staged passed `--registry <Block Artifactory>` to every npm command and
downloaded the managed Node.js runtime from the Artifactory mirror, so a
from-source build or a personal Mac off WARP could never get a managed
Claude/Codex bridge. Follow Berd #1107's split: public code carries no
Block URL, and the one population that needs one (Block laptops with no
~/.npmrc behind Cloudflare Gateway) gets it at build time.
npm registry precedence:
1. `STAGED_NPM_REGISTRY` (explicit override, passed as `--registry`);
2. otherwise no `--registry` — npm resolves `NPM_CONFIG_REGISTRY`,
`~/.npmrc` (incl. scoped entries and auth), then registry.npmjs.org;
3. if that fails with a gateway block page (stderr names
blocked.teams.cloudflare.com, or FETCH_ERROR/EJSONPARSE/"Unexpected
token"/"invalid json" mentioning registry.npmjs.org), no override was
given, and `option_env!("STAGED_FALLBACK_NPM_REGISTRY")` is set, reset
the staging dir and retry once through the fallback. 404s, dead
mirrors and ETARGET are not retried.
Both the floating bridge install and the `npm view` freshness probe
follow the same two-step. Probes additionally pass `--fetch-retries=0`
and `--fetch-timeout=<probe budget>` so a dead `~/.npmrc` host costs
15 s, not npm's ~70 s retry schedule; installs keep npm's defaults.
Install errors name the registry used ("npm default (per npm config)"
when none was passed) so Doctor can show it without a state.json change.
Node dist: `STAGED_NODE_DIST_URL` override, else https://nodejs.org/dist
(SHA-256 pinned by node-runtime.lock.json, so the host is not a trust
boundary). `option_env!("STAGED_FALLBACK_NODE_DIST_URL")` is retried once
on `ManagedNodeError::Network` only — never for a bad checksum, archive,
or HTTP status. Network errors now name the URL.
Removed: the `no-block-npm-registry` Cargo feature,
`BLOCK_NPM_REGISTRY_URL`, `BLOCK_NODE_DIST_BASE_URL`, and the Artifactory
default in scripts/update-node-runtime-lock.mjs. Doctor's option
builders take `npm_registry_override()` so checks and fixes still agree.
staged-release.yml exports both `STAGED_FALLBACK_*` values from
repository variables (no literal in the workflow); the README documents
the precedence and that Block builds must set those variables.
Tests: env-override/fallback helpers, the stderr classifier, the
fallback gate, a fake npm that fails with a block page until `--registry`
is passed (install retries once; override/404/no-fallback do not), probe
flag bounds and probe retry, and Node fixtures for refused-primary →
fallback success plus checksum-failure → no retry.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ighten the registry fallback Follow-ups from the review of 4628d2b (honor npm config instead of hardcoding Block URLs). Install/probe env divergence: the managed `npm install` inherited the Tauri process env while `latest_published_version` and Doctor's checks and fixes ran under the sanitized login-shell snapshot, so an `NPM_CONFIG_REGISTRY` exported from the shell rc steered the probe but not the install when the app was launched from Finder. `install_managed_tool` now takes the snapshot (`doctor::doctor_env_vars`, now pub(crate); the reconciler builds the same one), runs npm with `env_clear()` + snapshot, and overlays only the managed node bin on PATH and the private `NPM_CONFIG_CACHE`; `--prefix` stays on the command line. The `latest_published_version` doc now describes exactly that. Classifier: `classify_npm_failure` reports GatewayBlocked only for `blocked.teams.cloudflare.com` itself, or a FETCH_ERROR / EJSONPARSE / "invalid json" / "Unexpected token" failure whose *request* went to `registry.npmjs.org` (`request to https://registry.npmjs.org/`, `response body at https://registry.npmjs.org/`, or a `registry: https://registry.npmjs.org` line). A mirror's error body mentioning its upstream, ENOTFOUND for a custom mirror, and E404 are Other. A truncated body from the public registry is still accepted as blocked — one cheap extra attempt — and documented as such. When the fallback attempt also fails, the surfaced error keeps the original diagnosis: "npm default registry (registry.npmjs.org) was blocked by a network gateway; fallback <url> also failed: <err>". stderr capture keeps the TAIL (npm writes its `npm error code` block last, after warnings), dropping whole lines from the front past 64 KiB. Doctor-crate npm fixes: `run_doctor_fix` and `run_doctor_update` go through `execute_fix_with_gateway_fallback`, which — for an npm-backed command, with no `STAGED_NPM_REGISTRY` and a compiled-in fallback — reuses `gateway_fallback_registry` on the executor's error string (the doctor crate already surfaces the failed command's trimmed stderr as its error) and retries once with `ExecuteFixOptions { npm_registry: Some(fallback) }`. No doctor-crate plumbing was needed. Node dist: an explicit `STAGED_NODE_DIST_URL` now suppresses `STAGED_FALLBACK_NODE_DIST_URL`, matching the npm rule that an explicit override is never second-guessed (`fallback_for`); mid-body read `Network` errors name the URL like the `send()` error does. README and module docs updated. Tests: classifier cases (real block page, inline block page, `registry:` line, truncated body → blocked; Artifactory upstream mention, custom mirror ENOTFOUND, E404, offline npmjs → other), combined fallback error message, stderr tail (whole-line drop + oversized single line), a fake npm that dumps its env proving `NPM_CONFIG_REGISTRY`/`npm_config_registry` from the snapshot reach the install while the managed cache overlay and node-bin-first PATH win, a fallback-also-fails install, and the Node override-suppresses-fallback gate. No new backend data model fields or types; `NpmFailure`, `classify_npm_failure`, `gateway_fallback_registry` and `gateway_retry_failed_message` became pub(crate). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…and offer the managed install Doctor labels a Claude/Codex row "Managed by Staged" only when the bridge resolved from ~/.staged/packages/bin. Because that dir is first on the probe PATH, a Homebrew or npm-global copy can only win while the managed copy is absent — the launch reconcile has not run yet, or it failed (no route to the registry, a gateway block page, a mirror 404, a validation failure). The row then read as a plain green "Installed" pointing at /opt/homebrew/bin, indistinguishable from a healthy setup, with no hint that Staged's copy was missing and no way to get it short of waiting for a newer release to make the update button appear. Implements the Doctor surfacing from the registry-precedence note and proposals 1, 2 and 5 of the classification note. Claude/Codex rows (`flag_missing_managed_copy`): on a managing build, a row whose executable is not the managed shim drops to Warn (a row already warning keeps its own message in front) with "Using <path> — Staged's managed copy is missing", followed by the last reconcile's recorded error for that tool when there is one. raw_output gains the expected shim path, the last reconcile's outcome and age, the error with a classified hint, and the npm registry Doctor detected. The row's fix becomes `FixType::Bridge` with `managed_install_description(...)` — offered whether or not an update is available — which `run_doctor_fix` already routes to `install_managed_tool_logged` for these two checks. The freshness-pass redirect (`redirect_unmanaged_update_to_managed_install`) still fires and produces the identical description, so the Update and Fix buttons never disagree. Management off (`note_managed_installs_disabled`): when `managed_shim_bin_dir()` is None, the two rows keep their status and gain "(managed installs disabled: <reason>)" in message and raw_output, where `managed_tools_disabled_reason()` names the STAGED_ACP_TOOLS_DIR override dir, the `no-managed-acp-tools` build, or an unsupported platform. Node runtime row: a Missing runtime with a failed `lastReconcile` on record now warns — "Staged-managed Node.js <ver> could not be installed: <error>" — even with no installed tools, leading with the runtime download error (minus the reconciler's `<tool id>: ` prefix) and listing every recorded error, which already embed the URL, plus a hint; the reinstall fix is kept. Missing with no record, or a clean one, behaves as before. Hint classification lives next to `classify_npm_failure` as `classify_install_failure` / `install_failure_hint`: a gateway block page (including the "blocked by a network gateway; fallback … also failed" message) → add `registry=<mirror>` to ~/.npmrc or set STAGED_NPM_REGISTRY; E404 → the configured registry does not carry <package>; ENOTFOUND/ECONNREFUSED/ETIMEDOUT/reqwest "error sending request"/an install timeout naming its registry → check `registry=` in ~/.npmrc or STAGED_NODE_DIST_URL. Validation and entrypoint failures get no hint. For that to classify anything, the recorded npm error had to carry more than an exit status: `run_floating_npm_install_capturing` now appends npm's own `npm error code <CODE>` line and up to three detail lines (`npm_error_summary`, minus the `errno` repeat and log pointer, clipped) to the `NpmInstall` error. Error strings only — state.json's schema is unchanged, and nothing new is persisted. Registry detection: `configured_npm_registry` runs `npm config get registry` with the snapshot's npm (`find_on_env_path`, now pub(crate)) under the snapshot env, 5 s timeout, only when a report actually has an unmanaged Claude/Codex row; the label reads "<url> (per npm config)", "<url> (STAGED_NPM_REGISTRY)", or "npm default (…)" with no npm to ask. Frontend: no change needed — `canFix` already accepts a `bridge` fix on a non-pass row and `.check-message` wraps long text. README documents what the rows now say. Tests: user copy flagged with Bridge fix and otherwise byte-identical; recorded error + hint + registry on the row, other tool's error kept off it, already-warning row keeps its message; disabled note without a downgrade; Node Missing + failed reconcile → Warn with all errors and a hint, Missing + no/clean record unchanged; classifier cases for the three categories and the no-hint ones; npm error summary (old/new npm spelling, clipping, no block); disabled-reason parts; a fake npm proving the registry probe asks the snapshot's npm under the snapshot env. Verified: cargo fmt --check; cargo clippy --lib -- -D warnings (the three `cloned_ref_to_slice_refs` hits under `--tests` are pre-existing in session_queue/tests.rs and session_runner.rs); cargo test --lib -- doctor managed_acp_tools: 84 passed; pnpm run check: 0 errors 0 warnings; vitest src/lib/features/doctor: 59 passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… path and soften the first-launch row Follow-ups from the review of e905c94 (Doctor says when a managed bridge is missing). Report latency: `configured_npm_registry` ran after the `tokio::join!` of the other probes, so an unmanaged Claude/Codex row — the fresh-profile and failed-reconcile cases this feature targets — added up to 5 s of serial latency to `run_doctor_report`. The probe now joins the other probes. Whether to run it is decided up front from the shim dir (`any_managed_shim_missing`: the shim dir is first on the probe PATH, so a row can only resolve elsewhere while a shim is absent) rather than from the finished report; `managed_install_diagnostics` is now a synchronous assembly of the record and the label. The answer is cached per env snapshot for 60 s (`NpmRegistryCache`, the `LatestVersionCache` pattern; a `None` answer is cached too, since re-asking a hung or absent npm every pass is exactly the stall being avoided). The 5 s probe timeout is kept. `npm_registry` became `Option<String>`; a row rendered without a probe says "npm registry: not probed". First-launch window: with no reconcile on record, the row says "Using <path> — Staged's managed install hasn't completed yet" (still Warn, Fix kept) and the detail reads "last managed install: not run yet (runs at launch and daily)", so a healthy first launch no longer reads as a failure. Message length: the row stops at "… — Staged's managed copy is missing" plus `short_error_fragment` of the recorded error — npm's own `npm error code <CODE>` when present, else the first 120 chars ellipsized. The full error, hint and registry stay in raw_output for Copy details. Node runtime hint: `install_failure_hint` is consulted only when the headline is a runtime download failure ("Node.js runtime download failed…"), so a bridge's npm E404 as the headline can no longer yield "the configured registry does not carry Node.js". New `InstallFailureHint::NodeDownloadRejected(status)` for `HTTP 4xx` from the dist host → "the Node.js download URL returned HTTP <code> — check STAGED_NODE_DIST_URL"; 5xx gets no hint. `reconcile_headline` still picks any "Node.js runtime" error (archive, checksum, incomplete) as before. README: the row says "hasn't completed yet" before the first reconcile completes, and the hint list gains the download-URL case. No new persisted fields; state.json is unchanged. Tests: first-launch row (message, detail, Fix kept, already-warning variant); error-code fragment on the row with the full error in detail; ellipsized no-code error and "not probed"; `short_error_fragment` cases; shim-dir probe gate; Node row with a bridge E404 headline (no hint), an HTTP 404 download (URL hint), a checksum mismatch (no hint); classifier cases for HTTP 404/403/503 and the hint text; registry cache per snapshot (one npm spawn for two asks, `None` cached) and TTL expiry. Verified: cargo fmt --check (clean); cargo clippy --lib -- -D warnings (clean); cargo test --lib -- doctor managed_acp_tools: 90 passed; pnpm run check: 3453 files, 0 errors, 0 warnings; vitest src/lib/features/doctor: 4 files, 59 passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
builderbot/apps/staged/src-tauri/src/managed_acp_tools.rs
Lines 1042 to 1043 in 80fb461
When ~/.npmrc contains @agentclientprotocol:registry=..., adding only --registry does not force either managed bridge to STAGED_NPM_REGISTRY, because both package names are in that scope. npm's bundled registry documentation states that the registry URL is determined by package scope; locally, npm config get @agentclientprotocol:registry --registry=https://forced.example/ still returns the scoped URL. Thus users cannot escape a stale or unreachable scoped mirror with the advertised override; override the @agentclientprotocol registry configuration for these commands as well.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| npm_registry_override().as_deref(), | ||
| fallback_npm_registry(), | ||
| env_vars, |
There was a problem hiding this comment.
Key latest-version results by registry configuration
When a user changes ~/.npmrc or STAGED_NPM_REGISTRY while Staged is running, this probe now targets a dynamically selected registry, but the preceding one-hour cache remains keyed only by package name. A Doctor refresh can therefore show the old registry's version and confirmation text while the managed installer fetches @latest from the new registry; include the effective registry/environment in the cache key or invalidate the cache when it changes.
Useful? React with 👍 / 👎.
| // ----------------------------------------------------------------------------- | ||
| // npm registry policy |
There was a problem hiding this comment.
Split the new Rust responsibilities into focused submodules
This change expands managed_acp_tools.rs from 1,806 to 3,287 lines and doctor.rs from 2,569 to 3,560 lines by embedding registry policy, process execution, caching, diagnostics, reconciliation, and extensive tests in the existing modules. Split these responsibilities into focused submodules rather than making two already-large library files substantially harder to navigate and maintain.
AGENTS.md reference: apps/staged/AGENTS.md:L28-L31
Useful? React with 👍 / 👎.
… force the scoped registry and key the version cache by registry config Addresses the Codex review of PR #998 (80fb461). Scoped registry (P1): `--registry` only sets npm's *default* registry. Both managed bridges live under `@agentclientprotocol`, and npm resolves a scoped package's registry from its `@scope:registry` config first, so a `~/.npmrc` carrying `@agentclientprotocol:registry=<stale mirror>` kept winning over `STAGED_NPM_REGISTRY` and the gateway fallback alike (verified with npm 11.19: `npm config get @agentclientprotocol:registry --registry=<forced>` still answers the scoped URL; adding `--@agentclientprotocol:registry=<forced>` wins). `npm_registry_args` now emits `--registry <url>` plus `--@<scope>:registry=<url>` for a scoped package, and every site that passed a registry uses it: the floating bridge install, the `npm view` freshness probe, and — in the doctor crate — `apply_npm_registry` (fix commands, taking the scope from the first `@scope/` token in the command) and `latest_npm`. README and the policy module doc say so. Latest-version cache (P2): `LatestVersionCache` was keyed by package name only, so a `~/.npmrc` or `STAGED_NPM_REGISTRY` change while Staged ran let Doctor show the old registry's version for up to an hour while the installer fetched `@latest` from the new one. Entries are now keyed by `LatestVersionKey { package, RegistryFingerprint }`, where the fingerprint is the override, the env snapshot (its `npm`, `HOME`, `NPM_CONFIG_*` exports) and the user npmrc's identity (path, size, mtime — `NPM_CONFIG_USERCONFIG` in either spelling, else `$HOME/.npmrc`). Inserts drop entries past the TTL so superseded fingerprints do not linger. Module split (P1, AGENTS.md "split growing modules"): the two files had reached 3,287 and 3,560 lines, half of it tests. Pure moves, no behaviour change; cross-module items became `pub(super)`. managed_acp_tools/ mod.rs layout, env, the managed set, state.json, re-exports registry.rs registry policy: override/fallback, classifiers, hints install.rs install_managed_tool: stage, validate, swap, shim probe.rs npm view / npm config probes and their caches reconcile.rs reconcile epilogue validation.rs (unchanged) tests/{support,layout,registry,install,probe,reconcile,validation}.rs doctor/ mod.rs commands, option builders, fix/update dispatch login.rs interactive login runs (pub: the four login commands) managed_bridges.rs Claude/Codex row readouts and missing-copy flags runtime_checks.rs Node.js runtime and session-queue rows tests/{support,commands,login,managed_bridges,runtime_checks}.rs Each submodule includes its tests with `#[path]`; shared fixtures live in a `test_support` module. `lib.rs` registers the login commands as `doctor::login::*` (Tauri's handler macro needs the command's own module); `crate::doctor::start_doctor_login` etc. stay re-exported for web_server. The self-respawning validation test names its new test path. Tests: `npm_registry_args` scoped/unscoped; scoped-flag assertions on the install fallback/override argv and the probe argv; cache keyed by override, env and npmrc; `user_npmrc_path` precedence; a fake npm that answers with its npmrc's contents proving an edited npmrc re-probes within the TTL and an untouched one is a cache hit with npm gone; TTL insert pruning; doctor crate `apply_npm_registry` with a scoped package. Verified: cargo fmt --check; cargo clippy --lib -- -D warnings (clean; the three `--tests` lints in session_queue/tests.rs and session_runner.rs are pre-existing); cargo test --lib -- doctor managed_acp_tools: 94 passed (90 before + 4 new; test names diffed against HEAD); crates/doctor clippy --lib --tests clean and cargo test --lib: 152 passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ure scripts Staged CI on PR #998 failed nondeterministically in `install_runs_npm_under_the_env_snapshot` (env dump lacked the snapshot's NPM_CONFIG_REGISTRY, or carried the runner's hermit NPM_CONFIG_CACHE) and `configured_npm_registry_is_cached_per_env_snapshot` (npm "spawned" twice). Cause: `wait_until_executable`, the Linux ETXTBSY workaround behind `write_executable`, spawned each fresh fixture script with no arguments under the inherited test-process env and then killed it. `spawn` returns after `sh` has exec'd, so on Linux the kill can land after the script's first lines ran — and both fixtures put their side effect there (`env > log`, `printf x >> calls`). A stray run overwrote the real env dump with the CI env, or appended an extra call marker. macOS never loses the race, which is why it was green locally. Fix, in tests/support.rs: - `with_exec_probe_guard` inserts `[ -z "$STAGED_EXEC_PROBE" ] || exit 0` after the shebang; `write_executable` applies it to every script, and the validation tests' `write_node_shim` applies it before the production `write_shim`. - `wait_until_executable` runs the script with `env_clear()` plus `STAGED_EXEC_PROBE=1` and waits for it (asserting a clean exit) instead of kill-racing it, and asserts the script carries the guard so an unguarded fixture fails loudly rather than racing. - Dropped a redundant `wait_until_executable` after a `write_executable` in tests/probe.rs. Test-only; no product code changes. Verified: cargo fmt --check; cargo clippy --lib --tests (only the three pre-existing session_queue/session_runner lints); cargo test --lib -- managed_acp_tools doctor: 94 passed, run alternately against a HEAD worktree 3x each at load ~96 (all green, same timings). One run of each of the two 1 s-timeout validation tests failed under that load on this change, but neither repeated in the A/B runs. Linux is the real check (macOS never reproduced the race). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
`timeout_kills_bridge_and_native_child_and_preserves_previous_install` and `timeout_kills_native_child_after_bridge_exits_with_open_pipes` failed the pre-push hook in two of three full-suite runs at load ~30-45 with "probe did not spawn its native child: ... `--cli --version` timed out after 1 seconds". The 1 s budget starts at spawn and has to cover exec'ing the node shim and `sh` before the fixture even forks its native child; under parallel-suite load the timeout fired first (no "waiting for native CLI" in the excerpt). The tests only need the timeout to fire after the native child exists, so widen it to 5 s and the elapsed bound to 15 s. Test-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Summary
Staged hardcoded Block's Artifactory URLs for every npm command and for the managed Node.js download. That meant from-source builds and Macs off WARP could never get a managed Claude/Codex bridge. When a managed install failed, Doctor still showed a green "Installed" row pointing at a Homebrew or npm-global copy, so nothing told you Staged's copy was missing.
No hardcoded Block URLs
STAGED_NPM_REGISTRYoverrides the registry if set. Otherwise Staged passes no--registryand npm uses its own config (NPM_CONFIG_REGISTRY,~/.npmrcincluding scoped entries and auth, then registry.npmjs.org).https://nodejs.org/distby default, or fromSTAGED_NODE_DIST_URLif set. It is still SHA-256-pinned bynode-runtime.lock.json.STAGED_FALLBACK_NPM_REGISTRYandSTAGED_FALLBACK_NODE_DIST_URLare compiled in viaoption_env!. Each is retried once, and only when there's no explicit override:staged-release.ymlreads both fallbacks from repository variables, so no internal URL lives in the tree.no-block-npm-registryfeature,BLOCK_NPM_REGISTRY_URLandBLOCK_NODE_DIST_BASE_URL, and the Artifactory default inupdate-node-runtime-lock.mjs.--fetch-retries=0and a bounded--fetch-timeout, so a dead~/.npmrchost can't stall them.Doctor surfaces a missing managed copy
<path>— Staged's managed install hasn't completed yet" (first launch)<npm error code>" (after a failed install)STAGED_ACP_TOOLS_DIR, theno-managed-acp-toolsbuild, or an unsupported platform), the rows say why.npm error codeblock. These are error strings only; thestate.jsonschema is unchanged and there are no new persisted fields.Before merging
For Block release builds to keep working on managed laptops with no
~/.npmrc, set theSTAGED_FALLBACK_NPM_REGISTRYandSTAGED_FALLBACK_NODE_DIST_URLrepository variables.Testing
cargo fmt --check,cargo clippy --lib -D warnings,cargo test --lib -- doctor managed_acp_tools,pnpm run checkand the Doctor vitest suite all pass. The pre-push CI hooks also passed.🤖 Generated with Claude Code