Add hs2 command-line interface for Hammerspoon 2#46
Add hs2 command-line interface for Hammerspoon 2#46
Conversation
* Add hs.ipc module for IPC communication * Fix critical crash: passUnretained → passRetained in HSMessagePort * Prevents EXC_BAD_ACCESS during rapid hs2 command execution * Add Swift 6 compatibility annotations (@preconcurrency @unsafe) Tested: 20+ rapid hs2 commands execute successfully without crashes
… tool * docs/IPC.md: Comprehensive documentation of Hammerspoon 2 IPC protocol. * hs2/README.md: Overview and guide for the hs2 command-line tool. * scripts/test-hs2.sh: Test runner for hs2 integration tests. * Hammerspoon 2Tests/Fixtures/hs2: tests for hs2 * Hammerspoon 2Tests/HS2-TESTING-GUIDE.md: documentation for tests * Hammerspoon 2Tests/IntegrationTests: integration tests
Added defensive initialization checks throughout hs.ipc.js to ensure the critical storage objects always exist before being accessed. This prevents TypeError exceptions and ensures the IPC system can recover gracefully from unexpected state corruption. * scripts/test-hs2.sh (run_stress_tests): Add test for multiple console.log calls. * Hammerspoon 2/Modules/hs.ipc/hs.ipc.js (defensive checks): Ensure storage objects exist before access.
This commit fixes two issues with the hs2 CLI tool:
1. Error Recovery: hs2 now uses REPL-style error handling where JavaScript
errors are reported but don't terminate execution. When multiple -c commands
are specified, all will execute even if earlier ones error. This matches
standard REPL behavior where user code errors are reported but don't
terminate the tool.
2. Flag Mappings: Fixed command-line flags to match original hs tool:
- -N now forces colors (was: disable console mirroring)
- -C now enables console mirroring (was: force colors)
- Help text updated to reflect correct behavior
Technical Changes:
- Modified hs.ipc.js to return "ok" for JavaScript evaluation errors since
the IPC protocol successfully executed the command. The actual error
message is sent via MSG_ID.ERROR before returning.
- Updated flag parsing in main.swift to match original hs semantics
- JavaScript errors printed to stderr but don't stop command execution
- Exit code 0 indicates successful IPC communication (even if JS errored)
- Non-zero exit codes reserved for actual IPC/communication failures
Exit Code Semantics:
- Exit 0: IPC communication succeeded (user code may have had errors)
- Exit 69 (EX_UNAVAILABLE): Cannot connect to Hammerspoon 2
- Exit 65 (EX_DATAERR): IPC protocol/communication error
This enables multi-command workflows like:
hs2 -c "risky_command()" -c "fallback_command()"
where both commands execute regardless of whether the first one errors.
Documentation:
- Added error handling sections to hs2/README.md, docs/IPC.md, and CLAUDE.md
- Updated exit code descriptions to clarify IPC success vs user code errors
Files changed:
- hs2/main.swift: Fixed flag mappings (-N, -C)
- Hammerspoon 2/Modules/hs.ipc/hs.ipc.js: Return "ok" for JS eval errors
- hs2/README.md: Added error handling documentation
- docs/IPC.md: Added IPC error handling philosophy section
- CLAUDE.md: Updated hs2 CLI tool documentation
Test Changes (HS2CommandTests.swift):
- Updated 4 existing tests to expect exit code 0 for JS errors:
* testSyntaxError, testRuntimeError, testUndefinedVariable, testErrorExitCode
- Added 7 new error recovery tests (108 lines added):
* testErrorRecovery_SingleError
* testErrorRecovery_ErrorInFirstPosition
* testErrorRecovery_ErrorInMiddlePosition
* testErrorRecovery_ErrorInLastPosition
* testErrorRecovery_SyntaxErrorRecovery
* testErrorRecovery_MultipleErrors
* testErrorRecovery_SuccessfulCommandsStillWork
Remove duplicate fileExists(atPath:isDirectory:) in MockFileSystem and fix Swift 6 concurrency isolation in HS2CommandTests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove all debug logging (appendLineToURL, DEBUG fputs, console.log DEBUG/IPC) - Rewrite HSClient from Thread subclass to composition with proper synchronization (NSLock for exitCode, DispatchSemaphore for init/completion signaling) - Add port cleanup: deinit in HSMessagePort, activePorts tracking in IPCModule - Fix JS injection in tab completion (escape quotes/backslashes) - Replace GUI alert (NSAlert) with CLI error message - Fix bugs: redundant ternary, empty if-block, colorOutput same as reset, double-printing in hs.ipc.print, expression-vs-statement heuristic - Remove dead code: IPCMessage struct, IPCProtocolVersion, String.appendLineToURL - Fix -C flag default (false, so it actually enables mirroring), help text -m default - Improve tests: fix concurrent assertion, interactive mode test, pipe data race, add port deletion and symlink target tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation specs served their purpose during development and are preserved in git history. Removing from HEAD to keep the working tree clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two README.md files in the test target cause Xcode build error: "Multiple commands produce README.md" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Users can now retrieve log contents and command history from JavaScript. getConsole() formats log entries matching the console UI display, and getHistory() returns the eval history shared between the console view and the JS API via HammerspoonLog.shared.evalHistory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three bugs were found and fixed: 1. @objc thunk actor isolation crash: The original getConsole/getHistory methods on HSConsoleModule (a @mainactor class) crashed when called via IPC. The @objc thunk inserts dispatch_assert_queue(main_queue) which fails from CFMessagePort callbacks — these run on the main thread via CFRunLoop but NOT through GCD's main queue. Even marking methods nonisolated doesn't help: the @objc thunk still enforces it. Fix: Move getConsole/getHistory out of the @objc protocol entirely. Use nonisolated free functions exposed to JS as @convention(block) closures via HSConsoleReadInstaller, bypassing @objc thunks. 2. JSExport proxy property loss: Setting properties on JSExport proxy objects (e.g., hs.console.getConsole = func) silently fails — the proxy doesn't persist dynamically assigned JS properties. Prototype modifications also get lost when the GC reclaims and recreates wrappers with fresh prototypes. Fix: Use Object.defineProperty to override the 'console' getter on the ModuleRoot prototype. The overridden getter lazily creates and caches a plain JS wrapper (via Object.create) that inherits the module's methods and adds getConsole/getHistory as own properties. 3. First-access bootstrapping race: Loading hs.console.js during getOrCreate (triggered by hs.console access) meant the JS override took effect AFTER the current expression already resolved hs.console to the raw proxy — so the first call always failed. Fix: Split into two installers in JSEngine's init sequence: - HSConsoleReadInstaller (before ModuleRoot): registers the block closures as JS globals - HSConsoleGetterInstaller (after ModuleRoot): overrides the hs.console getter eagerly, before any user code runs Also: Mark HammerspoonLog.shared and its properties nonisolated(unsafe) for safe access from CFRunLoop callbacks. Add evalHistory to shared log. Add serialized integration tests including background thread safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The REPL loop condition was `while client.exitCode == EX_OK`, which caused the REPL to silently exit whenever executeCommand set a non-zero exit code (e.g., IPC timeout or unexpected response). The comment said "continue in interactive mode" but the code contradicted it. Fixes: - REPL loop now runs unconditionally and prints an error on IPC failure instead of silently quitting - executeCommand now prints the unexpected response to stderr when the server returns something other than "ok" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two issues fixed: 1. IPC registrations lost after idle period: The hs.ipc.js handler stored all state (registered instances, remote ports, handler functions) as properties on the hs.ipc JSExport proxy object. After a few minutes of inactivity, JavaScriptCore's garbage collector reclaims the proxy wrapper, and all properties are lost. New commands then fail with "instance not registered". This is the same JSExport GC issue that affected hs.console. Fix: Move all IPC state into closure-scoped JS variables (__ipcRegisteredInstances, __ipcRemotePorts, etc.) that are not attached to any JSExport proxy and survive GC cycles. 2. hs2 REPL silently exiting on errors: The REPL loop condition `while client.exitCode == EX_OK` caused it to exit without any message when executeCommand set a non-zero exit code. Also, executeCommand printed no error when the server response wasn't "ok". Fix: REPL loop now runs unconditionally, prints errors on failure, and continues. executeCommand reports unexpected responses to stderr. Added auto-reconnect: when hs2 gets "instance not registered", it re-registers and retries the command transparently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test the __ipcDefaultHandler JS function and closure-scoped state pattern that was introduced to survive JSExport proxy GC. Covers registration, unregistration, command/query execution, error cases, state persistence across operations, and multi-instance coexistence. Also adds "ipc" case to JSTestHarness.loadModules(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Compare the three approaches used across modules to survive JSExport proxy GC: closure-scoped JS variables (hs.ipc), protocol-backed Swift properties (hs.task), and Object.defineProperty getter overrides (hs.console). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unimplemented -A (auto-launch) flag from CLI, man page, and docs - Fix -a option description to match actual behavior (pass argument to script) - Add retain/release callbacks to HSClient CFMessagePortContext - Invalidate ports on registration failure in HSClient.runLoopMain() - Handle semaphore timeout in HSClient.start() with EX_TEMPFAIL - Remove unimplemented features from man page (syntax highlighting, multi-line editing, .cli.history) - Fix exit code 65 description: IPC protocol error, not JS execution error - Fix test-hs2.sh error tests to expect exit 0 for JS errors - Fix README debugging section with accurate diagnostics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These methods were implemented as free nonisolated functions exposed via a two-phase Object.defineProperty installer hack, under the assumption that @objc thunks on a @mainactor class would crash from non-main threads. Since JSC is single-threaded and these are only called from JavaScript, the concern doesn't apply. Add them directly to HSConsoleModuleAPI protocol and implement in HSConsoleModule. Protocol methods automatically survive GC because JSC re-exports them when recreating the proxy wrapper. Remove HSConsoleReadInstaller, HSConsoleGetterInstaller, free functions, JS wrapper hack in test harness, background thread tests, and Approach 3 from IPC docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap ANSI color escape sequences in the prompt with \x01/\x02 markers so libedit correctly calculates the visible prompt width. Without these, libedit counted escape codes as visible characters, miscalculating the cursor position and failing to fully erase old text when navigating history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add "What Survives GC Automatically" section to IPC docs explaining that protocol-declared methods need no workarounds. Rename "Two Solutions" to "Two Workarounds for Dynamic State" to clarify scope. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flag is plumbed through registration but no output is mirrored because the JS engine has no global print function for hs.ipc.js to replace, and hs.console.print() routes through Swift directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove commented-out dead code in JSTestHarness.registerCallback - Add non-functional comment to __ipcPrint, remove unused completions var - Add missing @mainactor to 3 console integration tests for consistency - Fix double UTF-8 decode in HSClient.executeCommand Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ription Add isRetry guard to prevent infinite recursion when auto-reconnect succeeds but the server continues returning "instance not registered". Mark -C console mirroring as not yet functional in the man page, matching the help text and IPC.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ests Production fixes: - Move default IPC port to closure-scoped var to survive JSExport GC (cmsj#4) - Make AKLog synchronous on main thread so getConsole() sees entries immediately (cmsj#3) - Add rollback to cliInstall when man page symlink fails (cmsj#2) - Use Atomic<Int> for callDepth in HSMessagePort (#1) - Remove redundant Bundle.main.bundlePath optional casts (cmsj#8) - Escape control chars in REPL tab completion query (cmsj#9) - Deduplicate date format via HammerspoonLogEntry.formattedLine (cmsj#11) - Remove dead print replacement code in hs.ipc.js (cmsj#10) Test fixes: - Remove misleading hammerspoonProcess field in HS2CommandTests (cmsj#5) - Read pipes concurrently to prevent deadlock on large output (cmsj#6) - Replace unsafeBitCast with direct setObject in JSTestHarness (cmsj#7) - Save/restore evalHistory in console tests for isolation (cmsj#13) - Handle port name conflicts gracefully in default port tests (cmsj#12) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix cross-thread CFRunLoop access in stopRunLoopAfterDelay using CFRunLoopPerformBlock + CFRunLoopWakeUp - Validate -a requires -c or file argument - Error on conflicting input modes (-s, -c, file) - Replace eval with direct execution in test-hs2.sh - Use graceful killall before -9 in test script Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use MainActor.assumeIsolated in deinit to safely access MainActor-isolated CFMessagePort and CFRunLoopSource properties. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore hs2 command-line tool target in project.pbxproj (was dropped during a previous project file restructure). The hs2 binary is built as a dependency of the app and embedded at Contents/Frameworks/hs2/. - Fix Swift 6.2 concurrency errors in HSIPCIntegrationTests: mark class nonisolated, use async setUp with MainActor.run, add @mainactor to test methods, load JS companion files into test harness context. - Add XCTSkipUnless for CLI install tests when hs2 binary is absent. - Increase testRunReturnsPromise timeout to reduce flakiness.
- Fix GC-unsafe hs.completionsForInputString: move from JSExport proxy assignment to a global variable, preventing silent loss after GC. - Use static module name list for tab completion since JSExport proxy properties are not enumerable via Object.keys() or for...in. - Add parameter docstrings to sendMessage, localPort, remotePort, cliInstall, cliUninstall, cliStatus (fixes docs linter CI failure). - Tighten localPort callback guard to also reject null/undefined values. - Remove dead historyFilePath property from HSInteractiveREPL. - Document single-REPL assumption on static completion state.
- Fix interactive mode detection: check isatty(STDIN_FILENO) instead of STDOUT_FILENO so piped stdout doesn't suppress REPL mode - Update -a flag help text and error message to mention -s (stdin) - Replace manual string escaping with JSONSerialization in tab completion to handle all control chars and Unicode edge cases - Add TODO for __ipcPrint wiring and sync comment for __ipcKnownModules
Console mirroring was never wired up because the JS engine has no global print to intercept. Remove the unused function, its stored reference to the original print, and the two tests that covered it. Update IPC.md to reflect the removal.
Add @objc moduleNames() to ModuleRootAPI and ModuleRoot so JS can query the available module list at runtime. This eliminates the hardcoded list in hs.ipc.js that could silently drift when modules are added or removed.
… -C removal - JS evaluation errors now return "error:js" instead of "ok", so the client sets EX_DATAERR (65) while still continuing multi-command sequences. Shell scripting (&&, ||, set -e) works correctly. - Add let/const scoping note to REPL banner so users know to use var. - Extract MSG_TIMEOUT constant replacing hardcoded 4.0 in hs.ipc.js. - Remove non-functional -C flag and all consoleMirroring plumbing (argument parsing, HSClient property, registration payload, MSG_ID CONSOLE, and related tests).
Console mirroring plumbing was removed in the previous commit but this enum case was missed. No code references it.
…istory - Apply colorForLogType to Text views so errors show red, warnings orange - Remove unused selectedRows state variable - Guard against up-arrow crash when evalHistory is empty
The exit code for JS evaluation errors changed from 0 to 65 (EX_DATAERR) but these files still referenced the old behavior: - scripts/test-hs2.sh: expected exit 0 for error tests - hs2/README.md: documented exit 0 for JS errors - HS2-TESTING-GUIDE.md: fixture template showed exit 1 for errors
- Only try implicit return for single-line code. Multiline code goes straight to statement evaluation. JavaScript Automatic Semicolon Insertion (ASI) silently turns "return \n// comment\nthrow ..." into "return undefined;", swallowing errors and producing wrong results. - Update test-hs2.sh to build the project itself so it always tests the current code, not a stale binary from DerivedData.
Replace hardcoded module name list with runtime introspection of the ModuleRootAPI protocol. New modules added to the protocol are now automatically included in tab completion without manual updates.
The readline completion callback makes a blocking sendToRemote call. This is safe because the IPC run loop runs on a dedicated thread (hs2-ipc-client), not on the main thread where readline blocks.
Greptile SummaryThis PR adds an Key changes:
Issues found:
Confidence Score: 3/5The PR is mostly safe to merge but has a user-facing correctness bug in the REPL hint and a potential UX freeze in tab completion that should be addressed. Score of 3 reflects: the core IPC machinery (CFRunLoop threading, semaphore synchronization, retain/release memory management) is implemented correctly; the ConsoleView bug fixes are clean; the dynamic moduleNames() approach is elegant. Points deducted for the incorrect 'var persistence' banner message (users following this hint will get a confusing ReferenceError), the tab-completion blocking issue (up to 8-second freeze), and test scripts that only check exit codes and not output content. hs2/HSClient.swift (incorrect REPL persistence hint in getBanner()); hs2/HSInteractiveREPL.swift (blocking completion call and inaccurate comment); scripts/test-hs2.sh (output content not verified) Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: hs2/HSClient.swift
Line: 361-365
Comment:
**Incorrect REPL persistence hint**
The banner tells users to *"Use 'var' for persistent bindings (let/const are scoped per entry)"*, but this advice is wrong. In `hs.ipc.js` every REPL entry is evaluated via `new Function('_cli', 'print', code)`. Variables declared with `var` inside a `new Function` body are **local to that function call** — they are not added to the JS global object and do not survive to the next entry. Neither `let` nor `const` nor `var` persist across entries.
The only way to create persistent state is an implicit-global assignment in non-strict mode (e.g. `x = 5`, which writes directly to `globalThis`) or mutating an existing global object.
A user who follows the banner's advice by typing `var counter = 0` and then `counter++` in the next entry will receive:
```
ReferenceError: Can't find variable: counter
```
which directly contradicts the hint.
Suggested fix:
```suggestion
let hint = "Persistent state: assign without var/let/const (e.g. x = 5) or use globalThis.x = 0"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: hs2/HSInteractiveREPL.swift
Line: 103-127
Comment:
**Tab completion can freeze the terminal for up to 2× timeout**
`client.sendToRemote(..., wantResponse: true)` calls `CFMessagePortSendRequest` internally, which spins the **calling thread's** run loop until the reply arrives. With `sendTimeout = recvTimeout = 4.0` (the default), this means the readline prompt is completely unresponsive for up to **8 seconds** whenever Hammerspoon is slow or temporarily unresponsive. There is no user-visible feedback during this freeze.
Consider passing a much shorter timeout for completion queries (e.g. 500 ms) so that tab-press failures are invisible to the user:
```swift
// Use a short timeout so a slow Hammerspoon doesn't freeze readline
if let responseData = client.sendToRemote(message, msgID: MSGID_QUERY,
sendTimeout: 0.5, recvTimeout: 0.5),
```
Note: `sendToRemote` would need to accept per-call timeout parameters to support this, or a separate helper could be added.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: hs2/HSInteractiveREPL.swift
Line: 98-102
Comment:
**Misleading comment about run-loop re-entry**
The comment says the `sendToRemote` call "does not re-enter the main thread's run loop." This is inaccurate. `CFMessagePortSendRequest` with `wantResponse: true` *does* spin the **calling thread's** run loop (via an internal `CFRunLoopRunInMode`) while waiting for the synchronous reply. Since the completion callback fires on the main thread, the main thread's run loop *is* re-entered briefly.
The point that matters for deadlock safety (that the `hs2-ipc-client` thread never needs to dispatch back to the main thread to deliver the reply) is correct but stated inaccurately. Suggested correction:
```suggestion
// This is a synchronous IPC call from the readline callback (main thread).
// CFMessagePortSendRequest briefly runs the main thread's run loop to receive
// the reply. This is safe from deadlock because the IPC run loop on
// hs2-ipc-client delivers the reply via a Mach port — it never needs to
// call back onto the main thread.
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: scripts/test-hs2.sh
Line: 114-141
Comment:
**`run_test` verifies exit codes only, not output content**
Tests such as "Simple print", "Math operations", and "Function definition" would pass even if `print()` were silently broken, as long as the process exits 0. For example:
```bash
run_test "Simple print" 0 \
"$HS2_BINARY" -q -c 'print("test")'
```
This only checks that the exit code is 0, not that `"test"` was actually printed.
The Swift `HS2CommandTests.swift` suite does check output content, so coverage is not zero. But for the shell script to serve as a meaningful regression guard, consider adding an optional `expected_output` parameter to `run_test` and using `grep` or exact comparison for the tests that care about specific values.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Document threading model for synchronous..." | Re-trigger Greptile |
hs2/HSClient.swift
Outdated
| let hint = "Use 'var' for persistent bindings (let/const are scoped per entry)" | ||
| if useColors { | ||
| return "\(colorBanner)Hammerspoon 2 REPL\(colorReset)\n\(hint)\n" | ||
| } else { | ||
| return "Hammerspoon 2 REPL\n\(hint)\n" |
There was a problem hiding this comment.
Incorrect REPL persistence hint
The banner tells users to "Use 'var' for persistent bindings (let/const are scoped per entry)", but this advice is wrong. In hs.ipc.js every REPL entry is evaluated via new Function('_cli', 'print', code). Variables declared with var inside a new Function body are local to that function call — they are not added to the JS global object and do not survive to the next entry. Neither let nor const nor var persist across entries.
The only way to create persistent state is an implicit-global assignment in non-strict mode (e.g. x = 5, which writes directly to globalThis) or mutating an existing global object.
A user who follows the banner's advice by typing var counter = 0 and then counter++ in the next entry will receive:
ReferenceError: Can't find variable: counter
which directly contradicts the hint.
Suggested fix:
| let hint = "Use 'var' for persistent bindings (let/const are scoped per entry)" | |
| if useColors { | |
| return "\(colorBanner)Hammerspoon 2 REPL\(colorReset)\n\(hint)\n" | |
| } else { | |
| return "Hammerspoon 2 REPL\n\(hint)\n" | |
| let hint = "Persistent state: assign without var/let/const (e.g. x = 5) or use globalThis.x = 0" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: hs2/HSClient.swift
Line: 361-365
Comment:
**Incorrect REPL persistence hint**
The banner tells users to *"Use 'var' for persistent bindings (let/const are scoped per entry)"*, but this advice is wrong. In `hs.ipc.js` every REPL entry is evaluated via `new Function('_cli', 'print', code)`. Variables declared with `var` inside a `new Function` body are **local to that function call** — they are not added to the JS global object and do not survive to the next entry. Neither `let` nor `const` nor `var` persist across entries.
The only way to create persistent state is an implicit-global assignment in non-strict mode (e.g. `x = 5`, which writes directly to `globalThis`) or mutating an existing global object.
A user who follows the banner's advice by typing `var counter = 0` and then `counter++` in the next entry will receive:
```
ReferenceError: Can't find variable: counter
```
which directly contradicts the hint.
Suggested fix:
```suggestion
let hint = "Persistent state: assign without var/let/const (e.g. x = 5) or use globalThis.x = 0"
```
How can I resolve this? If you propose a fix, please make it concise.| if let client = HSInteractiveREPL.completionClient { | ||
| // JSON-encode the input to safely embed it in a JS string literal | ||
| guard let jsonData = try? JSONSerialization.data(withJSONObject: [inputText]), | ||
| let jsonArray = String(data: jsonData, encoding: .utf8) else { | ||
| HSInteractiveREPL.currentCompletions = [] | ||
| return nil | ||
| } | ||
| // jsonArray is e.g. ["hs.win"] — extract the inner string including quotes | ||
| let jsonStr = String(jsonArray.dropFirst(1).dropLast(1)) | ||
| let query = "JSON.stringify(completionsForInputString(\(jsonStr)))" | ||
| let message = "\(client.localName)\0\(query)" | ||
|
|
||
| if let responseData = client.sendToRemote(message, msgID: MSGID_QUERY, wantResponse: true), | ||
| let response = String(data: responseData as Data, encoding: .utf8) { | ||
| // Parse JSON response | ||
| if let jsonData = response.data(using: .utf8), | ||
| let completions = try? JSONSerialization.jsonObject(with: jsonData) as? [String] { | ||
| HSInteractiveREPL.currentCompletions = completions | ||
| } else { | ||
| HSInteractiveREPL.currentCompletions = [] | ||
| } | ||
| } else { | ||
| HSInteractiveREPL.currentCompletions = [] | ||
| } | ||
| } |
There was a problem hiding this comment.
Tab completion can freeze the terminal for up to 2× timeout
client.sendToRemote(..., wantResponse: true) calls CFMessagePortSendRequest internally, which spins the calling thread's run loop until the reply arrives. With sendTimeout = recvTimeout = 4.0 (the default), this means the readline prompt is completely unresponsive for up to 8 seconds whenever Hammerspoon is slow or temporarily unresponsive. There is no user-visible feedback during this freeze.
Consider passing a much shorter timeout for completion queries (e.g. 500 ms) so that tab-press failures are invisible to the user:
// Use a short timeout so a slow Hammerspoon doesn't freeze readline
if let responseData = client.sendToRemote(message, msgID: MSGID_QUERY,
sendTimeout: 0.5, recvTimeout: 0.5),Note: sendToRemote would need to accept per-call timeout parameters to support this, or a separate helper could be added.
Prompt To Fix With AI
This is a comment left during a code review.
Path: hs2/HSInteractiveREPL.swift
Line: 103-127
Comment:
**Tab completion can freeze the terminal for up to 2× timeout**
`client.sendToRemote(..., wantResponse: true)` calls `CFMessagePortSendRequest` internally, which spins the **calling thread's** run loop until the reply arrives. With `sendTimeout = recvTimeout = 4.0` (the default), this means the readline prompt is completely unresponsive for up to **8 seconds** whenever Hammerspoon is slow or temporarily unresponsive. There is no user-visible feedback during this freeze.
Consider passing a much shorter timeout for completion queries (e.g. 500 ms) so that tab-press failures are invisible to the user:
```swift
// Use a short timeout so a slow Hammerspoon doesn't freeze readline
if let responseData = client.sendToRemote(message, msgID: MSGID_QUERY,
sendTimeout: 0.5, recvTimeout: 0.5),
```
Note: `sendToRemote` would need to accept per-call timeout parameters to support this, or a separate helper could be added.
How can I resolve this? If you propose a fix, please make it concise.
hs2/HSInteractiveREPL.swift
Outdated
| // Query Hammerspoon for completions. | ||
| // This is a synchronous IPC call from the readline callback, which runs on the | ||
| // main thread. This is safe because the IPC run loop lives on a separate thread | ||
| // (hs2-ipc-client) — the sendToRemote call blocks waiting for a response but | ||
| // does not re-enter the main thread's run loop. |
There was a problem hiding this comment.
Misleading comment about run-loop re-entry
The comment says the sendToRemote call "does not re-enter the main thread's run loop." This is inaccurate. CFMessagePortSendRequest with wantResponse: true does spin the calling thread's run loop (via an internal CFRunLoopRunInMode) while waiting for the synchronous reply. Since the completion callback fires on the main thread, the main thread's run loop is re-entered briefly.
The point that matters for deadlock safety (that the hs2-ipc-client thread never needs to dispatch back to the main thread to deliver the reply) is correct but stated inaccurately. Suggested correction:
| // Query Hammerspoon for completions. | |
| // This is a synchronous IPC call from the readline callback, which runs on the | |
| // main thread. This is safe because the IPC run loop lives on a separate thread | |
| // (hs2-ipc-client) — the sendToRemote call blocks waiting for a response but | |
| // does not re-enter the main thread's run loop. | |
| // This is a synchronous IPC call from the readline callback (main thread). | |
| // CFMessagePortSendRequest briefly runs the main thread's run loop to receive | |
| // the reply. This is safe from deadlock because the IPC run loop on | |
| // hs2-ipc-client delivers the reply via a Mach port — it never needs to | |
| // call back onto the main thread. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: hs2/HSInteractiveREPL.swift
Line: 98-102
Comment:
**Misleading comment about run-loop re-entry**
The comment says the `sendToRemote` call "does not re-enter the main thread's run loop." This is inaccurate. `CFMessagePortSendRequest` with `wantResponse: true` *does* spin the **calling thread's** run loop (via an internal `CFRunLoopRunInMode`) while waiting for the synchronous reply. Since the completion callback fires on the main thread, the main thread's run loop *is* re-entered briefly.
The point that matters for deadlock safety (that the `hs2-ipc-client` thread never needs to dispatch back to the main thread to deliver the reply) is correct but stated inaccurately. Suggested correction:
```suggestion
// This is a synchronous IPC call from the readline callback (main thread).
// CFMessagePortSendRequest briefly runs the main thread's run loop to receive
// the reply. This is safe from deadlock because the IPC run loop on
// hs2-ipc-client delivers the reply via a Mach port — it never needs to
// call back onto the main thread.
```
How can I resolve this? If you propose a fix, please make it concise.| run_test() { | ||
| local test_name="$1" | ||
| local expected_exit_code="$2" | ||
| shift 2 | ||
|
|
||
| TESTS_RUN=$((TESTS_RUN + 1)) | ||
| echo -n " Test $TESTS_RUN: $test_name ... " | ||
|
|
||
| local output | ||
| local exit_code | ||
|
|
||
| set +e | ||
| output=$("$@" 2>&1) | ||
| exit_code=$? | ||
| set -e | ||
|
|
||
| if [ $exit_code -eq $expected_exit_code ]; then | ||
| echo -e "${GREEN}PASS${NC}" | ||
| TESTS_PASSED=$((TESTS_PASSED + 1)) | ||
| return 0 | ||
| else | ||
| echo -e "${RED}FAIL${NC}" | ||
| echo " Expected exit code: $expected_exit_code, got: $exit_code" | ||
| echo " Output: $output" | ||
| TESTS_FAILED=$((TESTS_FAILED + 1)) | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
run_test verifies exit codes only, not output content
Tests such as "Simple print", "Math operations", and "Function definition" would pass even if print() were silently broken, as long as the process exits 0. For example:
run_test "Simple print" 0 \
"$HS2_BINARY" -q -c 'print("test")'This only checks that the exit code is 0, not that "test" was actually printed.
The Swift HS2CommandTests.swift suite does check output content, so coverage is not zero. But for the shell script to serve as a meaningful regression guard, consider adding an optional expected_output parameter to run_test and using grep or exact comparison for the tests that care about specific values.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/test-hs2.sh
Line: 114-141
Comment:
**`run_test` verifies exit codes only, not output content**
Tests such as "Simple print", "Math operations", and "Function definition" would pass even if `print()` were silently broken, as long as the process exits 0. For example:
```bash
run_test "Simple print" 0 \
"$HS2_BINARY" -q -c 'print("test")'
```
This only checks that the exit code is 0, not that `"test"` was actually printed.
The Swift `HS2CommandTests.swift` suite does check output content, so coverage is not zero. But for the shell script to serve as a meaningful regression guard, consider adding an optional `expected_output` parameter to `run_test` and using `grep` or exact comparison for the tests that care about specific values.
How can I resolve this? If you propose a fix, please make it concise.- Banner incorrectly advised using 'var' for persistent bindings. var/let/const are all function-scoped inside new Function() and do not persist. Changed to recommend globalThis assignment. - Updated tab completion comment: CFMessagePortSendRequest does spin the calling thread's run loop internally, but this is safe because the main thread has no CFRunLoop sources registered. Also noted the potential 8s block if Hammerspoon is unresponsive.
- Tests now verify stdout/stderr content, not just exit codes. Added .expected files for fixture output validation. - Fixed bug where semicolon-separated statements on a single line (e.g. "print(1); print(2)") only executed the first statement. The implicit return path turned this into "return print(1); print(2)" which returned after the first call. Now only single-expression code without semicolons uses implicit return.
Tab completion should feel instant or not happen at all. The default 4+4 second send+recv timeout is appropriate for commands but freezes the terminal too long on tab press if Hammerspoon is unresponsive. Added optional timeout parameter to sendToRemote, used by completion.
- Add DEFAULT_TIMEOUT constant for the -t flag default (4.0s) - Add completionTimeout constant for tab completion queries (1.0s) - Help text now references the constant instead of a literal
|
One more attempt... |
Summary
The
hs2CLI enables scripting and automation of Hammerspoon 2 from the terminal, matching thehsCLI from the original Hammerspoon. Users can execute JavaScript one-liners, run script files, pipe code via stdin, or use an interactive REPL with tab completion.Why
Hammerspoon's power comes from scriptability. Without a CLI, users can only interact through the GUI console. The CLI enables shell scripting integration (
hs2 -c '...' && ...), editor tooling, and faster iteration during development.Key design decisions
-c a -c b -c cruns all three but exits non-zero if any failed.hs.moduleNames()at runtime, which derives the list from theModuleRootAPIprotocol properties via ObjC runtime introspection. No hardcoded lists to maintain.1+1to print2without requiringreturn. But JavaScript Automatic Semicolon Insertion (ASI) breaks this for multiline code, so implicit return is only used for single-line input.sendToRemotecall. This is safe because the IPC run loop runs on a dedicated thread (hs2-ipc-client), not on the main thread where readline blocks.scripts/test-hs2.shbuilds the project before testing to guarantee it tests the current code, not a stale binary.ConsoleView fixes
Fixes three bugs in the in-app console: log entries now display colored by type, unused state removed, and up-arrow no longer crashes on empty history.
Not addressed
The
nonisolated(unsafe)finding on@Observableproperties inLogging.swiftis a separate architectural concern. The up-arrow crash also exists onmain— tracked in #43, fixed in PR #44.