Skip to content

Add hs2 command-line interface for Hammerspoon 2#47

Open
dmgerman wants to merge 52 commits intocmsj:mainfrom
dmgerman:hs2
Open

Add hs2 command-line interface for Hammerspoon 2#47
dmgerman wants to merge 52 commits intocmsj:mainfrom
dmgerman:hs2

Conversation

@dmgerman
Copy link
Copy Markdown

@dmgerman dmgerman commented Apr 6, 2026

Summary

The hs2 CLI enables scripting and automation of Hammerspoon 2 from the terminal, matching the hs CLI 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

  • Non-zero exit on JS errors (exit 65): Shell scripting requires reliable exit codes. JS evaluation errors set EX_DATAERR while still executing remaining commands in multi-command mode, so -c a -c b -c c runs all three but exits non-zero if any failed.
  • Dynamic module list: Tab completion queries hs.moduleNames() at runtime, which derives the list from the ModuleRootAPI protocol properties via ObjC runtime introspection. No hardcoded lists to maintain.
  • Single-expression implicit return: The REPL needs 1+1 to print 2 without requiring return. Implicit return is only used for single-line code with no semicolons. Multiline code and multi-statement code go through the statement path to avoid JavaScript Automatic Semicolon Insertion (ASI) bugs and early-return issues.
  • Synchronous IPC in tab completion: The readline completion callback makes a blocking sendToRemote call. CFMessagePortSendRequest spins the calling thread's run loop internally, but this is safe because the main thread has no CFRunLoop sources registered. A 1-second timeout (vs 4s default) keeps tab completion responsive.
  • Named constants for timeouts: DEFAULT_TIMEOUT (4.0s) for commands, completionTimeout (1.0s) for tab completion. No hardcoded values.
  • Self-building test script: scripts/test-hs2.sh builds the project before testing to guarantee it tests the current code. Tests validate output content, not just exit codes.
  • REPL banner: Documents that globalThis.x = 42 is needed for persistent bindings, since all declarations (var/let/const) are function-scoped inside new Function().

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 @Observable properties in Logging.swift is a separate architectural concern. The up-arrow crash also exists on main — tracked in #43, fixed in PR #44.

gitmsr and others added 30 commits March 20, 2026 18:16
* 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>
gitmsr and others added 22 commits March 20, 2026 18:16
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.
- 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
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 6, 2026

Greptile Summary

This PR adds the hs2 CLI tool — a well-designed CFMessagePort IPC client with a libedit REPL, tab completion, and exit-code semantics — alongside the server-side hs.ipc module and JS protocol handler. The ConsoleView refactor introduces a few minor regressions: auto-scroll silently fails when entries are filtered, the command echo was removed from the log, and the Autocomplete log-level option remains in the picker despite autocomplete no longer being available in the console.

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 quality and behavior suggestions.

No P0 or P1 defects found. The core IPC protocol, client lifecycle, and REPL are sound. The ConsoleView regressions are minor and don't block functionality.

Hammerspoon 2/Windows/ConsoleView.swift has three P2 issues (auto-scroll, echo, autocomplete picker) worth addressing post-merge.

Important Files Changed

Filename Overview
hs2/main.swift Entry point: argument parsing, mode detection, IPC client lifecycle — logic is clear and well-structured.
hs2/HSClient.swift CFMessagePort IPC client using a dedicated run loop thread with DispatchSemaphore synchronization — design is sound.
hs2/HSInteractiveREPL.swift libedit-based REPL with synchronous tab-completion IPC; minor inaccuracy in a timeout comment.
Hammerspoon 2/Windows/ConsoleView.swift Console simplified: auto-scroll regression when entries are filtered, command echo removed, Autocomplete level shown despite no autocomplete entries.
Hammerspoon 2/Modules/hs.ipc/HSMessagePort.swift New CFMessagePort Swift wrapper with retain/release context, recursion guard, and proper cleanup.
Hammerspoon 2/Modules/hs.ipc/IPCModule.swift IPC module: local/remote port creation, CLI install/uninstall helpers; broken-symlink edge case in cliInstall.
Hammerspoon 2/Modules/hs.ipc/hs.ipc.js JS-side IPC protocol handler with defensive error handling, instance isolation, and implicit-return heuristic for single-expression eval.
Hammerspoon 2/Engine/ModuleRoot.swift Adds moduleNames() via ObjC runtime introspection and exposes hs.ipc module — clean and self-maintaining.
Hammerspoon 2/Utilities/Logging.swift Adds synchronous logging on main thread and nonisolated(unsafe) for CFRunLoop-callback safety; tradeoffs are documented.
scripts/test-hs2.sh Self-building shell test script; comprehensive coverage of basic, error, fixture, and stress cases.

