Repository navigation
Conversation
|
Windows compatibility review — PR #1136 @ Scope: full base One finding (low–medium, user-facing guidance only). The CLI's Windows command is not runnable in the shells a Windows user is likely inFile: The script name is correct, but the command is not executable as printed: Neither Remedy direction: on Windows print the two steps separately, as the page does ( Verification (native Windows 11, CPython 3.12.13, isolated worktree The two shell probes above are the runnability evidence; the new tests assert the string only, not that a shell can execute it. Limitations: not exercised under PowerShell 7 or an elevated policy; the change is a printed message, not upgrade logic. |
|
Blocking: the Windows CLI guidance must be executable in supported Windows shells. Agreed. I independently checked the changed CLI path and the existing PowerShell-shaped RPC recovery path. The RPC path uses separate lines and remains valid, but the CLI now emits |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the Windows CLI guidance must be executable in supported Windows shells.
I independently reviewed the full base-to-head diff, the CLI and RPC callers, the installer behavior and relevant history, backward compatibility, test changes, and the repository's AGENTS.md/CLAUDE.md and CONTEXT-MAP.md architecture rules. The shared platform helper is otherwise coherent, preserves the RPC recovery text, and introduces no dependency or architecture issue. However, the Windows CLI command remains unusable as printed, which is the user-facing path this fix intends to repair.
Verification: uv run pytest tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_updates_upgrade.py -q initially could not enter test bodies because the fresh environment lacked the workspace raven_everos package. After uv sync --all-packages, the exact command passed: 314 passed, 2 skipped. uv run ruff check and uv run ruff format --check passed on all four changed files. I also checked that the new tests were additive rather than weakened; they cover helper selection but not the rendered Windows CLI command that fails here.
|
Blocking: the Windows CLI guidance this PR changes is not runnable as printed; that finding is already filed by @gloryfromca at this head and is not re-filed here. Scope: I reviewed all 4 changed files in this PR. None of them is under I examined the full base-to-head diff ( Confirmed findings:
Already filed, not re-filed:
Verification:
Checked and deliberately not reported:
|
`raven upgrade` printed `git pull && ./install.sh` on every platform, while the page's editable refusal already picked the install script by platform, and on Windows install.sh refuses to run and points at install.ps1. Add install_script() in raven/updates/ as the one definition that chooses the script, and read it from both the CLI's editable-branch message and the checkout recovery command the RPC refusal names, so the two spellings cannot drift apart again. Co-authored-by: GLM (GLM-5.3-Flash) <noreply@z.ai>
The CLI printed `git pull && .\install.ps1` on Windows. Windows PowerShell 5.1 rejects `&&` as a statement separator, and cmd.exe does not run a .ps1 by name, so neither shell could run the one line the editable refusal printed. checkout_update_commands() now renders the Windows form as two commands and names the PowerShell that ships with Windows; POSIX keeps the one-line form every release has printed. The release-notes template names both installers. The CLI tests now pin the platform seam instead of the host's os.name, so the POSIX expectations run unchanged on a Windows host. Co-authored-by: Claude (deepseek-flash) <noreply@anthropic.com>
f7aa03d to
2e28f90
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit resolves the outstanding Windows-shell blocker: the CLI emits separate update steps and explicitly launches install.ps1 through Windows PowerShell with execution-policy bypass. POSIX keeps its existing git pull && ./install.sh form. The rendered Windows CLI output now has direct coverage, the existing CLI cases are pinned to their intended POSIX branch rather than weakened, and the release-notes template now names both platform forms. I closed the superseded thread.
I reviewed the previous-to-current delta and the full base-to-head diff, affected CLI/RPC callers, installer behavior and relevant history, backward compatibility, test changes, and AGENTS.md/CLAUDE.md/CONTEXT-MAP.md architecture and language constraints. No new findings.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 passed with 326 passed and 2 existing truststore-dependent skips. Ruff check and format checks passed. make is unavailable in this environment, so I ran the underlying scripts/check_large_files.py and scripts/check_source_language.py commands directly against github/main..HEAD; both passed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision is a rebase of the previously reviewed fix onto current main. After refreshing github/main, the range-diff shows the code and tests are patch-equivalent; only the release-notes hunk was replayed against newer upstream wording, preserving the same platform-specific guidance. The resolved Windows-shell fix remains intact, and my earlier thread is still resolved.
I checked the full current base-to-head diff, the old-to-new range-diff, affected callers and history, backward compatibility, test strength, and AGENTS.md/CLAUDE.md/CONTEXT-MAP.md architecture and language constraints. No new findings.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 passed with 328 passed. Ruff check and format checks passed, as did scripts/check_large_files.py and scripts/check_source_language.py against github/main..HEAD.
|
Windows compatibility review — PR #1136 @ Scope: the PR's own diff at this head is Windows angle: no defect found. The change fixes a real Windows problem correctly — Windows PowerShell 5.1 rejects Verification (native Windows 11; detached worktrees
Coverage gaps: the printed guidance was verified through the tests and by reading, not pasted into a real PowerShell/cmd here; no live page editable-upgrade flow was driven. No Windows-specific issue found. |
The Windows guidance still replaced the environment while the served page held raven.exe, so uv tool install --force hit access denied. It now starts with raven web --stop, like the page's checkout recovery; the release-notes template names the same sequence and the stock-policy-safe PowerShell form. Keep the 67-character installer command on one line at narrow widths, warn when git pull fails, and pin the CLI's platform switch to os.name. The release-notes test now rejects the git pull && .\install.ps1 one-liner. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit closes the remaining Windows guidance gaps without regressing the earlier fix: it stops the resident gateway before replacing the executable, tells the reader not to install after a failed pull, keeps the PowerShell invocation on one copyable line, and aligns the release notes and platform tests. A merge-tree check against current github/main is conflict-free, and my earlier finding thread remains resolved.
I reviewed the new delta and full base-to-head diff, affected CLI/RPC and installer paths, relevant history, backward compatibility, test strength, and AGENTS.md/CLAUDE.md/CONTEXT-MAP.md architecture and language constraints. I tried to refute the shutdown requirement against the installer and web-stop behavior; Windows cannot reliably replace the running raven.exe, so the added step is warranted. No new findings.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 passed with 331 passed. Ruff check and format checks, git diff --check, scripts/check_large_files.py, and scripts/check_source_language.py all passed.
|
Windows compatibility re-review — PR #1136 @ Scope: the delta since the previously reviewed head Windows angle: correct, and each addition answers a real Windows constraint — Verification (native Windows 11; detached worktrees
Coverage gaps: the printed guidance was verified through the tests and by reading, not pasted into a real PowerShell/cmd here; no live page editable-upgrade flow was driven. No Windows-specific issue found. |
The page's editable recovery still named a bare `.\install.ps1`, which the stock Windows client policy (Restricted) refuses as a script file. The installer invocation now comes from install_command(), the same definition the CLI reads, and the page's git pull gates it so a failed pull does not reinstall a checkout that did not move. Co-Authored-By: Claude Code <noreply@anthropic.com>
The template test asserted the installer forms but not the `raven web --stop` step that keeps a running raven.exe from blocking `uv tool install --force`. Pin the Windows order too, and correct the CLI test docstring that still says the guidance is two lines. Co-Authored-By: Claude Code <noreply@anthropic.com>
The quick start showed one POSIX block and a bare PowerShell script parenthetical, so a Windows reader either ran install.sh (which refuses) or hit the stock execution policy. Name the stop, pull and policy-safe installer sequence the CLI and release notes now print. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The new full-command helper correctly keeps the CLI and RPC recovery on the same policy-safe Windows invocation, and the RPC now gates installation on a successful pull. The English and Chinese quick-start updates match the implemented three-step Windows sequence. Current-main merge simulation is conflict-free, and my prior finding thread remains resolved.
Nonblocking follow-ups:
checkout_update_commands()still says the page recovery embedsinstall_script(), but this revision intentionally changes that path toinstall_command(); the docstring should name the new shared seam when convenient.- The three new commits use
Co-Authored-By: Claude Code <noreply@anthropic.com>without the actual model ID required by AGENTS.md section 3.3. The repository's commit-message checker does not currently catch that attribution detail.
I reviewed the new delta and full base-to-head diff, all helper call sites, CLI/RPC and installer behavior, relevant history, backward compatibility, test strength, and AGENTS.md/CLAUDE.md/CONTEXT-MAP.md architecture and language constraints.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 passed with 335 passed. Ruff check and format checks, git diff --check, current-main git merge-tree, and the large-file, source-language, and commit-message scripts all passed.
The page's recovery now reads install_command(), not install_script(), so the helper's docstring named the wrong definition. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The new commit correctly updates checkout_update_commands() to name install_command(), resolving the stale-docstring follow-up without changing runtime behavior. The implementation remains mergeable and my earlier finding thread remains resolved.
One nonblocking follow-up remains: the new commit also uses Co-Authored-By: Claude Code <noreply@anthropic.com> without the actual model ID required by AGENTS.md section 3.3, so the same attribution issue now appears in four recent commits. The repository's commit-message checker still passes and does not enforce that detail.
I reviewed the one-line delta, full base-to-head diff context, affected helper call sites, current-main mergeability, backward compatibility, test strength, and the repository rules previously covered.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 passed with 335 passed. Ruff check and format checks, git diff --check, current-main git merge-tree, and the large-file, source-language, and commit-message scripts all passed.
|
All five findings from the re-review are fixed, plus two consistency fixes found while re-reviewing, in
Verification:
This is a comment, not a review approval; the PR still needs a formal approving review. |
The page's POSIX recovery still ran ./install.sh after a failed git pull, while the CLI and the release notes use the guarded one-liner. Keep the page on the same `git pull && ./install.sh` form so a failed pull never reinstalls an unmoved checkout. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Windows compatibility re-review — PR #1136 @ Scope: the delta since the previously reviewed head Windows angle: correct, and it closes the two real remaining Windows gaps — a bare Verification (native Windows 11; detached worktrees
Coverage gaps: the generated PowerShell recovery script was verified through the tests and by reading, not executed under a real Restricted-policy shell here; no live page editable-upgrade flow was driven. No Windows-specific issue found. |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: update the RPC classification test for the guarded POSIX recovery command.
The new POSIX git pull && ./install.sh behavior is sound and aligns the page, CLI, release notes, and quick-start guidance. However, the affected suite now fails because TestUpgradingFromAGatewayHostedPage.test_it_classifies_a_checkout_before_the_unsupervised_refusal still asserts that git pull is a complete standalone line. Update that existing assertion to the new guarded command while preserving its editable-before-unsupervised classification check.
I reviewed the new delta and full base-to-head context, callers, history, backward compatibility, test changes, current-main mergeability, and the repository rules previously covered. I tried to refute the failure as unrelated; it is directly caused by this commit's intended command-shape change.
Verification: uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0 produced 334 passed and 1 failed at tests/test_rpc_system.py:360. Ruff check and format checks, git diff --check, current-main git merge-tree, and the large-file, source-language, and commit-message scripts passed.
The case asserted an exact `git pull` line, which only holds on the Windows branch. The POSIX recovery now prints `git pull && ./install.sh`, so assert the substring the case actually means. Co-Authored-By: Claude Code <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The one-line delta fixes the failing RPC classification test without weakening its editable reason/classification coverage; I replied on and resolved my blocker thread. The focused upgrade, CLI, RPC, and release-plugin suite passes: 335 tests.
I rechecked the delta, affected recovery caller and test, backward compatibility, test integrity, and the previously reviewed architecture. The repository rules and prior CLAUDE/CONTEXT assessment are unchanged. Ruff lint/format, large-file, source-language, commit-message, diff-check, and merge-tree checks pass. The previously reported nonblocking commit-attribution follow-up remains.
LivXue
left a comment
There was a problem hiding this comment.
No blockers at 9d4e6953e.
The two re-review findings are fixed: the page's POSIX recovery now uses the guarded git pull && ./install.sh, and the RPC classification test accepts it. CI is green (22 success / 4 skipped / 0 failure), all review threads are resolved, and the PR description carries the required co-author trailer.
Summary
On a source checkout, Raven named the checkout's update command in two places with two spellings:
raven upgradeprintedgit pull && ./install.shon every platform, while the page'seditablerefusal fromsystem.upgradealready picked the install script by platform (.\install.ps1on Windows). On Windows the CLI's spelling was also unrunnable as printed: Windows PowerShell 5.1 rejects&&as a statement separator, cmd.exe cannot run a.ps1by name, and the stock client execution policy (Restricted) refuses a bare script file.raven/updates/upgrade.pynow holds both platform definitions:install_script()chooses the script,install_command()chooses the command that runs it, andcheckout_update_commands()composes the platform sequence from them. The page's checkout recovery readsinstall_command()inside its own PowerShell script; the CLI readscheckout_update_commands().POSIX keeps the one-line
git pull && ./install.shevery release has printed. Windows gets three steps:raven web --stopreleases the runningraven.exebeforeuv tool install --forcereplaces the environment. The installer line names the PowerShell that ships with Windows, so the guidance is runnable from cmd.exe and Git Bash as well as Windows PowerShell 5.1 and 7.-ExecutionPolicy Bypassapplies to that one process only and changes no machine policy.The page's editable recovery uses the same installer invocation, and it runs the installer only when
git pullsucceeded, so a failed pull does not reinstall a checkout that did not move. The release-notes template and the docs-site quick start (English and Chinese) name the same Windows sequence.Type
Verification
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
uv run pytest tests/test_updates_upgrade.py tests/test_cli_upgrade_commands.py tests/test_rpc_system.py tests/test_release_plugin_list.py -q -n 0-> 334 passed, 1 skipped (POSIX permission bits on Windows).New cases pin
install_command()on both platforms and its default, the page's policy-safe installer and pull guard, the page/CLI shared invocation, and the release-notes stop, pull and install order. Mutation checks: returning the bare script frominstall_command(), dropping the page guard, or droppingraven web --stopfrom the release notes turns the corresponding tests red. The sources were restored byte-for-byte after each mutation.Real shells on native Windows 11: the printed Windows installer command ran a stub
install.ps1from cmd.exe, Windows PowerShell 5.1 and Git Bash. The page's guarded pull-and-install lines, run under a parent-ExecutionPolicy Restricted, ran the installer after a successfulgit pulland skipped it after a failed one.uv run ruff checkanduv run ruff format --checkover the changed files -> clean;git diff --check-> clean;scripts/check_commit_messages.py origin/main..HEAD-> exit 0.CI runs the docs build and the full unit suite.
Risk
User-visible change: on Windows,
raven upgradeon a source checkout prints three commands instead of the unusable one line, and the page's editable refusal now runs the installer through a process-scoped policy bypass and only after a successful pull. On POSIX the printed CLI command is unchanged. The change alters advisory text, the page's recovery script, the release-notes template, the quick start, and the shared helper; no new command is executed by Raven itself. Rollback: revert this PR; the change is contained to the CLI, the page's recovery text, the release-notes template, the quick start, and one shared module.Related Issues
Fixes #1085
Co-authored-by: Claude (deepseek-flash) noreply@anthropic.com