feat(macos-control): add macOS AX control MCP tool#357
Closed
almirchowdhury wants to merge 1 commit into
Closed
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive macOS accessibility and Electron debugging capabilities through a native Swift helper binary, TypeScript adapters, orchestration layer, and integrated MCP tools. It removes DOCX document support, simplifies telemetry proxying, and updates build/packaging infrastructure to distribute the new macOS helper binaries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Handler
participant Orchestrator
participant AxAdapter
participant NativeHelper
rect rgba(100, 150, 200, 0.5)
Note over Client,NativeHelper: macOS Accessibility Flow
Client->>Server: CallTool (macos_ax_find)
Server->>Handler: handleMacosAxFind(args)
Handler->>Handler: validate schema
Handler->>Orchestrator: axFind(app, text, role...)
Orchestrator->>Orchestrator: isMacOS()
Orchestrator->>AxAdapter: find(app, text, role...)
AxAdapter->>AxAdapter: resolveHelperPath()
AxAdapter->>NativeHelper: spawn with JSON request
NativeHelper->>NativeHelper: list elements, filter by criteria
NativeHelper-->>AxAdapter: JSON response + AxElement[]
AxAdapter->>AxAdapter: toAxElement + cache signatures
AxAdapter-->>Orchestrator: MacosControlResult<AxElement[]>
Orchestrator-->>Handler: result
Handler->>Handler: toServerResult
Handler-->>Server: ServerResult
Server-->>Client: response
end
participant CdpAdapter
participant WebSocket
rect rgba(200, 150, 100, 0.5)
Note over Client,WebSocket: Electron Debug Flow
Client->>Server: CallTool (electron_debug_attach)
Server->>Handler: handleElectronDebugAttach(args)
Handler->>Orchestrator: electronDebugAttach(host, port, targetIndex)
Orchestrator->>CdpAdapter: attach(host, port, targetIndex)
CdpAdapter->>CdpAdapter: fetch /json/list (HTTP)
CdpAdapter->>WebSocket: connect to debugger URL
CdpAdapter->>WebSocket: CDP Runtime.enable
WebSocket-->>CdpAdapter: domain enabled
CdpAdapter->>CdpAdapter: store session
CdpAdapter-->>Orchestrator: ElectronDebugAttachResult
Orchestrator-->>Handler: result
Handler-->>Server: ServerResult
Server-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let point = CGPoint(x: bounds.midX, y: bounds.midY) | ||
| return coordinateClick(point: point) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: In the scrolling helper, the user-controlled amount is only lower-bounded and then converted directly to Int32, so a very large amount (from JSON input) will cause Int32(...) to trap at runtime due to overflow. Clamping the step count to Int32.max (or a smaller reasonable limit) before converting ensures the cast is always safe and prevents the helper from crashing on extreme values. [possible bug]
Severity Level: Major ⚠️
- ❌ Scroll command can crash helper on extreme amount values.
- ⚠️ UI automation scroll sequences fail on malformed input.
Suggested change
| let clampedSteps = min(steps, Int(Int32.max)) | |
| let signedSteps = direction.lowercased() == "up" ? clampedSteps : -clampedSteps | |
| let delta = Int32(signedSteps) |
Steps of Reproduction ✅
1. Run the helper binary built from this PR on macOS so that `main.swift` executes,
reaching the top-level command switch at
`native/macos-ax-helper/Sources/macos-ax-helper/main.swift:1268-1561`.
2. Send a `scroll` command on stdin with an extremely large `amount` encoded as a string,
e.g.:
`{"command":"scroll","args":{"x":100,"y":100,"amount":"9999999999999"}}`.
This is parsed by `parseRequest()` at ~1238–1248.
3. The `switch` in `main` enters the `"scroll"` case at ~1519. It calls
`parseDouble(args["x"])` / `parseDouble(args["y"])` at ~1011–1021 and
`parseInt(args["amount"], default: 3)` at ~997–1008.
For the string `"9999999999999"`, `parseInt` uses `Int(string)` and returns a very
large `Int` value (much greater than `Int32.max`).
4. The `"scroll"` case then calls `scrollAt(x:y:direction:amount:)` at line 683 with this
large `amount`. Inside `scrollAt`, `steps = max(1, amount)` remains the huge `Int`;
computing `Int32(direction.lowercased() == "up" ? steps : -steps)` at line 685 attempts to
convert a value outside the `Int32` range and triggers a runtime trap, crashing the helper
before the CGEvent is created.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** native/macos-ax-helper/Sources/macos-ax-helper/main.swift
**Line:** 685:685
**Comment:**
*Possible Bug: In the scrolling helper, the user-controlled `amount` is only lower-bounded and then converted directly to `Int32`, so a very large `amount` (from JSON input) will cause `Int32(...)` to trap at runtime due to overflow. Clamping the step count to `Int32.max` (or a smaller reasonable limit) before converting ensures the cast is always safe and prevents the helper from crashing on extreme values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| switch action { | ||
| case "wait": | ||
| let ms = max(0, parseInt(cmd["ms"], default: 500)) |
Contributor
There was a problem hiding this comment.
Suggestion: The wait action in batch commands multiplies an unbounded, user-controlled millisecond value by 1000 before passing it to usleep, so a very large ms will overflow the Int during ms * 1000 and crash the helper at runtime. Clamping the maximum wait duration (e.g. to a reasonable upper bound like 60 seconds) before multiplying prevents integer overflow and keeps the helper robust against malformed or malicious input. [possible bug]
Severity Level: Major ⚠️
- ❌ Batch requests can crash helper with large wait ms.
- ⚠️ Long-running automation sequences aborted on malformed input.
Suggested change
| let ms = max(0, parseInt(cmd["ms"], default: 500)) | |
| let rawMs = parseInt(cmd["ms"], default: 500) | |
| let ms = max(0, min(rawMs, 60_000)) // clamp to a reasonable upper bound (60s) |
Steps of Reproduction ✅
1. Run the helper binary built from this PR on macOS so that `main.swift` executes,
reaching the main dispatch loop at
`native/macos-ax-helper/Sources/macos-ax-helper/main.swift:1268-1561`.
2. Send a `batch` command on stdin with an excessively large millisecond value encoded as
a string, for example:
`{"command":"batch","args":{"commands":[{"action":"wait","ms":"9223372036854775807"}]}}`.
This is parsed by `parseRequest()` at ~1238–1248 into a `[String: Any]`.
3. The `switch` in `main` enters the `"batch"` case at ~1534, which calls
`parseObjectArray()` at ~987–995 and then `runBatchCommands(_:, stopOnError:)` at
~1210–1227 with the commands array.
4. Inside `runBatchCommands`, the first command is passed to `executeBatchCommand(_:)` at
~1039. For the `"wait"` action (case block starting at line 1043), `parseInt(cmd["ms"],
default: 500)` parses the string `"9223372036854775807"` into `Int.max`, since `parseInt`
uses `Int(string)` for string inputs (lines ~998–1007). The expression `ms * 1000` at line
1045 overflows 64‑bit `Int`, causing a runtime trap and terminating the helper process.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** native/macos-ax-helper/Sources/macos-ax-helper/main.swift
**Line:** 1044:1044
**Comment:**
*Possible Bug: The wait action in batch commands multiplies an unbounded, user-controlled millisecond value by 1000 before passing it to `usleep`, so a very large `ms` will overflow the `Int` during `ms * 1000` and crash the helper at runtime. Clamping the maximum wait duration (e.g. to a reasonable upper bound like 60 seconds) before multiplying prevents integer overflow and keeps the helper robust against malformed or malicious input.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+67
to
+79
| let handshakeBuffer = ''; | ||
| const onData = (chunk: Buffer) => { | ||
| handshakeBuffer += chunk.toString('utf8'); | ||
|
|
||
| const headerEnd = handshakeBuffer.indexOf('\r\n\r\n'); | ||
| if (headerEnd === -1) { | ||
| return; | ||
| } | ||
|
|
||
| const headers = handshakeBuffer.slice(0, headerEnd); | ||
| const remaining = Buffer.from(handshakeBuffer.slice(headerEnd + 4), 'utf8'); | ||
|
|
||
| if (!headers.startsWith('HTTP/1.1 101')) { |
Contributor
There was a problem hiding this comment.
Suggestion: The WebSocket handshake logic concatenates response bytes as UTF-8 text and then re-encodes leftover bytes back into a Buffer, which can corrupt the first WebSocket frame if it arrives in the same TCP packet as the HTTP 101 response, leading to malformed frame parsing and failed CDP communication. [logic error]
Severity Level: Critical 🚨
- ❌ CDP WebSocket sessions may fail to initialize.
- ⚠️ macOS control tool attach may intermittently fail.
- ⚠️ CDP evaluate calls can hang or timeout unexpectedly.
Suggested change
| let handshakeBuffer = ''; | |
| const onData = (chunk: Buffer) => { | |
| handshakeBuffer += chunk.toString('utf8'); | |
| const headerEnd = handshakeBuffer.indexOf('\r\n\r\n'); | |
| if (headerEnd === -1) { | |
| return; | |
| } | |
| const headers = handshakeBuffer.slice(0, headerEnd); | |
| const remaining = Buffer.from(handshakeBuffer.slice(headerEnd + 4), 'utf8'); | |
| if (!headers.startsWith('HTTP/1.1 101')) { | |
| let handshakeBuffer = Buffer.alloc(0); | |
| const onData = (chunk: Buffer) => { | |
| handshakeBuffer = Buffer.concat([handshakeBuffer, chunk]); | |
| const headerEnd = handshakeBuffer.indexOf('\r\n\r\n'); | |
| if (headerEnd === -1) { | |
| return; | |
| } | |
| const headers = handshakeBuffer.slice(0, headerEnd).toString('utf8'); | |
| const remaining = handshakeBuffer.slice(headerEnd + 4); |
Steps of Reproduction ✅
1. From any Node script in this repo, import `cdpAdapter` from
`src/tools/macos-control/cdp-adapter.ts` (export at line 493) and call
`cdpAdapter.attach({ host: '127.0.0.1', port: 9222 })`, which invokes `CdpAdapter.attach`
(around lines 309–379).
2. `CdpAdapter.attach` resolves a target and calls
`wsClient.connect(target.webSocketDebuggerUrl)` (around lines 344–347), which executes
`SimpleWebSocketClient.connect` in the same file (handshake logic around lines 39–111,
with `cleanup` shown at lines 56–60).
3. Run against a real CDP/WebSocket server (e.g., Chrome/Electron remote debugger) that
legally sends the HTTP `101 Switching Protocols` response and the first WebSocket frame in
the same TCP packet; Node will deliver both as a single `data` event into `onData` at
`connect(...)` (same region).
4. In the current implementation, `onData` converts the entire TCP chunk to a UTF‑8 string
(`handshakeBuffer += chunk.toString('utf8')`), finds `\r\n\r\n`, and then builds
`remaining` via `Buffer.from(handshakeBuffer.slice(headerEnd + 4), 'utf8')`. If the first
WebSocket frame bytes (binary, starting with `0x81`, `0x82`, etc.) are present after the
headers, they are decoded as text and re‑encoded, corrupting the frame payload and/or
length. When `this.consumeFrames()` (lines 152–211) later parses `this.readBuffer`, it
sees invalid opcodes/lengths and will either ignore data or close the connection, causing
CDP commands from `attach`/`evaluate` to fail or time out.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/macos-control/cdp-adapter.ts
**Line:** 67:79
**Comment:**
*Logic Error: The WebSocket handshake logic concatenates response bytes as UTF-8 text and then re-encodes leftover bytes back into a Buffer, which can corrupt the first WebSocket frame if it arrives in the same TCP packet as the HTTP 101 response, leading to malformed frame parsing and failed CDP communication.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+256
to
+259
| const GA_MEASUREMENT_ID = 'G-8L163XZ1CE'; // Replace with your GA4 Measurement ID | ||
| const GA_API_SECRET = 'hNxh4TK2TnSy4oLZn4RwTA'; // Replace with your GA4 API Secret | ||
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; |
Contributor
There was a problem hiding this comment.
Suggestion: Hardcoding the Google Analytics measurement ID and API secret in this function exposes credentials in source control, which is a security risk and prevents safe rotation or environment-specific configuration; these should be read from environment or configuration instead and the function should no-op if they are missing. [security]
Severity Level: Major ⚠️
- ⚠️ GA Measurement Protocol secret exposed in distributed code.
- ⚠️ Third parties can spoof tool-call analytics events.
- ⚠️ Harder to rotate credentials across environments.
- ⚠️ Telemetry config not centralized in configManager.
Suggested change
| const GA_MEASUREMENT_ID = 'G-8L163XZ1CE'; // Replace with your GA4 Measurement ID | |
| const GA_API_SECRET = 'hNxh4TK2TnSy4oLZn4RwTA'; // Replace with your GA4 API Secret | |
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | |
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | |
| const GA_MEASUREMENT_ID = process.env.GA_CALL_TOOL_MEASUREMENT_ID ?? ''; | |
| const GA_API_SECRET = process.env.GA_CALL_TOOL_API_SECRET ?? ''; | |
| if (!GA_MEASUREMENT_ID || !GA_API_SECRET) { | |
| // If GA credentials are not configured, skip sending telemetry for this event | |
| return; | |
| } | |
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; |
Steps of Reproduction ✅
1. Open the repository code and navigate to `src/utils/capture.ts`, bottom hunk around
line 255 (function `capture_call_tool`).
2. Observe that `GA_MEASUREMENT_ID` and `GA_API_SECRET` are hardcoded string literals
(`'G-8L163XZ1CE'` and `'hNxh4TK2TnSy4oLZn4RwTA'`) inside `capture_call_tool` at lines
256–257.
3. Build or run the MCP server so this file is bundled and shipped; anyone with access to
the code artifact (source, bundle, or npm package) can read these GA credentials directly
from the file.
4. Using those leaked values, a third party can send arbitrary events to the same GA
property via the GA4 Measurement Protocol, polluting analytics data and forcing a
credential rotation; this exposure occurs regardless of whether `capture_call_tool` is
invoked at runtime.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/capture.ts
**Line:** 256:259
**Comment:**
*Security: Hardcoding the Google Analytics measurement ID and API secret in this function exposes credentials in source control, which is a security risk and prevents safe rotation or environment-specific configuration; these should be read from environment or configuration instead and the function should no-op if they are missing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+264
to
268
| const GA_MEASUREMENT_ID = 'G-F3GK01G39Y'; // Replace with your GA4 Measurement ID | ||
| const GA_API_SECRET = 'SqdcIAweSQS1RQErURMdEA'; // Replace with your GA4 API Secret | ||
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: This function embeds a Google Analytics measurement ID and API secret directly in source code, leaking credentials and making them impossible to rotate safely; they should be obtained from environment/config and the function should safely do nothing when they are not set. [security]
Severity Level: Major ⚠️
- ⚠️ Core GA secret for general telemetry is hardcoded.
- ⚠️ All capture-based telemetry shares same exposed secret.
- ⚠️ External actors can pollute main analytics stream.
- ⚠️ No per-environment GA configuration possible.
Suggested change
| const GA_MEASUREMENT_ID = 'G-F3GK01G39Y'; // Replace with your GA4 Measurement ID | |
| const GA_API_SECRET = 'SqdcIAweSQS1RQErURMdEA'; // Replace with your GA4 API Secret | |
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | |
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | |
| const GA_MEASUREMENT_ID = process.env.GA_MEASUREMENT_ID ?? ''; | |
| const GA_API_SECRET = process.env.GA_API_SECRET ?? ''; | |
| if (!GA_MEASUREMENT_ID || !GA_API_SECRET) { | |
| // If GA credentials are not configured, skip sending telemetry for this event | |
| return; | |
| } | |
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; |
Steps of Reproduction ✅
1. Open `src/utils/capture.ts` and locate `export const capture` starting at line 263.
2. See that `GA_MEASUREMENT_ID` and `GA_API_SECRET` are hardcoded to `'G-F3GK01G39Y'` and
`'SqdcIAweSQS1RQErURMdEA'` at lines 264–265.
3. Note that this `capture` function is the central telemetry helper used by
`captureRemote` in the same file (bottom of `capture.ts`), so any feature using telemetry
will rely on these embedded credentials.
4. When the project is shared or published, these GA credentials are exposed in plain
text, allowing external parties to inject fake analytics events for general capture
traffic and complicating safe credential rotation or per-environment configuration.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/capture.ts
**Line:** 264:268
**Comment:**
*Security: This function embeds a Google Analytics measurement ID and API secret directly in source code, leaking credentials and making them impossible to rotate safely; they should be obtained from environment/config and the function should safely do nothing when they are not set.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } | ||
| } | ||
|
|
||
| if (import.meta.url === `file://${process.argv[1]}`) { |
Contributor
There was a problem hiding this comment.
Suggestion: The CLI guard compares import.meta.url directly to file://${process.argv[1]}, which won't match when the script is invoked with a relative path (the common case), so the tests will not auto-run when the file is executed directly via Node. [logic error]
Severity Level: Major ⚠️
- ⚠️ Direct `node test/test-electron-debug.js` runs no tests.
- ⚠️ Any npm script invoking file relatively skips these tests.
- ⚠️ macOS CDP adapter issues may go undetected.
Suggested change
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| const modulePath = | |
| import.meta.url.startsWith('file://') ? import.meta.url.slice('file://'.length) : import.meta.url; | |
| const invokedPath = (process.argv[1] || '').replace(/^[.][\\/]/, ''); | |
| if (invokedPath && modulePath.endsWith(invokedPath)) { |
Steps of Reproduction ✅
1. From the project root, run the test file directly with a relative path: `node
test/test-electron-debug.js` (file `test/test-electron-debug.js`, guard at lines 55–57).
2. Node sets `process.argv[1]` to the literal string `"test/test-electron-debug.js"`
(relative path), while `import.meta.url` is an absolute file URL like
`file:///abs/path/to/test/test-electron-debug.js`.
3. At `test/test-electron-debug.js:55`, the condition `import.meta.url ===
\`file://${process.argv[1]}\`` compares `file:///abs/path/to/test/test-electron-debug.js`
to `file://test/test-electron-debug.js`, which are not equal, so the `if` condition
evaluates to `false`.
4. Because the `if` block is skipped, `runTests()` is never called, the tests in this file
do not execute, and the process exits silently without the expected log lines defined in
`runTests()` (lines 39–53).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/test-electron-debug.js
**Line:** 55:55
**Comment:**
*Logic Error: The CLI guard compares `import.meta.url` directly to `file://${process.argv[1]}`, which won't match when the script is invoked with a relative path (the common case), so the tests will not auto-run when the file is executed directly via Node.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } | ||
| } | ||
|
|
||
| if (import.meta.url === `file://${process.argv[1]}`) { |
Contributor
There was a problem hiding this comment.
Suggestion: As in the other test file, the direct-execution guard uses a strict equality between import.meta.url and file://${process.argv[1]}, which fails for typical relative-path invocations and prevents the test runner from auto-executing when the file is run directly. [logic error]
Severity Level: Major ⚠️
- ⚠️ `node test/test-macos-control.js` runs without executing tests.
- ⚠️ Manual macOS control tests may silently be skipped.
- ⚠️ CLI exit status not reflecting test success or failure.
Suggested change
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| const modulePath = | |
| import.meta.url.startsWith('file://') ? import.meta.url.slice('file://'.length) : import.meta.url; | |
| const invokedPath = (process.argv[1] || '').replace(/^[.][\\/]/, ''); | |
| if (invokedPath && modulePath.endsWith(invokedPath)) { |
Steps of Reproduction ✅
1. From the repository root, execute the test script directly with a relative path, e.g.
`node test/test-macos-control.js` (file `test/test-macos-control.js`).
2. Node loads `test/test-macos-control.js` as an ES module and reaches the
direct-execution guard at lines 74–76: `if (import.meta.url ===
\`file://${process.argv[1]}\`) { ... }`.
3. In this execution, `import.meta.url` is an absolute file URL like
`file:///abs/path/test/test-macos-control.js`, while `process.argv[1]` is the relative
string `test/test-macos-control.js`, so `file://${process.argv[1]}` becomes
`file://test/test-macos-control.js`; the strict equality check therefore evaluates to
false.
4. Because the condition is false, `runTests()` is never called, so no
schema/platform-gating tests run and the process exits without printing `macOS control
tests passed!` or returning a failure status on test errors; this demonstrates the guard
logic prevents auto-execution for typical relative invocations.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/test-macos-control.js
**Line:** 74:74
**Comment:**
*Logic Error: As in the other test file, the direct-execution guard uses a strict equality between `import.meta.url` and `file://${process.argv[1]}`, which fails for typical relative-path invocations and prevents the test runner from auto-executing when the file is run directly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/build-mcpb.cjs (1)
45-48:⚠️ Potential issue | 🔴 CriticalFix syntax error after bundle dir creation.
Line 47 concatenates
}andfs.mkdirSyncwith no separator, which will throw at runtime.🐛 Proposed fix
if (fs.existsSync(BUNDLE_DIR)) { fs.rmSync(BUNDLE_DIR, { recursive: true }); -}fs.mkdirSync(BUNDLE_DIR, { recursive: true }); +} +fs.mkdirSync(BUNDLE_DIR, { recursive: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-mcpb.cjs` around lines 45 - 48, The snippet incorrectly concatenates the closing brace of the if block with the subsequent call to fs.mkdirSync, causing a syntax/runtime error; update the code around the BUNDLE_DIR handling (the if using fs.existsSync and fs.rmSync and the following fs.mkdirSync) so that the if block is properly closed and fs.mkdirSync is placed on its own statement (add a semicolon or newline between the block end and fs.mkdirSync) to ensure the bundle directory is recreated without syntax errors.src/tools/schemas.ts (1)
139-147:⚠️ Potential issue | 🟠 Major
hasValueonnew_stringbreaks the delete-by-replacement use case.
hasValue("")returnsfalse, so{ old_string: "some text", new_string: "" }now fails validation. Replacing text with an empty string is a valid and common way to delete a code block. This is a regression from the previous!== undefinedcheck.Only
old_stringneeds to be non-empty (there must be something to find);new_stringshould only need to be present (!== undefined).🐛 Proposed fix
- return (hasValue(data.old_string) && hasValue(data.new_string)) || + return (hasValue(data.old_string) && data.new_string !== undefined) || (hasValue(data.range) && hasValue(data.content));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/schemas.ts` around lines 139 - 147, The current refine uses hasValue for both data.old_string and data.new_string which rejects empty-string replacements; update the predicate so old_string must be non-empty (old_string !== undefined && old_string !== '') while new_string only needs to be present (new_string !== undefined) — e.g. replace the existing hasValue usage in the refine callback (referencing hasValue, data.old_string, data.new_string, data.range, data.content) with a condition that returns (old_string !== undefined && old_string !== '' && new_string !== undefined) || (data.range !== undefined && data.content !== undefined).
♻️ Duplicate comments (1)
server.json (1)
10-16: Same version downgrade concern assrc/version.ts.Both the top-level
"version"andpackages[0].versioninserver.jsonare updated to0.2.36, consistent with thesrc/version.tschange. Please see the comment there for the rationale request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.json` around lines 10 - 16, The two version fields in server.json (the top-level "version" and the packages[0].version entry) were changed to 0.2.36 but should match the intended release version; update these two fields to the correct version that matches src/version.ts (or revert them to the prior release if the downgrade was accidental) so all three locations (src/version.ts, server.json "version", and server.json packages[0].version) are consistent and reflect the intended release number.
🧹 Nitpick comments (16)
src/remote-device/remote-channel.ts (2)
137-149: Consider guarding against duplicate subscriptions.If
subscribeis called more than once, an existing channel could remain active and duplicate tool calls.🔁 Optional guard
async subscribe(deviceId: string, onToolCall: (payload: any) => void): Promise<void> { if (!this.client) throw new Error('Client not initialized'); // Store parameters for channel recreation this.deviceId = deviceId; this.onToolCall = onToolCall; console.debug(`⏳ Subscribing to tool call channel...`); // Create and subscribe to the channel + if (this.channel) { + await this.channel.unsubscribe(); + this.channel = null; + } await this.createChannel(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/remote-device/remote-channel.ts` around lines 137 - 149, The subscribe method can create duplicate channels if called multiple times; modify subscribe to detect an existing active subscription (e.g., check this.channel or an isSubscribed flag) and either no-op when the same deviceId/onToolCall are already registered or first cleanly unsubscribe/close the existing channel (call the existing channel's unsubscribe/close method or a helper like unsubscribeChannel) before overwriting this.deviceId and this.onToolCall and calling createChannel; ensure you still throw if !this.client and update the channel state after successful createChannel.
83-89: Add a user guard to updates for defense-in-depth.This avoids accidental cross-user updates if a deviceId is ever reused or RLS is misconfigured.
🛡️ Proposed change
async updateDevice(deviceId: string, updates: any) { if (!this.client) throw new Error('Client not initialized'); + if (!this.user?.id) throw new Error('User not initialized'); return await this.client .from('mcp_devices') .update(updates) - .eq('id', deviceId); + .eq('id', deviceId) + .eq('user_id', this.user.id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/remote-device/remote-channel.ts` around lines 83 - 89, The updateDevice method lacks a user-level guard and can accidentally update another user's device; modify updateDevice to include an explicit user check by filtering the update with the device owner column (e.g., .eq('user_id', <currentUserId>)) so the DB update is constrained to the authenticated user; obtain the current user id from the instance (e.g., this.userId / this.user?.id) or change the signature to accept a userId parameter, validate it's present (throw if missing), and include that .eq('user_id', userId) condition alongside .eq('id', deviceId) when calling this.client.from('mcp_devices').update(updates).src/tools/macos-control/role-aliases.ts (1)
8-8:linkalias includesAXButton— may cause unexpected over-matching.
'AXLink'is for navigational hyperlinks;'AXButton'is for interactive controls. Bundling them together means any query for"link"role elements will also match all buttons in the AX tree, which is likely broader than intended. If the goal is to capture link-styled buttons in specific frameworks, consider a separate dedicated alias (e.g.,linkbutton) rather than wideninglink.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/macos-control/role-aliases.ts` at line 8, The current 'link' alias maps to both 'AXLink' and 'AXButton', which over-matches; update the alias definitions by removing 'AXButton' from the 'link' mapping and instead add a new alias (e.g., 'linkbutton') that maps to ['AXLink', 'AXButton'] for cases that intentionally need both; specifically, edit the alias object where the 'link' key is defined to only include 'AXLink' and add a new key 'linkbutton' with ['AXLink', 'AXButton'] so queries for "link" no longer match generic buttons but framework-specific link-buttons can still be targeted.macos-control.md (1)
136-141: Vary repeated wording in optimization bullets for readability.Lines 138-141 start with “Add” three times; consider varying verbs to keep the list scannable.
💡 Suggested tweak
- - Add element path locators (`AXParent` chain) for stronger stale-ID recovery. - - Add incremental snapshot mode to avoid full tree scans on repeated `find`. - - Add richer CDP APIs (`DOM.querySelector`, click/type helpers) on top of `electron_debug_eval`. + - Introduce element path locators (`AXParent` chain) for stronger stale-ID recovery. + - Add incremental snapshot mode to avoid full tree scans on repeated `find`. + - Expand CDP APIs (`DOM.querySelector`, click/type helpers) on top of `electron_debug_eval`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@macos-control.md` around lines 136 - 141, The three consecutive bullets under the "Optimization opportunities" section all start with "Add" which is repetitive; update the three lines "Add element path locators (`AXParent` chain) for stronger stale-ID recovery.", "Add incremental snapshot mode to avoid full tree scans on repeated `find`.", and "Add richer CDP APIs (`DOM.querySelector`, click/type helpers) on top of `electron_debug_eval`." to use varied verbs (e.g., "Introduce element path locators...", "Implement incremental snapshot mode...", "Expose richer CDP APIs...") so the list reads more smoothly while preserving the same content and references.test/test-electron-debug.js (1)
1-34: Avoid port-collision flakiness in attach failure test.Using a fixed port (Line 29) can fail if the port is already in use. Consider selecting an unused port dynamically to make the test deterministic.
💡 Suggested change
-import assert from 'assert'; +import assert from 'assert'; +import net from 'net'; import { cdpAdapter } from '../dist/tools/macos-control/cdp-adapter.js'; +async function getUnusedPort() { + return await new Promise((resolve, reject) => { + const server = net.createServer(); + server.listen(0, '127.0.0.1', () => { + const { port } = server.address(); + server.close(() => resolve(port)); + }); + server.on('error', reject); + }); +} + async function testAttachFailureShape() { console.log('\n--- Test: electron debug attach failure shape ---'); const attachResult = await cdpAdapter.attach({ host: '127.0.0.1', - port: 59999, + port: await getUnusedPort(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-electron-debug.js` around lines 1 - 34, testAttachFailureShape uses a hardcoded port which can collide; update the test to pick an unused port dynamically before calling cdpAdapter.attach (e.g. bind a temporary net.Server to port 0 to get an ephemeral port or use a get-port helper), then pass that free port into cdpAdapter.attach so the attach failure assertion remains deterministic; change the port value referenced in testAttachFailureShape to the discovered port and close the temp server before invoking attach.src/tools/macos-control/cdp-adapter.ts (1)
13-22: Same unnecessaryas anycast as inorchestrator.ts.
codeis alreadyMacosControlErrorCode, matchingMacosControlError['code'].♻️ Fix
- code: code as any, + code,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/macos-control/cdp-adapter.ts` around lines 13 - 22, The makeError function currently casts code to any unnecessarily; remove the "as any" cast so the returned object uses the code value directly (makeError(code: MacosControlErrorCode, ...) should return MacosControlResult<never> with error.code typed as MacosControlErrorCode), ensuring the error shape matches MacosControlError and TypeScript infers the correct types without the cast.test/test-macos-control.js (2)
74-75: ESM main-guard may not work reliably on Windows.
import.meta.url === \file://${process.argv[1]}`can fail on Windows whereprocess.argv[1]uses backslashes butimport.meta.url` uses forward slashes. Consider using a normalized comparison:♻️ Suggested fix
-if (import.meta.url === `file://${process.argv[1]}`) { +import { fileURLToPath } from 'url'; +if (process.argv[1] && fileURLToPath(import.meta.url) === path.resolve(process.argv[1])) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-macos-control.js` around lines 74 - 75, The ESM main-guard using import.meta.url === `file://${process.argv[1]}` can fail on Windows due to slash/backslash differences; change the comparison to normalize both sides by converting import.meta.url to a filesystem path and resolving/normalizing process.argv[1] before comparing (e.g. use fileURLToPath(import.meta.url) and path.resolve/path.normalize on process.argv[1]) so the check that guards runTests() is reliable across platforms.
11-37: Schema tests look good but coverage is limited.Only 3 of the many new schemas are tested (
MacosAxFindArgsSchema,MacosAxClickArgsSchema,MacosAxBatchArgsSchema). Consider also testing negative cases (e.g.,MacosAxFindArgsSchemawithout bothtextandroleshould fail due to the.refine()constraint, andMacosAxClickArgsSchemawithoutidorapp+text).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-macos-control.js` around lines 11 - 37, Add negative test cases to testSchemas to improve coverage: assert that MacosAxFindArgsSchema.parse throws when neither text nor role is provided (use try/catch or assert.throws against MacosAxFindArgsSchema.parse({ app: 'System Settings' })), and assert that MacosAxClickArgsSchema.parse throws when neither id nor the app+text combination are present (e.g., parse({}) and parse({ app: 'Finder' }) should fail). Keep existing positive tests, use the same test harness/console messages, and reference the schema names MacosAxFindArgsSchema and MacosAxClickArgsSchema so failures prove the .refine() and click validation rules work.src/tools/macos-control/orchestrator.ts (1)
11-19: Unnecessaryas anycast on error code.The
codeparameter is already typed asMacosControlErrorCode, which matchesMacosControlError['code']. Theas anycast bypasses type safety for no reason.♻️ Suggested fix
function fail<T = never>(code: MacosControlErrorCode, message: string): MacosControlResult<T> { return { ok: false, error: { - code: code as any, + code, message, }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/macos-control/orchestrator.ts` around lines 11 - 19, The fail function is using an unnecessary and unsafe cast "as any" for the error code; update the function so the returned object assigns code directly (remove "as any") and ensure the types align by keeping MacosControlErrorCode for the parameter and returning MacosControlResult<T> with an error shaped as MacosControlError (i.e., ensure MacosControlError['code'] is compatible with MacosControlErrorCode); modify the fail function (symbols: fail, MacosControlErrorCode, MacosControlResult, MacosControlError) to remove the cast and, if TypeScript complains, update the MacosControlError type to accept MacosControlErrorCode rather than introducing any casts.src/server.ts (1)
997-1277: Consider gating macOS-only tools from the tool list on non-macOS platforms.All 15 macOS AX and Electron debug tools are always listed via
ListTools, even on Windows and Linux where they'll always returnUNSUPPORTED_PLATFORM. This adds noise to the tool catalog. Consider extending theshouldIncludeToolfunction to filter these out on non-darwin platforms:♻️ Suggested approach
function shouldIncludeTool(toolName: string): boolean { if (toolName === 'give_feedback_to_desktop_commander' && currentClient?.name === 'desktop-commander') { return false; } + + // macOS-only tools + if (process.platform !== 'darwin' && (toolName.startsWith('macos_ax_') || toolName.startsWith('electron_debug_'))) { + return false; + } return true; }src/tools/macos-control/ax-adapter.ts (2)
216-231: Signatures map grows unbounded.
rememberElementsstores element signatures indefinitely. For long-running sessions with extensive AX automation, this could accumulate thousands of entries. Consider adding an LRU eviction or size cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/macos-control/ax-adapter.ts` around lines 216 - 231, rememberElements stores element signatures indefinitely in this.signatures causing unbounded growth; update it to enforce a size cap (or LRU eviction) by tracking insertion order and pruning oldest entries when the map exceeds a configurable MAX_SIGNATURES. Modify rememberElements (and where this.signatures is constructed) to: check/define a MAX_SIGNATURES constant, insert/update entries into this.signatures, and if this.signatures.size > MAX_SIGNATURES remove the oldest key(s) (e.g., maintain a secondary queue/linked-list or use a Map iteration to delete the first entries) so the map never grows beyond the cap; keep references to element.id, element.app, element.role, title, label, text, bounds and ensure thread-safety if applicable.
95-124: Helper path resolution retries on every call when not found.When no valid helper binary is found,
helperPathCacheis set tonull(Line 122), but the cache check at Line 100 (!== null) doesn't distinguish "not yet resolved" from "resolved to nothing". EveryrunHelpercall will re-scan all candidates. Consider using a sentinel value (e.g.,undefinedvsnull) to cache the "not found" state and avoid repeated filesystem scans:♻️ Optional optimization
- private helperPathCache: string | null = null; + private helperPathCache: string | null | undefined = undefined; // undefined = not resolved, null = not found private async resolveHelperPath(): Promise<string | null> { - if (this.helperPathCache !== null) { + if (this.helperPathCache !== undefined) { return this.helperPathCache; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/macos-control/ax-adapter.ts` around lines 95 - 124, The cache currently uses helperPathCache: string | null = null which conflates "not found" with "not yet resolved" and causes repeated scans; change helperPathCache to string | null | undefined and initialize it to undefined, update the cache check in resolveHelperPath from !== null to !== undefined, ensure resolveHelperPath still sets helperPathCache to the found path or to null when no candidate exists, and adjust any callers (e.g., runHelper) that check helperPathCache to use the new undefined-vs-null semantics so a failed resolution is cached and filesystem scans are avoided.src/handlers/macos-control-handlers.ts (2)
82-85: Redundant?? []fallback —modifiersis alwaysstring[]after parsing.
MacosAxKeyArgsSchemaapplies.optional().default([]), so Zod's output type formodifiersis alreadystring[], neverundefined. The?? []is dead code and slightly misleads readers.🧹 Proposed cleanup
- return toServerResult(await macosControlOrchestrator.axKey(parsed.key, parsed.modifiers ?? [])); + return toServerResult(await macosControlOrchestrator.axKey(parsed.key, parsed.modifiers));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/macos-control-handlers.ts` around lines 82 - 85, In handleMacosAxKey, remove the redundant nullish fallback on parsed.modifiers because MacosAxKeyArgsSchema sets modifiers to .optional().default([]) so the parsed value is already string[]; call macosControlOrchestrator.axKey(parsed.key, parsed.modifiers) directly (keep MacosAxKeyArgsSchema.parse(args) and toServerResult wrapping as-is) to avoid misleading dead code.
97-100:as anycast onparsed.commandsdefeats type safety.The cast signals a type mismatch between what
MacosAxBatchCommandSchemainfers and whatorchestrator.axBatchexpects. Prefer exporting a concrete type (e.g.,z.infer<typeof MacosAxBatchCommandSchema>) from the schema and aligning the orchestrator's parameter type to it, so the cast is unnecessary.♻️ Suggested approach
+// In schemas.ts (or a shared types file) +export type MacosAxBatchCommand = z.infer<typeof MacosAxBatchCommandSchema>; // In orchestrator.ts — align axBatch signature: - axBatch(commands: SomeOtherType[], stopOnError?: boolean): ... + axBatch(commands: MacosAxBatchCommand[], stopOnError?: boolean): ... // In macos-control-handlers.ts: - return toServerResult(await macosControlOrchestrator.axBatch(parsed.commands as any, parsed.stopOnError)); + return toServerResult(await macosControlOrchestrator.axBatch(parsed.commands, parsed.stopOnError));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/macos-control-handlers.ts` around lines 97 - 100, Replace the unsafe cast on parsed.commands in handleMacosAxBatch by aligning the schema and orchestrator types: export a concrete type e.g. export type MacosAxBatchCommand = z.infer<typeof MacosAxBatchCommandSchema> from the schema file, update the macosControlOrchestrator.axBatch signature to accept MacosAxBatchCommand[] (or the exact inferred type), and then remove the "as any" in handleMacosAxBatch so parsed.commands is passed with the correct static type; ensure MacosAxBatchArgsSchema still describes commands as the exported type.src/tools/schemas.ts (2)
274-285:MacosAxClickArgsSchemadoesn't allow clicking byapp + role(inconsistent withMacosAxFindArgsSchema).
MacosAxFindArgsSchemaacceptstext OR roleas a locator.MacosAxClickArgsSchema's refinement requiresid OR (app AND text), making{app: "Finder", role: "Button"}invalid — even though role is a declared field andaxFindsupports it. If the orchestrator can click by role, the refinement should mirror the find schema.♻️ Suggested fix
}).refine( - data => !!data.id || (!!data.app && !!data.text), - { message: 'Provide either id, or app+text' } + data => !!data.id || (!!data.app && (!!data.text || !!data.role)), + { message: 'Provide either id, or app + text/role' } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/schemas.ts` around lines 274 - 285, The MacosAxClickArgsSchema refinement only allows id or (app and text), which rejects valid locators like {app, role}; update MacosAxClickArgsSchema to mirror MacosAxFindArgsSchema's locator rules by changing the refine predicate to accept id OR (app AND (text OR role)) (or the same logical expression used in MacosAxFindArgsSchema), and keep the error message consistent; locate the schema definition named MacosAxClickArgsSchema and adjust its refine callback accordingly so click arguments support app+role like axFind does.
308-326:MacosAxBatchCommandSchemahas no action-specific field validation.Every field is optional with no cross-field refinements, so structurally invalid commands (e.g.,
{ action: "type" }withouttext, or{ action: "key" }withoutkey) pass schema validation and will only fail at the orchestrator level. For better early feedback, consider a discriminated union or per-action refinements.♻️ Example discriminated union sketch
-export const MacosAxBatchCommandSchema = z.object({ - action: z.enum(['activate', 'find', 'click', 'find_and_click', 'type', 'key', 'wait', 'wait_for', 'get_state', 'scroll']), - app: z.string().optional(), - text: z.string().optional(), - // ... all optional -}); +export const MacosAxBatchCommandSchema = z.discriminatedUnion('action', [ + z.object({ action: z.literal('activate'), app: z.string() }), + z.object({ action: z.literal('type'), text: z.string() }), + z.object({ action: z.literal('key'), key: z.string(), modifiers: z.array(z.string()).optional().default([]) }), + z.object({ action: z.literal('wait'), ms: z.number() }), + z.object({ action: z.literal('find'), app: z.string(), text: z.string().optional(), role: z.string().optional(), /* ... */ }), + // ... remaining actions +]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/schemas.ts` around lines 308 - 326, MacosAxBatchCommandSchema currently allows any field combination because all fields are optional; change it to a discriminated union on the action property (or add a .superRefine switch on action) so each action value has its own required/optional fields (e.g., action: "type" requires text, action: "key" requires key, action: "click"/"find"/"find_and_click" require selectors like role/id/index or text, action: "wait"/"wait_for" require timeout_ms or ms, action: "scroll" requires direction and amount, etc.). Update the MacosAxBatchCommandSchema declaration to use z.discriminatedUnion('action', [...per-action schemas...]) or implement a .superRefine handler that asserts presence/shape of action-specific fields and reports clear zod errors referencing action, text, key, direction, amount, timeout_ms, ms, role, id, index as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 61-63: Duplicate .build/ entry found in .gitignore; remove the
redundant line so .build/ appears only once. Open the .gitignore, locate both
occurrences of the ".build/" entry and delete the duplicate (keep a single
.build/ line), then save and commit the cleaned .gitignore.
In `@manifest.template.json`:
- Around line 138-185: The manifest is missing three tool entries that are
registered in server.ts — macos_ax_find_and_click, macos_ax_get_state, and
macos_ax_list_elements — so add corresponding objects to manifest.template.json
(matching the naming used in server.ts) with short descriptions consistent with
the existing macos_ax_* entries; place them near the other macos_ax entries and
ensure the "name" strings exactly match the registered tool names
(macos_ax_find_and_click, macos_ax_get_state, macos_ax_list_elements) so clients
can discover them.
In `@native/macos-ax-helper/Sources/macos-ax-helper/main.swift`:
- Around line 853-889: The typeText function currently skips unmapped characters
(lookup in keyCodes and shiftChars) but still returns true; change typeText to
detect when any character is unmapped (e.g., track a Boolean like
hadSkippedChar) and return false if any character could not be mapped/typed;
update the guard that currently "continue"s on missing keyCode to set the flag
(or early-return false) so callers know typing failed; reference the typeText
function and the keyCodes/shiftChars maps when making the change.
In `@package.json`:
- Line 3: The package.json "version" field was regressed to 0.2.36; update the
"version" property in package.json to 0.2.38 or higher (matching or exceeding
origin/main) so npm publish will succeed—locate the "version" key in
package.json and increment its value to at least "0.2.38".
In `@src/remote-device/remote-channel.ts`:
- Around line 56-60: The debug log is printing PII via user?.email; update the
logging in the remote channel so it never emits email addresses. In the block
that assigns this._user and calls console.debug (referenced by this._user and
the console.debug('[DEBUG] Session set successfully, user:', user?.email) call),
replace the email emission with a non-PII identifier (e.g. user.id or a
redacted/masked value like <redacted-email>) and ensure any other debug
statements in the same scope do the same.
In `@src/tools/edit.ts`:
- Around line 374-419: The code misses validation when only one of parsed.range
or parsed.content is provided: after computing hasRange and hasContent (and
before the range vs text-replacement branches) add an explicit guard that if
exactly one is truthy (hasRange && !hasContent || !hasRange && hasContent)
returns createErrorResponse with a clear message like "Both range and content
must be provided together or both omitted"; this prevents falling through to the
text-replacement path and ensures the editRange branch only runs when both are
present.
In `@src/tools/macos-control/ax-adapter.ts`:
- Around line 45-52: The isExecutable function currently only checks existence;
update it to test execute permission by calling access(filePath, constants.X_OK)
(importing the constants symbol from 'fs') so the function verifies the file is
executable before returning true; keep the try/catch behavior and return false
on error so cached candidates won't be a non-executable binary.
In `@src/tools/macos-control/cdp-adapter.ts`:
- Around line 262-291: fetchTargets currently calls fetch(listUrl) with no
timeout and can hang; update CdpAdapter.fetchTargets to create an
AbortController (or use AbortSignal.timeout(timeoutMs)), pass its signal to
fetch({ signal }), and ensure you clear/handle the timeout: catch AbortError (or
check error.name === 'AbortError') and return makeError('CDP_CONNECT_FAILED',
'CDP endpoint request timed out') while preserving other error behavior; locate
the fetch call in fetchTargets and add the timeout value and signal handling
around it.
- Around line 67-93: The current onData handler uses handshakeBuffer as a UTF-8
string and does chunk.toString('utf8'), which corrupts any binary WebSocket
frame bytes that arrive after the HTTP/1.1 101 headers; change handshakeBuffer
to accumulate raw Buffer chunks (e.g., an array or a single Buffer), search for
the boundary bytes sequence Buffer.from('\r\n\r\n') in binary space, convert
only the header portion to string for the HTTP status check, and slice the
remaining bytes as a Buffer to append to this.readBuffer and call
this.consumeFrames(); keep cleanup(), resolve(), and reject() behavior the same
but operate on Buffers instead of string round-trips.
- Around line 24-244: The hand-rolled SimpleWebSocketClient (methods connect,
sendText, close and internal helpers consumeFrames/encodeClientFrame)
reimplements WebSocket framing, masking and handshake; replace it with the
standard 'ws' package: import WebSocket from 'ws', create a WebSocket(url, {
handshakeTimeout: ... }) in connect, wire 'message', 'close', 'error' events to
the existing onTextMessage and onCloseHandler, implement sendText by calling
ws.send(text) and close by calling ws.close(), and remove
consumeFrames/encodeClientFrame and raw socket handling to rely on the library's
tested behavior (ensure timeout handling and error propagation are preserved).
In `@src/tools/macos-control/orchestrator.ts`:
- Around line 118-135: The wait step is being skipped when timeout_ms is set but
args.text is missing because the condition uses "timeoutMs > 0 && args.text";
update the orchestrator logic to surface that behavior by logging or returning a
hint when a timeout is provided without text (so callers know axWaitFor wasn't
invoked), e.g., in the code around the axWaitFor call check args.timeout_ms and
args.text and emit a clear debug/warn via the existing logger or include a
skipped-wait flag in the response before proceeding to axFind; reference
axWaitFor, args.timeout_ms (timeoutMs), args.text and the find_and_click flow to
locate where to add the log/hint.
In `@src/tools/macos-control/role-aliases.ts`:
- Around line 31-36: The strip-`ax` branch in expandRoleAlias wrongly triggers
for PascalCase concrete roles; modify expandRoleAlias so the
"lower.startsWith('ax')" prefix-removal logic only runs when the original input
is already all-lowercase (e.g., input === lower) to ensure alias hints like
"axswitch" expand but concrete names like "AXButton" do not; update the
condition around ROLE_ALIASES lookup (and keep using ROLE_ALIASES and
withoutPrefix) so only fully-lowercase inputs are transformed and mapped to
alias groups.
In `@src/utils/capture.ts`:
- Around line 255-269: The GA API secrets and measurement IDs are hardcoded in
capture_call_tool and capture; update both functions to read GA_MEASUREMENT_ID
and GA_API_SECRET (or a single config object) from environment/config (e.g.,
process.env or shared config) and pass them to captureBase, and if the values
are missing, short‑circuit by returning early or skipping the call (log a
warning/error) to avoid sending requests with empty secrets; ensure captureBase
still receives the constructed GA_BASE_URL/G A_DEBUG_BASE_URL from those env
values and remove the plaintext constants.
In `@src/utils/system-info.ts`:
- Line 216: The mount-options string is being kept as a raw comma-separated
string (const options = parts[3]) which makes options.includes('ro') produce
false positives (matching substrings like errors=remount-ro); change the code so
options is an array by splitting the options field on commas (e.g.,
parts[3].split(',')) and keep the existing Array.prototype.includes('ro') check
used when computing DockerMount.readOnly; update any type annotations if needed
so options is a string[] for the readOnly logic in the DockerMount parsing
functions.
---
Outside diff comments:
In `@scripts/build-mcpb.cjs`:
- Around line 45-48: The snippet incorrectly concatenates the closing brace of
the if block with the subsequent call to fs.mkdirSync, causing a syntax/runtime
error; update the code around the BUNDLE_DIR handling (the if using
fs.existsSync and fs.rmSync and the following fs.mkdirSync) so that the if block
is properly closed and fs.mkdirSync is placed on its own statement (add a
semicolon or newline between the block end and fs.mkdirSync) to ensure the
bundle directory is recreated without syntax errors.
In `@src/tools/schemas.ts`:
- Around line 139-147: The current refine uses hasValue for both data.old_string
and data.new_string which rejects empty-string replacements; update the
predicate so old_string must be non-empty (old_string !== undefined &&
old_string !== '') while new_string only needs to be present (new_string !==
undefined) — e.g. replace the existing hasValue usage in the refine callback
(referencing hasValue, data.old_string, data.new_string, data.range,
data.content) with a condition that returns (old_string !== undefined &&
old_string !== '' && new_string !== undefined) || (data.range !== undefined &&
data.content !== undefined).
---
Duplicate comments:
In `@server.json`:
- Around line 10-16: The two version fields in server.json (the top-level
"version" and the packages[0].version entry) were changed to 0.2.36 but should
match the intended release version; update these two fields to the correct
version that matches src/version.ts (or revert them to the prior release if the
downgrade was accidental) so all three locations (src/version.ts, server.json
"version", and server.json packages[0].version) are consistent and reflect the
intended release number.
---
Nitpick comments:
In `@macos-control.md`:
- Around line 136-141: The three consecutive bullets under the "Optimization
opportunities" section all start with "Add" which is repetitive; update the
three lines "Add element path locators (`AXParent` chain) for stronger stale-ID
recovery.", "Add incremental snapshot mode to avoid full tree scans on repeated
`find`.", and "Add richer CDP APIs (`DOM.querySelector`, click/type helpers) on
top of `electron_debug_eval`." to use varied verbs (e.g., "Introduce element
path locators...", "Implement incremental snapshot mode...", "Expose richer CDP
APIs...") so the list reads more smoothly while preserving the same content and
references.
In `@src/handlers/macos-control-handlers.ts`:
- Around line 82-85: In handleMacosAxKey, remove the redundant nullish fallback
on parsed.modifiers because MacosAxKeyArgsSchema sets modifiers to
.optional().default([]) so the parsed value is already string[]; call
macosControlOrchestrator.axKey(parsed.key, parsed.modifiers) directly (keep
MacosAxKeyArgsSchema.parse(args) and toServerResult wrapping as-is) to avoid
misleading dead code.
- Around line 97-100: Replace the unsafe cast on parsed.commands in
handleMacosAxBatch by aligning the schema and orchestrator types: export a
concrete type e.g. export type MacosAxBatchCommand = z.infer<typeof
MacosAxBatchCommandSchema> from the schema file, update the
macosControlOrchestrator.axBatch signature to accept MacosAxBatchCommand[] (or
the exact inferred type), and then remove the "as any" in handleMacosAxBatch so
parsed.commands is passed with the correct static type; ensure
MacosAxBatchArgsSchema still describes commands as the exported type.
In `@src/remote-device/remote-channel.ts`:
- Around line 137-149: The subscribe method can create duplicate channels if
called multiple times; modify subscribe to detect an existing active
subscription (e.g., check this.channel or an isSubscribed flag) and either no-op
when the same deviceId/onToolCall are already registered or first cleanly
unsubscribe/close the existing channel (call the existing channel's
unsubscribe/close method or a helper like unsubscribeChannel) before overwriting
this.deviceId and this.onToolCall and calling createChannel; ensure you still
throw if !this.client and update the channel state after successful
createChannel.
- Around line 83-89: The updateDevice method lacks a user-level guard and can
accidentally update another user's device; modify updateDevice to include an
explicit user check by filtering the update with the device owner column (e.g.,
.eq('user_id', <currentUserId>)) so the DB update is constrained to the
authenticated user; obtain the current user id from the instance (e.g.,
this.userId / this.user?.id) or change the signature to accept a userId
parameter, validate it's present (throw if missing), and include that
.eq('user_id', userId) condition alongside .eq('id', deviceId) when calling
this.client.from('mcp_devices').update(updates).
In `@src/tools/macos-control/ax-adapter.ts`:
- Around line 216-231: rememberElements stores element signatures indefinitely
in this.signatures causing unbounded growth; update it to enforce a size cap (or
LRU eviction) by tracking insertion order and pruning oldest entries when the
map exceeds a configurable MAX_SIGNATURES. Modify rememberElements (and where
this.signatures is constructed) to: check/define a MAX_SIGNATURES constant,
insert/update entries into this.signatures, and if this.signatures.size >
MAX_SIGNATURES remove the oldest key(s) (e.g., maintain a secondary
queue/linked-list or use a Map iteration to delete the first entries) so the map
never grows beyond the cap; keep references to element.id, element.app,
element.role, title, label, text, bounds and ensure thread-safety if applicable.
- Around line 95-124: The cache currently uses helperPathCache: string | null =
null which conflates "not found" with "not yet resolved" and causes repeated
scans; change helperPathCache to string | null | undefined and initialize it to
undefined, update the cache check in resolveHelperPath from !== null to !==
undefined, ensure resolveHelperPath still sets helperPathCache to the found path
or to null when no candidate exists, and adjust any callers (e.g., runHelper)
that check helperPathCache to use the new undefined-vs-null semantics so a
failed resolution is cached and filesystem scans are avoided.
In `@src/tools/macos-control/cdp-adapter.ts`:
- Around line 13-22: The makeError function currently casts code to any
unnecessarily; remove the "as any" cast so the returned object uses the code
value directly (makeError(code: MacosControlErrorCode, ...) should return
MacosControlResult<never> with error.code typed as MacosControlErrorCode),
ensuring the error shape matches MacosControlError and TypeScript infers the
correct types without the cast.
In `@src/tools/macos-control/orchestrator.ts`:
- Around line 11-19: The fail function is using an unnecessary and unsafe cast
"as any" for the error code; update the function so the returned object assigns
code directly (remove "as any") and ensure the types align by keeping
MacosControlErrorCode for the parameter and returning MacosControlResult<T> with
an error shaped as MacosControlError (i.e., ensure MacosControlError['code'] is
compatible with MacosControlErrorCode); modify the fail function (symbols: fail,
MacosControlErrorCode, MacosControlResult, MacosControlError) to remove the cast
and, if TypeScript complains, update the MacosControlError type to accept
MacosControlErrorCode rather than introducing any casts.
In `@src/tools/macos-control/role-aliases.ts`:
- Line 8: The current 'link' alias maps to both 'AXLink' and 'AXButton', which
over-matches; update the alias definitions by removing 'AXButton' from the
'link' mapping and instead add a new alias (e.g., 'linkbutton') that maps to
['AXLink', 'AXButton'] for cases that intentionally need both; specifically,
edit the alias object where the 'link' key is defined to only include 'AXLink'
and add a new key 'linkbutton' with ['AXLink', 'AXButton'] so queries for "link"
no longer match generic buttons but framework-specific link-buttons can still be
targeted.
In `@src/tools/schemas.ts`:
- Around line 274-285: The MacosAxClickArgsSchema refinement only allows id or
(app and text), which rejects valid locators like {app, role}; update
MacosAxClickArgsSchema to mirror MacosAxFindArgsSchema's locator rules by
changing the refine predicate to accept id OR (app AND (text OR role)) (or the
same logical expression used in MacosAxFindArgsSchema), and keep the error
message consistent; locate the schema definition named MacosAxClickArgsSchema
and adjust its refine callback accordingly so click arguments support app+role
like axFind does.
- Around line 308-326: MacosAxBatchCommandSchema currently allows any field
combination because all fields are optional; change it to a discriminated union
on the action property (or add a .superRefine switch on action) so each action
value has its own required/optional fields (e.g., action: "type" requires text,
action: "key" requires key, action: "click"/"find"/"find_and_click" require
selectors like role/id/index or text, action: "wait"/"wait_for" require
timeout_ms or ms, action: "scroll" requires direction and amount, etc.). Update
the MacosAxBatchCommandSchema declaration to use z.discriminatedUnion('action',
[...per-action schemas...]) or implement a .superRefine handler that asserts
presence/shape of action-specific fields and reports clear zod errors
referencing action, text, key, direction, amount, timeout_ms, ms, role, id,
index as needed.
In `@test/test-electron-debug.js`:
- Around line 1-34: testAttachFailureShape uses a hardcoded port which can
collide; update the test to pick an unused port dynamically before calling
cdpAdapter.attach (e.g. bind a temporary net.Server to port 0 to get an
ephemeral port or use a get-port helper), then pass that free port into
cdpAdapter.attach so the attach failure assertion remains deterministic; change
the port value referenced in testAttachFailureShape to the discovered port and
close the temp server before invoking attach.
In `@test/test-macos-control.js`:
- Around line 74-75: The ESM main-guard using import.meta.url ===
`file://${process.argv[1]}` can fail on Windows due to slash/backslash
differences; change the comparison to normalize both sides by converting
import.meta.url to a filesystem path and resolving/normalizing process.argv[1]
before comparing (e.g. use fileURLToPath(import.meta.url) and
path.resolve/path.normalize on process.argv[1]) so the check that guards
runTests() is reliable across platforms.
- Around line 11-37: Add negative test cases to testSchemas to improve coverage:
assert that MacosAxFindArgsSchema.parse throws when neither text nor role is
provided (use try/catch or assert.throws against MacosAxFindArgsSchema.parse({
app: 'System Settings' })), and assert that MacosAxClickArgsSchema.parse throws
when neither id nor the app+text combination are present (e.g., parse({}) and
parse({ app: 'Finder' }) should fail). Keep existing positive tests, use the
same test harness/console messages, and reference the schema names
MacosAxFindArgsSchema and MacosAxClickArgsSchema so failures prove the .refine()
and click validation rules work.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.gitignoreFAQ.mdREADME.mdbuild-macos-helper.shknown-limitations.mdmacos-control.mdmanifest.template.jsonnative/macos-ax-helper/Package.swiftnative/macos-ax-helper/Sources/macos-ax-helper/main.swiftpackage.jsonplugin.yamlscripts/build-mcpb.cjsserver.jsonsrc/handlers/index.tssrc/handlers/macos-control-handlers.tssrc/remote-device/remote-channel.tssrc/search-manager.tssrc/server.tssrc/tools/edit.tssrc/tools/macos-control/ax-adapter.tssrc/tools/macos-control/cdp-adapter.tssrc/tools/macos-control/orchestrator.tssrc/tools/macos-control/role-aliases.tssrc/tools/macos-control/types.tssrc/tools/schemas.tssrc/utils/capture.tssrc/utils/files/base.tssrc/utils/files/docx.tssrc/utils/files/factory.tssrc/utils/system-info.tssrc/utils/usageTracker.tssrc/version.tstest/test-electron-debug.jstest/test-macos-control.js
💤 Files with no reviewable changes (4)
- src/search-manager.ts
- src/utils/files/docx.ts
- plugin.yaml
- src/utils/usageTracker.ts
Comment on lines
+61
to
+63
| .build/ | ||
| .claude | ||
| .build/ |
Contributor
There was a problem hiding this comment.
Duplicate .build/ entry.
.build/ appears twice (Lines 61 and 63). Remove the duplicate.
🔧 Proposed fix
.build/
.claude
-.build/
bin/macos/macos-ax-helper-darwin-arm64📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .build/ | |
| .claude | |
| .build/ | |
| .build/ | |
| .claude |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 61 - 63, Duplicate .build/ entry found in
.gitignore; remove the redundant line so .build/ appears only once. Open the
.gitignore, locate both occurrences of the ".build/" entry and delete the
duplicate (keep a single .build/ line), then save and commit the cleaned
.gitignore.
Comment on lines
+138
to
+185
| { | ||
| "name": "macos_ax_status", | ||
| "description": "Check macOS Accessibility automation status, helper readiness, and permissions." | ||
| }, | ||
| { | ||
| "name": "macos_ax_list_apps", | ||
| "description": "List regular macOS apps available for Accessibility control." | ||
| }, | ||
| { | ||
| "name": "macos_ax_find", | ||
| "description": "Find interactive macOS Accessibility elements in an app with text/role filtering." | ||
| }, | ||
| { | ||
| "name": "macos_ax_click", | ||
| "description": "Click a macOS Accessibility element by stable id or by app+text lookup." | ||
| }, | ||
| { | ||
| "name": "macos_ax_type", | ||
| "description": "Type text with macOS keyboard event injection." | ||
| }, | ||
| { | ||
| "name": "macos_ax_key", | ||
| "description": "Press a key with optional modifiers using macOS keyboard events." | ||
| }, | ||
| { | ||
| "name": "macos_ax_activate", | ||
| "description": "Activate a macOS app window by name or PID." | ||
| }, | ||
| { | ||
| "name": "macos_ax_wait_for", | ||
| "description": "Wait for a matching macOS Accessibility element to appear." | ||
| }, | ||
| { | ||
| "name": "macos_ax_batch", | ||
| "description": "Run a sequence of macOS Accessibility actions in one request." | ||
| }, | ||
| { | ||
| "name": "electron_debug_attach", | ||
| "description": "Attach to an Electron/Chromium CDP endpoint and create a debug session." | ||
| }, | ||
| { | ||
| "name": "electron_debug_eval", | ||
| "description": "Evaluate JavaScript through an attached Electron debug session." | ||
| }, | ||
| { | ||
| "name": "electron_debug_disconnect", | ||
| "description": "Disconnect an existing Electron debug session." | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare tools registered in server.ts ListTools vs manifest.template.json
echo "=== Tools in manifest ==="
rg '"name":\s*"(macos_ax_|electron_debug_)' manifest.template.json -o --no-filename | sort
echo ""
echo "=== macOS/electron tools in server.ts ListTools ==="
rg 'name:\s*"(macos_ax_|electron_debug_)' src/server.ts -o --no-filename | sortRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 673
🏁 Script executed:
#!/bin/bash
# Extract full tool names from manifest
echo "=== Tools in manifest.template.json ==="
rg '"name":\s*"(macos_ax_|electron_debug_)[^"]*"' manifest.template.json --replace '$1' | sort | uniq
echo ""
echo "=== Tools in server.ts ListTools ==="
rg 'name:\s*"(macos_ax_|electron_debug_)[^"]*"' src/server.ts --replace '$1' | sort | uniq
echo ""
echo "=== Count comparison ==="
echo "In manifest: $(rg '"name":\s*"(macos_ax_|electron_debug_)' manifest.template.json | wc -l)"
echo "In server.ts: $(rg 'name:\s*"(macos_ax_|electron_debug_)' src/server.ts | wc -l)"Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Extract tools from manifest
echo "=== Tools in manifest.template.json ==="
rg -A0 '"name":\s*"(macos_ax_|electron_debug_)[^"]*' manifest.template.json | grep -oP '(?<="name":\s*")[^"]*' | sort
echo ""
echo "=== Tools in server.ts ==="
rg -A0 'name:\s*"(macos_ax_|electron_debug_)[^"]*' src/server.ts | grep -oP '(?<=name:\s*")[^"]*' | sortRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Extract tools from manifest using sed
echo "=== Tools in manifest.template.json ==="
sed -n '/"name":\s*"macos_ax_\|"name":\s*"electron_debug_/p' manifest.template.json | sed 's/.*"name":\s*"\([^"]*\)".*/\1/' | sort
echo ""
echo "=== Tools in server.ts ==="
sed -n '/name:\s*"macos_ax_\|name:\s*"electron_debug_/p' src/server.ts | sed 's/.*name:\s*"\([^"]*\)".*/\1/' | sort
echo ""
echo "=== Tools in manifest only ==="
comm -23 <(sed -n '/"name":\s*"macos_ax_\|"name":\s*"electron_debug_/p' manifest.template.json | sed 's/.*"name":\s*"\([^"]*\)".*/\1/' | sort) <(sed -n '/name:\s*"macos_ax_\|name:\s*"electron_debug_/p' src/server.ts | sed 's/.*name:\s*"\([^"]*\)".*/\1/' | sort)
echo ""
echo "=== Tools in server.ts only ==="
comm -13 <(sed -n '/"name":\s*"macos_ax_\|"name":\s*"electron_debug_/p' manifest.template.json | sed 's/.*"name":\s*"\([^"]*\)".*/\1/' | sort) <(sed -n '/name:\s*"macos_ax_\|name:\s*"electron_debug_/p' src/server.ts | sed 's/.*name:\s*"\([^"]*\)".*/\1/' | sort)Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 799
Three tools registered in server.ts are missing from the manifest.
server.ts registers macos_ax_find_and_click, macos_ax_get_state, and macos_ax_list_elements but they are absent from manifest.template.json. If the manifest is used for tool discovery or distribution, clients won't know these tools exist.
Was this intentional (e.g., internal/advanced tools not exposed in the catalog)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifest.template.json` around lines 138 - 185, The manifest is missing three
tool entries that are registered in server.ts — macos_ax_find_and_click,
macos_ax_get_state, and macos_ax_list_elements — so add corresponding objects to
manifest.template.json (matching the naming used in server.ts) with short
descriptions consistent with the existing macos_ax_* entries; place them near
the other macos_ax entries and ensure the "name" strings exactly match the
registered tool names (macos_ax_find_and_click, macos_ax_get_state,
macos_ax_list_elements) so clients can discover them.
Comment on lines
+853
to
+889
| func typeText(_ text: String) -> Bool { | ||
| for char in text { | ||
| let needsShift: Bool | ||
| let baseChar: Character | ||
|
|
||
| if let mapped = shiftChars[char] { | ||
| needsShift = true | ||
| baseChar = mapped | ||
| } else if char.isUppercase { | ||
| needsShift = true | ||
| baseChar = Character(char.lowercased()) | ||
| } else { | ||
| needsShift = false | ||
| baseChar = char | ||
| } | ||
|
|
||
| guard let keyCode = keyCodes[String(baseChar)] else { | ||
| continue | ||
| } | ||
|
|
||
| guard let down = CGEvent(keyboardEventSource: nil, virtualKey: keyCode, keyDown: true), | ||
| let up = CGEvent(keyboardEventSource: nil, virtualKey: keyCode, keyDown: false) else { | ||
| continue | ||
| } | ||
|
|
||
| if needsShift { | ||
| down.flags = [.maskShift] | ||
| up.flags = [.maskShift] | ||
| } | ||
|
|
||
| down.post(tap: .cghidEventTap) | ||
| up.post(tap: .cghidEventTap) | ||
|
|
||
| usleep(20_000) | ||
| } | ||
|
|
||
| return true |
Contributor
There was a problem hiding this comment.
Don’t silently skip unmapped characters in typeText.
Line 869 currently drops unsupported characters but still returns true, so callers will assume the full string was typed. Consider returning false (or tracking skipped chars) when any character can’t be mapped.
🐛 Suggested fix
func typeText(_ text: String) -> Bool {
+ var skipped = false
for char in text {
let needsShift: Bool
let baseChar: Character
@@
- guard let keyCode = keyCodes[String(baseChar)] else {
- continue
- }
+ guard let keyCode = keyCodes[String(baseChar)] else {
+ skipped = true
+ continue
+ }
@@
}
- return true
+ return !skipped
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/macos-ax-helper/Sources/macos-ax-helper/main.swift` around lines 853 -
889, The typeText function currently skips unmapped characters (lookup in
keyCodes and shiftChars) but still returns true; change typeText to detect when
any character is unmapped (e.g., track a Boolean like hadSkippedChar) and return
false if any character could not be mapped/typed; update the guard that
currently "continue"s on missing keyCode to set the flag (or early-return false)
so callers know typing failed; reference the typeText function and the
keyCodes/shiftChars maps when making the change.
| { | ||
| "name": "@wonderwhy-er/desktop-commander", | ||
| "version": "0.2.37", | ||
| "version": "0.2.36", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "Current version:"
node -p "require('./package.json').version"
if git rev-parse --verify HEAD~1 >/dev/null 2>&1; then
echo "Previous commit version:"
git show HEAD~1:package.json | node -p "JSON.parse(require('fs').readFileSync(0,'utf8')).version"
fi
if git rev-parse --verify origin/main >/dev/null 2>&1; then
echo "origin/main version:"
git show origin/main:package.json | node -p "JSON.parse(require('fs').readFileSync(0,'utf8')).version"
fiRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 127
Bump version to 0.2.38 or higher. The version has regressed from 0.2.37 (in origin/main) to 0.2.36. NPM will reject publishing a lower version, causing npm publish to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 3, The package.json "version" field was regressed to
0.2.36; update the "version" property in package.json to 0.2.38 or higher
(matching or exceeding origin/main) so npm publish will succeed—locate the
"version" key in package.json and increment its value to at least "0.2.38".
Comment on lines
+56
to
+60
| console.debug('[DEBUG] Failed to get user:', userError.message); | ||
| throw userError; | ||
| } | ||
|
|
||
| if (!user) { | ||
| const noUserError = new Error('No user returned after setSession'); | ||
| console.error('[DEBUG] No user returned:', noUserError.message); | ||
| await captureRemote('remote_channel_get_user_empty', {}); | ||
| throw noUserError; | ||
| } | ||
|
|
||
| this._user = user; | ||
| console.debug('[DEBUG] Session set successfully, user:', user.email); | ||
| console.debug('[DEBUG] Session set successfully, user:', user?.email); |
Contributor
There was a problem hiding this comment.
Avoid logging user email (PII) in debug output.
user?.email can leak PII into logs; prefer a redacted identifier or user id.
🔒 Proposed change
- console.debug('[DEBUG] Session set successfully, user:', user?.email);
+ console.debug('[DEBUG] Session set successfully, user_id:', user?.id ?? 'unknown');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.debug('[DEBUG] Failed to get user:', userError.message); | |
| throw userError; | |
| } | |
| if (!user) { | |
| const noUserError = new Error('No user returned after setSession'); | |
| console.error('[DEBUG] No user returned:', noUserError.message); | |
| await captureRemote('remote_channel_get_user_empty', {}); | |
| throw noUserError; | |
| } | |
| this._user = user; | |
| console.debug('[DEBUG] Session set successfully, user:', user.email); | |
| console.debug('[DEBUG] Session set successfully, user:', user?.email); | |
| console.debug('[DEBUG] Failed to get user:', userError.message); | |
| throw userError; | |
| } | |
| this._user = user; | |
| console.debug('[DEBUG] Session set successfully, user_id:', user?.id ?? 'unknown'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/remote-device/remote-channel.ts` around lines 56 - 60, The debug log is
printing PII via user?.email; update the logging in the remote channel so it
never emits email addresses. In the block that assigns this._user and calls
console.debug (referenced by this._user and the console.debug('[DEBUG] Session
set successfully, user:', user?.email) call), replace the email emission with a
non-PII identifier (e.g. user.id or a redacted/masked value like
<redacted-email>) and ensure any other debug statements in the same scope do the
same.
Comment on lines
+262
to
+291
| export class CdpAdapter { | ||
| private sessions = new Map<string, CdpSession>(); | ||
|
|
||
| private async fetchTargets(host: string, port: number): Promise<MacosControlResult<ElectronDebugTarget[]>> { | ||
| const listUrl = `http://${host}:${port}/json/list`; | ||
|
|
||
| try { | ||
| const response = await fetch(listUrl, { method: 'GET' }); | ||
| if (!response.ok) { | ||
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP targets: HTTP ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| if (!Array.isArray(data)) { | ||
| return makeError('CDP_CONNECT_FAILED', 'Invalid CDP target list response'); | ||
| } | ||
|
|
||
| const targets: ElectronDebugTarget[] = data.map((item: any) => ({ | ||
| id: String(item?.id ?? ''), | ||
| type: String(item?.type ?? ''), | ||
| title: String(item?.title ?? ''), | ||
| url: String(item?.url ?? ''), | ||
| webSocketDebuggerUrl: item?.webSocketDebuggerUrl ? String(item.webSocketDebuggerUrl) : undefined, | ||
| })); | ||
|
|
||
| return { ok: true, data: targets }; | ||
| } catch (error) { | ||
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP endpoint: ${error instanceof Error ? error.message : String(error)}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
fetchTargets uses fetch without a timeout.
If the CDP target endpoint is unresponsive, fetch will hang indefinitely (no default timeout). Consider adding an AbortSignal.timeout():
♻️ Suggested fix
- const response = await fetch(listUrl, { method: 'GET' });
+ const response = await fetch(listUrl, { method: 'GET', signal: AbortSignal.timeout(6000) });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export class CdpAdapter { | |
| private sessions = new Map<string, CdpSession>(); | |
| private async fetchTargets(host: string, port: number): Promise<MacosControlResult<ElectronDebugTarget[]>> { | |
| const listUrl = `http://${host}:${port}/json/list`; | |
| try { | |
| const response = await fetch(listUrl, { method: 'GET' }); | |
| if (!response.ok) { | |
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP targets: HTTP ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| if (!Array.isArray(data)) { | |
| return makeError('CDP_CONNECT_FAILED', 'Invalid CDP target list response'); | |
| } | |
| const targets: ElectronDebugTarget[] = data.map((item: any) => ({ | |
| id: String(item?.id ?? ''), | |
| type: String(item?.type ?? ''), | |
| title: String(item?.title ?? ''), | |
| url: String(item?.url ?? ''), | |
| webSocketDebuggerUrl: item?.webSocketDebuggerUrl ? String(item.webSocketDebuggerUrl) : undefined, | |
| })); | |
| return { ok: true, data: targets }; | |
| } catch (error) { | |
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP endpoint: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| } | |
| export class CdpAdapter { | |
| private sessions = new Map<string, CdpSession>(); | |
| private async fetchTargets(host: string, port: number): Promise<MacosControlResult<ElectronDebugTarget[]>> { | |
| const listUrl = `http://${host}:${port}/json/list`; | |
| try { | |
| const response = await fetch(listUrl, { method: 'GET', signal: AbortSignal.timeout(6000) }); | |
| if (!response.ok) { | |
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP targets: HTTP ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| if (!Array.isArray(data)) { | |
| return makeError('CDP_CONNECT_FAILED', 'Invalid CDP target list response'); | |
| } | |
| const targets: ElectronDebugTarget[] = data.map((item: any) => ({ | |
| id: String(item?.id ?? ''), | |
| type: String(item?.type ?? ''), | |
| title: String(item?.title ?? ''), | |
| url: String(item?.url ?? ''), | |
| webSocketDebuggerUrl: item?.webSocketDebuggerUrl ? String(item.webSocketDebuggerUrl) : undefined, | |
| })); | |
| return { ok: true, data: targets }; | |
| } catch (error) { | |
| return makeError('CDP_CONNECT_FAILED', `Failed to query CDP endpoint: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/macos-control/cdp-adapter.ts` around lines 262 - 291, fetchTargets
currently calls fetch(listUrl) with no timeout and can hang; update
CdpAdapter.fetchTargets to create an AbortController (or use
AbortSignal.timeout(timeoutMs)), pass its signal to fetch({ signal }), and
ensure you clear/handle the timeout: catch AbortError (or check error.name ===
'AbortError') and return makeError('CDP_CONNECT_FAILED', 'CDP endpoint request
timed out') while preserving other error behavior; locate the fetch call in
fetchTargets and add the timeout value and signal handling around it.
Comment on lines
+118
to
+135
| const timeoutMs = args.timeout_ms ?? 0; | ||
|
|
||
| if (timeoutMs > 0 && args.text) { | ||
| const waited = await this.axWaitFor({ | ||
| app: args.app, | ||
| text: args.text, | ||
| role: args.role, | ||
| timeout_ms: timeoutMs, | ||
| depth: args.depth, | ||
| }); | ||
|
|
||
| if (!waited.ok) { | ||
| if (args.if_exists && waited.error?.code === 'TIMEOUT') { | ||
| return { ok: true, data: { skipped: true } }; | ||
| } | ||
| return { ok: false, error: waited.error }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
axWaitFor is silently skipped when only role is provided (no text).
Line 120 gates the wait step on timeoutMs > 0 && args.text, so a find_and_click with role + timeout_ms but no text will skip the wait entirely and proceed directly to axFind. This may be intentional since axWaitFor requires text, but callers providing timeout_ms with role-only queries might expect polling behavior.
Consider documenting this limitation in the tool description or logging a hint when timeout_ms is provided but text is missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/macos-control/orchestrator.ts` around lines 118 - 135, The wait
step is being skipped when timeout_ms is set but args.text is missing because
the condition uses "timeoutMs > 0 && args.text"; update the orchestrator logic
to surface that behavior by logging or returning a hint when a timeout is
provided without text (so callers know axWaitFor wasn't invoked), e.g., in the
code around the axWaitFor call check args.timeout_ms and args.text and emit a
clear debug/warn via the existing logger or include a skipped-wait flag in the
response before proceeding to axFind; reference axWaitFor, args.timeout_ms
(timeoutMs), args.text and the find_and_click flow to locate where to add the
log/hint.
Comment on lines
+31
to
+36
| if (lower.startsWith('ax')) { | ||
| const withoutPrefix = lower.slice(2); | ||
| if (ROLE_ALIASES[withoutPrefix]) { | ||
| return ROLE_ALIASES[withoutPrefix]; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
expandRoleAlias silently expands concrete AX role names into alias groups.
The "strip ax prefix" branch fires for any input that lowercases to something starting with "ax", including proper PascalCase AX roles such as "AXButton", "AXSwitch", and "AXLink". These are all alias keys after prefix removal, so they return the full alias group instead of the single role the caller specified.
Trace for "AXButton":
lower = "axbutton"ROLE_ALIASES["axbutton"]→undefinedlower.startsWith("ax")→true;withoutPrefix = "button"ROLE_ALIASES["button"]→['AXButton', 'AXToolbarButton', 'AXMenuButton']- Returns the expanded group — the exact
AXButtonintent is lost.
The doc-comment says the "ax" prefix form is for alias hints like "axswitch" (all-lowercase), not for concrete roles like "AXSwitch" (PascalCase). Guard the branch to only activate when the input is already fully lowercase:
🐛 Proposed fix
- if (lower.startsWith('ax')) {
+ // Only apply alias prefix expansion for all-lowercase hints (e.g., "axswitch"),
+ // not for concrete PascalCase AX roles (e.g., "AXSwitch").
+ if (trimmed === lower && lower.startsWith('ax')) {
const withoutPrefix = lower.slice(2);
if (ROLE_ALIASES[withoutPrefix]) {
return ROLE_ALIASES[withoutPrefix];
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/macos-control/role-aliases.ts` around lines 31 - 36, The strip-`ax`
branch in expandRoleAlias wrongly triggers for PascalCase concrete roles; modify
expandRoleAlias so the "lower.startsWith('ax')" prefix-removal logic only runs
when the original input is already all-lowercase (e.g., input === lower) to
ensure alias hints like "axswitch" expand but concrete names like "AXButton" do
not; update the condition around ROLE_ALIASES lookup (and keep using
ROLE_ALIASES and withoutPrefix) so only fully-lowercase inputs are transformed
and mapped to alias groups.
Comment on lines
255
to
+269
| export const capture_call_tool = async (event: string, properties?: any) => { | ||
| // Old property (G-8L163XZ1CE) — keeps lower-volume tool events | ||
| const GA_OLD_ID = 'G-8L163XZ1CE'; | ||
| const GA_OLD_SECRET = 'hNxh4TK2TnSy4oLZn4RwTA'; | ||
| const GA_OLD_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_OLD_ID}&api_secret=${GA_OLD_SECRET}`; | ||
|
|
||
| // New property (dc_high_volume) — receives highest-volume tool events to avoid 1M/day BQ export limit | ||
| const GA_NEW_ID = 'G-ZDF1M5403Z'; | ||
| const GA_NEW_SECRET = 'cUEilpa0SpWfc2UjblDtKQ'; | ||
| const GA_NEW_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_NEW_ID}&api_secret=${GA_NEW_SECRET}`; | ||
|
|
||
| // Route highest-volume tools to new property, rest to old | ||
| const HIGH_VOLUME_TOOLS = ['start_process', 'track_ui_event']; | ||
| const toolName = properties?.name; | ||
| const gaUrl = HIGH_VOLUME_TOOLS.includes(toolName) ? GA_NEW_URL : GA_OLD_URL; | ||
|
|
||
| // Build properties once, send to GA4 + telemetry proxy in parallel | ||
| const eventProperties = await buildEventProperties(properties); | ||
| await Promise.all([ | ||
| captureBase(gaUrl, event, properties), // GA4 (routed by tool name) | ||
| sendToTelemetryProxy(event, eventProperties), // direct BigQuery (all events) | ||
| ]); | ||
| const GA_MEASUREMENT_ID = 'G-8L163XZ1CE'; // Replace with your GA4 Measurement ID | ||
| const GA_API_SECRET = 'hNxh4TK2TnSy4oLZn4RwTA'; // Replace with your GA4 API Secret | ||
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
| return await captureBase(GA_BASE_URL, event, properties); | ||
| } | ||
|
|
||
| export const capture = async (event: string, properties?: any) => { | ||
| const GA_MEASUREMENT_ID = 'G-F3GK01G39Y'; | ||
| const GA_API_SECRET = 'SqdcIAweSQS1RQErURMdEA'; | ||
| const GA_MEASUREMENT_ID = 'G-F3GK01G39Y'; // Replace with your GA4 Measurement ID | ||
| const GA_API_SECRET = 'SqdcIAweSQS1RQErURMdEA'; // Replace with your GA4 API Secret | ||
| const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
| const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`; | ||
|
|
||
| // Build properties once, send to both GA4 and telemetry proxy in parallel | ||
| const eventProperties = await buildEventProperties(properties); | ||
| await Promise.all([ | ||
| captureBase(GA_BASE_URL, event, properties), // existing GA4 | ||
| sendToTelemetryProxy(event, eventProperties), // new: direct BigQuery | ||
| ]); | ||
| return await captureBase(GA_BASE_URL, event, properties); |
Contributor
There was a problem hiding this comment.
Avoid hardcoding GA API secrets; load from env/config.
The GA API secrets at Line 257 and Line 265 are committed in plaintext. This is a security risk and triggers secret-scanner alerts. Load secrets from environment/config and short‑circuit if missing.
🔐 Suggested secure loading pattern
export const capture_call_tool = async (event: string, properties?: any) => {
- const GA_MEASUREMENT_ID = 'G-8L163XZ1CE'; // Replace with your GA4 Measurement ID
- const GA_API_SECRET = 'hNxh4TK2TnSy4oLZn4RwTA'; // Replace with your GA4 API Secret
+ const GA_MEASUREMENT_ID =
+ process.env.GA_MEASUREMENT_ID ?? (await configManager.getValue('gaMeasurementId'));
+ const GA_API_SECRET =
+ process.env.GA_API_SECRET ?? (await configManager.getValue('gaApiSecret'));
+ if (!GA_MEASUREMENT_ID || !GA_API_SECRET) return;
const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`;
const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`;
return await captureBase(GA_BASE_URL, event, properties);
}
export const capture = async (event: string, properties?: any) => {
- const GA_MEASUREMENT_ID = 'G-F3GK01G39Y'; // Replace with your GA4 Measurement ID
- const GA_API_SECRET = 'SqdcIAweSQS1RQErURMdEA'; // Replace with your GA4 API Secret
+ const GA_MEASUREMENT_ID =
+ process.env.GA_MEASUREMENT_ID ?? (await configManager.getValue('gaMeasurementId'));
+ const GA_API_SECRET =
+ process.env.GA_API_SECRET ?? (await configManager.getValue('gaApiSecret'));
+ if (!GA_MEASUREMENT_ID || !GA_API_SECRET) return;
const GA_BASE_URL = `https://www.google-analytics.com/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`;
const GA_DEBUG_BASE_URL = `https://www.google-analytics.com/debug/mp/collect?measurement_id=${GA_MEASUREMENT_ID}&api_secret=${GA_API_SECRET}`;
return await captureBase(GA_BASE_URL, event, properties);
}🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 257-257: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 265-265: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/capture.ts` around lines 255 - 269, The GA API secrets and
measurement IDs are hardcoded in capture_call_tool and capture; update both
functions to read GA_MEASUREMENT_ID and GA_API_SECRET (or a single config
object) from environment/config (e.g., process.env or shared config) and pass
them to captureBase, and if the values are missing, short‑circuit by returning
early or skipping the call (log a warning/error) to avoid sending requests with
empty secrets; ensure captureBase still receives the constructed GA_BASE_URL/G
A_DEBUG_BASE_URL from those env values and remove the plaintext constants.
| const mountPoint = parts[1]; | ||
| const fsType = parts[2]; | ||
| const options = parts[3].split(','); | ||
| const options = parts[3]; |
Contributor
There was a problem hiding this comment.
options.includes('ro') produces false positives — reverts a correct token-based check
errors=remount-ro is a standard ext4 mount option (meaning "remount read-only on I/O error") and is commonly present on read-write filesystems. Because the new code uses a raw substring search, any mount line containing that option will be misreported as read-only, causing DockerMount.readOnly to be true when the mount is actually writable. The previous split-then-includes approach was correct and should be restored.
Concrete false-positive example from a typical Linux bind-mount entry:
/dev/sda1 /workspace ext4 rw,relatime,errors=remount-ro,data=ordered 0 0
- Old:
['rw', 'relatime', 'errors=remount-ro', 'data=ordered'].includes('ro')→false✅ - New:
'rw,relatime,errors=remount-ro,data=ordered'.includes('ro')→true❌
🐛 Proposed fix
- const options = parts[3];
+ const options = parts[3].split(',');- const isReadOnly = options.includes('ro');
+ const isReadOnly = options.includes('ro');With options as a string[], the Array.prototype.includes call at line 252 is already correct — no change needed there. Only line 216 needs reverting.
Also applies to: 252-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/system-info.ts` at line 216, The mount-options string is being kept
as a raw comma-separated string (const options = parts[3]) which makes
options.includes('ro') produce false positives (matching substrings like
errors=remount-ro); change the code so options is an array by splitting the
options field on commas (e.g., parts[3].split(',')) and keep the existing
Array.prototype.includes('ro') check used when computing DockerMount.readOnly;
update any type annotations if needed so options is a string[] for the readOnly
logic in the DockerMount parsing functions.
Owner
This needs to be better, I wonder if we can do this on installation somehow
|
Owner
Proposal: OS-gating — Don't register macOS tools on Windows/LinuxThe macOS AX tools (12 tools) and Electron debug tools (3 tools) are macOS-only but currently always registered in Proposal: Gate these tools at registration time in function shouldIncludeTool(toolName: string): boolean {
if (toolName === "give_feedback_to_desktop_commander" && currentClient?.name === "desktop-commander") {
return false;
}
// macOS-only tools — don't register on Windows/Linux
if (process.platform !== "darwin" && (toolName.startsWith("macos_ax_") || toolName.startsWith("electron_debug_"))) {
return false;
}
return true;
}CodeRabbit already flagged this as a nitpick, but it's more than cosmetic — it's 15 unnecessary tool descriptions eating context window for the majority of users. |
Owner
Proposal: Smarter Electron debug connection —
|
Owner
Proposal: AX → CDP auto-fallback for Electron appsProblemWhen Proposal: Detect Electron apps and suggest CDP fallbackWhen AX returns empty elements for an app:
{
"elements": [],
"hint": "electron_app_detected",
"suggestion": "This appears to be an Electron app. AX tree is typically empty for Electron renderers. Use electron_debug_connect to interact with the app DOM via CDP instead."
}This saves the LLM from wasting calls on a dead end and points it to the right tool immediately. |
Owner
Proposal: Consolidate 12 AX tools into fewer, more focused toolsProblem12 separate AX tools is a lot of surface area. Each tool description consumes context window tokens, and the LLM has to choose between many similar-sounding options ( Proposal: Consolidate to ~4 tools
This reduces tool count by 8, saves ~2,000+ tokens of descriptions, and simplifies the LLM's decision-making. The This is a longer-term consideration — not necessarily for this PR, but worth planning for. |
Owner
Refined proposal: Lazy auto-build + permission prompt on first AX useRefining my previous proposals further based on discussion. Two separate triggers:1. Helper binary build — on first ANY tool call (background)
2. AX permissions — only when an AX tool is actually called
Why this is better than a setup tool
|
Owner
|
We decided to do it differently so closing it |
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
2 participants
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.
User description
Adds macOS Accessibility (AX) control as an MCP tool.
Includes native helper wiring, handlers, and schemas.
Prebuilt helper binaries are intentionally ignored.
CodeAnt-AI Description
Add macOS Accessibility automation and Electron debug tools to the MCP server
What Changed
Impact
✅ Automate macOS UI from MCP on macOS hosts✅ Inspect and run JavaScript inside Electron apps via CDP✅ Verify and diagnose Accessibility permission and helper readiness💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Removed Features
Documentation