Comments Outside Diff (1)

  1. Hammerspoon 2/Modules/hs.ipc/IPCModule.swift, line 912-939 ([link](https://github.com/cmsj/hammerspoon2/blob/bdc67cb0cc1f5738a591f0b049678347618d86f5/Hammerspoon 2/Modules/hs.ipc/IPCModule.swift#L912-L939))

    P2 Broken symlink causes a misleading install failure

    FileManager.fileExists(atPath:) follows symlinks and returns false when the target doesn't exist (broken symlink). If a stale broken symlink already sits at binaryDest, fileExists returns false, the code enters the else branch, and createSymbolicLink throws an "already exists" error whose message doesn't explain that a broken symlink is the culprit. Using fm.destinationOfSymbolicLink(atPath:) (which succeeds for broken links) to check for any pre-existing link before attempting creation would give a clearer error and avoid the misleading failure.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: Hammerspoon 2/Modules/hs.ipc/IPCModule.swift
    Line: 912-939
    
    Comment:
    **Broken symlink causes a misleading install failure**
    
    `FileManager.fileExists(atPath:)` follows symlinks and returns `false` when the target doesn't exist (broken symlink). If a stale broken symlink already sits at `binaryDest`, `fileExists` returns `false`, the code enters the `else` branch, and `createSymbolicLink` throws an "already exists" error whose message doesn't explain that a broken symlink is the culprit. Using `fm.destinationOfSymbolicLink(atPath:)` (which succeeds for broken links) to check for *any* pre-existing link before attempting creation would give a clearer error and avoid the misleading failure.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 61-63

Comment:
**Auto-scroll misses filtered entries**

`proxy.scrollTo(logs.entries.last?.id)` scrolls to the last entry in the *unfiltered* `logs.entries` array. When the user has an active search string or a `minimumLogLevel` that hides the newest entry, `ForEach` won't contain that ID and `scrollTo` silently does nothing. The old `"logBottom"` sentinel lived inside the same VStack as the filtered entries and was always reachable.

```suggestion
                .onChange(of: logs.entries) {
                    let visible = logs.entries.filter {
                        $0.logType.rawValue >= minimumLogLevel.rawValue &&
                        (searchString.isEmpty || $0.msg.contains(searchString))
                    }
                    proxy.scrollTo(visible.last?.id)
                }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 99-113

Comment:
**Command echo removed from console log**

The previous `handleSubmit()` called `AKInfo("> \(evalString)")` before evaluating, giving users a persistent record of submitted commands interleaved with their output. The new `onSubmit` block omits this. If the removal is intentional (e.g., to reduce noise), a brief comment would clarify intent; otherwise this is a silent behavioral regression.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 117-121

Comment:
**`Autocomplete` level in picker but no autocomplete entries are produced**

Tab completion was removed from `ConsoleView` in this PR, so `HammerspoonLogType.Autocomplete` entries will never appear. The previous code filtered it out of the Picker with `.filter { $0 != .Autocomplete }`. Now that it's selectable, a user who picks "Autocomplete" as the minimum log level will see a blank console with no obvious explanation.

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: 107-110

Comment:
**Incorrect worst-case timeout in comment**

The comment says "blocks for up to sendTimeout + recvTimeout (default 8s total)", but this call passes `completionTimeout` (1.0 s) as the custom timeout for both send and recv, so the actual worst-case block is 2.0 s, not 8 s.

```suggestion
        // Note: if Hammerspoon is unresponsive, this blocks for up to
        // completionTimeout × 2 = 2s (1s send + 1s recv).
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Hammerspoon 2/Modules/hs.ipc/IPCModule.swift
Line: 912-939

Comment:
**Broken symlink causes a misleading install failure**

`FileManager.fileExists(atPath:)` follows symlinks and returns `false` when the target doesn't exist (broken symlink). If a stale broken symlink already sits at `binaryDest`, `fileExists` returns `false`, the code enters the `else` branch, and `createSymbolicLink` throws an "already exists" error whose message doesn't explain that a broken symlink is the culprit. Using `fm.destinationOfSymbolicLink(atPath:)` (which succeeds for broken links) to check for *any* pre-existing link before attempting creation would give a clearer error and avoid the misleading failure.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Extract timeout constants, avoid hardcod..." | Re-trigger Greptile

Comment on lines 61 to 63
.onChange(of: logs.entries) {
proxy.scrollTo("logBottom", anchor: .bottom)
proxy.scrollTo(logs.entries.last?.id)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Auto-scroll misses filtered entries

proxy.scrollTo(logs.entries.last?.id) scrolls to the last entry in the unfiltered logs.entries array. When the user has an active search string or a minimumLogLevel that hides the newest entry, ForEach won't contain that ID and scrollTo silently does nothing. The old "logBottom" sentinel lived inside the same VStack as the filtered entries and was always reachable.

Suggested change
.onChange(of: logs.entries) {
proxy.scrollTo("logBottom", anchor: .bottom)
proxy.scrollTo(logs.entries.last?.id)
}
.onChange(of: logs.entries) {
let visible = logs.entries.filter {
$0.logType.rawValue >= minimumLogLevel.rawValue &&
(searchString.isEmpty || $0.msg.contains(searchString))
}
proxy.scrollTo(visible.last?.id)
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 61-63

Comment:
**Auto-scroll misses filtered entries**

`proxy.scrollTo(logs.entries.last?.id)` scrolls to the last entry in the *unfiltered* `logs.entries` array. When the user has an active search string or a `minimumLogLevel` that hides the newest entry, `ForEach` won't contain that ID and `scrollTo` silently does nothing. The old `"logBottom"` sentinel lived inside the same VStack as the filtered entries and was always reachable.

```suggestion
                .onChange(of: logs.entries) {
                    let visible = logs.entries.filter {
                        $0.logType.rawValue >= minimumLogLevel.rawValue &&
                        (searchString.isEmpty || $0.msg.contains(searchString))
                    }
                    proxy.scrollTo(visible.last?.id)
                }
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +99 to 113
.onSubmit {
evalHistory.append(evalString)
evalIndex = -1
if let result = JSEngine.shared.eval(evalString) {
// FIXME: This is a disgusting hack, there must be a better way to detect if result is a Bool type of NSNumber?
let typeString = "\(type(of: result))"
if typeString == "__NSCFBoolean" {
let boolResult = result as! NSNumber
AKConsole("\(boolResult.boolValue)")
} else {
AKConsole(String(describing: result))
}
}
}
.onSubmit {
activeCompletion = nil
handleSubmit()
evalString = ""
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Command echo removed from console log

The previous handleSubmit() called AKInfo("> \(evalString)") before evaluating, giving users a persistent record of submitted commands interleaved with their output. The new onSubmit block omits this. If the removal is intentional (e.g., to reduce noise), a brief comment would clarify intent; otherwise this is a silent behavioral regression.

Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 99-113

Comment:
**Command echo removed from console log**

The previous `handleSubmit()` called `AKInfo("> \(evalString)")` before evaluating, giving users a persistent record of submitted commands interleaved with their output. The new `onSubmit` block omits this. If the removal is intentional (e.g., to reduce noise), a brief comment would clarify intent; otherwise this is a silent behavioral regression.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 117 to 121
Picker("Minimum log level", selection: $minimumLogLevel) {
ForEach(HammerspoonLogType.allCases.filter { $0 != .Autocomplete }){ item in
ForEach(HammerspoonLogType.allCases) { item in
Text(item.asString)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Autocomplete level in picker but no autocomplete entries are produced

Tab completion was removed from ConsoleView in this PR, so HammerspoonLogType.Autocomplete entries will never appear. The previous code filtered it out of the Picker with .filter { $0 != .Autocomplete }. Now that it's selectable, a user who picks "Autocomplete" as the minimum log level will see a blank console with no obvious explanation.

Prompt To Fix With AI
This is a comment left during a code review.
Path: Hammerspoon 2/Windows/ConsoleView.swift
Line: 117-121

Comment:
**`Autocomplete` level in picker but no autocomplete entries are produced**

Tab completion was removed from `ConsoleView` in this PR, so `HammerspoonLogType.Autocomplete` entries will never appear. The previous code filtered it out of the Picker with `.filter { $0 != .Autocomplete }`. Now that it's selectable, a user who picks "Autocomplete" as the minimum log level will see a blank console with no obvious explanation.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +107 to +110
// nothing to re-enter. The IPC run loop for receiving messages lives on a
// separate thread (hs2-ipc-client).
// Note: if Hammerspoon is unresponsive, this blocks for up to sendTimeout +
// recvTimeout (default 8s total).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Incorrect worst-case timeout in comment

The comment says "blocks for up to sendTimeout + recvTimeout (default 8s total)", but this call passes completionTimeout (1.0 s) as the custom timeout for both send and recv, so the actual worst-case block is 2.0 s, not 8 s.

Suggested change
// nothing to re-enter. The IPC run loop for receiving messages lives on a
// separate thread (hs2-ipc-client).
// Note: if Hammerspoon is unresponsive, this blocks for up to sendTimeout +
// recvTimeout (default 8s total).
// Note: if Hammerspoon is unresponsive, this blocks for up to
// completionTimeout × 2 = 2s (1s send + 1s recv).
Prompt To Fix With AI
This is a comment left during a code review.
Path: hs2/HSInteractiveREPL.swift
Line: 107-110

Comment:
**Incorrect worst-case timeout in comment**

The comment says "blocks for up to sendTimeout + recvTimeout (default 8s total)", but this call passes `completionTimeout` (1.0 s) as the custom timeout for both send and recv, so the actual worst-case block is 2.0 s, not 8 s.

```suggestion
        // Note: if Hammerspoon is unresponsive, this blocks for up to
        // completionTimeout × 2 = 2s (1s send + 1s recv).
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants