fix(hooks): use SCRIPT_NAME variable in run-hook.cmd to avoid arg-par…#1175
Open
ZakAnun wants to merge 1 commit into
Open
fix(hooks): use SCRIPT_NAME variable in run-hook.cmd to avoid arg-par…#1175ZakAnun wants to merge 1 commit into
ZakAnun wants to merge 1 commit into
Conversation
…sing edge cases Fixes obra#1142. On Windows with a user profile path containing spaces (e.g. C:\Users\Artur Sebesta\...), the SessionStart hook can fail to invoke the target script correctly because of how the Windows CMD argument parser expands %~1 when HOOK_DIR also contains spaces. Changes: - Add `setlocal` to isolate HOOK_DIR/SCRIPT_NAME from the calling environment - Capture %~1 into SCRIPT_NAME before constructing the bash invocation, so the quoted path "%HOOK_DIR%%SCRIPT_NAME%" is assembled from two stable, separately-expanded variables rather than an inline %~1 substitution that can interact with surrounding quotes unexpectedly - Keep %2..%9 (no shift) to forward extra arguments without injecting the script name as a spurious extra positional argument — a regression that was introduced by an earlier attempted fix using %* after shift Add tests: - tests/hooks/test-run-hook-wrapper.sh: verifies hooks.json routing, absence of %* in the CMD branch, and the Unix polyglot branch end-to-end with a spaced directory path and spaced arguments - tests/hooks/fixtures/check-args: fixture that asserts exactly two specific argument values are received - tests/hooks/fixtures/check-no-args: fixture that asserts no extra arguments are injected when run-hook.cmd is called with only a script name (simulates the real SessionStart invocation) CI evidence (windows-latest + ubuntu-latest, all green): https://github.com/ZakAnun/superpowers/actions/runs/24438382711 Made-with: Cursor
Owner
|
Ooh! Thank you so much. As a not-windows person, I'm not always up on the right way to do things. We'll run this through a bit of local testing, but assuming it checks out, I'm +1 to merge. |
Author
Thank you for your affirmation. I would be honored if I could help with this project. |
5 tasks
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
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.
Fixes #1142.
What problem are you trying to solve?
On Windows systems where the user profile path contains spaces (e.g.
C:\Users\Artur Sebesta\...), the SessionStart hook silently fails to execute. Insiderun-hook.cmd, the bash invocation is constructed as"%HOOK_DIR%%~1"where%HOOK_DIR%(set from%~dp0) already contains the spaced path. The inline%~1expansion adjacent to%HOOK_DIR%inside the quoted string causes the Windows CMD parser to misinterpret the boundary, resulting in bash receiving a truncated path such as/c/Users/Arturand failing to find the script. Superpowers bootstrap context is never injected at session start. This is a real startup failure experienced by users with spaces in their Windows username — not a theoretical concern.What does this PR change?
In the Windows CMD branch of
hooks/run-hook.cmd:setlocalto isolateHOOK_DIRandSCRIPT_NAMEfrom the calling environment%~1intoSCRIPT_NAMEbefore constructing the bash invocation, so"%HOOK_DIR%%SCRIPT_NAME%"is assembled from two stable, separately-expanded variables rather than mixing%HOOK_DIR%with an inline%~1inside the same quoted string%2..%9withoutshiftto correctly forward extra arguments — using%*aftershiftwas considered and rejected because%*does not update aftershiftin Windows CMD, which would re-inject the script name as a spurious$1into every target scriptAdds regression tests under
tests/hooks/:tests/hooks/test-run-hook-wrapper.sh— verifies hooks.json routing, absence of%*in the CMD branch, and the Unix polyglot branch end-to-end with a spaced directory path and spaced argumentstests/hooks/fixtures/check-args— fixture asserting exactly two specific argument values are receivedtests/hooks/fixtures/check-no-args— fixture asserting no extra arguments are injected whenrun-hook.cmdis called with only a script name (simulates the realsession-startinvocation)Is this change appropriate for the core library?
Yes.
run-hook.cmdis the core hook entry point for all Superpowers users on Windows (cmd.exe, PowerShell, WSL). Spaces in a Windows username is a common real-world configuration. This fix improves baseline plugin startup reliability for all affected users without touching any skill content or adding any dependency.What alternatives did you consider?
Call
bashdirectly inhooks/hooks.json— this was the approach taken in PR fix(hooks): avoid SessionStart path split on Windows with spaced user… #1158. The maintainer closed it with the explicit comment thatrun-hook.cmdmust remain in the call chain because it does essential heavy lifting for Windows cmd.exe, PowerShell, and WSL. This approach is a non-starter.Use
%*aftershift— capture the script name first,shift, then use%*for remaining args. Discarded because in Windows CMD,%*always expands to the original full argument list and does not update aftershift. This would re-inject the script name as a spurious$1into every target script.Current approach (chosen) — keep
run-hook.cmdfully intact, fix only the variable expansion in the bash invocation. Minimal and targeted, retains all existing wrapper functionality.Does this PR contain multiple unrelated changes?
No. One bug, one fix, plus regression tests for that fix. All changes are directly related to the Windows spaced-path hook failure.
Existing PRs
PR #1158 addressed the same issue (#1142) but by bypassing
run-hook.cmdentirely — callingbashdirectly fromhooks.json. The maintainer closed it becauserun-hook.cmdmust stay in the call chain. This PR takes the opposite approach: keeprun-hook.cmdas the entry point and fix the argument expansion inside it. The two approaches are mutually exclusive; this one respects the maintainer's stated constraint.Environment tested
CI run (windows-latest + ubuntu-latest, both green):
https://github.com/ZakAnun/superpowers/actions/runs/24438382711
Evaluation
windows-latestwith a hooks directory path containing spaces, testing spaced argument forwarding and the no-extra-args invariantrun-hook.cmdinvoked bash with a truncated path, hook silently failed. After: bash receives the fully quoted path and executes correctly;check-no-argsfixture confirms the script name is not re-injected as a spurious argumentRigor
%*is absent from the CMD branchHuman review