Skip to content

feat(*): fall back to mirrors when a download host cannot be reached - #1053

Merged
0xKT merged 40 commits into
mainfrom
feat/china_mirror_fallbacks
Oct 26, 2026
Merged

0xKT merged 40 commits into
mainfrom
feat/china_mirror_fallbacks

Conversation

@LivXue

@LivXue LivXue commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Summary

From a mainland-China line with no proxy, several hosts a Raven install and its runtime download from do not answer at all: huggingface.co resolves to an address that drops every packet, raw.githubusercontent.com and GitHub release downloads time out more often than not, and astral.sh, PyPI, nodejs.org or the npm registry can fail the same way. Each has a mirror inside the country that serves the same files. This change makes every download that has one fall back to it, and only when the default host cannot be reached at all.

The rule, in one place for every download:

  • The default host is always asked first. Only a failure to reach it moves the download on: a name that does not resolve, no route to it, a connection refused, reset or left unanswered, a connection the machine itself refuses (a sandbox or a local firewall), TLS that never got going. An HTTP error is the host's answer and stays final, and so is a certificate that fails verification (whatever re-signs the host's traffic re-signs the mirror's too). Behind a proxy, the proxy's answer to the request for a tunnel speaks for the host: a 4xx (403 for a host its rules block, 407 for credentials it was not given) is final, since the mirror would be asked through the same proxy and would only route around its rules, while a 5xx says the proxy could not reach the host and counts as unreachable. A SOCKS proxy that cannot connect counts as unreachable whatever its reply. Two downloads do not say why they failed, uv's own installer and Playwright's Chromium download, so the hosts they download from are asked, and the mirror stands in only when none of them can be reached; the installers fetch uv's installer themselves, so astral.sh's answer to that fetch is final too. The LibreOffice dmg and the CJK font, checked against digests the script pins, try their mirror after their usual hosts fail in any way.
  • Five places apply that rule, each with the client it uses: the runtime (urllib for launches, httpx for downloads), the upgrade helper, install.sh (curl) and install.ps1 (PowerShell). A test runs all five against the same loopback hosts and proxies and holds them to one table.
  • A host the user chose is never replaced: HF_ENDPOINT; a uv index in the environment, in a uv.toml or on the command line; where uv's installer downloads uv from (UV_DOWNLOAD_URL, INSTALLER_DOWNLOAD_URL, UV_INSTALLER_GHE_BASE_URL, UV_INSTALLER_GITHUB_BASE_URL); where uv fetches a Python from (UV_PYTHON_INSTALL_MIRROR, UV_PYTHON_DOWNLOADS_JSON_URL, UV_ASTRAL_MIRROR_URL, or python-install-mirror or python-downloads-json-url in a uv.toml); PLAYWRIGHT_DOWNLOAD_HOST or PLAYWRIGHT_CHROMIUM_DOWNLOAD_HOST; an npm registry set anywhere npm reads one.
  • No choice outlives the download it was made for. A fallback sets nothing that stays in a process which goes on to fetch packages for a user's project: the installers restore what they set before raven web starts; at runtime the mirror goes only on a uvx launch's own command line, or on the command line of the npm that installs an npx launch's packages into Raven's own directory.
  • RAVEN_NO_MIRROR=1 (read like Raven's other opt-outs) keeps every download on its default host. It is read in the environment of the launch as well as in Raven's own, whatever the case of the name, so a user who sets it in an agent's own environment, or in the profile of the machine an agent runs on, means it there; a Raven node is told the host's own value, and the daemon reads the machine's own environment as well. Both installers' headers and the quick start (English and Chinese) say so.
  • Each switch says so, with the way to turn it off. The exceptions are quiet on purpose: the LibreOffice dmg and the CJK font, whose pinned digests mean no source can change what is installed, and the codex version lookup.

Where it applies:

  • raven resources: the tokenizer dictionary through jsDelivr; the deepdoc models through hf-mirror.com, asked without the user's Hugging Face token, which belongs to huggingface.co. A host with a mirror behind it gets a 15 s connect and 30 s read timeout instead of 120 s, and the files after a switch go straight to the mirror.
  • install.sh and install.ps1: uv from the newest wheel on the PyPI mirror, or on the indexes the user named, asked in uv's order (each of UV_INDEX's, then UV_DEFAULT_INDEX or UV_INDEX_URL, or else the default index a uv.toml names), when uv's installer could not reach its hosts, checked against the sha256 the index publishes; Node from npmmirror, checked against the SHASUMS256.txt beside it; uv's PyPI index and Python downloads; the npm registry for a source build's npm ci, after npm says which registry it reads, asked in the directory the install runs in, because npm reads the .npmrc of the project beside it, and set for that build alone; one Chromium retry through npmmirror when none of playwright's own hosts answers; and, in install.sh alone, the macOS LibreOffice dmg (TUNA) and the Linux CJK font (jsDelivr). The variables are set for the install's own downloads and restored before raven web starts, so the reader's PowerShell session gets its environment back under irm | iex.
  • install.ps1 now runs uv's installer in Windows PowerShell, a process of its own, as uv documents (powershell -ExecutionPolicy ByPass -c ...): that installer ends a failed download with exit 1, which under Invoke-Expression ended this install too and, under irm | iex, the reader's whole session, before any fallback could run. What it prints, and what playwright prints, goes to the console rather than into the value the step hands back.
  • raven upgrade: the helper points uv at the PyPI and Python mirrors (decided once per upgrade; the relaunched Raven does not inherit them).
  • Runtime launches: ACP agents, CLI agents, MCP stdio servers and agents started on a registered machine, decided on the machine that runs the command, per launch, from a HEAD probe cached for 10 minutes, and only when the mirror itself answers, so a package already in the runner's cache still starts offline.
    • uvx: the mirror goes on uvx's own command line as --default-index and nowhere else. uv keeps a flag to itself (measured: given as UV_DEFAULT_INDEX an index reaches the child, and in a user's project makes uv run rewrite uv.lock; given as the flag it does not reach the child), and uvx reads no project's uv.toml (measured), so the flag replaces no choice a project made.
    • npx: npm hands its registry, however it was given (a variable, a flag or an npmrc; measured with npm 11.12), to everything npx starts, where as npm_config_registry it outranks the registry a project names in its own .npmrc. So npx does not run: the packages it would fetch are installed into $RAVEN_HOME/cache/npx/<key> (one directory per set of specs) by an npm given --registry on its own command line, and the bin npx would have run starts from there with the launch's own environment; on Windows through node, with the options the bin's first line names, as npm's own shims read it. An exact version is installed once; a range, a tag or a bare name is brought up to date at each such launch with npm update, as npx asks the registry each time, and a failed update starts the install made before. Launches of the same specs take turns on a file lock, and npm gets 10 minutes, past which its process group is ended. An install that was killed leaves a directory that looks installed and is not, so a marker file says whether the last run of npm finished and an install without one is done again; an npm that outlived the Raven that started it is waited for rather than run beside, and a Raven node keeps its installs under the node's own directory rather than in the home of the user it runs as. Left to npx as configured: any option other than -y and -p; a spec that is not a registry's (a path, URL, git or alias); a bin or package the project already has, or a bin among npm's global ones; a project whose .npmrc names a registry, scoped or not, which Raven's directory would not read; and an MCP server started in a sandbox.
    • On a Raven node the preparation is bounded: the daemon decides on the machine it runs on, and the host gives the answer 60 s, so a preparation past a budget the protocol names is abandoned and the command starts as it was configured. The answer comes in time and no agent is left running that no host holds. The clock starts before the preparation is queued, so the wait for a worker of the pool counts against the budget, and launches that outnumber the pool's workers are answered within it too: the whole preparation is bounded from the event loop, and the probes and the npm config read are bounded by the same deadline.
    • uv run, npm install, npm exec, pnpm and bun launches are left alone. The WhatsApp bridge's own npm install, which runs in Raven's own directory and starts nothing that stays, and the codex dist-tags lookup fall back as the installers do.

Not covered: Raven's own release files (GitHub Releases has no mirror), the one-line installer URL itself, LibreOffice on Windows (winget) and nltk data.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

The full-suite figures below were measured before the review rounds, after the first of two rebases (every commit replayed unchanged) and before #1030 and #1051 landed on main. The review rounds changed install.sh, install.ps1, the quick start, the runtime's mirror module and its four launch sites, and the upgrade helper, each with its tests; their own checks follow, at the final head, rebased onto main at e8332e5. CI runs the Linux selection there too.

Linux, in a python:3.12 uv container (16 vCPU), the whole default selection on head and on base (origin/main) in one container session, each tree committed into a repository of its own:

  • Head: 1 failed, 34361 passed, 243 skipped, 2 xfailed. Base: 1 failed, 34130 passed, 243 skipped, 2 xfailed.
  • The one failure is the same on both, test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing: the container runs as root, which reads a file whose mode forbids it.
  • An earlier head run of the same selection failed test_cli_doctor_commands.py::test_doctor_empty_config_is_invalid once (doctor printed no Config line). It did not come back in 96 further runs of that check on each tree at four workers, nor in the run above, and no mirror code runs there: the doctor subprocess inherits the suite's RAVEN_NO_MIRROR=1.
  • scripts/coverage_gate.py diff --base-ref origin/main: 99.67% (305 of 306 changed lines) at that point; the one uncovered line is the Windows branch of the uv.toml lookup. The upgrade helper is carried as text and run as a script of its own, which coverage does not measure; its tests run it, and the mutants below reach it.

At the final head, in a Linux container with sh, curl 7.88, PowerShell 7.6 and Node 22 (npm 10.9): the test files this change reaches (test_install_script, test_cli_onboard_commands, test_utils_mirrors, test_updates_upgrade, test_subagent_host_env, test_subagent_third_party, test_mcp_client_transport, test_node_server, test_channels_whatsapp, test_providers_codex_client_version, test_resources_fetch), 1564 passed, 1 skipped (a case only macOS can run), and at the head of the last round, over the same files without test_cli_onboard_commands, 1345 passed, 2 skipped (a case only macOS can run, and the Windows PowerShell 5.1 case). The installer tests pass with a uv of the machine's own at /usr/bin/uv and with an /etc/uv/uv.toml that names an index, neither of which they read.

Windows 11, native, over the same files with PowerShell 7 off PATH so its cases skip on both sides (they run on Linux above): head 25 failed, 1242 passed, 180 skipped; main 25 failed, 1001 passed, 25 skipped (it lacks this change's test files); the failing ids are the same on both sides, POSIX shell stand-ins and file modes, and nltk, which that environment does not have. At the head of the last round, over the same files with PowerShell 7 on PATH: 24 failed, 1038 passed, 165 skipped, and the 24 failing ids are the same as at the head of the previous round on this machine: nltk and the dictionary, the CLI backend's POSIX process groups, and two cases of the Windows console's own encoding. The real-npm test ran there too, with npm 11.12.

Windows PowerShell 5.1, live: the reachability table through loopback proxies gives the expected answer in all 13 of its HTTP cases (5.1 has no SOCKS), where the previous head counted a proxy's 500, 502, 503 and 504 as answers; with uv's installer run in the child powershell.exe, an installer that works returns only uv's path and its output reaches the console; one that ends with exit 1 leaves the script running and hands over to the mirror only when releases.astral.sh and GitHub cannot be reached; a fetch astral.sh answered stays final; with a host named for uv's installer nothing is probed and the mirror is not used; the Python mirror stays unset beside each Python source a user can name; the uv index is read as intended in 11 cases. Install-Chromium with a python that prints returns only its true or false.

Mutation: 222 one-construct mutants, each run against the tests that guard it after those tests passed unmutated, and each turned them red: 2 for this round's changes (the mirror's answer reaching the fetch, and its answer being ignored), 27 for the last round's changes (the opt-out in the launch environment and in the case of its name, a connect the machine refuses and a file of our own, the budget of a launch and the wait for another install, the marker of a finished install, the npm of a dead Raven, the cache of a node, the node's own cache directory and the budget the host waits for, the host's own switch and the switch the host forwards, the mark of a uv.toml in the runtime, in the helper and in both installers, the quoted keys of a uv.toml in both installers, the directory npm is asked in and the scope of its fallback in both installers, and the npm fallback put back), and the earlier sets, re-run where their code still exists, re-anchored where it moved. At the final head: 57 for the last round's changes (the tunnel status in each of the five deciders, no route, the certificate carried as urllib's reason, each npx shape and each case left to npx, the install's refresh, lock and deadline, the bin's containment and start, and each launch site handing over where it runs); and the earlier sets, re-run where their code still exists, re-anchored where it moved: 24 of the 31 in the runtime's Python, 23 of the 25 in install.sh and 19 of the 20 in install.ps1, all 18 of the first review round's, 20 of the second's 23 and 32 of the third's 33 (the rest targeted code the review rounds removed, or make the same change as a newer mutant).

This round changed the node's answer, the switch's case, the node's own install, what a proxy that could not be used counts as, the order an ACP launch asks in, and where the switch's other case is asked of it. The order reaches the paths that prefetch: the mirror is asked before the fetch, in the pool and in the local ping as well as in the direct launch, so a launch the mirror took over is never fetched from a registry it could not reach. The budget's clock started inside the worker, so the wait for one of the pool's workers did not count against it: on a small node, where the daemon admits more launches at once than the pool has workers, the launch that waited for one lost the machine's link and took the agents already running on it with it. The clock now starts before the work is queued, the whole preparation is bounded from the event loop, the probes and the npm config read are bounded by the same deadline, and a host whose probe ran out of time moves nothing at all. The switch is read in either case of its name now, in this process's own environment as well as in the launch's, so it turns the fallback off where Raven runs and travels to a node that runs the launch. The node's own install, which came with #1068, read the same switch by a name of its own and in one case only; it reads it through the runtime's reader now, so the two cannot read it differently. httpx cannot use a SOCKS proxy without the socksio package and says so with an ImportError, which the rule did not know and answered that the host had answered -- so the only copy of the rule that could use a SOCKS proxy was the only one that took the mirror for an outage it could not have seen. A proxy that could not be used at all counts: it was never asked, which is what the copies that cannot use a SOCKS proxy at all have always answered. The ACP launch asked the npx fetch before the fallback, and that fetch goes to the default registry, the very one the fallback decides about: so on a machine that cannot reach the registry the fetch failed first, the launch raised npm's own error, and the fallback that exists for that case was never consulted. The fallback is asked first now, and the fetch is given its answer: an npx launch the mirror took over is started from what the mirror installed, and there is nothing left for npx to fetch. Windows keeps one name for a variable, so a switch set in another case lands under the one name and the reader is never asked about it: the case that said the switch was read in either case passed on Windows for that reason rather than because the reader reads either case. The other case is asked where a system can hold it, on POSIX, and the name the install and the runtime share is asked everywhere.

Windows 11, native, over the files this round reaches (test_utils_mirrors, test_node_server, test_node_client, test_mcp_client_transport, test_subagent_host_env, test_subagent_third_party): 560 passed, 41 skipped; with the install's own file added, 296 passed, 21 skipped. With the ACP files the fetch touches added, over the fifteen files of both rounds: 1406 passed, 188 skipped; the 43 failing ids are the same as at the head without this change (nltk and the dictionary, the CLI backend's POSIX process groups, the ACP journal's file cases, and two cases of the Windows console's own encoding). The table every copy of the rule answers alike, on a Windows install without the socksio package: 4 passed, 1 skipped, where it failed at the previous head on the two SOCKS cases; with the socksio package, in a Linux container, the same table passes (3 passed, 2 skipped). The ACP launch's own case, the fetch given the argv the fallback answered with, is in test_subagent_host_env. The switch's other case, asked on POSIX where a system can hold it, is in test_utils_mirrors and in test_node_install. Linux, in a python:3.12 uv container: test_utils_mirrors 184 passed, 1 skipped; test_node_client 94 passed; test_node_install with test_utils_mirrors 316 passed, 1 skipped; test_node_server 114 passed and 7 failed, the seven being the cases that read the process tree with ps, which that image does not have and which fail on the base tree as well. The daemon's own case, launches that outnumber the pool's workers and each answered within the budget, passes there. Nine one-construct mutants of this round each turn a case red: the bound from the event loop removed, the probe's own bound removed, a host that ran out of time moving anyway, and the worker taking a clock of its own instead of the caller's, the two readers of the switch reading the name in one case only, the rule counting a proxy that could not be used as one that answered, and the fetch asked before the fallback, and the switch's own environment read in one case only; the last five are caught where the name of an environment variable is case-sensitive, or where the socksio package is not installed.

