Repository navigation
fix(*): stop offering a checkout the page upgrade, name each refusal's recovery - #1003
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the editable/unsupervised overlap still offers a command that cannot work.
I reviewed the full github/main...HEAD diff, the upgrade and installer callers, the originating issue and relevant history, backward compatibility, test changes, and the repository's AGENTS/CLAUDE/context constraints. The reason-specific command plumbing and editable notice suppression otherwise look coherent, and I did not find weakened tests.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (83 passed); npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts (25 passed); TypeScript type-check and ESLint on the four touched web files passed; git diff --check github/main...HEAD passed. I also reproduced the overlap directly: editable + gateway-hosted + no supervisor returns reason=unsupervised with raven upgrade as the first recovery command.
1ce1419 to
6f15a78
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision fixes the editable/unsupervised overlap and adds the missing regression coverage. I re-read the full github/main...HEAD diff and the new delta, relevant upgrade and installer callers and history, backward compatibility, test strength, architecture boundaries, and AGENTS/CLAUDE/context constraints. I found no new plain error or weakened test.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (84 passed); npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts (25 passed); TypeScript type-check passed; git diff --check github/main...HEAD passed. My prior thread is replied to and resolved.
|
Not a blocker (review of 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.
|
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the newly reported in-flight metadata failure must be resolved.
New evidence changes my standing clean verdict. I reproduced that when _is_editable_install() raises PackageNotFoundError in the install-replacement window, system_upgrade() now lets it escape before the existing in_progress refusal handler; through the dispatcher this becomes internal_error and restores the unsafe default raven upgrade fallback. The open review thread from @0xKT carries the full finding, so I am not duplicating or re-grading it.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py still passes (84 passed), confirming the current test setup does not catch this path. A direct invocation with the editable probe raising PackageNotFoundError reproduced the uncaught exception.
6f15a78 to
2bc1fb9
Compare
|
@gloryfromca resolved in this push: the editable probe's PackageNotFoundError is answered as in_progress, through the same sentence the release lookup uses (one constant, both readers), so the card never falls back to the withheld default. The regression test drives the raising probe and asserts the refusal; it errors uncaught on the previous head, as in your direct invocation. |
|
Blocking: malformed install metadata still escapes the refusal handlers. The |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the existing malformed-metadata path still needs to be brought under a refusal handler.
I reviewed the new delta and the resulting github/main...HEAD diff, including the upgrade metadata callers and refusal ordering, the bundled recovery-copy changes, backward compatibility, test changes, repository rules/context, and the unrelated formatter-only files required by the whole-tree hook. The PackageNotFoundError replacement-window regression is fixed and covered, and I found no separate new finding. The existing open blocker remains partially unresolved because _is_editable_install() can also raise UpgradeError for malformed direct_url.json, which still escapes before the later not_upgradable handler.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (85 passed); npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts (25 passed); git diff --check github/main...HEAD passed. A direct probe raising UpgradeError("Malformed Raven installation metadata") reproduced the uncaught exception.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new guard now covers both metadata failures introduced by the early editable probe: PackageNotFoundError becomes in_progress, and malformed-metadata UpgradeError becomes not_upgradable with its detail preserved. I reviewed the delta and resulting github/main...HEAD diff, relevant callers and history, refusal ordering and backward compatibility, test strength, repository rules/context, and the formatter/generated-file cleanup. I found no new plain error or weakened test. The remaining open thread belongs to another reviewer, so I left it untouched.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (86 passed); npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts (25 passed); TUI lint:rpc, lint:i18n, and type-check passed; git diff --check github/main...HEAD passed. A direct malformed-metadata probe returned reason=not_upgradable with the expected detail.
|
@gloryfromca one more push after the approval, from a self-review pass -- four gaps in what the card actually shows, all inside the branch's own theme:
The in_progress adopt-the-run reload onto a still-running CLI-upgraded server (pre-existing) and the three hand-mirrored copies of the install classification stay declared as follow-ups. rpc suites 86, ui-web 27, both generators in sync. |
|
Blocking: the new no-command UI path must pass the required TypeScript check. The four runtime gaps are addressed coherently, but |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the new no-command refusal path must type-check before this can merge.
I reviewed the new delta and the resulting github/main...HEAD diff, the backend refusal and web-card callers, translation flow, backward compatibility, test strength, repository rules/context, and prior review history. The four self-review fixes are directionally sound, and the runtime suites pass, but the required TypeScript check fails in three places introduced by this push. This satisfies the late-round blocker bar: the change introduces it, every type-check/build gate reaches it, and the branch cannot pass that gate without a code change.
Named nonblocking follow-up: when command is null, UpgradeShade.open().fail() still appends gui.upg.manual to failure.sub, so the card says "Run this in a terminal instead:" while intentionally rendering no command row.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (86 passed); npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts (27 passed); git diff --check github/main...HEAD passed; ./node_modules/.bin/tsc --noEmit -p tsconfig.json failed with three errors.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the current delta and the full main-to-head diff under AGENTS.md, including the RPC, update-notice, TUI, and web callers; relevant history and compatibility; and whether the tests were weakened. The three strict-TypeScript failures are fixed without changing the intended no-command runtime branch. Verification: 86 targeted Python tests passed, 27 targeted Vitest tests passed, tsc --noEmit passed, and git diff --check passed.
The previously named nonblocking follow-up remains: when a refusal intentionally has no command, the card hides the command row but still appends the manual-command prompt. That copy should eventually be conditional too, but the card remains dismissible and this does not hold this revision. No new plain errors found.
|
A note on the red unit shard, so the X does not read as a test failure: shard 3/4 of the latest run reports |
|
Windows compatibility review of head No new actionable Windows compatibility issue found in the inspected scope: the complete 13-file diff, upgrade/install classification and callers, RPC refusal ordering, web rendering/copying, both catalogues, TUI formatting changes, tests, and all existing discussion pages/threads. Code inspection confirms the editable recovery selects Native Windows verification: 86 targeted Python tests and 27 targeted web tests passed; Limitations: upgrade/spawn calls are mocked in these tests. No installer, live upgrade/restart, real browser clipboard, PowerShell execution-policy scenario, full repository suite or TUI runtime test was exercised. This is compatibility coverage of the inspected changes, not a general merge verdict. Exact verification commands and resultsCommands ran in an isolated detached checkout, with temporary profile, Raven/OAuth home and test directories. Imports were checked against that checkout. UTF-8 mode was enabled. $run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\3c7e882fc3954e1cb719654d256eb750'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'envs/review'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'
# 1003-upgrade: 86 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\3c7e882fc3954e1cb719654d256eb750\pr1003\head'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\3c7e882fc3954e1cb719654d256eb750\pr1003\head'
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_rpc_system.py' 'tests/test_updates_update_notice.py' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\3c7e882fc3954e1cb719654d256eb750\checks\1003-upgrade\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\3c7e882fc3954e1cb719654d256eb750\checks\1003-upgrade\junit.xml'
git diff --check dcc019a1b68ff51135b8c9fada2a584a82ebb6b9 194a3eae38b8517077fb7f35dd3aa22c7c135249 # exit 0 |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the three-commit delta and the full main-to-head change under AGENTS.md, CLAUDE.md, and the runtime/web context rules; I also traced the metadata probe through plan_upgrade, checked its callers and history, compatibility, and whether tests were weakened. The replacement-window and malformed-metadata cases now exercise the real _direct_url_data/_is_editable_install path through the production dispatcher. Unexpected probe failures no longer escape as -32603, and plan_upgrade rechecks install eligibility before any helper can spawn, so I found no concrete unsafe-upgrade path from the broad fallback.
Verification: the first test attempt had 89 setup errors because uv recreated the environment without the dev workspace plugin; after the repository-prescribed uv sync --frozen --extra dev --dev, the unchanged command passed all 89 affected Python tests. The source-language gate and git diff --check also passed. The Python-only delta does not disturb the previously verified web type-check and tests.
The previously named nonblocking follow-up remains: a refusal with no command hides the command row while retaining the manual-command prompt. The card is dismissible, so this still does not hold the revision. I found no new plain error.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the test-only delta and rechecked it against the full main-to-head behavior, project rules and context, the production metadata-probe caller chain, history, compatibility, and test strength. The new plan_upgrade tripwires make the real-probe regressions fail if the early guard is removed; they prevent the later release lookup from masking that defect. No runtime behavior changed and no test was weakened.
Verification: 89 affected Python tests passed; the source-language gate and git diff --check passed.
The previously named nonblocking follow-up remains unchanged: a no-command refusal hides the command row but retains the manual-command prompt. I found no new plain error.
An editable install under a hand-started gateway hit the unsupervised refusal first, whose recovery opens with raven upgrade -- a command that always refuses a checkout, so the overlap was offered a step that cannot work. The refusal order is now availability, then the nature of the install, then the gateway's process answers: not_serving, editable, unsupervised, busy. The overlap test arms the hosted-unsupervised state with the editable probe true and asserts the checkout answer wins.
The pre-commit prettier pass checks the whole tree, so a commit that touches nothing of theirs still fails on their drift; formatting only, no behavior.
The editable probe reads install metadata outside the refusal handlers, so during an upgrade already in flight -- uv mid-replacement of the environment -- its PackageNotFoundError escaped as internal_error, whose card falls back to the default command this branch exists to withhold. The probe now answers in_progress through the same sentence the release lookup does, hoisted to one constant both read. The unsupervised recovery says the quiet part now: the upgrade does not stop a hand-started gateway, and a new raven web would only attach to the old one still running, so the reader is told to stop this Raven first. The zh editable note joins its neighbours on a full-width comma. The TUI catalogue is regenerated.
_is_editable_install raises UpgradeError for a direct_url.json it cannot read, and the new guard caught only PackageNotFoundError, so malformed metadata still escaped as internal_error -- whose card falls back to the default command this branch exists to withhold. The guard now answers not_upgradable with the exception's own message, as the release lookup's handler does. rpc/generated.ts is restored to the generator's output: the previous commit's tree-wide prettier pass had reformatted it, which the gen-rpc check compares by hash.
Self-review after the approval, four gaps in what the card actually shows. The stop-first advice for a hand-started gateway lived only in the server detail: the card prefers the catalogue key, whose text still said to just start Raven again -- the catalogue now carries the advice, en and zh. A not_upgradable refusal offered no command, and the card falls back to the default raven upgrade when none is given -- the very command this refusal proved fails, so the card learns an explicit no command (the row is dropped) and not_upgradable maps to it. The editable probe reads the filesystem on the serving loop; it runs off the loop now, as the release lookup below it always has. The checkout refusal's catalogue mapping and the no-command pass-through are pinned by tests.
NO_COMMAND.has received reason as string | undefined, the test indexed an array element strict mode cannot prove exists, and the copy button still offered failure.command to a copyCommand that had not learned null. npm run type-check passes on all three now.
The editable-install probe reads install metadata, and the guard around it caught only PackageNotFoundError and UpgradeError. Anything else the read can hit -- an undecodable direct_url.json, an OSError from a filesystem racing the in-flight install -- still escaped as internal_error, whose card falls back to the default `raven upgrade` this branch exists to withhold. The probe only classifies a checkout, so an unreadable probe is not a checkout: it now falls back to editable_install = False and the release lookup below answers as it does for an ordinary install. The clause sits after the two narrow ones on purpose, so PackageNotFoundError still answers in_progress and UpgradeError still answers not_upgradable.
The probe cases already in the file replace _is_editable_install itself, so they only show the handler names the right reason for an exception it is handed. These three replace the importlib.metadata distribution lookup underneath instead, so the genuine _direct_url_data and _is_editable_install run end to end through the dispatcher. PackageNotFoundError still answers in_progress, and a direct_url.json that does not parse still answers not_upgradable. An OSError from the read, which neither narrow clause names, now answers as an ordinary install and the upgrade is handed off; before the catch-all it escaped as internal_error (-32603). The stub answers for raven only and forwards every other name, because importlib.metadata.version calls distribution internally.
The replacement-window and malformed-metadata tests left plan_upgrade real. It reads the same metadata as the probe and refuses with the same reason, so both tests passed with the probe's guard deleted (verified by mutation). Make the release lookup unreachable so the refusal has to come from the probe.
9c5e21c to
aa7740c
Compare
aa7740c to
e37f9b0
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The wake-up named aa7740c, but that revision had already been force-pushed away; GitHub reports e37f9b0 as the current PR head, so this review is attached to the revision that can merge.
Reviewed the current delta and full main-to-head diff under the project rules and runtime/web context, including refusal producers and consumers, history, compatibility, and test strength. The server now sends the no-command verdict per refusal, preserving the default command for plan failures where it still works; the shade removes both the row and the manual-command label for explicit null. This resolves my previously named follow-up. No tests were weakened and I found no new plain error.
Verification: 88 affected Python tests passed, 29 targeted web tests passed, strict TypeScript passed, and the source-language and git diff checks passed.
A card whose refusal proved no command works dropped the command row but kept the 'run this in a terminal instead' label, pointing at a command it did not show; and which reasons meant that lived in a client-side set, so the plan path's not_upgradable -- the raven executable is missing, where the CLI upgrade genuinely works -- lost a working suggestion, and the two check_failed origins could not be told apart. The refusal now carries the verdict itself: _refuse takes a sentinel that sends command as an explicit null, the probe's not_upgradable and check_failed use it, and the card hides the row and the label on null while an absent command still means the default. Pinned on both sides: the probe's refusals carry an explicit null, the plan path's keeps the default, and the shade drops row and label together.
e37f9b0 to
ede6a2a
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the rebuilt head against current main, including the repository rules and domain context, the full diff and latest delta, upgrade callers/history, backward compatibility, and whether the consolidated tests weakened coverage. The explicit-null versus omitted-command contract is now consistent end to end: refusal-specific null suppresses both the command row and manual-run label, while an absent command retains the usable terminal fallback. The probe tests exercise the real metadata boundary with release planning tripwired, and unexpected probe failures still reach the release lookup's own classified response.
Verification: uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py (88 passed); targeted Vitest (29 passed); strict TypeScript, source-language gate, and git diff --check all passed. No new plain error found.
…#1086) ## Summary Follow-ups to #1003 on the commands the upgrade card offers when `system.upgrade` refuses. Six gaps were left open there; each is fixed here and pinned by tests that hold on any runner. **Unsupervised gateway, Windows ordering.** The card offered `raven upgrade` then `raven web` on every platform. On native Windows `raven upgrade` hands the install to a helper and returns at once ("Wait for the completion message before running Raven again"); the helper waits for its parent process to exit, stops whatever still runs from the old environment, and then runs `uv tool install`. A `raven web` on the next line therefore starts inside that install. The Windows command now ends after `raven upgrade`, and the sentence tells the reader to start Raven with `raven web` once the upgrade reports it has finished. On Linux and macOS `raven upgrade` execs the helper and installs in the foreground, so `raven web` still follows it on the next line. This is the same sequence the quick-start page already documents for a command-line update. **Unsupervised gateway, the missing stop.** The sentence told the reader to stop this Raven first, but the command had no stop step. The command now starts with `raven web --stop`, which stops the gateway recorded in `serve.json` even when no supervisor is running, the hand-started case this refusal is for. The server's English sentence names each command the card copies, and the page's translated sentence (en and zh) is reworded to describe the steps on both platforms. **Source checkout, `git pull` in the wrong repository.** The command was `git pull` then the installer, with no directory change, so it pulled whatever repository the terminal was in. It now moves into the checkout the install was made from, read from the install's PEP 610 metadata (`direct_url.json`) through a new `raven.updates.upgrade.editable_checkout()`, which `editable_checkout_status()` now reuses. POSIX gets `cd <shlex-quoted path>`; PowerShell gets `Set-Location -LiteralPath '<path>'`, with every single quote PowerShell recognises doubled, the four curly ones included, so a profile folder spelled with one cannot end the literal early. When the path cannot be read the refusal sends an explicit null command and the card drops the row, since a bare `git pull` is the wrong suggestion. The `_NO_COMMAND` comment and the page's matching comment now say "none it can stand behind" rather than "proved none can work". **Source checkout, reinstalling under a running Raven (Windows).** The Windows checkout command reached `.\install.ps1` with the checkout's Raven running. `uv tool install --force` replaces the environment that Raven runs from, and Windows cannot delete an executable still in use (the review captured `WinError 5`); `install.ps1` stops a resident gateway only in `Start-Web`, after that install. The Windows command now starts with `raven web --stop`, so the serving Raven is gone before the installer begins. POSIX replaces a running executable in place and `install.sh` stops and restarts the gateway itself after the install, so the POSIX command is unchanged. The exact-command test asserts each platform's form, and a separate test pins the stop ahead of the installer. **The upgrade helper's leftover sweep (pre-existing, Windows).** `stop_leftovers_of` embeds the uv tool directory in a single-quoted PowerShell string and doubled only the ASCII apostrophe. PowerShell also reads the four curly single quotes as quote characters, so a path carrying U+2019 (as in a profile folder spelled O\u2019Neil) produced `TerminatorExpectedAtEndOfString` / `UnexpectedToken` and the sweep could not run. All five are doubled now. `ask()` also ignored the query's exit code, so a query that failed left an empty stdout that read as "no survivors" before `uv tool install --force` deleted the environment something might still run from; a non-zero exit is now "could not ask", and the helper refuses the install instead of guessing. The queries also run with `$ErrorActionPreference = 'Stop'`, because a non-terminating `Get-CimInstance` failure otherwise still exits 0, measured in both installed PowerShell versions. **Tests that only checked the runner's platform.** The checkout test asserted `.\install.ps1 if os.name == "nt" else ./install.sh`, the production condition copied, so an inverted branch could not fail it. Both commands are now built by functions that take the platform as an argument (`_checkout_recovery`, `_unsupervised_recovery`), switched by `_on_windows()`. The tests parametrize the platform and assert the exact POSIX and Windows forms through the real handler, and a separate test pins `_on_windows()` to `os.name`. Two existing assertions encoded the old behaviour and were changed: the unsupervised command (`raven upgrade\nraven web`) and the checkout command's `startswith("git pull")`. The Windows quoting test now locates the `Set-Location` line rather than assuming it comes first. Not changed here: the CLI's own message for a source checkout (`raven upgrade` on an editable install) still prints `git pull && ./install.sh` on every platform. It already prints the checkout path above that line, so only its Windows spelling is off, and it belongs to a CLI change rather than this card. **Coordination with #1087.** Both branches rewrite the source-checkout refusal in `raven/rpc/methods/system.py` and conflict textually there. #1087 adds a second route to the same refusal: `plan_upgrade` raises `EditableInstallError` when the probe cannot classify the install, and `system.upgrade` maps it through `_refuse_checkout()`. After both land, that second route must build its command with `_checkout_recovery(editable_checkout(), ...)`, falling back to the explicit null, so both refusals name the same recovery. This branch does not add that route. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Earlier round, Linux, base `2cbacbaa3`: - `uv run --frozen --all-extras pytest -n 0 tests/test_rpc_system.py tests/test_updates_upgrade.py -k "<the 11 new or changed test names>"`: 21 passed. Before the fix the same selection failed on the missing helpers and on the old command forms; the curly-quote cases failed on the ASCII-only escape and passed once it covered all four. - Adjacent suites, `uv run --frozen --all-extras pytest -n 4` over `test_rpc_system`, `test_updates_upgrade`, `test_cli_upgrade_commands`, `test_updates_install_guard`, `test_updates_beta_channel`, `test_updates_update_notice`, `test_rpc_serve_control`, `test_cli_serve_commands`, `test_cli_gateway_commands`, `test_cli_gateway_page`, `test_cli_gateway_health`, `test_i18n_boundary`, `test_i18n_prompts`, `test_i18n_sensitive_notes`, `test_channels_display_names`, `test_stint_record`: 673 passed, 2 skipped (truststore on this interpreter), exit 0. - Mutation pass over `raven/rpc/methods/system.py`, one change at a time against `tests/test_rpc_system.py`: 12 of 12 killed (inverted `_on_windows`, swapped checkout branches, dropped `cd`, unquoted POSIX path, no PowerShell escape, ASCII-only PowerShell escape, `raven web` added back to the Windows command, the stop dropped on either platform, either handler ignoring the platform, a bare `git pull` when the path is unreadable). - `ruff check` and `ruff format --check` on the four Python files, `ty check` on the two source files, `lint-imports` (10 kept, 0 broken): all pass. - `node ui-tui/scripts/gen-i18n.mjs --check`: up to date after regenerating the TUI catalogue. - `scripts/check_commit_messages.py`, `commitlint`, `scripts/check_large_files.py`, `scripts/check_source_language.py` over the branch: all exit 0. The Linux runs above are the earlier round's; this head was not re-run on Linux. The commits since then touch the same two source files and their two test files only, and the platform-independent tests and checks below ran natively here. At this head, native Windows (Windows 11, CPython 3.12.13, `PYTHONUTF8=1`, a scratch `USERPROFILE`): - `pytest -n 0 tests/test_rpc_system.py tests/test_updates_upgrade.py`: 266 passed, 1 skipped (the POSIX permission-bit case), exit 0. - Native PowerShell probes of the generated sweep script: for a uv tool directory containing U+2019, both Windows PowerShell 5.1.26100.9711 and PowerShell 7.6.7 parsed the script and read `$root` back with code point U+2019 intact. The same script with an unknown CIM class exited 1 with empty stdout in both, which the exit-code check turns into "could not ask". - Doubling each of the five quote characters round-tripped to the original character in both shells; the un-doubled form produced a parse error in both. - Five new mutants, one at a time, each killed by the tests added here: dropping the exit-code check (two tests red), dropping the `survivors is None` guard (one red), ASCII-only escaping (four curly-quote cases red, the ASCII control green), dropping the Windows stop line (the exact-command Windows case and the ordering test red), dropping `$ErrorActionPreference = 'Stop'` (the loud-failure test red). - `ruff check`, `ruff format --check`, `git diff --check`: clean. `ty check` on the two source files exits 0 with its two pre-existing `url2pathname` deprecation warnings. `scripts/check_commit_messages.py origin/main..HEAD`: exit 0. An actual `install.ps1` reinstallation with a running Raven was not executed, and neither was an actual upgrade helper run. That Windows refuses to replace a running executable is the review's native evidence; the stop ordering is asserted as a string, and the helper's quoting and exit code are executed against both native parsers above. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed The quick-start page already describes the corrected order, so no docs change was needed. ## Risk User-visible: the upgrade card's copyable commands for the unsupervised and source-checkout refusals change, and so does the unsupervised sentence in both languages. On Windows the unsupervised command no longer starts Raven; the reader does so once the upgrade reports it has finished. On Windows the checkout command now stops the serving Raven before `git pull`, so a pull the reader abandons leaves Raven stopped until they start it again, and a source checkout whose install metadata does not name a local directory now shows no command row instead of a `git pull` that may target the wrong repository. The checkout path from the install metadata is put into a shell command the reader copies, so it is quoted for each shell rather than spliced in. The upgrade helper now refuses the install when its process query exits non-zero. That turns a Windows upgrade which used to continue through an unanswerable query into a refusal that says nothing was changed; the reader can retry once PowerShell can answer. Rollback: revert this branch. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues Follows up #1003. Coordinates with #1087 on the second refusal route described above.
…1023) ## Summary A machine registered by password under an ssh config alias, under a `HostKeyAlias`, or under a non-canonical address could never log in, and its registration blamed the password. Reported by 0xKT in the post-merge review of #1005 (point 1); point 2 is not in this PR. Cause: the askpass helper answers only `<user>@<host>'s password:` and `(<user>@<host>) Password:`, with the host as the registry row spells it. ssh names the host it resolved instead: the `HostName` the owner's ssh config gives an alias, the canonical form of a numeric address (`::1` for `0:0:0:0:0:0:0:1`), or the `HostKeyAlias` in place of either. Measured with OpenSSH 8.9p1 against a loopback test sshd: `alice@devbox` with `HostName 127.0.0.1` asks `alice@127.0.0.1's password: `, `alice@aliasbox` with `HostKeyAlias labbox` asks `alice@labbox's password: `, and `alice@0:0:0:0:0:0:0:1` asks `alice@::1's password: `. The helper refused each, ssh sent an empty password, and the probe appended "The password was refused, or this machine does not accept password logins". What changes: - `transport.ssh_env` now names, as the prompt the helper waits for, the host `ssh -G` resolves for the row: the `hostkeyalias` it prints, else its `hostname`. `ssh -G` is asked with the password connection's own options, before ssh starts and never from the helper (a helper that ran `ssh -G` itself once recursed into thousands of processes against a test ssh that called askpass), with askpass switched off for anything it starts, a 10 s timeout and tolerant decoding. When ssh cannot say (no ssh, a failure, a hang, no `hostname` line) the row's spelling stands, which is the behaviour before this change. - A password row whose host is the id of a `Host` alias Raven itself keeps in `~/.ssh/config` for a key machine is refused by name when that alias sends ssh to another address, or when its block is not the one Raven writes (a mark gone, a keyword Raven does not write such as `ProxyCommand` or `Include`, a missing or second `HostName`, a second name on the `Host` line), before ssh starts, and its password is not sent (review round, blocking thread). One whose `HostName` is its own id is followed. Without this, a key machine registered under the id `labbox`, which writes `Host labbox` / `HostName <its address>`, made the helper of a password machine registered at `labbox` wait for, and answer, the key machine's prompt; the model can register such a machine through `ops_connection_add`. The managed marks are read from the config ssh reads (the account's passwd home, which `$HOME` does not move) and from the one Raven writes (`~`); names compare ignoring case, the cautious direction, though OpenSSH is not case-blind here (measured 2026-10-20: `Host labbox` took `labbox` and not `LabBox`), and a block with one mark left still counts. `ALIAS_MARK` moves to `transport` so both modules share it. `machines.add` reports this refusal as `invalid`. The owner's own aliases resolve as before. - A declined prompt is no longer reported as a refused password anywhere. `connection_add.probe` says ssh asked something other than the account's password and that the password was not sent; `remote.explain` (node install, the tunnel) says the same instead of "check the password kept for it"; `machines.add` has a new reason, `prompt`, ahead of `login`, carried through `openrpc.json`, `models.py`, both generated clients, the register sheet and the en/zh catalog. A wrong password still reads as before. - When `ssh -G` fails, times out, cannot start or names no host, the fallback is no longer silent: `ssh_env` hands the reason to the helper (`RAVEN_MACHINE_PROMPT_UNRESOLVED`, empty when resolved), whose refusal then names it and the prompt it waited for, and the probe and `remote.explain` name the failed resolution instead of sending the owner to a key. - The resolution runs a subprocess (about 4 ms here), so every async caller that built a password runner on the event loop now builds it in a thread: exec's machine channel (`run_on_machine`), the openai sub-agent tunnel (`remote.tunnel`), and on-call's status, round-zero setup and backend in `ops_submit`, a declared action's runner, the campaigns listing, `ops_kill`, `ops_outputs`, the two case-dict tools, a reading's runner and the event watcher's backend. - `docs-site/docs/machines.md` (both languages), `CONTEXT.md` and the unreleased CHANGELOG entry say the prompt is the one for the address as ssh resolves it, so an alias from the owner's ssh config works, and that an alias Raven wrote for another machine is refused. Review round (gloryfromca, 0xKT): - Blocking thread (a managed alias redirects another machine's password): fixed as above, at the one place the helper's environment is built, so it also covers machines registered before the alias was written. Registration now refuses the pair up front too (second round, below). - 1 (on-call builds the runner on the loop): fixed at the ten on-call call sites above. Each has a test except the edit tool of the case-dict pair, which shares its helper with the case-changes tool's. - 2 (two of three classifiers still blame the password): fixed. On "the zh reader gets only the headline": the sheet shows ssh's English detail under the headline in both languages; what was wrong is that the zh (and en) headline told the reader to check the password. - Nit 1 (two ACP tests run the real `ssh -G`): fixed; they resolve through a stand-in, and the module now fails any test that runs the real `ssh -G` without `-F`. - Nit 2 (`ssh_defaults` is a second `ssh -G` reader that still decodes strictly): fixed in the second round, below. - Nit 3 (silent fallback): fixed as above. Second round (closing what the first left open): - One reader of `ssh -G`. `transport.resolve_with_ssh` is now the only one: the password machine's prompt and a key machine's registration defaults (`connection_add.ssh_defaults`: port, user, identity files in ssh's order) both resolve through it, each with its own command line. The key reader used to raise on a config byte that is not UTF-8 and on an ssh it could not start, and handed its `ssh -G` the caller's terminal and askpass; now it decodes tolerantly, reads such failures as "ssh cannot say" (empty defaults, as a missing ssh already was), and starts nothing that finds an askpass or a terminal. Where ssh answered before, a key machine's defaults are unchanged; the existing defaults tests pass as they were. - Registration refuses the colliding pair in either order, with what to do: a key machine whose id is a registered password machine's address (compared ignoring case, the cautious direction; OpenSSH itself took `Host labbox` for `labbox` only), and a password machine at a registered key machine's id (its alias may not be written yet) or at an alias Raven keeps with no machine behind it, a lone mark included (`connection_add.alias_collision`). `machines.add` (as `invalid`), the CLI and `ops_connection_add` ask before anything is kept, resolved or reached; `connection_add.write` asks again under the registry lock, which is what stops `raven ops connection add --skip-probe` (it wrote such a pair, since nothing connected). The refusal on connecting stays, for pairs registered before this check or written by hand. - Host rewriting. `ssh -G` makes none of the DNS lookups `CanonicalizeHostname` makes, while a connection that canonicalizes re-reads the config under the canonical name, where a `Host <id>` block Raven wrote can apply (OpenSSH 8.9 reports applying it under `-v`); with a `HostKeyAlias` the prompt would still match the helper. The password connection options now carry `CanonicalizeHostname=no`, which outranks the owner's config, so the connection reaches the host `ssh -G` named. The one later pass left is `Match final`: measured, it applies a Raven block re-read under the owner's `HostName`, but ssh keeps the first `HostName` it read, the owner's, and a test pins that. A machine that relied on canonicalization already failed at the prompt (the helper waited for the uncanonicalized name); it is registered at its full name. - Test isolation. Every password connection now asks `ssh -G` first, and seven tests that stubbed the connection still reached the real one through the resolution (two node-link tests, four registration cases, one aborted CLI add). The ACP module's guard is now a `no_real_ssh_g` fixture applied to the nine modules whose code reaches the reader, and those tests answer `-G` with a stand-in. A lock test pins that `transport.ssh_env` is the one place that builds the environment handing ssh a kept password, which every path (registration probes, CLI, `machines.add`, exec's machine channel, the ACP tunnel and node link, on-call runners and rsync) goes through. Windows test correction: the structural askpass-owner test now renders its discovered repository-relative paths with POSIX separators. Its scan, matching predicate, expected owner and assertion are unchanged, so native Windows no longer fails solely because pathlib displays backslashes. Third review round (0xKT; each point answered on the PR): - A key machine at its own name (`gpu1` at `gpu1`) beside a password machine at `gpu1` was refused at registration and on every connection, though ssh still logs in at `gpu1`: fixed. `transport.managed_alias_hostnames` reads where each managed block sends ssh, and `ssh_env` and `alias_collision` refuse only when a block in either home sends the name elsewhere or is not the one Raven writes; registration also asks the registry row the next add rewrites the block from, in both orders. The refusals that remain point at the IP address or full domain name. - The on-call event watcher built a campaign's backend on the loop: it now builds it with `asyncio.to_thread`, like the tools, and a new test counts the loop's heartbeats while a slow factory builds it. - The two resolution-failure tests capped every `ssh -G` stand-in at 0.3 s, which macOS's first exec of a freshly written script (320-530 ms) exceeded: only the stand-in that sleeps keeps that cap now, and a new case that starts 0.45 s late must still be named by its exit status. After main merged #1018 (0xKT, blocking; confirmed by gloryfromca): - `no_real_ssh_g` sat on `subprocess.run` and knew a stand-in only by the ssh first on `PATH`, so on the merged tree it stopped the two `ssh_defaults` tests #1018 rewrote before their Python stand-in, a swapped `subprocess.Popen`, took over. The guard is now a `subprocess.Popen` subclass that judges the argv it is finally handed: a wrapper's rewritten command line passes, and an `ssh -G` that names the computer's own ssh fails, named bare or by path, even with a stand-in first on `PATH`. A swapped `Popen` alone is not taken as a stand-in. Two more tests pin what the guard judges: an ssh started without -G runs, and the PATH it searches is the one handed to Popen. Found while re-checking every registration path against the narrower alias check: - `ops_connection_add` asked `alias_collision` before the row carried its address, so the model's path still refused key machine `gpu1` at `gpu1` beside a password machine at `gpu1`, naming "this machine's address ('')", while the CLI, `machines.add` and the registry write accepted the pair. The address now goes on an ssh row before the check; an ssh row with no address is still refused before anything is reached. Left as it was, and why: - The resolution reuses the connection's options (`-p`, the password-only options). No test notices if it read others, since none of them changes `HostName` or `HostKeyAlias`; the two mutants that swap them survive, and this is why. - An owner's own `Host` block with the same name as a managed id, placed earlier in the file, is refused too when Raven's block for that name sends ssh elsewhere: ssh would follow the owner's block, but a name Raven also routes to another address is ambiguous, and refusing sends no password anywhere. A key and a password registration of one physical machine under a shared name register as a pair when the key machine's alias names its own id as the address (key machine `gpu1` at `gpu1`, password machine `gpu1-shared` at `gpu1`); a pair whose alias would send ssh elsewhere is still refused, and the refusal says to register the password machine at its IP address or full domain name. - `test_the_table_agrees_with_the_ssh_on_this_computer` in `test_shell_machine_channel.py` (from #552, not touched here) asks the computer's own `ssh -G` how ssh parses its arguments, so it reads the runner's ssh config; it is not under the new guard, and the runs below deselect it. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Current head, rebased onto main with #1003: `git range-diff` maps the 18 earlier commits as `=`, in order, and two commits sit on top: the guard's two tests and the registration fix above. - The registration fix: its tool test failed at the commit before it, refused with "this machine's address ('')", and passes after. Single-construct mutants: 5 of 5 killed (the old check order, the address set after the check, the address left off the row, the alias check removed, the empty-address refusal removed). `pytest -n 2 -p no:randomly` over `test_ops_connection_add.py`, `test_cli_ops_connection_commands.py`, `test_shell_machine_channel.py`, `test_agents_code_launcher.py` and `test_agents_oncall_launcher.py`: 359 passed, 1 skipped; with the tool file as it was before the fix, 358 passed, 1 skipped, and only the new test failed. - The guard: both `ssh_defaults` tests and the three guard tests (named bare, by path, a `Popen` wrapper that leaves ssh alone): 5 passed. Mutants of the two conditions the guard's two new tests pin (the `-G` check dropped, this process's `PATH` read instead of the one handed to `Popen`) each fail one of them. gloryfromca, on this base before the two commits on top: every module under the guard, 492 passed with 4 existing tcsh/csh skips. - Full suite, `pytest -q -n 4 -p no:randomly` on py3.12 with child processes importing the tree under test: 13 failed, 34431 passed, 182 skipped, 2 xfailed. Those 13 tests, rerun alone on main with #1003 and on this tree with the tool file as it was before the registration fix, fail the same way; none is in a file this PR changes. - Native Windows (Python 3.13.12) on the head before this rebase, whose 18 commits are patch-identical: 554 tests passed and 59 POSIX-only cases skipped, including both `ssh_defaults` stand-in cases, the guard tests, the registration collisions, the on-call thread boundaries and the RPC schema check; 5 probes with the real Windows OpenSSH against a scratch `-F` config only; 165 machine-page tests, the web and TUI type checks, both RPC generator checks and the TUI i18n check. - `ruff check` and `ruff format --check` over the changed files, the commit message lint, `scripts/check_source_language.py origin/main...HEAD`, `scripts/check_large_files.py origin/main...HEAD` and `git diff --check`: pass. Earlier rebase onto main with #992 (the branch had ten commits then): - `git range-diff` maps all ten commits as `=`, and the branch's own diff is identical to the one before the rebase apart from hunk offsets. The rebase had no conflict and changed no file outside this PR. The second-round, first-round and native Windows results further down were measured before this rebase and keep their original scope. - The `unit 1/4` failure on the previous head (`test_a_cancel_while_this_computer_goes_on_ends_the_command_and_keeps_no_record`, `assert remote_runs.pending() == [] and not remote_runs.was_left("t1")`) was a mark one test left for the next, not this PR's code. Both cases of `test_node_stderr_replay.py::test_a_left_cli_run_s_reason_on_stderr_reaches_its_pick_up`, which run in the same shard, end with dispatch `t1` marked as left and never read. A later test on the same worker that cancels `t1` while this computer goes on then reads that stale mark. Reproduced by running the two in that order on one worker: before the rebase the cancel test fails with the CI assertion verbatim. On plain main and on the rebased branch the same three tests pass, because #992's `_this_process_goes_on` fixture now clears the marks after every test. The cancel test passes on its own on all three trees. No test or product code was changed for it. - The strict idle-ceiling failure in `unit 4/4` on the head before last (setup idle of 3.45 s and 3.04 s in two unmarked tests whose own work takes about 0.4 s and 0.01 s) is read as a runner stall. No `slow` or `production_timing` marker was added, since it would record a reason that is not true. It did not recur in the 4/4 shard of the previous head. - On the rebased tree, serially (`-n 0`), with py3.12 and child processes that import the tree under test: `pytest -n 0 --idle-ceiling-strict` over the 17 test modules this PR touches plus `test_rpc_bootstrap.py`, `test_subagent_acp_remote.py`, `test_subagent_cli_remote.py`, `test_subagent_manager.py`, `test_resume_remote.py`, `test_remote_resume.py`, `test_node_stderr_replay.py`, `test_acp_client_resume.py` and `test_remote_runs.py`: 1113 passed, 7 skipped, nothing deselected, and no test started with a left mark from an earlier one. `tests/test_subagent_cli_remote.py` alone: 60 passed. `test_rpc_schema_match.py`, `test_rpc_registration.py`, `test_rpc_instances_declare_their_errors.py` and `test_channels_display_names.py`: 525 passed. - Gates on the rebased branch: `ruff check` and `ruff format --check` over the repository's lint targets, `ty check` over the source targets, `lint-imports` (10 kept, 0 broken), `scripts/check_commit_messages.py origin/main..HEAD`, `scripts/check_large_files.py origin/main..HEAD`, `scripts/check_source_language.py origin/main...HEAD`, `git diff --check`, and the generator checks of ui-web (`gen:check`) and ui-tui (`lint:rpc`, `lint:i18n`), since main and this branch both edit `openrpc.json` and the generated clients: pass. The ui-web vitest suite was not re-run locally; CI runs it on the new head. Second round: - New tests, each watched failing at the commit before it and passing after: 4 for the one reader (an undecodable byte raised `UnicodeDecodeError`, an ssh that cannot start raised `PermissionError`, the key machine's `ssh -G` read the terminal's input and inherited askpass, and the two machines used two readers), 2 for canonicalization (real OpenSSH reported `canonicalizehostname true` / `always` for the password options before), 8 for registration (the writer in both orders, a lone mark with no machine, the model's tool, `machines.add` and the CLI with `--skip-probe` in both orders; the CLI wrote the pair before). Lock tests that pass on both sides: owners' rows that do not collide still register; `Match final` keeps the owner's `HostName`; one function builds the askpass environment (a stray module naming its variable fails it). - Single-construct mutants for this round: 14 of 14 killed (the reader's tolerant decoding, askpass, stdin and start-failure handling, a separate defaults reader, canonicalization back on, the collision check removed from the writer, `machines.add`, the CLI and the tool in turn, either direction, the case fold, the registry lookup and the managed-alias lookup). - `pytest -n 0 tests/test_ops_transport.py tests/test_ops_connection_add.py tests/test_rpc_machines.py tests/test_cli_ops_connection_commands.py tests/test_acp_remote.py tests/test_node_client.py tests/test_shell_machine_channel.py tests/test_agents_oncall_flow_*.py tests/test_rpc_schema_match.py -k "not test_the_table_agrees_with_the_ssh_on_this_computer"`: 1846 passed, 9 skipped, 14 deselected. On the merged tree: 1823 passed, 9 skipped, 23 failed; the 23 are 22 `test_node_client` IDs plus `test_install_node_closes_the_live_node_it_asked`, and the same IDs fail on a plain `origin/main` tree run the same way (their node is started with `python -I` and imports another tree than the one under test), so none is introduced. - `ruff check` and `ruff format --check` over the changed Python files, `ty check` over the changed source files, `lint-imports` (10 kept, 0 broken), `scripts/check_source_language.py origin/main...HEAD`, `scripts/check_large_files.py origin/main...HEAD`, `scripts/check_commit_messages.py origin/main..HEAD`, `commitlint`, `git diff --check`: pass. Nothing in the web client, TUI or RPC contract changed this round. First round, still standing for the code it covered: - 16 tests for the managed alias and the declined-prompt wording, 1 for the register sheet, 8 for on-call, the two ACP tests under the guard; 18 of 19 mutants killed (the survivor was the ACP guard itself, with both tests on stand-ins). - `vitest run src/features/extAgents/ExtAgentsPage.test.tsx`: 160 passed (165 on the merged tree); the ui-web contract gates (24 passed), `tsc --noEmit`, `eslint` and the three generator checks: clean. - End-to-end runs against an asyncssh test sshd on loopback with the real OpenSSH client (alias, `HostKeyAlias`, `0:0:0:0:0:0:0:1` and keyboard-interactive alias rows register and log in; an sshd that asks only "Verification code:" gets an empty answer). They were not re-run for the second round, and the full suite was not run for either round. Those historical runs did not use native Windows; the native follow-up below is separate. Native Windows follow-up (Windows 11 build 26300, Python 3.13.12, UTF-8 mode): - Reproduced the reported structural test failure before editing: the discovered path was `raven\ops\transport.py`, while the expected repository spelling used `/`. The one-line `.as_posix()` correction passed the focused test with the strict idle ceiling enabled. - After rebasing and a frozen workspace/dev sync, ran: ```powershell uv run --frozen --no-sync pytest tests/test_ops_transport.py -n 0 -q --tb=short --idle-ceiling-strict --basetemp "$evidenceDir/rebased" --junitxml "$evidenceDir/rebased.xml" ``` Result: 10 passed, 45 skipped, exit 0. The skips are 43 POSIX askpass/helper cases and two POSIX file-mode cases. `$evidenceDir` is an external report directory. Imports were verified to resolve to the dedicated worktree; profile and OAuth paths were isolated. - An earlier native full-module run passed every assertion but failed the strict idle gate in `test_the_runner_sends_what_it_always_sent` (3.43 s idle). The final run above retained the strict gate and passed; no timing marker, ceiling or test assertion was weakened. - Full-PR `uv run --frozen --no-sync pre-commit run --from-ref origin/main --to-ref HEAD` passed using Git Bash. Its frontend hooks return without running their tools when the corresponding `node_modules` is absent; frontend tests/generators were not rerun locally for this test-only correction. - `uv run --frozen --no-sync ty check` over all changed production Python files passed. `uv run --frozen --no-sync lint-imports` kept all 10 contracts. The complete-range source-language, large-file and commit-message gates, lockfile-installed commitlint, and `git diff --check origin/main...HEAD` passed. On Windows the commit-message wrapper could not spawn `npx`, so its two checks were run directly: the installed `commitlint.cmd` and `scripts.commit_lint.check_commit_message`. - The previous remote head's Linux unit shard had 8666 passed, 23 skipped and 1 xfailed, but failed the strict idle ceiling for `test_the_window_starts_when_the_run_is_released` and `test_an_install_the_login_path_misses_is_not_reported_as_absent` (3.45 s and 3.04 s). Those tests and the ceiling were not changed by this correction. New-head CI is assessed separately. - No live SSH server, Linux suite, browser run, or full-repository native suite was run for this correction. Earlier broad results are retained above with their original scope. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed ## Risk - A machine registered under an alias, a `HostKeyAlias` or a non-canonical address now logs in; nothing else changes which prompt the helper answers. The password still goes only to the host ssh connects to for the row, and never through a `Host` block Raven wrote that sends ssh to another address or is not the block Raven writes. - A password machine registered at a name that is also a key machine's id is now refused by name, with the password unsent, when that machine's alias sends ssh to another address; before this PR its login went to the key machine's address and failed there. Registering such a pair is refused in either order on every registration path, and a pair already registered is refused on connecting. A key machine whose alias names its own address (`gpu1` at `gpu1`) leaves a password machine at `gpu1` free to register and log in. - A key machine's registration defaults come from the same reader: unchanged where ssh answered, empty instead of an exception where it could not (an undecodable config byte, an ssh that cannot start). - Password connections no longer canonicalize the host; a password machine that relied on `CanonicalDomains` to complete a short name is registered at its full name. - Every password connection starts one `ssh -G` first (about 4 ms here; 10 s at most, after which the row's spelling stands), off the event loop everywhere. It reads the owner's ssh config, so a `Match exec` there runs once more. - `machines.add` can answer the new reason `prompt`; the web client and the TUI types are regenerated with it. The askpass helper's bytes change, and it is rewritten on the next password connection, as before. - Rollback: revert the commits; nothing is stored or migrated. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues Follows the post-merge review of #1005.
… refusal (#1087) ## Summary Two follow-ups to #1003, both from its review. - A checkout that `plan_upgrade` finds now gets the same `editable` refusal as one the pre-check finds. `system.upgrade` classifies a checkout first, but its probe takes an unreadable metadata read as "not a checkout" and lets the upgrade go on. `plan_upgrade` then reads the metadata again and refused the checkout with a plain `UpgradeError`, which reached the page as `not_upgradable` with no command, so the card fell back to `raven upgrade`, which a checkout refuses. In review this read as "plan_upgrade's own editable refusal can no longer be reached from the page"; it is unreachable only while the probe can read the metadata. - `plan_upgrade` now raises `EditableInstallError`, an `UpgradeError` subclass, and checks for a checkout before the release lookup: "already up to date" against a release says nothing true about a checkout, and the old order let that refusal answer first. `system.upgrade` maps `EditableInstallError` to the editable refusal through one helper shared with the pre-check, so both name `git pull` and the platform's install script. - The guard in `plan_upgrade` stays on purpose. An `./install.sh` checkout is an editable uv tool install with a receipt of its own, so `_uv_tool_target()` resolves for it; without the guard, the path above would plan a release wheel over the checkout. - The web client's cast of the refusal payload now types `data.command` as `string | null`. The server sends an explicit null when a refusal proved no command can work, and the card hides its command row on exactly that null; the old `string` type did not describe the wire. Its only reader passes it to `shade.fail(..., command?: string | null)`, which already took null, so nothing narrowed on the old type. Not changed here: - On a hand-started gateway the hosted checks run before `plan_upgrade`, so the probe-failure path there still answers `unsupervised` (its copied commands start with `raven upgrade`). It needs the probe read to fail, and the pre-check already answers a readable checkout first. - The CLI still prints `git pull && ./install.sh` on every platform while the page picks `.\install.ps1` on Windows; #1085 tracks one shared definition. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - [x] Relevant tests pass locally: `uv run --frozen --python 3.12 --all-extras --all-packages pytest -q -p no:cacheprovider -n 2 tests/test_rpc_system.py tests/test_updates_update_notice.py` -> 89 passed. The new test drives the real `plan_upgrade`: the first metadata read raises, the second finds a checkout, the release lookup and the handoff are tripwired, and it asserts two reads, reason `editable` and the `git pull` plus install-script command. - [x] After the rebase onto `e8332e5d`, on native Windows with CPython 3.12.13: `pytest tests/test_rpc_system.py tests/test_updates_update_notice.py tests/test_updates_upgrade.py` -> 305 passed, 1 skipped (a POSIX permission-bit case). Main's #1086 landed the shared checkout recovery after this branch was cut, so the refusal now names the checkout's own path (`raven/rpc/methods/system.py::_checkout_recovery`) instead of a hardcoded `git pull` line; the branch keeps the PR's `EditableInstallError` mapping and the one shared helper, and the new test patches `editable_checkout` and asserts the command carries `git pull` and the install script rather than starting with it. Web: `npx vitest run src/app/updates.test.ts src/state/upgradeShade.test.ts` -> 29 passed; `npm run type-check` exit 0, with `tsc --listFiles` confirming it read this tree's `src/app/updates.ts`. - [x] The new test fails without the fix: with only the source changes reverted it fails (`assert 1 == 2`, the release lookup ran before the guard). Four single-construct mutants each fail it too: dropping the `EditableInstallError` mapping (`not_upgradable`), raising a plain `UpgradeError` from the guard (`not_upgradable`), deleting the guard (`1 == 2`), and moving the guard back after the release lookup (`1 == 2`). - [x] Web: `npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts` -> 29 passed; `npm run type-check` exit 0, with `tsc --listFiles` confirming it read this tree's `src/app/updates.ts`; `npx eslint src/app/updates.ts` clean. - [x] Relevant lint / type checks pass locally: `ruff check` and `ruff format --check` on the three touched Python files; `ty check` on the two touched modules; `lint-imports` 10 kept, 0 broken; `git diff --check`; `make check-commits check-large-files check-source-language` exit 0. - [ ] User-facing docs or screenshots are updated when needed: no copy changed; the probe-failure path now shows the existing editable refusal text. ## Risk - [x] Security impact considered: the command strings are the existing constants chosen by reason, never built from input. - [x] Backward compatibility considered: `EditableInstallError` subclasses `UpgradeError`, so every existing `except UpgradeError` still catches it; `plan_upgrade` has one caller, `system.upgrade`. For an install that is not a checkout the order change only moves one local metadata read ahead of the release lookup; a malformed `direct_url.json` is refused by the pre-check before `plan_upgrade` runs. - [x] Rollback path is clear: plain revert. ## Related Issues Follow-up to #1003. #1085 tracks the CLI and page spelling of the checkout update command (not changed here).
…#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>
Summary
raven upgrade.update_notice._upgrade_command_works()now excludes editable installs, as its docstring always claimed: an editable uv-tool install (a checkout under ./install.sh) passes the receipt check alone, and bothraven upgradeand the page's fallback refuse it.system.upgraderefusals carry an optionaldata.commandthe card copies:unsupervisednamesraven upgradethenraven web, and a neweditablerefusal fires before the release lookup and namesgit pullplus the platform's install script (.\install.ps1on Windows,./install.shelsewhere).raven upgrade) always refuses a checkout.editableframe is translated (gui.upg.why.editable, en and zh, TUI catalogue regenerated)._is_editable_installto False: a dev venv (and CI) install raven editable from the tree, so without the pin the new pre-check would fire everywhere.Type
Verification
uv run pytest tests/test_rpc_system.py tests/test_updates_update_notice.py-> 84 passed (the overlap test arms the hosted-unsupervised state with the editable probe true and asserts the checkout answer wins);npx vitest run src/state/upgradeShade.test.ts src/app/updates.test.ts-> 25 passed (both file orders);npm run type-checkand eslint on the four touched web files cleanruff check+ruff format --checkon the touched Python files;ty checkidentical to a stashed baseline (zero new)Risk
commandkeep today's card verbatim; the button's absence on checkouts is the docstring's promised behaviourRelated Issues
Part of #716 (the remainder scoped in its thread; #806 already landed most of A and B)