fix(repo): pythinker-cli rename sweep + feat(installer): hosted Windows uninstaller - #252
Merged
Conversation
The GitHub repository was renamed from Pythoughts-labs/pythinker-code to Pythoughts-labs/pythinker-cli, and the old name's redirect no longer applies. Update every repo-slug constant and URL: install scripts (all copies), the in-app updater, the default feedback repo, the docs-pages repository guard, winget/homebrew/scoop workflow links, README, docs, and matching tests.
Add scripts/uninstall.ps1, served at https://pythinker.com/uninstall.ps1 mirroring the installer bootstrap (irm ... | iex). It runs the registered Inno uninstaller silently, sweeps validated install dirs, PATH entries (user + system, kind-preserved), Start Menu shortcuts, uninstall registry keys (32/64-bit views), and stale installer temp dirs, then verifies the final machine state with fail-closed semantics and a structured summary. Safety: registry-provided paths are never deleted or executed without validation (leaf named Pythinker plus on-disk evidence, no reparse-point component, trusted unins<N>.exe only); processes are stopped per-PID only when rooted in a validated install dir with StartTime/Path revalidation; the script never self-elevates and never calls exit, so piping through iex cannot close the host session. User data is kept unless PYTHINKER_PURGE_DATA=1. Ship byte-identical copies in docs/public and web/public with matching _headers cache entries, document the one-liner in the README Windows section and the getting-started guide, and cover sync/parsing/README assertions in test_installation_docs.py.
…n policy Replace the removal engine with a snapshot-based design (Get-SafeTreeSnapshot + Remove-SafeSnapshotNow): complete-or-fail-closed tree inspection, per-entry non-recursive deletes that never rely on PS 5.1 reparse traversal, and re-inspection before reboot scheduling. Tighten policy: recursive sweeps run only against known install dirs (LOCALAPPDATA + ProgramFiles x2); registry-selected custom installs are never swept or executed automatically - they get exact PATH-token removal and a manual-action report. Inno uninstallers execute only non-elevated, user-scope, from a known dir, as a trusted unins<N>.exe; no registry selected binary ever runs with an elevated token. Also: canonicalize all registry-provided paths (reject roots, UNC/device, drive-relative, reparse components), re-read PATH under the writable handle before SetValue, broadcast WM_SETTINGCHANGE only on actual change, prove installer-temp ownership via AppId marker or verified SHA-256, handle 32-bit registry views natively, dedupe warnings, and bound the uninstaller run with a 10-minute timeout. Uses DirectorySeparatorChar in path-prefix checks (identical to '\' on Windows; keeps the logic testable cross-platform).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change redirects repository references from ChangesRepository maintenance and uninstall flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Uninstaller
participant Registry
participant Processes
participant FileSystem
User->>Uninstaller: Start uninstall
Uninstaller->>Registry: Discover and validate installation
Uninstaller->>Processes: Stop validated processes
Uninstaller->>FileSystem: Remove validated installation data
Uninstaller->>Registry: Clean eligible PATH and uninstall entries
Uninstaller->>FileSystem: Verify cleanup and report results
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 6
🤖 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 @.github/ISSUE_TEMPLATE/1-bug-report.yml:
- Line 12: Update the discussion link text in the issue template so its visible
label matches the existing destination, using “pythinker-cli/discussions”
instead of “pythinker-code/discussions.”
In `@README.md`:
- Line 745: Update the README setup instructions so the cd command following the
git clone command targets the pythinker-cli directory created by the repository
URL instead of pythinker-code.
In `@scripts/uninstall.ps1`:
- Line 438: Replace the -ine comparison in scripts/uninstall.ps1 at lines
438-438 with the equivalent -not and -ieq form, and apply the identical change
in web/public/uninstall.ps1 at lines 438-438 and docs/public/uninstall.ps1 at
the corresponding line so all mirrored uninstallers remain byte-identical.
In `@src/pythinker_code/feedback_repo.py`:
- Line 3: Add the previous repository value "Pythoughts-labs/pythinker-code" to
_LEGACY_DEFAULT_FEEDBACK_GITHUB_REPOS so saved configurations migrate to
DEFAULT_FEEDBACK_GITHUB_REPO. Add a regression test in tests/core/test_config.py
covering that exact legacy value and verifying the migration behavior.
In `@src/pythinker_code/skills/pythinker-code-help/SKILL.md`:
- Line 48: Update the documentation URL references in SKILL.md to use the
pythinker-cli path consistently: replace pythinker-code in the Base URL,
llms.txt URL, and page URL pattern while preserving the existing URL structure.
In `@tests/test_installation_docs.py`:
- Around line 195-199: Update
test_native_powershell_uninstaller_is_parseable_when_pwsh_is_available so the
pwsh is None branch calls pytest.skip with an appropriate message instead of
returning; add the pytest import if needed, while leaving the available-pwsh
syntax-check path unchanged.
🪄 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
Run ID: 263536e7-03d4-4524-ad35-52bd1c44ed8a
⛔ Files ignored due to path filters (9)
docs/.vitepress/config.tsis excluded by!docs/**docs/en/guides/getting-started.mdis excluded by!docs/**docs/en/index.mdis excluded by!docs/**docs/en/release-notes/changelog.mdis excluded by!docs/**docs/history/CHANGELOG-pre-0.8.0.mdis excluded by!docs/**docs/public/_headersis excluded by!docs/**docs/public/install.ps1is excluded by!docs/**docs/public/install.shis excluded by!docs/**docs/public/uninstall.ps1is excluded by!docs/**
📒 Files selected for processing (44)
.github/ISSUE_TEMPLATE/1-bug-report.yml.github/ISSUE_TEMPLATE/config.yml.github/pull_request_template.md.github/workflows/docs-pages.yml.github/workflows/homebrew-tap.yml.github/workflows/scoop-bucket.yml.github/workflows/winget.ymlCHANGELOG.mdCONTRIBUTING.mdREADME.mdSECURITY.mdexamples/feedback-worker/wrangler.jsoncpackages/homebrew-tap/generate-formula.pypackages/linux-installer/README.mdpackages/linux-installer/build.shpackages/pythinker-core/CHANGELOG.mdpackages/pythinker-core/pyproject.tomlpackages/pythinker-core/src/pythinker_core/contrib/chat_provider/openai_legacy.pypackages/pythinker-core/tests/api_snapshot_tests/test_openai_legacy.pypackages/pythinker-host/pyproject.tomlpackages/pythinker-review/pyproject.tomlpackages/pythinker-review/src/pythinker_review/output/sarif.pypackages/scoop-bucket/generate-manifest.pypackages/scoop-bucket/pythinker-code.json.tmplpackages/windows-installer/installer.isspyproject.tomlscripts/install-native.shscripts/install.ps1scripts/uninstall.ps1sdks/pythinker-sdk/pyproject.tomlsrc/pythinker_code/feedback_repo.pysrc/pythinker_code/native.pysrc/pythinker_code/skills/pythinker-code-help/SKILL.mdsrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/update.pytests/core/test_config.pytests/core/test_simple_compaction.pytests/test_installation_docs.pytests/test_native.pytests/ui_and_conv/test_shell_feedback_slash.pyweb/public/_headersweb/public/install.ps1web/public/install.shweb/public/uninstall.ps1
Use [string]::Equals(..., OrdinalIgnoreCase) for the Pythinker leaf-name check; identical semantics, and the repo has no typos config to suppress the PowerShell -ine operator being flagged as a misspelling.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
scripts/uninstall.ps1 (6)
726-734: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInvoke the system
taskkill.exeby absolute path. Both copies resolvetaskkill.exebut invoke the bare command name, so PowerShell can select an earlierPATHentry. Use a path under[Environment]::SystemDirectoryin both files.🤖 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 `@scripts/uninstall.ps1` around lines 726 - 734, Update the taskkill resolution and invocation in scripts/uninstall.ps1 at lines 726-734 and web/public/uninstall.ps1 at lines 726-734 to use the absolute taskkill.exe path under [Environment]::SystemDirectory, rather than resolving or invoking the bare command name; preserve the existing error handling and process termination behavior.
167-175: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when admin detection errors.
In both
scripts/uninstall.ps1andweb/public/uninstall.ps1,Test-IsAdminreturns$falsewhen identity detection throws.Test-CanExecuteUninstallerthen permits aCurrentUserrecord to reachInvoke-InnoUninstaller. Return an indeterminate state on error and allow execution only after an explicit non-admin result.🤖 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 `@scripts/uninstall.ps1` around lines 167 - 175, Update Test-IsAdmin in scripts/uninstall.ps1 lines 167-175 and web/public/uninstall.ps1 lines 167-175 to return an indeterminate value rather than $false when identity detection throws. Update Test-CanExecuteUninstaller in both files to permit execution only when Test-IsAdmin explicitly returns a non-admin result, preventing indeterminate detection from reaching Invoke-InnoUninstaller.
732-734: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict process termination to validated processes.
Both copies validate only the selected Pythinker process, then run
taskkill.exe /F /T. The/Tswitch force-terminates all descendant processes, including worker or command subprocesses that the script does not validate. Remove/T, or validate each descendant PID before termination.🤖 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 `@scripts/uninstall.ps1` around lines 732 - 734, Restrict forced termination to validated processes by removing the /T switch from the taskkill.exe invocation in both scripts/uninstall.ps1 lines 732-734 and web/public/uninstall.ps1 lines 732-734; leave the existing PID validation and error handling unchanged.
1250-1254: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck for
PythinkerSetup*before starting the uninstall.In both scripts, the check runs after process shutdown and install-directory deletion. Since
PythinkerSetup*is not stopped, an active installer can continue while the uninstall removes its files. Move the guard toInvoke-PythinkerUninstall, before process shutdown and any deletion.🤖 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 `@scripts/uninstall.ps1` around lines 1250 - 1254, The PythinkerSetup* guard currently runs too late, after shutdown and deletion begin. In scripts/uninstall.ps1 at lines 1250-1254 and web/public/uninstall.ps1 at lines 1250-1254, move the setup-process check from Remove-StaleInstallerTempDirs into Invoke-PythinkerUninstall so it executes before any process shutdown or install-directory deletion; preserve the existing message and early return behavior.
856-883: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake snapshot validation and deletion atomic.
Both files use path strings after a separate snapshot. If a directory component is replaced with a junction or symlink,
Remove-Itemcan resolve the parent reparse point and delete a file in the redirected target. Use handle-relative deletion or an equivalent no-redirection ownership boundary. Apply the same fix toRegister-PendingDeleteSnapshot, which passes snapshot paths toMoveFileEx.🤖 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 `@scripts/uninstall.ps1` around lines 856 - 883, Make snapshot validation and deletion atomic in scripts/uninstall.ps1 lines 856-883 and web/public/uninstall.ps1 lines 856-883 by replacing path-based Remove-SafeSnapshotNow deletion with handle-relative deletion or an equivalent ownership boundary that cannot traverse newly introduced junctions or symlinks. Apply the same no-redirection protection to Register-PendingDeleteSnapshot in both files before passing snapshot paths to MoveFileEx.
692-699: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the initial process termination identity-safe.
Both copies read
$process.Pathand$process.StartTime, then callStop-Process -Id $process.Id. If the process exits and Windows reuses the PID before this call, the command can terminate an unrelated process. Retain a handle to the original process and terminate through that handle, or use an equivalent identity-safe operation. Fail closed when identity or handle acquisition fails.🤖 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 `@scripts/uninstall.ps1` around lines 692 - 699, Make the initial termination logic in scripts/uninstall.ps1 lines 692-699 and web/public/uninstall.ps1 lines 692-699 identity-safe by retaining and validating the original process handle, then stopping through that handle instead of using Stop-Process with only $process.Id. Fail closed when acquiring or validating the handle or identity fails, and apply the same change in both copies.
🤖 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.
Outside diff comments:
In `@scripts/uninstall.ps1`:
- Around line 726-734: Update the taskkill resolution and invocation in
scripts/uninstall.ps1 at lines 726-734 and web/public/uninstall.ps1 at lines
726-734 to use the absolute taskkill.exe path under
[Environment]::SystemDirectory, rather than resolving or invoking the bare
command name; preserve the existing error handling and process termination
behavior.
- Around line 167-175: Update Test-IsAdmin in scripts/uninstall.ps1 lines
167-175 and web/public/uninstall.ps1 lines 167-175 to return an indeterminate
value rather than $false when identity detection throws. Update
Test-CanExecuteUninstaller in both files to permit execution only when
Test-IsAdmin explicitly returns a non-admin result, preventing indeterminate
detection from reaching Invoke-InnoUninstaller.
- Around line 732-734: Restrict forced termination to validated processes by
removing the /T switch from the taskkill.exe invocation in both
scripts/uninstall.ps1 lines 732-734 and web/public/uninstall.ps1 lines 732-734;
leave the existing PID validation and error handling unchanged.
- Around line 1250-1254: The PythinkerSetup* guard currently runs too late,
after shutdown and deletion begin. In scripts/uninstall.ps1 at lines 1250-1254
and web/public/uninstall.ps1 at lines 1250-1254, move the setup-process check
from Remove-StaleInstallerTempDirs into Invoke-PythinkerUninstall so it executes
before any process shutdown or install-directory deletion; preserve the existing
message and early return behavior.
- Around line 856-883: Make snapshot validation and deletion atomic in
scripts/uninstall.ps1 lines 856-883 and web/public/uninstall.ps1 lines 856-883
by replacing path-based Remove-SafeSnapshotNow deletion with handle-relative
deletion or an equivalent ownership boundary that cannot traverse newly
introduced junctions or symlinks. Apply the same no-redirection protection to
Register-PendingDeleteSnapshot in both files before passing snapshot paths to
MoveFileEx.
- Around line 692-699: Make the initial termination logic in
scripts/uninstall.ps1 lines 692-699 and web/public/uninstall.ps1 lines 692-699
identity-safe by retaining and validating the original process handle, then
stopping through that handle instead of using Stop-Process with only
$process.Id. Fail closed when acquiring or validating the handle or identity
fails, and apply the same change in both copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7945df5-09a4-406a-bd85-865b561a67bc
⛔ Files ignored due to path filters (1)
docs/public/uninstall.ps1is excluded by!docs/**
📒 Files selected for processing (2)
scripts/uninstall.ps1web/public/uninstall.ps1
- issue template: make the discussions link label match its pythinker-cli target - README: fix 'cd pythinker-code' after cloning pythinker-cli.git - feedback: migrate saved Pythoughts-labs/pythinker-code repo values (the pre-rename bundled default) to pythinker-cli, with a regression test - help skill: point documentation base/llms.txt/page URLs at the canonical pythinker-cli GitHub Pages path instead of relying on the rename redirect - tests: report a missing pwsh as pytest.skip instead of a silent pass
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 `@src/pythinker_code/skills/pythinker-code-help/SKILL.md`:
- Around line 22-24: Update the fenced URL block in the SKILL.md content to
declare the text language, using a text fence around the existing URL and
preserving the URL unchanged.
🪄 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
Run ID: 53361d05-c28c-4a99-b008-f80a3731bd87
📒 Files selected for processing (7)
.github/ISSUE_TEMPLATE/1-bug-report.ymlCHANGELOG.mdREADME.mdsrc/pythinker_code/feedback_repo.pysrc/pythinker_code/skills/pythinker-code-help/SKILL.mdtests/core/test_config.pytests/test_installation_docs.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
Two changes on this branch:
Repo rename sweep (
fix(repo)): update all repository URLs and repo-slug constants toPythoughts-labs/pythinker-cliafter the GitHub repository rename (install scripts, updater, feedback default repo, docs, workflows, packaging).Hosted Windows uninstaller (
feat(installer)): addscripts/uninstall.ps1, served athttps://pythinker.com/uninstall.ps1(irm https://pythinker.com/uninstall.ps1 | iex), mirroring howinstall.ps1is hosted — with byte-identical copies indocs/publicandweb/public, matching_headersentries, README + getting-started docs, and sync/parse/README coverage intests/test_installation_docs.py.Uninstaller design
unins<N>.exeverified), then sweeps PATH entries (user + system, registry value kind preserved, value re-read under the writable handle), Start Menu shortcuts, uninstall registry keys (native 32/64-bit views), and owned stale installer temp dirs (AppId marker or verified SHA-256 proof), and verifies the final machine state.$script:/global leaks, encoding restored), never callsexit, soirm | iexcannot close the user's PowerShell host;powershell.exe -Filestill gets a non-zero exit code on unresolved issues.%USERPROFILE%\.pythinkeris kept unlessPYTHINKER_PURGE_DATA=1is set (and the purge is verified).Test plan
make check-pythinker-code— ruff + format + pyright all passmake test-pythinker-code— 8446 passed + 65 e2e passedtests/test_installation_docs.py— 16/16 (sync,_headers, pwsh parse, README/guide one-liners)MoveFileExreboot scheduling) — recommended before first release advertising the URLhttps://pythinker.com/uninstall.ps1becomes resolvable when this merges and the site redeploys.Summary by CodeRabbit
New Features
Documentation
Chores