Repository navigation
feat(cli): add opt-in shell-local sp shorthand - #283
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42799b5434
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ($MyInvocation.InvocationName -ne '.') { | ||
| throw 'Dot-source sp.ps1 to change the current PowerShell scope.' | ||
| } | ||
| $SessionPeerSpExisting = @(Get-Command -Name sp -All -ListImported -ErrorAction SilentlyContinue) |
There was a problem hiding this comment.
When the default sp alias has been removed and an unimported module advertises an sp command, this exact-name Get-Command lookup can auto-import that module and execute its initialization code merely by dot-sourcing the supposedly passive activator. -ListImported does not prevent this exact-name path: the upstream implementation invokes LookupCommandInfo for an exact-name miss before applying the ListImported guard. Use a lookup that cannot trigger module auto-loading; the session-peer check below has the same issue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4f2446d. Exact-name command discovery now temporarily disables module auto-loading in the current lookup scope and restores the caller's prior local preference in finally; this preserves native/PATH/.cmd lookup without importing an advertised module. Added an isolated module with an initialization marker plus an ordinary exact-lookup positive control, and preference-preservation checks. Current-head Windows PowerShell 5.1 and the wheel/sdist pwsh job both pass. No profile or persistent preference changes.
| if ($SessionPeerSpExisting.Count -eq 1 -and $SessionPeerSpMarker -and $SessionPeerSpMarker.Value -eq $true -and | ||
| $SessionPeerSpExisting[0].CommandType -eq 'Alias' -and $SessionPeerSpExisting[0].Definition -eq 'session-peer') { |
There was a problem hiding this comment.
If an sp executable or function becomes available after activation—for example after a PATH change—PowerShell retains the unchanged owned alias while Get-Command -All returns both it and the newly hidden command; the documented purpose of -All is to return every same-named command. The Count -eq 1 condition therefore rejects -Remove and leaves the owned alias stuck even though it is still safe to remove. Identify the visible owned alias by type and definition without requiring it to be the only result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4f2446d (ownership check introduced in dc5a93c). Removal identifies the visible unchanged alias by its type/definition and the activator ownership marker, without requiring it to be the only same-name command. Added a hidden function after activation, then verified removal deletes only the owned alias and preserves the newly exposed function. Changed/unowned aliases still refuse. Current-head native PowerShell 5.1 and pwsh fixtures pass.
Summary\n\nCloses #242.\n\n- Package passive Bash/zsh and PowerShell activation assets; sourcing/dot-sourcing explicitly opts into a shell-local alias for the existing canonical command.\n- Keep console entry points, canonical parser, scripts/SSH/reply instructions, profiles, PATH and installation managers unchanged. No unconditional sp executable and no Python/TypeScript dispatcher.\n- Refuse executable/alias/function collisions, including PowerShell default sp=Set-ItemProperty. Repeated owned activation/removal is idempotent; changed aliases are never removed. The alias follows canonical command selection through manager updates.\n- Add a read-only manager-interpreter asset locator, exact wheel/sdist packaging contracts, and matching English/Korean/Japanese/Simplified Chinese setup/removal guidance. Standalone uses reviewed checkout/verified source-distribution files; installer and release asset contracts are unchanged.\n\n## Type\n\n- [x] feat\n\n## Validation\n\n- Isolated macOS Bash/zsh: canonical version/help/list/non-delivering dry-run/malformed argument output and exits; quoted Unicode/option separator/stdin/nonzero exit; existing alias/function/executable collisions; repeated activation/removal, changed alias refusal, canonical selection update, missing runtime and incorrect activation usage.\n- Distribution source suite: 40 tests, 34 passed and 6 environment/platform skips; documentation suite: 32 passed; generator --check and git diff --check passed.\n- Built isolated wheel and sdist; exact content contract passed, each installed in a separate temporary environment, installed CLI version and isolated Python asset locator passed. Local candidate package keeps existing development version; no release/publication occurred.\n- Windows CI exercises PowerShell native equivalence and lifecycle in a child process. PowerShell is not installed locally; no local Windows/PowerShell result is claimed. Default-alias removal exists only in the isolated test fixture, never the activator.\n- No live agent submission, user installation, profile/PATH change, fleet/relay operation, merge, tag or publishing.\n\n## Notes\n\nShell-scoped activation intentionally cannot persist through closing a shell; removing a package cannot mutate an already-running shell. Users remove the owned alias before uninstall/switch. cmd.exe and automation remain on session-peer. Python and TypeScript implementations are not installed simultaneously. CI/current-head review remains a gate, not completed merely by this PR creation.\n\n## Final local follow-up\n\nThe POSIX removal operation uses a separate asset so sourced activation never interprets inherited caller positional arguments. A regression covers caller arguments including ; PowerShell keeps explicit .\n\nFinal-head local distribution suite: 40 tests / 34 passed / 6 environment skips; documentation: 32 passed; Bash asset shellcheck passed; final wheel and sdist build plus exact content checks: 4 passed. Separate temporary wheel/sdist installs and isolated pipx/uv tool installs verified module asset discovery and canonical alias activation. Manager uninstall removed only these temporary tool installations. The local pipx smoke used its supported pip backend because the installed uv backend version was older than pipx requires; no global tool upgrade was made.\n\nEarlier candidate Windows PowerShell 5.1 and Linux PowerShell Core tests passed after fixture-only delimiter/encoding corrections. The current final-head CI and automatic reviews must be checked separately; prior-head green checks are not final-head evidence.
Final local follow-up
The POSIX removal operation uses a separate
sp-remove.shasset so sourced activation never interprets inherited caller positional arguments. A regression covers caller arguments including--remove; PowerShell keeps explicit-Remove.Final-head local distribution suite: 40 tests / 34 passed / 6 environment skips; documentation: 32 passed; Bash asset shellcheck passed; final wheel and sdist build plus exact content checks: 4 passed. Separate temporary wheel/sdist installs and isolated pipx/uv tool installs verified module asset discovery and canonical alias activation. Manager uninstall removed only these temporary tool installations. The local pipx smoke used its supported pip backend because the installed uv backend version was older than pipx requires; no global tool upgrade was made.
Earlier candidate Windows PowerShell 5.1 and Linux PowerShell Core tests passed after fixture-only delimiter/encoding corrections. The current final-head CI and automatic reviews must be checked separately; prior-head green checks are not final-head evidence.