fix(windows): avoid encoded PowerShell updater - #36
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors Windows native updates to spawn installers directly (no PowerShell -EncodedCommand), standardizes Inno installer flags across bootstraps to ChangesWindows Installer and Update Refactoring
Sequence DiagramsequenceDiagram
participant Updater
participant OS
participant InstallerExe
Updater->>OS: spawn_installer
OS->>InstallerExe: execute_installer
InstallerExe->>Updater: detached_exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/public/install.ps1`:
- Line 201: The script assigns to the automatic PowerShell variable $args which
shadows the built-in; rename the local variable (for example to $installArgs or
$scriptArgs) everywhere it is defined and referenced in docs/public/install.ps1
(look for the $args = @( ... ) declaration and subsequent uses) to avoid
PSAvoidAssignmentToAutomaticVariable; ensure all references (parameter
expansion, passing to functions, splatting, etc.) are updated consistently and
run the script to verify behavior remains identical.
In `@packages/windows-installer/build.ps1`:
- Around line 100-108: The /SPythinkerSign argument can lose embedded quotes on
PowerShell 5.1 when passed to the native iscc.exe because $signCommand embeds
quoted -File "$signScript"; fix by avoiding nested quoted paths: either (A)
build $signCommand to call a small wrapper .cmd (e.g., create a sign-wrapper.cmd
that runs powershell -NoProfile -NonInteractive -File "%~1" %2 and set
$signCommand to "cmd.exe /C \"path\to\sign-wrapper.cmd\" `"$signScript`" `$f"),
or (B) convert $signScript to its short (8.3) path via GetShortPathName and use
that short path in $signCommand; then append that safer $signCommand into
$isccArgs (still gated by $signingConfigured) and add a diagnostic log of
$isccArgs before invoking & $iscc.Source so you can confirm the exact argument
delivered to iscc.exe.
In `@scripts/install.ps1`:
- Line 201: The script assigns to PowerShell's automatic variable $args which
shadows the built-in and triggers PSAvoidAssignmentToAutomaticVariable; rename
the local variable (for example to $scriptArgs or $installArgs) everywhere it is
constructed and referenced (the $args = @(...) assignment and subsequent uses)
so no code assigns to or reads from the automatic $args; update any dependent
references to use the new name consistently.
In `@src/pythinker_code/ui/shell/update.py`:
- Around line 777-785: The returned installer argument list in
_windows_native_installer_args omits the /CURRENTUSER flag, causing in-app
updates to attempt system-wide installs; update the function
_windows_native_installer_args to include "/CURRENTUSER" in the returned list
(alongside "/SILENT", "/NORESTART", "/CLOSEAPPLICATIONS",
"/NORESTARTAPPLICATIONS") so the Inno Setup installer runs per-user consistent
with the PowerShell bootstraps.
- Around line 1007-1008: Move the module-level constant
WINDOWS_UPDATE_STAGING_MAX_AGE_SECONDS from its current mid-file location into
the top-of-file constants block (immediately after imports alongside the other
constants) so all configuration constants are grouped together; then delete the
duplicate definition where it currently appears mid-file to avoid redundancy and
ensure the code references the single top-level constant.
In `@tests/ui_and_conv/test_shell_update.py`:
- Around line 749-775: Update the test expectations in
test_spawn_detached_windows_installer_uses_inno_directly_not_powershell to
include the missing /CURRENTUSER flag so it matches the installer args produced
by update._windows_native_installer_args() and consumed by
update._spawn_detached_windows_installer(); specifically, adjust the expected
args list in that test (the launched variable assertion) to include
"/CURRENTUSER" among the flags so the assertion reflects the actual arguments
returned by the update module.
In `@web/public/install.ps1`:
- Line 201: The script assigns to the automatic PowerShell variable $args which
shadows built-in parameters; rename that variable (for example to $installArgs
or $scriptArgs) wherever it is declared (the line with "$args = @(") and update
all subsequent references to use the new name so you no longer assign to the
automatic $args (this will satisfy PSAvoidAssignmentToAutomaticVariable and
avoid confusing shadowing).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e09222d-f686-4edc-8c56-6601b0f36bde
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mddocs/public/install.ps1packages/windows-installer/build.ps1packages/windows-installer/installer.issscripts/install.ps1src/pythinker_code/ui/shell/update.pytests/test_installation_docs.pytests/test_release_update_pipeline.pytests/ui_and_conv/test_native_update_parity.pytests/ui_and_conv/test_shell_update.pyweb/public/install.ps1
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_release_update_pipeline.py`:
- Around line 81-89: Replace the fragile white-box assertions in the test that
check exact PowerShell command and log text with contract-focused checks: assert
that signing is enabled by checking "/DUseInnoSignTool=1" is in build_script,
assert the env-driven sign script path is wired by checking
"PYTHINKER_INNO_SIGN_SCRIPT" (and that "/SPythinkerSign=$signCommand" remains
present), and keep the negative assertion that the old encoded-command form
'-File `"$signScript`"' is not present; remove assertions that pin the exact
cmd.exe/powershell invocation string and the specific "build.ps1: invoking Inno
Setup with arguments:" log text so the test validates behavior not internal
quoting/logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5cbb92f-6354-4ea6-abea-a3b21e022a7f
📒 Files selected for processing (9)
README.mddocs/public/install.ps1packages/windows-installer/build.ps1scripts/install.ps1src/pythinker_code/ui/shell/update.pytests/test_release_update_pipeline.pytests/ui_and_conv/test_native_update_parity.pytests/ui_and_conv/test_shell_update.pyweb/public/install.ps1
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/update.py (1)
827-833:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle fallback installer spawn failure explicitly
Line 827 currently performs a second
subprocess.Popen(...)without a guard. If that call raises,/updatecan crash with a traceback instead of a clear next step for the user.Proposed fix
def _run_native_installer(installer_path: Path) -> None: @@ if _spawn_detached_windows_installer(installer_path): sys.exit(0) - subprocess.Popen( - [str(installer_path), *_windows_native_installer_args()], - creationflags=getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) - | getattr(subprocess, "DETACHED_PROCESS", 0), - close_fds=True, - ) - sys.exit(0) + try: + subprocess.Popen( + [str(installer_path), *_windows_native_installer_args()], + creationflags=getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + | getattr(subprocess, "DETACHED_PROCESS", 0), + close_fds=True, + ) + except OSError as exc: + logger.exception("Failed to launch Windows installer:") + raise typer.Exit("Failed to launch installer. Please run /update again or install manually.") from exc + sys.exit(0)As per coding guidelines,
**/*.py: Keep exceptions actionable. User-facing CLI errors should explain what to do next.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/update.py` around lines 827 - 833, The second subprocess.Popen call that launches the fallback installer (using installer_path and _windows_native_installer_args with creationflags) can raise and currently crashes /update; wrap this subprocess.Popen(...) plus the subsequent sys.exit(0) in a try/except that catches Exception, emit a clear, actionable user-facing message (e.g., via the module's CLI logger or click.echo/print) explaining the installer spawn failed and what to do next (path to installer, manual run instructions), and then exit with a non-zero status; ensure the exception details are included in the log for debugging while keeping the message user-friendly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/windows-installer/installer.iss`:
- Around line 104-106: The current use of StringChangeEx(OrigPath, Param, '',
True) is unsafe because it removes Param as a substring anywhere in OrigPath;
remove that raw substring replacement and only perform delimiter-aware removals
using the existing StringChangeEx calls that target ';' + Param, Param + ';'
(and if desired also ';' + Param + ';' or a normalized ';' + OrigPath + ';'
approach) so only exact PATH entries are removed; update the logic around the
StringChangeEx calls (referencing StringChangeEx, OrigPath and Param) to
normalize/boundary-wrap the path string or explicitly search/replace ';' + Param
+ ';' variants instead of the plain Param replacement.
In `@tests/test_release_update_pipeline.py`:
- Around line 75-76: The test currently asserts exact source snippets ("NewPath
:= Param + ';' + OrigPath" and its negation) which couples the test to
implementation text; replace these fragile asserts with behavior-based checks
against the installer_script variable—e.g., assert that the NewPath assignment
puts Param before OrigPath (check the order or match a looser regex for "NewPath
:= ... Param ... OrigPath") and that the reversed order does not occur; update
the assertions in tests/test_release_update_pipeline.py to verify
ordering/behavior rather than exact source-string equality using the
installer_script identifier.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/update.py`:
- Around line 827-833: The second subprocess.Popen call that launches the
fallback installer (using installer_path and _windows_native_installer_args with
creationflags) can raise and currently crashes /update; wrap this
subprocess.Popen(...) plus the subsequent sys.exit(0) in a try/except that
catches Exception, emit a clear, actionable user-facing message (e.g., via the
module's CLI logger or click.echo/print) explaining the installer spawn failed
and what to do next (path to installer, manual run instructions), and then exit
with a non-zero status; ensure the exception details are included in the log for
debugging while keeping the message user-friendly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c49019b-91e3-4299-ad78-b2ff29e76194
📒 Files selected for processing (4)
packages/windows-installer/installer.isssrc/pythinker_code/ui/shell/update.pytests/test_release_update_pipeline.pytests/ui_and_conv/test_native_update_parity.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-EncodedCommandhelper with direct Inno installer launch/SILENTprogress plus Restart Manager flags instead of fully suppressed setupVerification
make check-pythinker-codeuv run pytest tests/ui_and_conv/test_shell_update.py tests/ui_and_conv/test_native_update_parity.py tests/test_release_update_pipeline.py tests/test_installation_docs.py -qpackages/windows-installer/build.ps1,scripts/install.ps1,docs/public/install.ps1,web/public/install.ps1Summary by CodeRabbit
Improvements
Documentation