Also run: ruff check and ruff format --check clean; lint-imports 10 kept; ty reports the same diagnostics as base on every changed module (none in the new one); deptry matches base; no added line outside docs-site/ is non-ASCII; the docs build passes with --strict; commitlint, scripts/check_commit_messages.py, scripts/check_source_language.py and scripts/check_large_files.py pass over the 19 commits, each beside a control commit it refuses; install.ps1 parses under Windows PowerShell 5.1.

Measured again on 2026-10-22 from the same China Unicom line with no proxy (three rounds): every host with a mirror either answered directly or its mirror did, in every round (huggingface.co and the dictionary on raw.githubusercontent.com in none, hf-mirror.com and jsDelivr in all); Raven's own release files on github.com answered in none or one of three.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

A mirror is trusted as far as the host it stands in for, except where a digest is pinned in the script (LibreOffice, the CJK font). When a fallback fires, the mirror is the only integrity authority for what it serves: uvx packages from TUNA and npx packages from npmmirror carry no lockfile or digest tied to PyPI or registry.npmjs.org (npm checks each tarball against the integrity the mirror's own metadata lists), PlugHub-vetted MCP entries included, whose vetting refuses an entry that names an index or a registry itself so that the confirm dialog shows what runs: the fallback adds --default-index to a uvx launch, and starts an npx entry's bin from Raven's own install of the same package instead of through npx; the Node tarball is checked against the SHASUMS256.txt on the mirror, where main checked nodejs.org's own; the uv wheel against the digest the index lists beside its link, skipped when the index lists none; Chromium and Raven's own dependency closure come from the mirror as served. Those checks catch a corrupt download and not a dishonest mirror. Anything that makes the default host unanswerable triggers the switch, an on-path party included; the mirror hosts are constants and their TLS is verified, so the switch can be forced but not pointed elsewhere. Nothing of the user's credentials reaches a mirror: the Hugging Face token is withheld. With every host reachable nothing changes except one HEAD request per default host every 10 minutes before a uvx or npx launch; a host that drops packets costs that launch up to 5 s once per 10 minutes. RAVEN_NO_MIRROR=1 restores the previous behaviour everywhere.

Checked and left as they are:

  • The runtime's probe asks from the Raven process, through urllib's proxies (the environment's, or the system's on Windows and macOS when the environment names none), and install.ps1 probes through PowerShell's; uv and npm in the launched process read only the environment's and their own configuration. Where those differ a fallback can be missed, which is the old behaviour, or taken needlessly, through a mirror that answered.
  • An npx launch's bin started from Raven's directory gets the launch's environment as it is, without what npx adds for the programs it starts: npm's npm_* variables and the node_modules/.bin directories on PATH. $RAVEN_HOME/cache/npx keeps one install per set of specs, as npx keeps its cache, and nothing removes them; a Raven node's installs are under the node's own directory instead, where its records are. A range, tag or bare name costs each such launch an npm update (about 1 to 3 s measured against npmmirror). On Windows a bin whose first line is not node's starts through npx as configured.
  • A SOCKS proxy's reply is not read: a refusal by its rules switches like a host it could not reach, since PowerShell has the reply only in an exception's text and urllib and Windows PowerShell 5.1 cannot use a SOCKS proxy at all. PowerShell 7 before 7.4 has no HttpRequestError, so there a refused tunnel and a certificate failure both count as unreachable.
  • uv records the index an install used in uv-receipt.toml (measured): after an installer fell back, a manual uv tool upgrade raven resolves from the mirror. The installers' and raven upgrade's own uv tool install --force do not reuse it (measured).
  • The installers read uv.toml with patterns rather than a TOML parser. A default = true at the start of a line or after { or , counts as a chosen index, a commented-out one included, which errs toward leaving the user's setup alone. Its address is read from index-url, an [[index]] table or an inline entry with a bare url key; a key may be quoted, as TOML allows, and a file with a byte-order mark is read past it, as uv reads it. An index a uv.toml names any other way -- an address that spans lines, say -- keeps the mirror away from uv's own download without being used, and the warning says to set UV_DEFAULT_INDEX.
  • The hosts uv's installer and playwright download from are asked with a HEAD request after they failed; one that answers it while the download itself stalled keeps the mirror away, the old behaviour.
  • Any of uv's settings for where uv or a Python comes from counts as the user's choice whatever it names, releases.astral.sh or GitHub included. UV_ASTRAL_MIRROR_URL is counted though uv 0.13.4 leaves it out of its published settings reference, because uv acts on it.
  • Under Windows PowerShell 5.1 an npm that warns on stderr while answering reads as one that cannot say (measured with a stand-in npm.ps1 whose child process warns; PowerShell 7 reads the answer), so a source build there keeps the registry it had. A release install builds nothing with npm.

Related Issues

N/A

@LivXue
LivXue requested review from 0xKT and gloryfromca October 21, 2026 14:32

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: the uv bootstrap must preserve the unreachable-only fallback boundary, and its new tests must be deterministic.

I reviewed the complete github/main...HEAD diff and the surrounding download, package-runner, resource, installer, upgrade, node, MCP, ACP, release, and plugin-list paths. I also checked the repository rules and context map, callers and configuration precedence, relevant branch history, backward compatibility, whether tests were weakened, and the runtime/remote-node architecture. I found no weakened tests or additional architecture violations.

Verification: git diff --check github/main...HEAD, scripts/check_source_language.py github/main..HEAD, and scripts/check_large_files.py github/main..HEAD passed. The focused pytest set completed with 1167 passed, 15 failed, and 34 skipped: three failures reproduce the inline /usr/bin/uv harness defect; the other twelve require the optional NLTK/datrie segmenter environment, and uv sync --all-extras --dev could not build datrie because this runner has no gcc.

Comment thread install.sh Resolved
Comment thread tests/test_install_script.py Resolved
@LivXue

LivXue commented Oct 21, 2026

Copy link
Copy Markdown
Member Author

The Windows installer job failed on the previous head: Ensure-Uv ran uv's installer in a PowerShell of its own and returned what that process printed beside uv's path, so the caller ran the lot as the uv command (The term 'downloading uv 0.13.4 ... uv.exe' is not recognized as a name of a cmdlet). Fixed in 945980b, with the same fix for Install-Chromium, which returned playwright's progress output beside its true or false: that read as true, so a failed Chromium download was never reported. The tests that guard both now use an installer and a python that print; both were also run live under Windows PowerShell 5.1.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: honor uv installer base URLs the operator explicitly configured.

The two previous blockers are fixed, and I replied to and resolved both threads. I reviewed the new four-commit delta plus its surrounding POSIX/PowerShell installer paths, and rechecked the full change's repository rules, callers, history, backward compatibility, configuration precedence, test strength, and runtime architecture. I found no weakened tests.

Verification: uv run pytest tests/test_install_script.py passed with 141 passed and 41 PowerShell-only skips; git diff --check github/main...HEAD, scripts/check_source_language.py github/main..HEAD, and scripts/check_large_files.py github/main..HEAD passed. PowerShell is not installed on this runner, so those platform-only cases could not run locally.

Comment thread install.sh Resolved

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The prior installer-host blocker is fixed: explicit uv sources now prevent Raven from substituting TUNA, and the same precedence protection covers every Python-download source uv 0.13.4 reads. I verified the current delta, the full PR context, installer/runtime/upgrade callers, history, backward compatibility of the default fallback, repository rules, and test strength; I found no weakened coverage or new plain error.

Verification: uv run pytest tests/test_install_script.py tests/test_utils_mirrors.py tests/test_updates_upgrade.py (446 passed, 54 environment-dependent skips); uv run pytest tests/test_cli_onboard_commands.py (356 passed); source-language, large-file, and git diff --check gates passed. PowerShell-only paths could not run locally because pwsh is unavailable, but their tests were expanded consistently and the author reports native Windows and PowerShell verification. The settled thread I opened has been replied to and resolved.

Comment thread raven/utils/mirrors.py Resolved
@0xKT

0xKT commented Oct 22, 2026

Copy link
Copy Markdown
Member

Not a blocker -- two things from acceptance on cd45989 that do not block this PR, written down so they are accepted knowingly rather than found later; the blocking item is in the thread on raven/utils/mirrors.py. Both re-checked by me against code read at the head and an offline probe (loopback only).

Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

  1. Disclosure: when the fallback fires, the mirror becomes the only integrity authority for code that runs. That is this PR's purpose and it is documented, with RAVEN_NO_MIRROR=1 to turn it off; the points below are what a maintainer accepts with it, by default, for every user:

    • npx packages come from registry.npmmirror.com and uvx packages from pypi.tuna.tsinghua.edu.cn with no lockfile or digest tied to the official registry. That includes PlugHub-vetted MCP entries, which receive the user's API keys in their environment. raven/market/vetting.py refuses a catalogue entry that adds --registry, --default-index, NPM_CONFIG_* or UV_*, so that "what the confirm dialog shows is what gets executed"; the fallback adds exactly those at launch (raven/utils/mirrors.py, the npx and uvx branches of _package_launch).
    • The Node runtime's tarball is checked against the SHASUMS256.txt fetched from the same mirror (install.sh:334-342); main checked it against nodejs.org's own file, under a comment calling it supply-chain integrity. The uv wheel's sha256 comes from the mirror's own index page and is skipped when absent (install.sh, install_uv_from_mirror). Chromium comes from the mirror host (install.sh:781-784). raven's own dependency closure goes through TUNA with the release constraints exported --no-hashes (.github/workflows/release.yml:166).
    • What triggers it is anything that makes the official host unanswerable: a DNS failure, a refused or reset connection, a timeout. The mirror hosts are constants and their TLS is verified, so an on-path party cannot pick the host, but it can force the switch.
  2. Four deciders of "cannot be reached" disagree on a proxy that refuses CONNECT. raven/utils/mirrors.py says only a failure to reach a host moves a download, and that the installer and upgrade copies are held equal by a test; that test compares host URLs, not the decision. One loopback proxy answering every CONNECT with 403 Forbidden, HTTPS_PROXY pointed at it (nothing left loopback):

    • httpx raises ProxyError, and mirrors.unreachable counts it as unreachable, so raven resources and the codex version lookup move to the mirror;
    • urllib raises URLError(Tunnel connection failed: 403), which mirrors.unreachable does not count, so mirrors._answers says answered and npx/uvx launches keep the default;
    • the upgrade helper's answers() says unreachable, so raven upgrade moves to the mirror;
    • curl exits 56, which install.sh's connection_failed counts, so the installer moves too.

    So where an administrator's proxy blocks pypi.org, huggingface.co or npm by policy, the installer, raven upgrade, raven resources and the codex lookup route around the block to the mirror (if the proxy allows that host), while launches treat the same refusal as final. docs-site/docs/quick-start.md says "An HTTP error from the usual host does not switch hosts". Either reading can be defended; one rule with four answers is the gap.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: preserve user-configured npm registries and uv indexes across mirror fallback.

The new evidence in @0xKT's open review thread changes my stance on this unchanged revision. I verified the three concrete paths against the current code: the npx launch exports npm_config_registry into the long-lived agent and thereby outranks a registry in a project it later enters; install.sh's line-oriented uv config check misses an inline index = [{ ..., default = true }]; and the uv-wheel bootstrap consults only UV_DEFAULT_INDEX and UV_INDEX_URL, not UV_INDEX or uv.toml. These are configured-source precedence failures introduced by the fallback rather than pre-existing behavior. I am relying on the open thread for the finding and am not duplicating or re-grading it.

Verification: uv run pytest tests/test_utils_mirrors.py tests/test_install_script.py passed with 244 passed and 52 PowerShell-only skips. That green result does not cover the reported cases.

@LivXue

LivXue commented Oct 22, 2026

Copy link
Copy Markdown
Member Author

Windows review: the existing configured-source blocker also reproduces in install.ps1.

Reviewed head cd459895aba22ceecae34b9bbb04b289bac69d9a, base/merge base e1c44e575858c7e04e032f179ce4b4b21dd1d907. Read the complete 24-file diff, both installers, runtime mirror decisions and launch/download consumers, upgrade helper, added tests and all paginated discussions.

[P2] Preserve an inline default uv index in the Windows installer (install.ps1:671-683, called by Install-Raven at line 697). This supplements item 2 of the existing configured-source thread with native Windows evidence; that report demonstrated the shell installer.

For example, in a file selected by UV_CONFIG_FILE:

index = [{ url = "https://corp.example.internal/simple", default = true }]

Test-UvIndexChosen only recognizes line-leading index-url = or default = true. It misses this valid inline table, so when the default PyPI probe says unreachable, Use-PackageMirrors sets UV_DEFAULT_INDEX=https://pypi.tuna.tsinghua.edu.cn/simple, overriding the operator's configured default. The runtime's TOML-based check handles this shape. Direction: make the installer's configured-index decision handle TOML structure consistently, and add this inline form beside the existing table-form tests.

Isolated reproduction: actual functions extracted from this head's PowerShell AST, on Windows PowerShell 5.1.26100.9711 and PowerShell 7.6.7. Only reachability was stubbed to false; no installer entry point or download ran. Both engines produced:

Case Result
Inline default index Not recognized; TUNA override applied
Ordinary [[index]] with default = true Recognized; no override
Explicit UV_DEFAULT_INDEX Original value preserved
RAVEN_NO_MIRROR=1 No override

The selected configuration path contained spaces and non-ASCII characters. Both probe commands exit 1, deliberately signaling the unexpected inline override. Base comparison is code reading: neither Use-PackageMirrors nor a UV_DEFAULT_INDEX assignment exists in the base installer, so this automatic override is introduced here.

Other native verification (Python 3.13.12): 92 mirror utility cases, 19 PowerShell installer cases, and 41 resource/version-query/MCP cases passed. These passes do not cover away the inline-index failure. Other items in the existing registry/index thread remain separate; this is not a general merge verdict.

Limits: focused tests and isolated functions only; no full repository suite, production install/upgrade, real registry/download outage, complete ACP/remote-node integration, or POSIX/macOS execution. Runtime/download tests use controlled peers or mocks.

Exact verification commands

Python ran from the detached head with frozen dev dependencies, isolated profile/Raven/OAuth state and imports pinned to that checkout. The external PowerShell probe extracts only the configuration/mirror-environment functions and restores the child process's environment.

uv sync --frozen --python 3.13 --extra dev
# mirrors; exit 0
uv run --frozen --no-sync pytest tests/test_utils_mirrors.py -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-mirrors\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-mirrors\junit.xml

# installer-ps; exit 0
uv run --frozen --no-sync pytest tests/test_install_script.py::test_the_windows_installer_draws_the_same_line tests/test_install_script.py::test_a_windows_install_takes_packages_and_pythons_from_the_mirrors_and_gives_the_session_back tests/test_install_script.py::test_a_windows_install_keeps_an_index_in_a_uv_toml tests/test_install_script.py::test_a_windows_install_keeps_a_python_source_the_user_chose tests/test_install_script.py::test_a_windows_install_reads_only_the_uv_toml_uv_config_file_names tests/test_install_script.py::test_a_windows_install_whose_hosts_answer_is_left_alone tests/test_install_script.py::test_the_windows_opt_out_keeps_pypi tests/test_install_script.py::test_a_windows_source_build_asks_npm_which_registry_it_reads tests/test_install_script.py::test_a_windows_chromium_download_shows_its_progress_and_still_reports_how_it_went -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-installer-ps\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-installer-ps\junit.xml

# downloads-launch; exit 0
uv run --frozen --no-sync pytest tests/test_resources_fetch.py::TestDictionaryMirror tests/test_resources_fetch.py::TestDeepdocMirror tests/test_providers_codex_client_version.py tests/test_mcp_client_transport.py::test_a_stdio_server_starts_the_way_the_mirror_fallback_says tests/test_mcp_client_transport.py::test_a_stdio_servers_own_registry_wins_over_the_mirror tests/test_mcp_client_transport.py::test_a_sandboxed_stdio_server_starts_the_same_way tests/test_mcp_client_transport.py::test_a_server_whose_registry_answers_starts_as_it_always_did -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-downloads-launch\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-downloads-launch\junit.xml

# PowerShell 7.6.7; exit 1
& 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\probes\uv-inline-index.ps1' -InstallerSource 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\pr1053\head\install.ps1' -EvidenceRoot 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-inline-ps7'
# Windows PowerShell 5.1; exit 1
powershell.exe -NoProfile -ExecutionPolicy Bypass -File 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\probes\uv-inline-index.ps1' -InstallerSource 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\pr1053\head\install.ps1' -EvidenceRoot 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\2261d810cb144a6abf2e1c1096f44f2d\checks\1053-inline-ps51'

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The configured-source blocker is fixed on this revision. The npx fallback now confines the mirror to Raven's cache install and launches the package with the original environment; the four launch callers preserve that boundary. Both installers recognize inline default indexes and use UV_INDEX, environment defaults, or readable uv.toml defaults for the uv-wheel bootstrap, while an unreadable chosen form prevents substitution. The direct, HTTP-proxy, and SOCKS decisions now follow one rule across runtime downloads, runtime launches, the upgrade helper, and both installers. I did not resolve the other reviewer's thread.

I covered the repository rules, github/main...HEAD plus the range-diff across the rebase, the five new commits, launch/download callers, history, backward compatibility, architecture boundaries, and whether tests were weakened. The prior fourteen patches are patch-identical after the rebase; the new coverage expands rather than relaxes the assertions.

Verification: the first dev-only run of the eleven affected files reported 1131 passed, 65 skipped, and 13 failed: twelve lacked optional NLTK/datrie dependencies, and the SOCKS runtime case lacked optional socksio. With the declared channel extra installed, the complete cross-implementation reachability matrix passed (4 passed, 1 PowerShell-only skip). The affected suite excluding only the two unrelated optional-resource classes that this host cannot install without gcc then passed with 1132 passed and 65 platform-only skips. Source-language, large-file, and git diff --check gates passed; the worktree is clean.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility review: two additional configured-index failures reproduced natively.

Scope: complete 24-file diff and relevant installer, launch/download, upgrade and test callers at head cf362aa17d85426e9445fed5e2b2906f046d87c1; base/merge base 4315993300e538ec954c79565cbc8585006c8053. The previous inline-default-index case now passes under PowerShell 7.6.7 and Windows PowerShell 5.1.26100.9711. The cases below add different native evidence to the configured-source discussion.

  1. [P2] Read the selected configuration path literally — install.ps1:714, also the path checks/reads in Get-UvConfigIndex.

    A valid UV_CONFIG_FILE path containing [config] is interpreted as a wildcard by Test-Path $path and Get-Content $path. With a real file named selected [config] 测.toml, both inline and ordinary [[index]] defaults are missed. When the default host is unreachable, Use-PackageMirrors therefore sets UV_DEFAULT_INDEX to TUNA over the user's configured index. Reproduced on both PowerShell engines, with only reachability stubbed; ordinary paths, explicit environment settings and the opt-out were controls. Use -LiteralPath consistently for these configuration checks and reads. A temporary copy with that change preserves the chosen index in both engines.

  2. [P2] Decode uv.toml as UTF-8 in Windows PowerShell 5.1 — install.ps1:729.

    This Get-Content uses the system ANSI encoding in 5.1. A BOM-less UTF-8 uv.toml containing a non-ASCII index URL is accepted by uv, but this host's GB2312 reader corrupts its path before Get-UvNamedIndexes hands it to Install-UvFromMirror (lines 262–278). For a URL containing café/测试, the actual value becomes caf茅/娴嬭瘯; the bootstrap then asks a different /uv/ address when the normal uv download is unavailable. The same file reads correctly in PowerShell 7; changing only this reader to UTF-8 in a scratch control also fixes 5.1. Specify UTF-8 when reading uv configuration and cover a raw UTF-8 URL under 5.1.

Evidence labels: these are isolated reproductions of the actual functions extracted from this head's PowerShell AST; no installer entry point or production download ran. The download consequences follow the real callers above. Base comparison is code reading: neither the automatic mirror override nor the uv-wheel/config-value reader exists at the base, so these are introduced paths, not inherited installer failures.

Verification:

  • Mirror utilities: 161 passed, 2 skipped, including the real npm/Node launch against a loopback registry. Selected resource, version-query, MCP, ACP/CLI and upgrade callers: 71 passed.
  • Unmodified Windows-installer selection: 30 passed, 1 failed, then -x stopped it. Its child-process harness omits Windows environment variables: first it cannot execute powershell.exe, and adding only PATHEXT exposes a managed-PowerShell startup failure. With PATHEXT, SYSTEMROOT, WINDIR and COMSPEC restored only in that test child's environment, the same 75 selected cases pass. This is a controlled harness result, not an unmodified-suite pass.
  • The cross-client table initially lacked optional socksio. After syncing the declared channel extra, its three Python implementations pass and the POSIX installer skips; its PowerShell case also passes with the same child-environment control. git diff --check passes.
  • Both bracket-path probes fail at the reviewed head and pass with a literal-path scratch control. The UTF-8 probe fails on 5.1, passes on 7, and passes on 5.1 with the UTF-8 reader control. The prior inline-index probe passes on both engines.

Limits: focused native checks only; no full suite, live install/upgrade, real mirror outage, POSIX/macOS execution or complete remote-node integration. The scratch controls do not change repository files.

Exact verification commands

Python used an isolated uv environment, temporary profile/Raven/OAuth directories and UTF-8 mode. Imports were checked against the pinned checkout. The external test plugin restores only the four Windows variables described above, leaving the installer and repository tests untouched.

$run = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/7de18cba650e4f9bb485b4cdff545a58'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'
Set-Location -LiteralPath (Join-Path $run 'pr1053/head')
uv sync --frozen --extra dev --dev # exit 0
# 1053-utils: 161 passed, 0 failed, 2 skipped; exit 0
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_utils_mirrors.py' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-utils\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-utils\junit.xml'
# 1053-callers: 71 passed, 0 failed, 0 skipped; exit 0
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_resources_fetch.py::TestDictionaryMirror' 'tests/test_resources_fetch.py::TestDeepdocMirror' 'tests/test_providers_codex_client_version.py' 'tests/test_mcp_client_transport.py' 'tests/test_subagent_host_env.py::test_an_acp_child_starts_the_way_the_mirror_fallback_says' 'tests/test_subagent_third_party.py::test_cli_backend_starts_the_way_the_mirror_fallback_says' 'tests/test_channels_whatsapp.py::test_the_bridge_install_fetches_from_a_mirror_when_npm_cannot_be_reached' 'tests/test_channels_whatsapp.py::test_the_bridge_install_inherits_the_environment_when_npm_answers' 'tests/test_updates_upgrade.py::TestTheHelperFallsBackToAMirror' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-callers\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-callers\junit.xml'
# 1053-installer-ps: 30 passed, 1 failed, 0 skipped; exit 1
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py' '-k' 'windows' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-installer-ps\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-installer-ps\junit.xml'
# 1053-installer-ps-winenv: 75 passed, 0 failed, 0 skipped; exit 0
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py' '-k' 'windows' '-p' 'no:randomly' '-p' 'install_harness_env' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-installer-ps-winenv\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-installer-ps-winenv\junit.xml'
uv sync --frozen --extra dev --extra channel-telegram --dev # exit 0; supplies socksio
# 1053-rule-with-socks: 3 passed, 1 failed, 1 skipped; exit 1
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_every_copy_of_the_rule_answers_each_case_alike' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-rule-with-socks\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-rule-with-socks\junit.xml'
# 1053-rule-ps-winenv: 1 passed, 0 failed, 0 skipped; exit 0
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\pr1053\head';$env:PYTHONPATH += ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_every_copy_of_the_rule_answers_each_case_alike[install.ps1]' '-p' 'no:randomly' '-p' 'install_harness_env' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-rule-ps-winenv\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\7de18cba650e4f9bb485b4cdff545a58\checks\1053-rule-ps-winenv\junit.xml'
$src = Join-Path $run 'pr1053/head/install.ps1'
& (Join-Path $run 'probes/uv-inline-index.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-inline-ps7') # exit 0
powershell.exe -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-inline-index.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-inline-ps51') # exit 0
& (Join-Path $run 'probes/uv-bracket-config.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-bracket-ps7') # exit 1
powershell.exe -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-bracket-config.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-bracket-ps51') # exit 1
& (Join-Path $run 'probes/uv-config-encoding.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-encoding-ps7') # exit 0
powershell.exe -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-config-encoding.ps1') -InstallerSource $src -EvidenceRoot (Join-Path $run 'checks/1053-encoding-ps51') # exit 1
# Controls: same probe, -InstallerSource pointing at the scratch copies; all exit 0:
# install-utf8-control.ps1: only the config-value reader gains -Encoding UTF8/-LiteralPath.
# install-literal-utf8-control.ps1: the config checks/reads also gain -LiteralPath.
$config = (Get-Content -LiteralPath (Join-Path $run 'checks/1053-encoding-ps51/result.json') -Raw -Encoding UTF8 | ConvertFrom-Json).config
uv --offline --config-file $config cache dir # accepted, exit 0; malformed-TOML control exits 2
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check 4315993300e538ec954c79565cbc8585006c8053..cf362aa17d85426e9445fed5e2b2906f046d87c1 # exit 0

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: read the selected uv configuration literally and as UTF-8 in the Windows installer.

The new native-Windows evidence changes my stance on this unchanged revision. The configured-index fix still uses wildcard-aware Test-Path and Get-Content, so a valid UV_CONFIG_FILE path containing brackets is missed and TUNA can override the selected index. The same reader leaves encoding unspecified, so Windows PowerShell 5.1 decodes a BOM-less UTF-8 uv.toml through the system ANSI code page and can request a corrupted non-ASCII index URL. Both failures are in the newly added fallback path and the supplied scratch controls isolate the required literal-path and UTF-8 corrections. I am relying on that Windows report rather than duplicating it into another review thread.

Verification here: uv run --locked --extra channel-telegram --extra dev pytest tests/test_install_script.py -q passed with 178 passed and 62 PowerShell-only skips. That green result does not exercise either native-Windows failure.

@LivXue
LivXue force-pushed the feat/china_mirror_fallbacks branch from cf362aa to 4b5b8a7 Compare October 23, 2026 03:18

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: fix the existing Windows configured-index failures before merge.

I reviewed the rebased revision and the complete delta from cf362aa17d85. Range-diff shows the prior 19 PR patches are unchanged; the only PR-specific addition is the test-harness environment forwarding in 4b5b8a700. That is a valid native-Windows harness fix and does not weaken the tests, but it neither changes install.ps1 nor adds coverage for the two reported cases. The installer still reads UV_CONFIG_FILE with wildcard-aware Test-Path/Get-Content and reads BOM-less files with the Windows PowerShell default encoding, so the standing bracketed-path and UTF-8 failures remain concrete.

Verification: uv run --locked --extra channel-telegram --extra dev pytest tests/test_install_script.py -q passed with 178 tests and skipped 62 PowerShell-only tests on this host. git diff --check github/main...HEAD passed. I also covered the repository rules, rebased diff/history, the affected installer caller and harness, backward compatibility, test-strengthening/weakening, and the applicable architecture boundaries; the rest of the previously reviewed patch series is patch-identical after the rebase.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The Windows configured-index blocker is fixed. Test-UvConfigHas and Get-UvConfigIndex now use literal paths and explicit UTF-8 decoding at the shared read points, so every installer caller gets the correction. The two focused tests cover the bracketed UV_CONFIG_FILE path and BOM-less UTF-8 URL, and withdrawing either flag recreates the concrete failures reported on the previous revision.

Verification: uv run --locked --extra channel-telegram --extra dev pytest tests/test_install_script.py -q passed with 178 tests and 64 PowerShell-only skips on this host; git diff --check github/main...HEAD passed. The current Windows installer CI job also passes. I reviewed the revision delta, affected helpers and callers, test changes, history, backward compatibility, repository rules, and applicable architecture boundaries; the tests were strengthened rather than weakened. I also swept the review threads I opened, and all are already resolved.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility review of head b66929590eb03aa27d7090f0fbbd2ef3ce42c5da against base 568e8c86396e66bc54b205a603c032bc9f265f0e. Scope: all 24 changed files, installer/runtime callers, tests, and existing discussions. The exact-SHA source archive was independently verified against Git tree 522219f1f90444cfa77eccec3ffcd6692c322667.

The two runtime problems from the preceding Windows review are fixed: bracketed UV_CONFIG_FILE paths work, and BOM-less UTF-8 index URLs survive both PowerShell 5.1 and 7. No further Windows runtime defect was found in the inspected scope.

[P3] Exercise the URL value reader in the UTF-8 regression test — tests/test_install_script.py:3167-3181.

Reproduced test coverage gap: this case only asserts that Use-PackageMirrors leaves UV_DEFAULT_INDEX unset. Its harness loads Get-UvConfigIndex but never calls it; Test-UvIndexChosen can recognize the ASCII key even when the URL value is misdecoded. It also runs pwsh, whose default encoding already is UTF-8. In a temporary installer copy I removed only -Encoding UTF8 from Get-UvConfigIndex: this new test still passed, while the same copy returned a corrupted URL under native PowerShell 5.1 (GB2312), reproducing the old bootstrap-reader failure. The unmodified head returned the exact original URL. Please assert the actual Get-UvConfigIndex result, including a non-ASCII URL, and exercise that assertion in Windows PowerShell 5.1. This is a gap in the new regression test; the current runtime fix works.

Verification on native Windows: installer Windows cases 77 passed; mirror utility cases 161 passed, 2 skipped; shared installer-rule cases 4 passed, 1 skipped. The utility run included real npm/Node requests to a loopback registry. Isolated functions extracted from the actual installer passed bracketed paths and UTF-8 URL probes in PowerShell 5.1.26100.9711 / 7.6.7; the mutation control above gave 1 pytest pass / a failing 5.1 URL assertion. git diff --check passed. This extends the prior complete review: the rebase retained the earlier commits, 22 changed files have identical contents, and both subsequent file changes were inspected in full. No live installation, external mirror outage, POSIX installer execution, or full repository suite was attempted; the 3 skips and those scenarios remain coverage limits.

Exact verification commands and results

Each Python invocation used an isolated uv environment and temporary profile/Raven/OAuth/temp directories. UTF-8 mode was enabled and imports were asserted to belong to the selected checkout.

$run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'

# 1053-current-ps: 77 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py' '-k' 'windows' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-ps\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-ps\junit.xml'

# 1053-current-utils: 161 passed, 0 failed, 2 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_utils_mirrors.py' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-utils\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-utils\junit.xml'

# 1053-current-rule: 4 passed, 0 failed, 1 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_every_copy_of_the_rule_answers_each_case_alike' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-rule\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-current-rule\junit.xml'

# 1053-utf8-test-mutant: 1 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\pr1053\head-archive' + ';' + (Join-Path $run 'probes')
$env:RAVEN_REVIEW_INSTALLER_CONTROL = Join-Path $run 'probes/install-without-value-utf8.ps1'
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_a_windows_install_decodes_a_uv_config_file_as_utf8' '-p' 'no:randomly' '-p' 'utf8_reader_control' '-s' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-utf8-test-mutant\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\82c93effef024a64859ac411362314ec\checks\1053-utf8-test-mutant\junit.xml'
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check 568e8c86396e66bc54b205a603c032bc9f265f0e..522219f1f90444cfa77eccec3ffcd6692c322667 # exit 0

The function-only probes AST-extracted the named installer functions, with no install entry point. Exact final probe invocations (all exit 0 except the deliberate mutation, exit 1):

$installer = Join-Path $run 'pr1053/head-archive/install.ps1'
pwsh -NoLogo -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-bracket-config.ps1') -InstallerSource $installer -EvidenceRoot (Join-Path $run 'checks/1053-final-uv-bracket-config-pwsh') # exit 0
pwsh -NoLogo -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-config-encoding.ps1') -InstallerSource $installer -EvidenceRoot (Join-Path $run 'checks/1053-final-uv-config-encoding-pwsh') # exit 0
powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-bracket-config.ps1') -InstallerSource $installer -EvidenceRoot (Join-Path $run 'checks/1053-final-uv-bracket-config-powershell.exe') # exit 0
powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-config-encoding.ps1') -InstallerSource $installer -EvidenceRoot (Join-Path $run 'checks/1053-final-uv-config-encoding-powershell.exe') # exit 0
powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File (Join-Path $run 'probes/uv-config-encoding.ps1') -InstallerSource (Join-Path $run 'probes/install-without-value-utf8.ps1') -EvidenceRoot (Join-Path $run 'checks/1053-final-mutant-encoding-ps51') # exit 1, URL differs

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Addressed the UTF-8 regression gap in cfd8581. You were right that the previous case could pass without exercising the value reader: it only asserted UV_DEFAULT_INDEX, Test-UvIndexChosen matches the ASCII key, and the case ran under pwsh whose default encoding is already UTF-8.

Change:

  • The package-step harness now prints the value Get-UvConfigIndex returns.
  • test_a_windows_install_decodes_a_uv_config_file_as_utf8 is parametrised over pwsh and powershell.exe and asserts that value equals the intact non-ASCII URL, alongside the existing unchanged-mirror assertion.

Native Windows controls (Windows 11, powershell.exe 5.1.26100.9711 and pwsh 7.6.7):

  • Fix in place: both parameters pass, and CONFIG equals the original URL.
  • With only -Encoding UTF8 removed from Get-UvConfigIndex: the powershell parameter fails with CONFIG showing the ANSI mis-decoding, while pwsh still passes. That is now the case the test pins.

tests/test_install_script.py with the narrower run: the two parametrised cases pass. Full file on this host: 128 passed, 114 POSIX-only skips, 1 failed. The failure is test_every_copy_of_the_rule_answers_each_case_alike[runtime-downloads], which reproduces with my change stashed and is caused by the optional socksio dependency not being installed here (ModuleNotFoundError), not by this patch.

ruff check and ruff format --check are clean on the file. I have not resolved any thread and am not claiming approval.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The new revision closes the UTF-8 regression-test gap. The package harness exposes Get-UvConfigIndex directly, the test asserts the intact non-ASCII URL, and it runs against both pwsh and powershell.exe. The shell parameter defaults to the previous pwsh behavior, so existing harness callers are unchanged. The production fix remains unchanged.

Verification: uv run --locked --extra channel-telegram --extra dev pytest tests/test_install_script.py -q passed with 178 tests and 65 environment-dependent skips; the two shell-specific UTF-8 parameters are among the skips on this Linux host. The recorded native Windows controls pass under PowerShell 7.6.7 and Windows PowerShell 5.1, and removing -Encoding UTF8 makes only the 5.1 parameter fail as intended. git diff --check github/main...HEAD passed, and the current Windows installer CI job passes. I reviewed the delta, affected harness callers, test-strengthening risk, history, backward compatibility, repository rules, and applicable architecture boundaries. All threads I opened remain resolved.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility re-review at head cfd85814604466eaeee63566bd3e9c9bd930a77c, base 568e8c86396e66bc54b205a603c032bc9f265f0e.

No additional Windows compatibility issue was found in the inspected scope. The previous UTF-8 regression-test gap is addressed: the harness calls Get-UvConfigIndex, checks the intact URL, and runs the assertion under both shells. The PR's full 24-file scope is retained from the preceding inspection: Git comparison confirms that 23 files are byte-identical and the only revision delta is in tests/test_install_script.py. I read that entire delta, the affected harness/functions/callers, the current description, and updated discussions; production code and the recorded base are unchanged.

Native verification: both UTF-8 cases pass under PowerShell 7.6.7 and Windows PowerShell 5.1.26100.9711 (this host's ANSI encoding is GB2312). In a temporary installer copy outside the checkout, removing only -Encoding UTF8 from Get-UvConfigIndex now makes the 5.1 case fail its CONFIG == url assertion with the misdecoded URL; the PowerShell 7 case still passes. This confirms the new test detects the actual regression reported previously.

Current-head results: installer Windows selection 78 passed; shared reachability-rule selection 4 passed, 1 skipped (needs POSIX sh/curl). The deliberate mutation control was 1 passed, 1 expected failure. git diff --check passed, and tracked checkout files remained unchanged.

Coverage limits: this round exercised the changed installer harness and shared rule; the unchanged runtime/mirror suites were not rerun. No live installation, external mirror outage, POSIX installer execution, or full repository suite was attempted. These checks do not establish full Windows compatibility.

Exact verification commands and results

Each Python invocation used an isolated uv environment and temporary profile/Raven/OAuth/temp directories. UTF-8 mode was enabled and imports were asserted to belong to the selected checkout.

$run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'

# 1053-windows: 78 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py' '-k' 'windows' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-windows\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-windows\junit.xml'

# 1053-rule: 4 passed, 0 failed, 1 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_every_copy_of_the_rule_answers_each_case_alike' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-rule\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-rule\junit.xml'

# 1053-utf8-test-mutant: 1 passed, 1 failed, 0 skipped; exit 1
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\pr1053\head' + ';' + (Join-Path $run 'probes')
$env:RAVEN_REVIEW_INSTALLER_CONTROL = Join-Path $run 'probes/install-without-value-utf8.ps1'
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_a_windows_install_decodes_a_uv_config_file_as_utf8' '-p' 'no:randomly' '-p' 'utf8_reader_control' '-s' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-utf8-test-mutant\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\1e07a2d636424c94815b23a0317b7c4b\checks\1053-utf8-test-mutant\junit.xml'
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check 568e8c86396e66bc54b205a603c032bc9f265f0e..cfd85814604466eaeee63566bd3e9c9bd930a77c # exit 0

@LivXue
LivXue force-pushed the feat/china_mirror_fallbacks branch from cfd8581 to 7250844 Compare October 23, 2026 04:41

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This revision is a clean rebase onto the new main. Range-diff shows every one of the 22 PR patches is patch-identical to the previously reviewed revision; the only added patch is the unrelated upstream DAG/UI change from main, with no overlap in the mirror implementation or installer tests.

Verification after the rebase: uv run --locked --extra channel-telegram --extra dev pytest tests/test_install_script.py -q passed with 178 tests and 65 environment-dependent skips, and git diff --check github/main...HEAD passed. Both Ubuntu and Windows installer CI jobs pass. I covered the rebased diff and history, affected callers, backward compatibility, test integrity, repository rules, and architecture boundaries; no tests were weakened. All threads I opened remain resolved.

Comment thread raven/node/server.py Resolved
Comment thread raven/utils/mirrors.py Resolved
@0xKT

0xKT commented Oct 23, 2026

Copy link
Copy Markdown
Member

Not a blocker -- other notes from round 2 on 7250844 (the two blocking items are in their own threads; round 1's thread is closed as fixed).

Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

  1. The installers ask npm for its registry in the wrong directory. use_npm_mirror runs npm config get registry in the directory install.sh was started from (install.sh:475), then npm ci runs in ui-tui/ui-web (:504, :531) with the exported npm_config_registry, which outranks a project .npmrc there. A registry set only in the checkout's ui-web/.npmrc or ui-tui/.npmrc is replaced by npmmirror when registry.npmjs.org is down (reproduced with real npm 11.17; control from ui-web keeps it). The reverse also happens: started from another project whose .npmrc names a registry, the fallback is skipped although npm ci in ui-web goes to npmjs. install.ps1 has the same shape (Get-NpmRegistry before Push-Location, :684-702). Narrow (local-source install, untracked per-directory .npmrc), but it is the "an npm registry set anywhere npm reads one" promise; asking from the directory npm ci runs in fixes both directions.

  2. A uv.toml with a UTF-8 BOM, or with quoted keys ("index-url" = ..., "default" = true), is read by uv but missed by install.sh (and for the BOM, by the runtime and the upgrade helper), so the mirror replaces that user's index.

  3. npx fallback cache: a killed npm install leaves a directory that is later taken as finished (for an exact version npm is never run again for it); the "launches of the same specs take turns" lock does not survive a Raven restart while an orphaned npm still runs; on a node the cache lands in the machine user's ~/.raven, outside node_dir.

  4. The reach rule still disagrees on one input: a connect the OS refuses locally (EPERM/EACCES, e.g. a sandbox or firewall) counts as unreachable in the urllib copies but not in the others.

  5. For whichever of this PR and fix(*): fall back to a mainland PyPI mirror when a machine's index is unreachable or slow #1068 lands second: fix(*): fall back to a mainland PyPI mirror when a machine's index is unreachable or slow #1068 adds its own RAVEN_NO_MIRROR reader and a TUNA + Aliyun list in raven/node/install.py with a different policy (it also moves on when an index is slow and demotes the machine's configured pip index); nothing ties the two.

Checked and fine: no mirror variable reaches any launched process and no user-set variable is changed (8 launcher x program pairs, base vs head); nothing from uv.toml or UV_INDEX is ever evaluated by the shell ($(...), backticks, ; stay literal); RAVEN_NO_MIRROR=1 is honoured on every installer path on the computer it is set on.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: resolve the Raven-node launch timeout and opt-out propagation failures.

Two new blocking threads now stand on this unchanged revision, so my earlier clean stance is no longer current. I am not re-grading those findings: the cited code paths do show that Daemon.launch waits for mirror preparation inside the host client’s 60-second agent/launch budget even though npm may run for 600 seconds, and that package_launch checks only the deciding process environment for RAVEN_NO_MIRROR, not the launch environment assembled for a node agent. The thread reproductions determine the concrete failures and the authors of those threads retain their severity judgement.

Verification: uv run --locked --extra channel-telegram --extra dev pytest tests/test_utils_mirrors.py tests/test_node_server.py -q passed with 281 tests and one macOS-only skip. That green result does not cover the newly reported node timeout or opt-out propagation scenarios. I did not resolve or reply inside either thread because I did not open them.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main at a7bf9f8 (the #1063 squash) with no file overlap; the head is now 7250844. Nothing in this PR's 24-file diff changed, and the base is now current.

Post-rebase on native Windows: the 7 Windows focused cases in tests/test_install_script.py pass, including both UTF-8 shell parameters.

CI on 7250844 is complete: 21 passed, 2 skipped. Windows ACP stdio, Windows self-upgrade and the Windows installer job are all green.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility review of head 72508440e91e48b1e29bc66ff374bdc14d86ef61, base a7bf9f8928653d3c48e0c44b0a56ad36dc6e8892. The complete 24-file diff is byte-identical to the preceding review and all 22 patches are unchanged by the rebase. I checked the upstream integration delta, callers, and new discussions, and added these native Windows confirmations of existing reports.

[P2] A launch-level mirror opt-out is also ignored for local Windows launches — mirrors.py:823; additional platform evidence for the existing opt-out thread.

Reproduced decision path: on native Windows, the actual package_launch(["uvx.exe", "fake-agent"], env, cwd) added --default-index <TUNA> despite env["RAVEN_NO_MIRROR"] == "1". Setting the variable in the deciding process instead preserved the original argv. Network reachability was replaced with a deterministic outage/mirror verdict; no program was launched. Caller evidence: local AcpClient.launch passes the configured entry environment at client.py:475, before its Windows spawn branch; CliAgentBackend likewise passes the assembled launch environment. enabled() reads only the parent process, so the issue extends beyond the remote-node scenario. Honor the opt-out in the supplied launch environment using the same truth-value rules.

[P2] The Windows source-build fallback overrides a target project's npm registry — install.ps1:629; native PowerShell confirmation of the existing registry note, item 1.

Reproduced with PowerShell 7.6.7 and real npm 11.12.1: the actual installer helpers read npm's default from a separate caller directory, then set npm_config_registry when reachability was stubbed false. Reading npm configuration from the build directory then returned npmmirror despite its .npmrc selecting https://corp.example.invalid/. The paired control that queried inside the target directory retained that registry. Caller evidence: Build-WebAssets makes the decision before either Push-Location, so this affects local-source UI builds. Query each build's registry in its own directory and scope/restore each fallback so the two builds do not override each other's choices.

Both decision paths are introduced by this PR relative to base; the base's local launcher and source-build code do not apply these rewrites. Current-head UTF-8 regression parameters passed under PowerShell 7 and Windows PowerShell 5.1 (2 passed). The launch-environment assertion failed as described (1 failed); the npm probe reported the registry mismatch (exit 1) with a passing target-directory control. git diff --check passed and tracked checkout files stayed unchanged.

The node launch-timeout report was read and its deadline path inspected; its daemon reproduction was not run here (the daemon imports POSIX fcntl at both base and head). No real package download, npm installation, complete installer/agent launch, remote-node session, or full repository suite was exercised. The PowerShell probe extracted actual helper definitions without running the install entry point.

Exact verification commands and results

Each Python invocation used an isolated uv environment and temporary profile/Raven/OAuth/temp directories. UTF-8 mode was enabled and imports were asserted to belong to the selected checkout.

$run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'

# 1053-utf8: 2 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\pr1053\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\pr1053\head' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_install_script.py::test_a_windows_install_decodes_a_uv_config_file_as_utf8' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\checks\1053-utf8\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\checks\1053-utf8\junit.xml'

# 1053-optout: 0 passed, 1 failed, 0 skipped; exit 1
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\pr1053\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\pr1053\head' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\probes\test_windows_launch_optout.py' '-p' 'no:randomly' '-v' '-s' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\checks\1053-optout\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\59a8cd2e486a46398e884c5026a20de1\checks\1053-optout\junit.xml'
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check a7bf9f8928653d3c48e0c44b0a56ad36dc6e8892..72508440e91e48b1e29bc66ff374bdc14d86ef61 # exit 0

The npm reproduction used an isolated profile, empty user/global npmrc files, task-owned project directories, real npm config get registry reads, and a stubbed unreachable default registry. No install command ran.

pwsh -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Bypass -File (Join-Path $run 'probes/npm-project-registry.ps1') -InstallerSource (Join-Path $run 'pr1053/head/install.ps1') -EvidenceRoot (Join-Path $run 'checks/1053-npm-registry')
# exit 1: targetAfterCallerChoice = https://registry.npmmirror.com
# targetControl = https://corp.example.invalid/

The install read the switch out of the environment itself, and imported os
for that alone. It reads the switch through the runtime's reader now, and
the import goes with it.

Co-authored-by: MiniMax (minimax-m3.1-flash) <noreply@minimax.io>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: the concurrent node-launch thread remains open pending its owner's re-verification of the deadline fix.

I reviewed the rebased patch stack against github/main, its range-diff from e0c858041908, the new node-install opt-out consolidation, and the node launch protocol/callers and regression coverage. The prior patches are unchanged apart from expected rebase integration; the new commit makes node installation use the runtime's case-insensitive RAVEN_NO_MIRROR reader, with no circular dependency or compatibility issue found.

Independently, the current implementation now starts the preparation deadline before executor queueing, applies it to the whole async preparation, and tests concurrent launches that outnumber executor workers. The focused installer/runtime launch suite passed: 862 passed, 1 skipped (macOS-only). I found no additional blocker and no weakened test. I also checked the repository instructions, relevant runtime terminology, history, and the rebased callers.

The remaining thread was opened by another reviewer and has not yet been withdrawn or resolved, so the whole-record stance must remain blocking until its owner accepts the fix or asks for another change.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: the concurrent node-launch thread remains open pending its owner's re-verification of the deadline fix.

The only delta from the reviewed head removes the now-unused os import from raven/node/install.py. I checked the changed file, its opt-out caller and tests, the commit history, repository instructions, backward compatibility, and whether coverage was weakened; this cleanup changes no behavior and introduces no issue.

Focused verification passed: 532 passed, 1 skipped (macOS-only). The existing blocker was raised by another reviewer, whose thread remains unresolved with no newer reply, so I cannot clear it on this revision.

@LivXue

LivXue commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility re-review of PR #1053 at head ce9fd55cca5e82a18c2b140805248dcdb58e29f5, merge base e8332e5d4c9690215e6962899064d07648d9badf. The previously reviewed head was e0c85804190886168132d9965987ab11babd7e4d (whose own merge base was 2cbacbaa3adf459640e39cc33890ac412c18e221), so this review covers the branch as it now stands. Native Windows 11 (10.0.26300 x64), Python 3.13.12, uv 0.13.0, httpx 0.28.1, pwsh 7.6.7, Windows PowerShell 5.1.26100.9711, real OpenSSH at C:\Windows\System32\OpenSSH\ssh.EXE.

How the scope of this review was set

The branch was force-rewritten onto a newer base, so the raw previous->new diff is 150 files, +12013/-901, and most of it is upstream movement rather than this PR. The PR's own contribution at this head is the diff against its merge base: 29 files, +7973/-160. On the nine paths that carry the substance of the rewrite that is:

raven/acp_client/client.py          +7
raven/node/install.py               +9/-3
raven/updates/upgrade.py            +152
tests/conftest.py                   +17
tests/test_node_client.py           +56
tests/test_node_install.py          +17
tests/test_subagent_host_env.py     +64
tests/test_subagent_third_party.py  +46
tests/test_updates_upgrade.py       +288

Three sizeable pieces of the previous->new diff are already present in the merge base and are
therefore upstream, not PR changes. They are listed here so that the numbers above are read
correctly:

  • raven/node/install.py +349 (the MIRRORS, _faster, mirror_script and _TIME_OWN work): upstream.
  • the no_real_ssh_g fixture in tests/conftest.py: upstream. The PR's own tests/conftest.py
    change is a different, new autouse fixture _no_mirror_fallback that sets RAVEN_NO_MIRROR=1
    for the whole suite.
  • the curly-quote doubling in raven/updates/upgrade.py::stop_leftovers_of, the
    out.returncode != 0 guard and the editable_checkout refactor: upstream.

I reviewed the PR's own contribution as the primary object and separately verified those three
upstream pieces on real Windows, because they are Windows-only code that touches the same
paths.

What the PR's own contribution does

The PR's real change is one idea carried through five places: when a package registry or a
package index cannot be reached, fall back to a mirror, and let one switch (RAVEN_NO_MIRROR)
turn the fallback off everywhere at once. raven/node/install.py stops carrying its own
RAVEN_NO_MIRROR reader and uses raven.utils.mirrors.opt_out() instead, so the node installer
and the runtime read one name and one truthiness set. raven/acp_client/client.py asks
mirrors.package_launch_async() for the argv to start, so a uvx or npx agent is fetched from a
mirror when its default registry is down. raven/updates/upgrade.py gives the standalone
upgrade helper the same idea (it cannot import raven, so it carries a copy of the probe, the
uv.toml reader and the unreachable rule). tests/conftest.py turns the fallback off for the
whole suite so no test reaches the network, and the new tests assert that the switch is read
the same way everywhere and that a launch starts the argv the fallback answers with.

Commands run and real results

All runs used the shared venv, absolute paths, and -c \pyproject.toml plus
--rootdir , with a unique --basetemp each time.

$root = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024'
$env:UV_PROJECT_ENVIRONMENT = "$root\venv"; $env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'
uv run --frozen --no-sync --project "<HEAD>" pytest -c "<HEAD>\pyproject.toml" \
  --rootdir "<HEAD>" "<HEAD>\tests\test_x.py" -p no:randomly -p no:cacheprovider \
  -n 0 --tb=short -q --basetemp "$root\scratch\<unique>"
  1. Eight required files (test_node_install, test_node_client, test_updates_upgrade,
    test_subagent_host_env, test_subagent_third_party, test_install_script, test_utils_mirrors,
    test_resources_fetch) at the NEW head:
    60 failed, 1043 passed, 185 skipped in 243.16s exit 1
  2. Same eight files at the PREVIOUS head:
    60 failed, 980 passed, 181 skipped in 236.94s exit 1
    Failure sets compared by test id: IDENTICAL (60 vs 60, same ids, zero added, zero removed).
  3. Same files at the NEW BASE (tests/test_utils_mirrors.py does not exist at the base, so 7 files):
    61 failed, 705 passed, 78 skipped in 77.49s exit 1
    The 2 base-only failures, fixed by the PR:
    test_install_script.py::test_a_windows_node_download_that_cannot_be_verified_is_never_extracted[digest-mismatch-mismatch]
    test_install_script.py::test_a_windows_node_download_that_matches_its_published_digest_is_installed
  4. tests/test_node_server.py alone at the NEW head:
    1 skipped in 0.37s exit 5
    SKIPPED ... "the daemon is POSIX: it locks with fcntl and listens on a unix socket"
  5. The PR's own new tests (by -k, across the five changed test files) at the NEW head:
    23 passed, 760 deselected in 11.40s exit 0
  6. All ten modules that request the no_real_ssh_g fixture at the NEW head:
    4 failed, 357 passed, 142 skipped in 27.45s exit 1
    The same four at the PREVIOUS head: 4 failed (identical ids) exit 1
    The same four at the NEW BASE: 4 failed (identical ids) exit 1
  7. The remaining PR test files (test_channels_whatsapp, test_mcp_client_transport,
    test_providers_codex_client_version, test_node_server) at the NEW head:
    95 passed, 1 skipped in 19.19s exit 0
    Same four at the NEW BASE: 87 passed, 1 skipped in 19.42s exit 0
  8. git diff --check e0c8580 ce9fd55 -> exit 0
    git diff --check e8332e5 ce9fd55 -> exit 0
  9. Socksio availability, in the shared venv at the NEW head:
    uv run --frozen --no-sync python -c "import socksio"
    ModuleNotFoundError: No module named 'socksio' exit 1

Findings

1. PR-OWNED, STILL STANDS: the socksio gap defeats the mirror fallback on Windows

The previously reported defect is unchanged and still reproduces at the new head.

a. The three files are byte-identical between the two heads:
git diff e0c8580 ce9fd55 -- raven/utils/mirrors.py -> empty, exit 0
git diff e0c8580 ce9fd55 -- tests/test_install_script.py -> empty, exit 0
git diff e0c8580 ce9fd55 -- pyproject.toml -> empty, exit 0
None of them is in the PR's own contribution, so none of them was touched by this rewrite.

b. socksio is absent: ModuleNotFoundError: No module named 'socksio' (exit 1).
httpx 0.28.1 raises the ImportError at httpx/_transports/default.py:187-193:
"Using SOCKS proxy, but the 'socksio' package is not installed.
Make sure to install httpx using pip install httpx[socks]."

c. Why the ImportError escapes the unreachable check. raven/utils/mirrors.py:124-181 is
unreachable(exc). It walks the cause chain and matches each error against the tuple built at
raven/utils/mirrors.py:164-167:
network = (ConnectionError, TimeoutError, socket.gaierror, ssl.SSLError)
network += (httpx.TimeoutException, httpx.NetworkError, httpx.RemoteProtocolError,
httpx.ProxyError)
An ImportError (chain: ImportError -> ModuleNotFoundError) is in none of those, is not an
OSError with a no-route or locally-refused errno, and is not a certificate failure, so
unreachable() falls through to return False at raven/utils/mirrors.py:181. False means
"the host answered". The httpx caller in tests/test_install_script.py:1845-1851 catches
Exception and answers not mirrors.unreachable(exc), so the host reads as REACHABLE.

Standalone repro at the new head (real httpx, real env, real mirrors module):
socksio importable? NO -> ModuleNotFoundError No module named 'socksio'
httpx 0.28.1
socks5h://127.0.0.1:1 -> raised ImportError
message: Using SOCKS proxy, but the 'socksio' package is not installed. ...
mirrors.unreachable(exc) = False
=> runtime-downloads would answer REACHABLE: True
chain: ['ImportError', 'ModuleNotFoundError']

d. The gate is tests/test_install_script.py:1933, assert said == _REACHABILITY, inside
test_every_copy_of_the_rule_answers_each_case_alike[runtime-downloads]. Real failure:
{'socks-host-unreachable': True} != {'socks-host-unreachable': False}
{'socks-refused-by-its-rules': True} != {'socks-refused-by-its-rules': False}
It fails at the NEW head, fails at the PREVIOUS head (same failure ids, see run 2), and the
test does not exist at the NEW BASE (the base has no _runtime_downloads, no _REACHABILITY
and no test_every_copy_of_the_rule_answers_each_case_alike). So this is a PR-introduced
defect that is still live: the PR added both raven/utils/mirrors.py (the file does not exist
at the base at all) and the parity test that fails on a stock Windows install.

Fix options, either is enough: make unreachable() treat an ImportError whose message names
socksio as "host not reachable" (mirrors.py:159-181), or declare socksio as a real
dependency of the httpx path on win32 (pyproject.toml) rather than leaving it to the
telegram extra.

2. REGRESSION (introduced by the PR, platform-independent): the new mirror fallback in the ACP launch path can never fire for an npx launch

raven/acp_client/client.py:468 calls the upstream npx_fetch.ensure_fetched() and only
raven/acp_client/client.py:480 calls the PR's new
argv = await mirrors.package_launch_async(argv, {**base_env, **(env or {})}, cwd).
ensure_fetched raises AcpConnectionError carrying npm's own words when the fetch fails, and a
fetch fails exactly when the default npm registry cannot be reached, which is the one case
package_launch exists to cover. So the fallback is consulted only when the fetch already
succeeded.

Proved on the new head by driving the real AcpClient.launch with ensure_fetched and
mirrors.package_launch instrumented:
case 1, the fetch fails (registry unreachable):
launch raised AcpConnectionError: acp agent 'npx-demo': npx failed: npm ERR! ...
mirrors.package_launch consulted? False []
case 2, the fetch succeeds:
mirrors.package_launch consulted? True [['npx', '-y', '@scope/server']]

This is not a Windows issue, and I say so plainly: it is platform-independent and it is
introduced by this PR. The PR's own new test
tests/test_subagent_host_env.py::test_an_acp_child_starts_the_way_the_mirror_fallback_says
cannot catch it: it mocks mirrors.package_launch and launches a python command, never an npx
one. The same ordering does not affect uvx, because fetchable_package() returns None for a
non-npx command and ensure_fetched returns at once.

3. NON-WINDOWS (platform-independent, code is correct): uv.toml reading in the new upgrade helper

The PR adds uv_configs(), names_a_default_index() and mirror_environment() to the standalone
upgrade helper (raven/updates/upgrade.py). Checked against the real uv on this machine rather
than assumed: uv 0.13.0 REJECTS default-index as an unknown field in uv.toml ("unknown field
default-index, expected one of ..."), and index-url is the accepted top-level key. So
checking config.get("index-url") is the only correct spelling, and both the new helper copy
and raven/utils/mirrors.py:964-971 check the right key. The Windows path list
(APPDATA then PROGRAMDATA, user before machine) matches uv's own discovery and matches
mirrors.py:933-942. BOM handling uses utf-8-sig in both, as uv does. Quoted TOML keys are
normalised by tomllib. The env names UV_DEFAULT_INDEX / UV_INDEX_URL / UV_INDEX are treated as
"the user chose an index", which is the conservative direction (never replace a chosen host).
No defect found.

4. NON-WINDOWS (test validity on Windows): the new switch-name test cannot fail on Windows

tests/test_node_install.py::test_the_switch_is_the_runtime_s_own_and_is_read_in_either_case
parametrises the switch name over "RAVEN_NO_MIRROR" and "raven_no_mirror" and asserts the
install reads it in either case. On Windows it passes for the wrong reason: Python's os.environ
on Windows upper-cases every key, so monkeypatch.setenv("raven_no_mirror", "1") stores it as
RAVEN_NO_MIRROR. Verified on this machine:
os.environ key for lower_case_probe: ['LOWER_CASE_PROBE']
getenv(LOWER_CASE_PROBE) = 1
So the test does not exercise mirrors._env_value's case-insensitive reader on Windows; it would
pass even with a case-sensitive reader. The production code is right (mirrors.py:95-105 compares
key.lower()), only the test's Windows value is nil. A POSIX-only assertion would be needed to
cover it.

5. Windows coverage gap: the no_real_ssh_g guard's own "-G" path never runs on Windows

The guard is upstream rather than a PR change, but it sits on the same paths this PR touches,
so I verified it on this machine. It works as documented. Driving the
shipped guard code verbatim (real ssh on PATH at C:\Windows\System32\OpenSSH\ssh.EXE):
CASE 1 [ssh -G real ssh] -> AssertionError: the real ssh -G ran against the owner's own config
CASE 2 [ssh -G stand-in] -> allowed (guard did not fire)
CASE 3 [abs path to real ssh] -> AssertionError (Path(...).stem handles ssh.exe)
CASE 4 [cmd /c ssh -G] -> NOT BLOCKED (guard bypassed)
CASE 5 [ssh -G -F config] -> allowed (guard stood aside, by design)
Popen[bytes] = subprocess.Popen[bytes]; the subclass keeps it: P[bytes]
isinstance checks against subprocess.Popen still hold
shutil and os are both imported at conftest module scope (conftest.py:11 and :41), so the
fixture does not raise NameError.

CASE 4 is a theoretical bypass only: the only ssh -G call site in the whole tree is
raven/ops/transport.py:518 (probe = [argv[0], "-G", *argv[1:]]), which never goes through
cmd or powershell. Nothing in the repo calls ssh that way.

The real problem is different and is a Windows coverage gap. The stand-in ssh the tests write
is a file named ssh with no extension and a #!{python} shebang
(tests/test_node_client.py:149-151). On Windows:
shutil.which('ssh', path=bin_dir_only) = None (PATHEXT has no bare entry)
run(abs path to stand-in): OSError [WinError 193] %1 is not a valid Win32 application
run('ssh' with stand-in first on PATH): rc = 0, out = "host x / user livxu / hostname x /
port 22 / addressfamily any / batch" <- that is the REAL OpenSSH ssh, not the stand-in
So on Windows the stand-in is never executed and the machine's own ssh.exe is what answers any
ssh -G. The guard is the only thing standing between a test and the owner's real
~/.ssh/config, and it works, but the PR's own additions to this area never exercise it:
the _FAKE_SSH "-G" branch (tests/test_node_client.py:50-53) and the PATH monkeypatch the
PR adds to the two password-machine tests (tests/test_node_client.py:1860 and :1879) are
both in tests that are skipped on Windows. Confirmed:
-k password_machine -> 2 skipped, 92 deselected in 6.68s
SKIPPED ... "the stand-in ssh hands the ssh line to a POSIX shell"
The same applies to the 47 test_subagent_third_party.py failures below: the whole CLI backend
surface is dark on Windows because its fake agent is sh.

6. Verified CORRECT (not a defect): the PowerShell curly-quote handling in upgrade.py

This is upstream (already in the merge base) rather than a PR change, but it is Windows-only
code, so I checked it against real PowerShell rather than against the mocked test. The code
doubles every quote in the class ['\u2018\u2019\u201a\u201b] before the path goes into a
single-quoted PowerShell string (raven/updates/upgrade.py:544).

Real Windows PowerShell 5.1 (what shutil.which("powershell") returns here, and what the
production code prefers) and pwsh 7.6.7 agree exactly, driven the way the code drives them
(-Command, ASCII-only codepoint dump):
plain rc=0 C:\Users\Bob.raven
ascii_doubled rc=0 C:\Users\O'Neil.raven
curly_single rc=1 "The string is missing the terminator: '." <- U+2019 ends the string
curly_doubled rc=0 C:\Users\O\u2019Neil.raven\ <- original char restored
curly_left_single rc=1 "The string is missing the terminator: '."
curly_left_doubled rc=0 C:\Users\O\u2018Neil.raven
lssa_single rc=1 terminator error
lssa_doubled rc=0 C:\Users\O\u201aNeil.raven
ldb_single rc=1 terminator error
ldb_doubled rc=0 C:\Users\O\u201bNeil.raven
two_curly_doubled rc=0 C:\Users\O\u2019\u2019Neil
The codepoint dump confirms the doubled curly quote yields the ORIGINAL U+2019, not an ASCII
quote, so the path is preserved exactly. Both PowerShell versions agree. The change is
correct on Windows.

Its tests, however, only assert the generated script text with a mocked subprocess.run
(tests/test_updates_upgrade.py, the test_upgrade_helper_cannot_close_its_quoted_root_early
family). They would pass on a PowerShell that behaved differently. I verified the behaviour
out of band, as above, and it matches, so the code and the assertion agree today.

7. INHERITED, environment gap (not a code defect): 12 failures in tests/test_resources_fetch.py

All 12 are ModuleNotFoundError: No module named 'nltk' raised by the test's own
import nltk. The shared venv has no nltk. Identical at the new head, the previous head and
the base. Not a Windows issue and not caused by this PR.

8. INHERITED, test design (not a Windows code defect): 47 failures in tests/test_subagent_third_party.py

Every one fails the same way: the fake CLI agent is the program sh, and sh is not on PATH
on Windows, so the launch dies with FileNotFoundError [WinError 2] at
raven/agent/subagent/backends/cli_agent.py:555 (asyncio.create_subprocess_exec). The captured
stderr says: "Subagent [t1] CLI agent 'envcheck': ['sh']". Identical at the new head, the
previous head and the base (47 in each). The whole CLI-backend surface is therefore unverified
on Windows. Not introduced by this PR.

9. INHERITED: 4 failures in the modules that use the ssh guard

test_acp_remote.py::test_each_agent_gets_an_owner_only_ssh_log,
test_agents_oncall_flow_transport.py::test_a_local_runner_answers_in_the_same_shape_as_ssh,
test_agents_oncall_flow_transport.py::test_the_local_cap_does_not_depend_on_the_timeout_binary,
test_cli_ops_connection_commands.py::test_adding_an_ssh_machine_writes_a_host_alias.
Same four, same ids, at the new head, the previous head and the base. Not introduced by this
PR; not in the PR's 29-file list.

10. Windows coverage gap: tests/test_node_server.py is entirely skipped

1 skipped, exit code 5 (no tests collected), reason "the daemon is POSIX: it locks with fcntl
and listens on a unix socket". The Raven node daemon has zero Windows coverage at this head,
as at the previous head and the base. The file is a PR-owned file (it is in the 29-file list),
so the gap is the PR's to own.

11. Note on the upgrade helper's probe and SOCKS

The upgrade helper's copy of the unreachable rule uses urllib, not httpx, so the socksio gap
does not reach it. On Windows urllib with a socks5:// proxy in the environment attempts a
plain TCP connect to the proxy and fails with WinError 10061 (ConnectionRefusedError), which
the helper's connection_failed() reads as "host not reached", so it would move the install to
the mirror. That is the safe direction and it is not a defect, but it is worth knowing that
the helper's probe and raven/utils/mirrors.unreachable() disagree about SOCKS on Windows: one
uses httpx and hits the ImportError, the other uses urllib and does not.

What I did NOT cover

  • I did not run the whole test suite, only the 8 required files plus test_node_server.py, the
    4 remaining PR test files, the 10 modules that use no_real_ssh_g, and a -k selection of the
    PR's new tests. No number in this report comes from a suite-wide run.
  • I did not run the ui-web or ui-tui JavaScript tests, and I did not review any TypeScript.
  • I did not review the 121 files of the whole-tree previous->new diff that are not in the PR's
    29-file list, beyond checking which side of the merge base each of them falls on. The
    upstream-attributed parts of the nine named paths (install.py +349, the no_real_ssh_g fixture,
    the upgrade.py PowerShell changes) I did read and did verify on real Windows.
  • I did not exercise a real ssh connection to a remote machine, a real node install on a remote
    POSIX host, a real uv install, a real npm install, or a real package download. The install.py
    steps are POSIX shell scripts that run on the REMOTE machine; nothing in that module is
    executed on the Windows host, and the new tests for it assert generated script text and plan
    objects only, never a shell.
  • I did not run the failing socksio test with socksio installed. The shared venv must not be
    modified, so the "4 passed with --with socksio" counter-check from the earlier review was not
    reproduced here.
  • I did not verify the behaviour of Windows PowerShell 5.1 on a machine with a non-CJK ANSI
    codepage; my 5.1 probe drove the script with -Command (Unicode), which is how the code
    drives it, so the console codepage does not enter into it.
  • I did not check the PR's own 29-file diff for files outside the nine paths listed above
    (install.ps1, install.sh, raven/mcp/client.py, raven/node/server.py, raven/resources.py,
    raven/channels/adapters/whatsapp/bridge.py, raven/providers/codex_client_version.py and
    their tests) beyond running their test files.

This is a scoped compatibility report, not a merge verdict.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The owner of the remaining concurrent-launch finding has re-verified the fix on this head and resolved the thread. That was the sole reason my stance remained blocking after my review and focused test run; with it settled, no outstanding objection remains from the code or discussion record.

@0xKT

0xKT commented Oct 24, 2026

Copy link
Copy Markdown
Member

Not a blocker; graded A on ce9fd55 (merged onto main e8332e5). One test is timing-fragile on macOS.

Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

test_utils_mirrors.py::test_an_npm_past_its_deadline_is_ended_with_what_it_started writes a stub npm and gives _npm_install half a second (_NPX_INSTALL_TIMEOUT = 0.5) before it reads the pid the stub writes. On macOS the first exec of a freshly written script can take longer than that, so the process group is ended before the stub's first line runs, started.pid is never written, and the test fails with FileNotFoundError (4 of 4 runs here). The same call outside pytest on this machine: unsandboxed, the stub had not run its first line by the deadline; under a sandbox it had. CI runs on Ubuntu and Windows only, and this PR's checks are green, so it bites a developer on a Mac rather than CI. Waiting for started.pid to appear before the deadline counts, or a longer timeout with the stub signalling readiness, would make it deterministic.

httpx cannot use a SOCKS proxy without the socksio package and says so
with an ImportError. The rule walked the error's chain, knew none of
those, and answered that the host had answered -- so the only copy of the
rule that could use a SOCKS proxy was the only one that took the mirror
for an outage it could not have seen. The copies that cannot use a SOCKS
proxy at all, urllib and the Windows PowerShell, answer that the host
was not reached, and the table every copy answers alike held on Linux
only because the socksio package was there.

A proxy that could not be used at all counts: it was never asked. The
rule says so now, and the table holds again on an install without the
socksio package, which is every Windows install that does not ask for
the telegram channel extra.
The fetch holds what npx would fetch, so that the handshake is not cut
off by a download. It goes to the default registry, the very one the
mirror fallback decides about, and it was asked first: so on a machine
that cannot reach the registry the fetch failed, the launch raised npm's
own error, and the fallback that exists for that case was never
consulted.

The fallback is asked first now, and the fetch is given its answer: an
npx launch the mirror took over is started from what the mirror
installed, and there is nothing left for npx to fetch. A launch the
fallback left alone is fetched as before.

Test: the fetch is given the argv the fallback answered with, so it
does not go to the registry the fallback is about.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; suggestions only, and they are marked inline.

The new delta correctly recognizes httpx's actual missing-socksio ImportError as a request that never reached the configured proxy or host. I checked the classifier's exception-chain ordering, its resource/version/probe callers, the shared reachability contract, repository instructions, history, backward compatibility, and the added coverage; I found no regression or weakened test.

Focused mirror/provider/upgrade verification passed: 438 passed, 2 environment-specific skips. The five-implementation reachability table passed for the four available implementations; PowerShell was skipped because it is not installed here. I also reproduced the real httpx exception shape with socksio unavailable and confirmed the new verdict.

The sole remaining suggestion is the existing nonblocking note about a timing-fragile macOS test; this commit neither changes nor worsens it.

@LivXue

LivXue commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

The socksio gap is re-verified on the new head and fixed, and the regression the Windows report named in its second item is fixed too. Could you re-verify the open thread on raven/node/server.py against 4ced030f?

The socksio gap, re-verified at ce9fd55c and fixed:

  • It still stands. git diff e0c85804 ce9fd55c -- raven/utils/mirrors.py tests/test_install_script.py pyproject.toml is empty, so the rebase left it as it was; the socksio package is absent on a stock Windows install, and the table every copy of the rule answers alike failed there on the two SOCKS cases (socks-refused-by-its-rules and socks-host-unreachable both answered True, the host as reachable).
  • httpx cannot use a SOCKS proxy without the socksio package and says so with an ImportError. The rule walked the error's chain, knew none of those, and answered that the host had answered. The copies that cannot use a SOCKS proxy at all, urllib and the Windows PowerShell, answer that the host was not reached, so the httpx copy was the only one that took the mirror for an outage it could not have seen.
  • A proxy that could not be used at all counts: it was never asked. The rule says so now. The table holds again on an install without the socksio package: Windows, native, 4 passed and 1 skipped where it failed at the previous head; with the socksio package, in a Linux container, 3 passed and 2 skipped. A one-construct mutant that takes the ImportError for a proxy that could not be used turns the two SOCKS cases red on Windows again.
  • The other option the report named -- declaring socksio a real dependency of the httpx path -- was not taken: the rule answers right without a new dependency, and the extra that carries the package stays where it was.

The regression, fixed: the ACP launch asked the npx fetch before the mirror fallback, and that fetch goes to the default registry, the very one the fallback decides about. On a machine that cannot reach the registry the fetch failed first, the launch raised npm's own error, and the fallback was never consulted -- the case the fallback exists for. The fallback is asked first now, and the fetch is given its answer: an npx launch the mirror took over is started from what the mirror installed, and there is nothing left for npx to fetch. A launch the fallback left alone is fetched as before. The new case in test_subagent_host_env pins the order, and a one-construct mutant that asks the fetch first turns it red.

Verification: Windows, native, 560 passed and 41 skipped over the files these changes reach, and 296 passed and 21 skipped with the install's own file. Linux, in a python:3.12 uv container: test_utils_mirrors 184 passed and 1 skipped, test_node_client 94 passed, test_node_install with test_utils_mirrors 316 passed and 1 skipped, test_node_server 114 passed and 7 failed, the seven being the cases that read the process tree with ps, which that image does not have and which fail on the base tree as well. ruff, commitlint, the source-language and large-file gates pass. Eight one-construct mutants of these changes each turn a case red.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; suggestions only, and they are marked inline.

Reviewed the two-file delta from ec04a795 to 4ced030f, the npx-fetch implementation, the pool and probe callers, the relevant history from the newly merged prefetch change, backward-compatibility implications, and whether the added test reaches the production entry points. The direct AcpClient.launch ordering is sound and the focused suites pass: uv run --locked --extra channel-telegram --extra dev pytest tests/test_subagent_host_env.py tests/test_acp_npx_fetch.py tests/test_utils_mirrors.py tests/test_acp_client_launch.py -q (267 passed). One production-caller gap remains inline; it has practical operator workarounds, so under this change's late-round bar it does not hold the merge. Repository rules and the full branch diff were also rechecked; git diff --check is clean.

Comment thread raven/acp_client/client.py Resolved
Windows keeps one name for a variable, so a switch set in another case
lands under the one name and the case-insensitive reader is never asked
about it: the case that said the switch was read in either case passed
on Windows for that reason, not because the reader reads either case.

The other case is asked where a system can hold it, on POSIX, and the
name the install and the runtime share is asked everywhere. The code was
right in both; the case that said so was worth nothing on Windows.
@LivXue

LivXue commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

The last item of the Windows report is addressed too: the case that said the switch is read in either case could not fail on Windows, because Windows keeps one name for a variable and the switch set in another case lands under the one name, so the case-insensitive reader was never asked about it.

The other case is now asked where a system can hold it, on POSIX, in both places that read the switch in their own environment: test_utils_mirrors for the runtime and test_node_install for the install. The name the install and the runtime share is asked everywhere. The code was right in both; the case that said so was worth nothing on Windows.

In a Linux container the new cases pass, and a one-construct mutant that reads the switch in one case only turns exactly the two of them red.

The head is now 744f0951, with three commits since ce9fd55c: the socksio gap fixed, the ACP launch's order fixed, and this. ruff, commitlint, the source-language and large-file gates pass. Could you re-verify the open thread on raven/node/server.py against that head?

One item of the report is not this branch's to answer, and is left as it was: the daemon has no Windows coverage at all, because the daemon is POSIX -- it locks with fcntl and listens on a unix socket -- so tests/test_node_server.py cannot run on Windows at all. That is the daemon's own design, not something this change introduced, and giving the daemon a Windows transport is a far larger change than a mirror fallback should carry. The cases in that file are run in the Linux container instead, where they pass apart from the seven that read the process tree with ps, which that image does not have and which fail on the base tree as well.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; suggestions only, and they are marked inline.

Reviewed the test-only delta from 4ced030f to 744f0951, the opt_out implementation and node-install caller it exercises, the preceding test history, cross-platform environment semantics, backward compatibility, and whether coverage was weakened. The split is correct: POSIX now exercises a genuinely distinct lower-case process-environment key, Windows still checks the canonical shared name, and the explicit launch-environment mapping remains covered in both cases. uv run --locked --extra channel-telegram --extra dev pytest tests/test_node_install.py tests/test_utils_mirrors.py -q passes (319 passed), and git diff --check is clean. No new finding; the existing nonblocking prefetch suggestion remains unchanged.

@LivXue

LivXue commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility re-review of the delta at head 744f0951675d7d38df85a45561b5c5db77cf9b82, merge base e8332e5d4c9690215e6962899064d07648d9badf.

The previously reviewed head was ce9fd55cca5e82a18c2b140805248dcdb58e29f5. The delta between it and the new head is 5 files, +103/-13: raven/acp_client/client.py +10/-2, raven/utils/mirrors.py +8/-1, tests/test_node_install.py +15/-4, tests/test_subagent_host_env.py +53, tests/test_utils_mirrors.py +30/-5. It teaches unreachable() to read an ImportError that names socksio as "the host was not reached", swaps the two calls in AcpClient.launch so the mirror is asked before the npx fetch and the fetch is handed the mirror's answer, splits the case-insensitive switch-name tests into a POSIX-only branch and a canonical-name branch that still runs on Windows, and adds one test for the new call order and one for the new rule. One correction to the brief I was given: raven/acp_client/client.py IS in this delta (10 lines), and that is exactly where the second previously reported defect is fixed.

Native Windows verification (Python 3.13.12, UTF-8; Windows 11 10.0.26300 x64)

Shared venv, httpx 0.28.1, pytest 9.0.3, xdist 3.8.0, pwsh 7.6.7. socksio is not installed: ModuleNotFoundError: No module named 'socksio', exit 1. raven.__file__ was printed under the prescribed pytest form in every run and resolved to the checkout under test in each case (head, new2\head, base).

revision files result exit
new head 744f0951 8 59 failed, 1044 passed, 188 skipped in 239.89s 1
previous head ce9fd55c 8 60 failed, 1043 passed, 185 skipped in 255.26s 1
merge base e8332e5d 7 (tests/test_utils_mirrors.py does not exist at the base) 61 failed, 705 passed, 78 skipped in 86.32s 1
new head, tests/test_node_server.py alone 1 1 skipped in 0.40s (POSIX daemon: fcntl and a unix socket) 5

Failure sets compared by test id, previous head -> new head: exactly one test changed status, tests/test_install_script.py::test_every_copy_of_the_rule_answers_each_case_alike[runtime-downloads], and zero tests were added. Base -> new head: the two failures the PR fixes are test_a_windows_node_download_that_cannot_be_verified_is_never_extracted[digest-mismatch-mismatch] and test_a_windows_node_download_that_matches_its_published_digest_is_installed, and nothing is introduced by the PR.

The reachability parity gate on its own: at the new head 4 passed, 1 skipped (install.sh, "POSIX sh and curl"), exit 0. At the previous head 1 failed, 3 passed, 1 skipped, exit 1, with {'socks-refused-by-its-rules': True} != {'socks-refused-by-its-rules': False} and {'socks-host-unreachable': True} != {'socks-host-unreachable': False}. At the base the gate does not exist (no _REACHABILITY, no _runtime_downloads, no test_every_copy_of_the_rule_answers_each_case_alike).

The delta's own new and changed tests at the new head: 8 passed, 3 skipped, 344 deselected, exit 0. The three skips are the new POSIX-only branches: 2 in tests/test_node_install.py:423 and 1 in tests/test_utils_mirrors.py:78.

Exact verification commands and results
$root = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\run-20261024'
$env:UV_PROJECT_ENVIRONMENT = "$root\venv"
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'; $env:PYTHONDONTWRITEBYTECODE = '1'

# 0. socksio is absent, httpx version
uv run --frozen --no-sync --project "$HEAD" python -c "import socksio"
#   ModuleNotFoundError: No module named 'socksio'                     exit 1
uv run --frozen --no-sync --project "$HEAD" python -c "import httpx; print(httpx.__version__)"
#   0.28.1                                                             exit 0

# 1. the 8 required files, at each revision (absolute paths, -c and --rootdir pinned)
uv run --frozen --no-sync --project "<HEAD>" pytest -c "<HEAD>\pyproject.toml" \
  --rootdir "<HEAD>" "<HEAD>\tests\test_utils_mirrors.py" \
  "<HEAD>\tests\test_install_script.py" "<HEAD>\tests\test_node_install.py" \
  "<HEAD>\tests\test_subagent_host_env.py" "<HEAD>\tests\test_updates_upgrade.py" \
  "<HEAD>\tests\test_resources_fetch.py" "<HEAD>\tests\test_node_client.py" \
  "<HEAD>\tests\test_subagent_third_party.py" \
  -p no:randomly -p no:cacheprovider -n 0 --tb=short -q \
  --basetemp "<root>\scratch\<unique>"
#   new head 744f0951 : 59 failed, 1044 passed, 188 skipped in 239.89s   exit 1
#   prev head ce9fd55c : 60 failed, 1043 passed, 185 skipped in 255.26s   exit 1
#   base     e8332e5d : 61 failed,  705 passed,  78 skipped in  86.32s   exit 1
#                      (tests/test_utils_mirrors.py is absent at the base)

# 2. the parity gate, new head and previous head
uv run --frozen --no-sync --project "$HEAD" pytest -c "$HEAD\pyproject.toml" \
  --rootdir "$HEAD" "$HEAD\tests\test_install_script.py" \
  -p no:randomly -p no:cacheprovider -n 0 --tb=short -q \
  --basetemp "<root>\scratch\bt_d4gate" -k "every_copy_of_the_rule"
#   new head       : 4 passed, 1 skipped, 259 deselected in 32.37s        exit 0
#   previous head  : 1 failed, 3 passed, 1 skipped, 259 deselected       exit 1
#        {'socks-refused-by-its-rules': True} != {'socks-refused-by-its-rules': False}
#        {'socks-host-unreachable': True} != {'socks-host-unreachable': False}

# 3. the delta's own new and changed tests, new head
uv run --frozen --no-sync --project "$HEAD" pytest -c "$HEAD\pyproject.toml" \
  --rootdir "$HEAD" "$HEAD\tests\test_subagent_host_env.py" \
  "$HEAD\tests\test_utils_mirrors.py" "$HEAD\tests\test_node_install.py" \
  -p no:randomly -p no:cacheprovider -n 0 --tb=short -q \
  --basetemp "<root>\scratch\bt_d4newtests" \
  -k "mirror_before_it_fetches or socks_proxy_whose_support or our_own_environment or the_switch_is or the_launch_environment" -v
#   8 passed, 3 skipped, 344 deselected in 8.78s                          exit 0

# 4. the node daemon file, new head
uv run --frozen --no-sync --project "$HEAD" pytest -c "$HEAD\pyproject.toml" \
  --rootdir "$HEAD" "$HEAD\tests\test_node_server.py" \
  -p no:randomly -p no:cacheprovider -n 0 --tb=short -q \
  --basetemp "<root>\scratch\bt_d4nodeserver" -rs
#   1 skipped in 0.40s  "the daemon is POSIX: it locks with fcntl and listens on a unix socket"
#                                                                            exit 5

# 5. the rule, real httpx error, both heads (PYTHONPATH pinned to the checkout)
uv run --frozen --no-sync --project "$HEAD" python d4_socks_probe.py
#   new head      : CONSTRUCTION raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed.
#                   unreachable(httpx socksio ImportError)   = True
#   previous head : CONSTRUCTION raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed.
#                   unreachable(httpx socksio ImportError)   = False

# 6. the npx launch order, real AcpClient.launch, both heads
uv run --frozen --no-sync --project "$HEAD" python d4_defectb_probe.py
#   previous head : LAUNCH raised AcpConnectionError: acp agent 'mirror-probe': npx could not fetch @scope/server (exit 1)
#                   mirrors.package_launch consulted? False []
#   new head      : LAUNCH started OK
#                   mirrors.package_launch consulted? True [['npx', '-y', '@scope/server']]
#                   ensure_fetched saw argv: [[...python.exe, ...child.py, '--as-the-answer-says']]
#                   real fetchable_package(...) -> None

# 7. the two production httpx call sites, new head, real SOCKS proxy in the environment
uv run --frozen --no-sync --project "$HEAD" python d4_prod_probe.py
#   codex _fetch_latest raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed.
#   fetch_dictionary raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed.
#   -> the jsDelivr mirror fallback did NOT run
#   -> unreachable() of it would be: True

# 8. Windows os.environ key handling
#   os.environ stores the lowercase name as: ['LOWER_CASE_PROBE']
#   getenv(LOWER_CASE_PROBE) = 1
#   mirrors.opt_out({'raven_no_mirror': '1'}) = True
#   mirrors.opt_out({'RAVEN_NO_MIRROR': '1'}) = True

# 9. npm on Windows is a .cmd; the PR's own npm probe
#   shutil.which('npm') = C:\Program Files\nodejs\npm.CMD
#   subprocess.run([npm, 'config', 'get', 'registry']) rc = 0 | out = https://registry.npmjs.org/

# 10. whitespace check
git -C "$root\repo" diff --check ce9fd55cca5e82a18c2b140805248dcdb58e29f5 744f0951675d7d38df85a45561b5c5db77cf9b82
#   exit 0
git -C "$root\repo" diff --shortstat e8332e5d4c9690215e6962899064d07648d9badf 744f0951675d7d38df85a45561b5c5db77cf9b82
#   29 files changed, 8064 insertions(+), 161 deletions(-)

Findings

Defect A (the socksio gap defeats the mirror fallback on Windows): CHANGED. The rule is fixed; the production consequence is not.

The previously reported mechanism is gone. raven/utils/mirrors.py:168 now reads:

if any(isinstance(error, ImportError) and "socksio" in str(error).lower() for error in chain):
    return True

It sits before the network tuple built at raven/utils/mirrors.py:170, so the httpx ImportError is no longer mistaken for "the host answered". Measured against the real httpx 0.28.1 error, unreachable() returns True at the new head and False at the previously reviewed head (raven/utils/mirrors.py:181 was the return False that produced the old verdict; the tuple is at :164 there). The PR's own parity gate now passes on Windows: 4 passed, 1 skipped at the new head against 1 failed at the previous head with exactly the previously reported assertion. That test does not exist at the merge base, so the gate was a PR-introduced test and the delta makes it green.

What is still broken, and is INHERITED rather than fixed. httpx raises the socksio ImportError when the httpx.Client is constructed, not when a request is made (verified: CONSTRUCTION raised: ImportError). Both production httpx call sites therefore still die instead of falling back, at the new head as much as at the previous one:

  • raven/resources.py:235 builds httpx.Client(follow_redirects=True, timeout=_TIMEOUT) outside the try at :240 whose handler at :245-247 consults mirrors.unreachable(exc). Driving the real raven.resources.fetch_dictionary with a SOCKS proxy in the environment: fetch_dictionary raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed. and the jsDelivr mirror fallback did NOT run.
  • raven/providers/codex_client_version.py:106 catches only httpx.HTTPError, and an ImportError is not one. Driving the real codex_client_version._fetch_latest with the same environment: codex _fetch_latest raised: ImportError | Using SOCKS proxy, but the 'socksio' package is not installed.

So on a stock Windows install behind a SOCKS proxy, raven resources (the deepdoc dictionary download) and the codex version check still abort rather than fall back to jsDelivr or the mirror. The new rule reaches the PR's own gate because the gate's httpx copy constructs its client inside the try (tests/test_install_script.py:1846-1851); the production code does not have that shape. Either declaring socksio a real dependency on win32, or moving the client construction inside the guarded block (and widening the codex handler), would close the gap.

Defect B (the PR's npx mirror fallback can never fire for an npx launch): FIXED.

raven/acp_client/client.py was in this delta after all, and the two calls are now in the other order: mirrors.package_launch_async(...) at :481, npx_fetch.ensure_fetched(...) at :486. At the previously reviewed head the order was ensure_fetched at :468 and package_launch_async at :480.

Driving the real AcpClient.launch with npx -y @scope/server, the real npx_fetch.fetchable_package deciding whether a fetch is needed, and a mirror that answers with an installed script:

  • previously reviewed head: LAUNCH raised AcpConnectionError: acp agent 'mirror-probe': npx could not fetch @scope/server (exit 1) and mirrors.package_launch consulted? False []. The mirror was never asked, exactly as previously reported.
  • new head: LAUNCH started OK, mirrors.package_launch consulted? True [['npx', '-y', '@scope/server']], and ensure_fetched received the mirror's answer, for which fetchable_package returns None, so no fetch goes to the dead registry.

The PR's own new test tests/test_subagent_host_env.py::test_an_acp_launch_asks_the_mirror_before_it_fetches_the_package pins this order and passes on Windows (it spawns a real python.exe child, so it is not mocked away).

REGRESSION (introduced by this delta): none.

No test that passed at the previously reviewed head or at the merge base fails at the new head. The only status change across the 8 files is the parity gate going from failed to passed.

INHERITED (also present at the base or at the previously reviewed head)

  1. 12 failures in tests/test_resources_fetch.py, all ModuleNotFoundError: No module named 'nltk' raised by the test's own import. The shared venv has no nltk. Identical at the new head, the previous head and the base.
  2. 47 failures in tests/test_subagent_third_party.py, all because the fake CLI agent is the program sh, which is not on PATH on Windows, so the launch dies with FileNotFoundError [WinError 2]. Identical at all three revisions. The whole CLI-backend surface stays dark on Windows.
  3. The socksio residue at the two production httpx call sites, described under Defect A above: present at the new head, at the previously reviewed head and (as the absence of any fallback at all) at the base.
  4. Windows coverage gap: tests/test_node_server.py is entirely skipped. 1 skipped, exit 5 (nothing collected), reason "the daemon is POSIX: it locks with fcntl and listens on a unix socket". The Raven node daemon has zero Windows coverage. The file is in the PR's 29-file list, so the gap is the PR's to own.

NON-WINDOWS (platform-independent; reported, but not a Windows issue)

  1. Defect B was never a Windows issue. The ordering bug in AcpClient.launch was platform-independent, and the fix is platform-independent too. It is fixed on every platform.
  2. The new socksio unit test asserts a hand-written literal, not the real httpx error. tests/test_utils_mirrors.py::test_a_socks_proxy_whose_support_is_not_installed_is_unreachable builds ImportError("Using SOCKS proxy, but the 'socksio' package is not installed.") by hand. That string does match httpx 0.28.1 today (the raise site is httpx/_transports/default.py, in both the sync and the async transport), so the test is honest as written, but it would keep passing if a future httpx reworded the message and stopped mentioning socksio, while the fallback silently stopped working. A test that raised the real error (or asserted against the httpx message) would close that gap.

On the delta's own test changes (read and judged)

  • tests/test_utils_mirrors.py: the two Windows-vacuous switch-name tests were split correctly. test_our_own_environment_is_read_in_another_case (os.environ path) is now skipif(sys.platform == "win32"), because Windows upper-cases every os.environ key (re-confirmed on this machine: os.environ stores the lowercase name as: ['LOWER_CASE_PROBE']), so it would have passed for the wrong reason. The dict-path test test_the_launch_environment_is_read_whatever_its_case still runs on Windows and is meaningful there, because _env_value compares key.lower() explicitly (raven/utils/mirrors.py:95-105).
  • tests/test_node_install.py: same treatment. test_the_switch_is_read_in_either_case is now POSIX-only, and the canonical-name coverage is not lost: test_raven_no_mirror_times_nothing_and_asks_no_mirror (tests/test_node_install.py:405) still runs on Windows and asserts the install asks no mirror. The name-identity assertion was moved into its own always-running test test_the_switch_is_the_runtime_s_own.
  • tests/test_subagent_host_env.py: the new ordering test is a real test, not a mock-only one; it launches an actual python.exe child and reads what it wrote.
  • raven/utils/mirrors.py: the new rule is placed before the network tuple, and it is narrow enough (an ImportError whose text names socksio). It does not change how a host that answered is judged.

Coverage limits

  • I ran only the 8 required files, tests/test_node_server.py, the parity gate, and a -k selection of the delta's own tests. No number in this report comes from a suite-wide run, and I did not run the ui-web or ui-tui JavaScript tests.
  • I did not review the 25 files of the PR's 29-file diff that this delta does not touch, beyond what the three test runs above exercise.
  • I did not install socksio into the shared venv, so I did not verify the parity gate with socksio present, and I did not check how the rule behaves on a POSIX host where socksio is importable.
  • I did not exercise a real npm install, a real npx fetch, a real registry outage or a real SOCKS proxy. The SOCKS evidence above comes from real httpx and real urllib code paths driven with a SOCKS proxy URL in the environment and a closed local port, not from a live proxy.
  • I did not exercise the install.sh copy of the rule on this machine; that branch of the parity gate skips on Windows ("POSIX sh and curl"), so only the urllib, httpx, upgrade-helper and install.ps1 copies were compared here.
  • I did not check the behaviour of Windows PowerShell 5.1 as opposed to pwsh 7.6.7.
  • I did not modify the Raven repository at D:\livxu\Documents\Raven, did not commit, did not push, and did not post anything to GitHub.

This is a scoped compatibility report, not a merge verdict.

@LivXue

LivXue commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility correction at head 744f0951675d7d38df85a45561b5c5db77cf9b82, merge base e8332e5d4c9690215e6962899064d07648d9badf.

This supersedes one statement in my previous comment at this head. That comment graded the second previously reported defect - the PR's npx mirror fallback never firing for an npx launch - as FIXED. That grade was too strong, and it is withdrawn. The defect is CHANGED, not fixed: the fix reached the direct launch path and the PR's own gate, and it did not reach the two other launch paths that prefetch from the default registry.

The observation is not mine. gloryfromca's P2 on this PR says the same thing, and their evidence is the better one: they read the other two call sites, while I drove only AcpClient.launch directly and generalised from it. I am restating it here because my earlier comment told the author the opposite, and a comment that says "FIXED" must not be left standing. Their grading is the one to keep - P2, nonblocking ("this is nonblocking at this late stage because an operator can explicitly configure npm's registry or prefetch the package manually") - and they add the point my earlier comment missed: the new regression test invokes AcpClient.launch directly, so it cannot expose either earlier prefetch.

What I checked at this head. Every call site of npx_fetch.ensure_fetched in the tree, and the order of the two calls in AcpClient.launch:

git grep -n 'ensure_fetched' 744f0951675d7d38df85a45561b5c5db77cf9b82 -- '*.py'

Three production call sites, and only one of them is behind the mirror:

call site what it passes the mirror consulted first?
raven/acp_client/client.py:486 argv, the result of mirrors.package_launch_async at :481 yes - this is the fix
raven/acp_client/pool.py:441 command, the launch command as configured no
raven/agent/subagent/probe.py:1146 cfg.command, as configured no

raven/acp_client/pool.py does not mention mirrors at all (0 occurrences at this head), so there is nothing on that path that could consult the fallback. Its prefetch is also deliberately placed ahead of the per-agent lock, before any launch:

# raven/acp_client/pool.py:438-441
if launcher is None:
    # Ahead of the lock, which `drop` waits on too: a first fetch can run for
    # minutes. Launches that overlap still share the one fetch (`npx_fetch`).
    await npx_fetch.ensure_fetched(name, command, env=launch_env, cwd=cwd)

The pool then launches with the same un-mirrored command, so even the launch that follows would resolve against the default registry again:

# raven/acp_client/pool.py:491-500
else:
    client = await AcpClient.launch(
        name=name,
        command=command,
        ...

And raven/agent/subagent/probe.py:1142-1151 prefetches the same way on the local ACP check path, ahead of the probe's own bounds:

# raven/agent/subagent/probe.py:1142-1151
if not machine and getattr(cfg, "kind", None) == "acp":
    # Ahead of the bounds, not inside them: a first download is npx's
    # work, not the handshake's, and one a bound ran out on was ended
    # half-installed (`npx_fetch`). The launch below then finds it done.
    await npx_fetch.ensure_fetched(
        getattr(cfg, "name", "") or "",
        getattr(cfg, "command", "") or "",
        env=getattr(cfg, "env", None),
        cwd=getattr(cfg, "cwd", None) or str(scratch.path),
    )

npx_fetch.ensure_fetched raises AcpConnectionError when the fetch exits non-zero (raven/acp_client/npx_fetch.py:248), and it is awaited directly (raven/acp_client/npx_fetch.py:145), so the exception propagates out of the call. On the pool path and the probe path that happens before any mirror is consulted, which is the one case the fallback exists for: the default npm registry cannot be reached.

Platform. This is not a Windows issue, and I say so plainly: the same two paths miss the mirror on every platform. It is in this comment because it corrects a statement of mine that was wrong for every platform, not because Windows behaves differently here.

What does still hold from my previous comment at this head. The parity gate now passes on Windows - tests/test_install_script.py's test_every_copy_of_the_rule_answers_each_case_alike gives 4 passed / 1 skipped, exit 0, against 1 failed / 3 passed / 1 skipped at the previous head - and the socksio rule itself is real: raven/utils/mirrors.py:168 now reads an ImportError naming socksio as "the host was not reached", and the httpx 0.28.1 error is classified as unreachable where it was not before. The first previously reported defect keeps its own shape: the unit rule is fixed and the production consequence is not, because httpx raises that ImportError while building the client, and raven/resources.py:235 constructs the client outside the try whose handler at :245-247 consults unreachable().

The general form of the miss. A fix that lands and a fix that works are different claims. I checked that the swap of the two calls in AcpClient.launch had landed and that the gate went green, and I read that as the defect being fixed. The rule the skill's later-round step states is the one I skipped: a finding that disclosed a second site is only closed when both sites moved. Here the second and third sites had not moved, and one of them is the path a pooled agent actually starts through.

Coverage limits. This correction is source reading plus the earlier comment's own runs: the three call sites, their order, and the files' contents at this head were read from the exact commit; I did not drive the pool or the probe path end to end, and I did not stand up a registry that refuses an npx fetch. The earlier comment's test numbers and probes are unchanged and are not restated here. This is a scoped compatibility report, not a merge verdict.

LivXue and others added 2 commits October 25, 2026 21:05
The fetch holds what npx would fetch, and it goes to the default registry
-- the very one the mirror's answer is about. The direct launch asked the
mirror first, so a launch the mirror took over was never fetched from a
registry it could not reach; the pooled path and the local ping prefetched
first, and both raised the npx download failure before the fallback was
consulted.

The fetch asks the mirror now, in the same order, with the same
environment the launch asks it with: a launch the mirror took over is
started from what the mirror installed, and there is nothing left to fetch.
The question is asked only where a fetch is about to happen, so a warm
cache costs nothing, and the mirror's install is made before the pool takes
its lock.

Tests: a launch the mirror took over is not fetched, and the pooled path
that a user dispatches through is not fetched either.

Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
The case gave npm half a second and then read the pid its stub had
written. On macOS the first exec of a freshly written script can take
longer than that, so npm was ended before its first line ran, the pid was
never written, and the case failed with a FileNotFoundError.

The deadline is long enough for the stub to start, the pid is waited for
rather than read at once, and a stub that never started says so.

Co-authored-by: Claude (claude-opus-5-5[1m]) <noreply@anthropic.com>
@LivXue

LivXue commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

Fixed in this round.

The case gave npm half a second and then read the pid its stub had
written. On macOS the first exec of a freshly written script can take
longer than that, so npm was ended before its first line ran, the pid was
never written, and the case failed with a FileNotFoundError.

The deadline is now long enough for the stub to start, the pid is waited
for rather than read at once, and a stub that never started says so
instead of raising a FileNotFoundError. The case is marked
production_timing, which the suite's idle ceiling exempts: it waits for
the deadline on purpose, and the deadline is what the case is about.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

Reviewed the three-file delta from 744f0951 to 125930f4, the direct, pooled, and local-ping callers, the npx cache/deduplication and mirror-install paths, the relevant prefetch history, backward compatibility, repository rules, and whether either regression test was weakened. The shared prefetch now consults the mirror after the warm-cache fast path and before the pool lock, so both previously missed callers are covered while default-registry failures retain npm's own error. The macOS timing adjustment still exercises a real deadline and descendant cleanup; it now waits explicitly for proof that the stub started. uv run --locked --extra channel-telegram --extra dev pytest tests/test_acp_npx_fetch.py tests/test_subagent_host_env.py tests/test_subagent_acp.py tests/test_utils_mirrors.py -q passes (601 passed), and git diff --check is clean. I also closed the settled prefetch thread; no thread I opened remains unresolved.

@0xKT

0xKT commented Oct 26, 2026

Copy link
Copy Markdown
Member

Not a blocker; graded A on 125930f (merged onto main 25e1b23). The reordering works: the mirror is now asked before the npx fetch on every path. One cost the fix adds, for whoever touches the pool next.

Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

AcpConnectionPool.acquire calls npx_fetch.ensure_fetched ahead of its lock on every acquire, before it checks whether a live connection can be reused (raven/acp_client/pool.py:441). ensure_fetched returns early only when the npx cache entry is complete. When the mirror takes an npx launch over, npx never runs, so that entry stays cold for good. Every later acquire therefore reaches mirrors.package_launch_async again (npx_fetch.py:146), even when it then reuses the running agent:

  • each acquire spawns npm config get registry prefix;
  • for a spec that is not an exact version (a tag such as @latest, or a bare name), _npx_install also runs npm update --registry <mirror> in the prefix the running agent executes from. The docstring at mirrors.py:691 promises that "at each launch", not at each acquire;
  • the first launch installs twice, once in ensure_fetched and once in AcpClient.launch. The ping path (probe.py:1146) adds a third ask.

The reachability probes themselves are cached (mirrors._verdicts), so it is the npm processes that repeat. With a stub npm, three acquires of one some-acp-agent@latest row on the mirror ran install, then update on every acquire, all against the same live agent. The shipped presets pin exact versions, so for them the repeat is one npm config get per acquire. A user's own unpinned npx -y foo@latest row pays an npm update on every dispatch in exactly the setting this PR is for.

Two shapes would keep it to once per launch. Either skip the question in ensure_fetched once the mirror has installed the spec (treat that install as the completed fetch), or move the pool's ensure_fetched below the live-connection reuse check.

Verified for the grade:

  • Each real entry point (AcpClient.launch, AcpConnectionPool.acquire, the ping) with a refusing default registry and an answering mirror, all on loopback with stub npm/npx and the network sandbox: all three now launch from the mirror. On ce9fd55 they failed at the npx fetch.
  • With the registry reachable, argv and the fetch are unchanged.
  • RAVEN_NO_MIRROR asks nothing.
  • The socksio clause is pinned by tests. The deadline test now passes 5 of 5 here (the graded head failed it every time).
  • Mutants of the reorder are killed. Remaining reds match main exactly (test_node_server cannot exec setuid ps in the sandbox; test_node_client subprocesses lack the tree on PYTHONPATH).

@gloryfromca

Copy link
Copy Markdown
Member

No blockers; suggestions only, and they are marked inline.

The new acceptance note changes my standing label from clean to nonblocking. Its measured cost is real: after the mirror takes over an npx command, the npx cache remains cold, so pooled acquires can ask npm again before discovering that the live agent is reusable; an unpinned custom command can also run npm update on every dispatch. This does not hold the PR: shipped presets use exact versions, an operator can pin a custom row, and the reporter explicitly leaves the optimization for the next pool change. No earlier finding is reopened.

@0xKT
0xKT merged commit 807de02 into main Oct 26, 2026
28 of 31 checks passed
@0xKT
0xKT deleted the feat/china_mirror_fallbacks branch October 26, 2026 07:32
0xKT added a commit that referenced this pull request Oct 27, 2026
## Summary

Bump the package version from 0.2.5 to 0.2.6 for the next release. Only
pyproject.toml and uv.lock change.

Since v0.2.5, main merged 70 PRs: 15 features, 48 fixes, three test, two
docs,
one perf and one chore. The larger features are portable Agent Plugins
1.0
packages with their MCP servers (#1110), the compacting history strategy
with
source refs and checkpoints (#1031), sub-agent runs as their own
trajectory
spans and tabs (#1091, #1094, #1096, #1100), download mirror fallback
(#1053),
Keenable as a keyless web_search and web_fetch vendor (#1088), Opper as
an
OpenAI-compatible gateway (#869), and the MiniMax image and video roles
(#1013).
None carries a BREAKING CHANGE footer. The maintainer chose a patch
bump, the
way v0.2.1 through v0.2.5 were for their features.

A v0.2.6-rc1 tag goes first to dry-run the release pipeline, as v0.2.5
did. The
stable tag and release notes follow once this lands.

uv.lock is regenerated with uv 0.12.12, which leaves only the raven
version
line changed (uv 0.9.x rewrites unrelated dependency markers and uv
0.13.x
raises the lock revision).

## Type

- [ ] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [x] Other

## Verification

- `uvx --from uv==0.12.12 uv lock` on a clean worktree cut from
origin/main:
  Updated raven v0.2.5 -> v0.2.6
- `git diff --numstat origin/main..HEAD`: one line each in
pyproject.toml and
  uv.lock
- `uvx --from uv==0.13.3 uv lock --check`: exit 0, so the
  `uv export --locked` step in release.yml accepts the lock
- `PYTHONPATH=. python3 scripts/check_commit_messages.py
origin/main..HEAD`:
  exit 0
- `npx commitlint --from origin/main --to HEAD --config
commitlint.config.cjs`:
  exit 0
- No test run: the change is version metadata only.

- [ ] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [ ] User-facing docs or screenshots are updated when needed

## Risk

No code change. Rollback is reverting this commit before any v0.2.6 tag
is
pushed.

- [ ] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes

## Related Issues

N/A

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca pushed a commit that referenced this pull request Oct 28, 2026
…#1132)

## Summary

Bring the release notes template's Install and Upgrade sections up to
what 0.2.6
ships, so the next CI draft does not need these three edits by hand:

- Install: when a download host cannot be reached, the installer falls
back to
a mirror and says so, except for Raven's own release files, which come
from
GitHub Releases and have no mirror; `RAVEN_NO_MIRROR=1` keeps every
download
  on its default host (#1053).
- Upgrade: a page served by a source checkout refuses the sidebar
upgrade too,
  and the note says so next to the checkout's own remedy, `git pull` and
`./install.sh` in the checkout, since `raven upgrade` refuses an
editable
  install (#1003, #1087).
- Upgrade: neither `raven upgrade` nor the update row downloads the
knowledge
reader's word dictionary and PDF models, so an install that never
fetched
them runs `raven resources` once. The 0.2.5 and 0.2.6 notes carried this
  paragraph by hand.

Only `.github/release-notes-template.md` changes. The template is read
by
`release.yml` when a tag is pushed, so this affects the next release's
draft and
not the v0.2.6 draft already written.

## Type

- [ ] Fix
- [ ] Feature
- [x] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other

## Verification

- `git grep -n RAVEN_NO_MIRROR origin/main -- install.sh install.ps1
raven/utils/mirrors.py`:
the opt-out is read in both installers and in
`raven/utils/mirrors.py:46`
- `git grep -n editable origin/main -- raven/updates/update_notice.py`:
the
  update row is withheld for editable installs (line 213)
- `git grep -n resources origin/main -- raven/updates/upgrade.py
raven/rpc/methods/system.py`:
  no match, so neither upgrade path fetches the resources
- `git grep -n "Could not resolve" origin/main -- install.sh
install.ps1`: the
Raven wheel is resolved from GitHub Releases with no mirror in either
installer
- `raven/cli/upgrade_commands.py:41-56` on `origin/main`: `raven
upgrade` exits
  1 for an editable install and names `git pull && ./install.sh`
- `npx commitlint --from origin/main --to HEAD --config
commitlint.config.cjs`: exit 0
- `PYTHONPATH=. python3 scripts/check_commit_messages.py
origin/main..HEAD`: exit 0
- No test run: the change is a Markdown template only.

- [ ] Relevant tests pass locally
- [x] Relevant lint / type checks pass locally
- [x] User-facing docs or screenshots are updated when needed

## Risk

No code change. The next release draft carries the new paragraphs;
rollback is
reverting this commit.

- [ ] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes

## Related Issues

N/A

---------

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
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.

3 participants