Fix bug where AXe isn't bundled for Smithery installs, also improve AXe detection logic #164
Conversation
cameroncooke
force-pushed
the
fix-axe-bundle-smithery
branch
from
f2b6b82 to
13f64f0
Compare
WalkthroughThis pull request enhances AXe binary resolution and Smithery bundle verification. Changes include: adding a CI workflow step to verify Smithery bundles; creating documentation on AXe binary configuration via environment variables (XCODEBUILDMCP_AXE_PATH, AXE_PATH); adding a verification script and npm script for bundle validation; refactoring bundle-axe.sh to skip macOS-specific verification on other platforms; modifying smithery.config.js to ensure pre-build AXe bundling; updating the doctor tool to dynamically resolve AXe from environment variables, bundled paths, or system PATH; introducing new helper functions for multi-source AXe resolution; and updating test expectations and user-facing error messages to reflect flexible AXe configuration rather than bundled-only assumptions. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @docs/investigations/issue-163.md:
- Line 14: Update the sentence "Smithery packaging omits bundled binaries and
server does not fallback to system axe." by changing the verb "fallback" to the
two-word verb form "fall back" so it reads "server does not fall back to system
axe", ensuring correct grammar in the hypothesis line.
🧹 Nitpick comments (4)
docs/CONFIGURATION.md (1)
60-62: Specify language for fenced code block.The fenced code block should specify
bashorshellas the language identifier for proper syntax highlighting and rendering.🔎 Proposed fix
-``` +```bash XCODEBUILDMCP_AXE_PATH=/opt/axe/bin/axe</details> </blockquote></details> <details> <summary>smithery.config.js (1)</summary><blockquote> `27-32`: **Enhance error message with actionable guidance.** The error message at line 31 could provide more context to help users diagnose and resolve the issue. <details> <summary>🔎 Proposed fix</summary> ```diff } else { - throw new Error(`AXe bundle missing at ${bundledAxePath}`); + throw new Error( + `AXe bundle missing at ${bundledAxePath}. ` + + `The bundling script may have failed. Run 'bash scripts/bundle-axe.sh' manually to diagnose.` + ); }src/mcp/tools/ui-testing/__tests__/key_press.test.ts (1)
91-103: Consider extracting the mock AXe helpers to a shared test utility.The
mockAxeHelpersobject withcreateAxeNotAvailableResponseis duplicated across many tests in this file (and likely other test files). Extracting this to a shared test utility would reduce duplication and ensure consistent error message expectations across the test suite.This is a minor refactor suggestion that could be addressed in a follow-up PR.
scripts/bundle-axe.sh (1)
147-163: Good cross-platform handling for CI builds.The macOS-only verification is sensible since Linux CI runners cannot execute macOS binaries. The warning message clearly indicates that verification was skipped.
One minor observation:
AXE_VERSIONis reused on line 159/162 for a different purpose (detected version from binary) than its original use (input version from environment on line 22). While this works correctly since the original value is no longer needed after line 29, consider using a distinct variable name likeDETECTED_AXE_VERSIONfor clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/ci.yml.smithery/index.cjsdocs/CONFIGURATION.mddocs/TROUBLESHOOTING.mddocs/investigations/issue-163.mdpackage.jsonscripts/bundle-axe.shscripts/verify-smithery-bundle.shsmithery.config.jssrc/mcp/tools/doctor/lib/doctor.deps.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/utils/axe-helpers.tssrc/utils/axe/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript file extensions (.ts) for all relative imports to ensure compatibility with native TypeScript runtimes
Never use .js extensions for internal file imports; always use .ts extensions
Use .js extensions only for external package imports (e.g., @modelcontextprotocol/sdk/server/mcp.js)
Files:
src/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/utils/axe/index.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/doctor/lib/doctor.deps.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/utils/axe-helpers.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx}: Enforce strict Dependency Injection (DI) testing philosophy - completely ban the use of Vitest mocking utilities (vi.mock(), vi.fn(), vi.spyOn(), etc.)
Use injectable executors (CommandExecutor and FileSystemExecutor) for all external interactions instead of mocking
Import core logic functions from tool files and pass mock executors to simulate different test outcomes
Files:
src/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
When Claude Code is detected, automatically consolidate multiple content blocks into a single text response separated by --- dividers to work around MCP specification violation
Files:
src/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/utils/axe/index.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/doctor/lib/doctor.deps.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/utils/axe-helpers.ts
🧠 Learnings (5)
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to src/tools/**/*.ts : Organize tools into directories by their functionality and use automatic loading via plugin-based MCP architecture
Applied to files:
package.jsonsrc/mcp/tools/doctor/lib/doctor.deps.ts
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.{ts,tsx} : Use .js extensions only for external package imports (e.g., modelcontextprotocol/sdk/server/mcp.js)
Applied to files:
package.json
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.test.{ts,tsx} : Import core logic functions from tool files and pass mock executors to simulate different test outcomes
Applied to files:
src/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/doctor/lib/doctor.deps.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/utils/axe-helpers.ts
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Set XCODEBUILDMCP_ENABLED_WORKFLOWS environment variable to a comma-separated list of workflow directory names to limit the toolset; session-management workflow is always auto-included
Applied to files:
docs/CONFIGURATION.md
📚 Learning: 2025-07-22T19:54:12.588Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: example_projects/iOS/.cursor/rules/errors.mdc:0-0
Timestamp: 2025-07-22T19:54:12.588Z
Learning: Please don't fix any code errors unless reported by XcodeBuildMCP server tool responses.
Applied to files:
docs/CONFIGURATION.md
🧬 Code graph analysis (2)
src/mcp/tools/doctor/lib/doctor.deps.ts (1)
src/utils/axe-helpers.ts (1)
resolveAxeBinary(89-106)
src/utils/axe-helpers.ts (2)
src/mcp/tools/logging/stop_device_log_cap.ts (1)
existsSync(253-259)src/utils/axe/index.ts (4)
resolveAxeBinary(7-7)getAxePath(3-3)areAxeToolsAvailable(5-5)createAxeNotAvailableResponse(2-2)
🪛 LanguageTool
docs/investigations/issue-163.md
[grammar] ~14-~14: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...ts bundled binaries and server does not fallback to system axe. Findings: Issue repo...
(NOUN_VERB_CONFUSION)
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...on:** Confirmed. Missing bundled binary blocks all UI automation and simulator video c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~35-~35: Possible missing comma found.
Context: ...ctor can report axe dependency present but UI automation unsupported. ### 2026-01...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~40-~40: The official name of this software platform is spelled with a capital “H”.
Context: ...sh. Evidence: package.json:21-44, .github/workflows/release.yml:48-55, `smithery...
(GITHUB)
[uncategorized] ~46-~46: The official name of this software platform is spelled with a capital “H”.
Context: ...README.md:11-74, smithery.yaml:1-3, .github/workflows/release.yml:48-62, `.gitigno...
(GITHUB)
[misspelling] ~51-~51: Did you mean “Side-effect” (=adverse effect, unintended consequence)? Open compounds are not hyphenated.
Context: ... fail when custom plugins are supplied. Side-effect bundling in smithery.config.js avoids...
(AFFECT_EFFECT)
[uncategorized] ~68-~68: The preposition ‘of’ seems more likely in this position.
Context: ...irements and verify bundled/ presence in Smithery artifacts during CI.
(AI_HYDRA_LEO_REPLACE_IN_OF)
🪛 markdownlint-cli2 (0.18.1)
docs/CONFIGURATION.md
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (26)
package.json (1)
27-27: LGTM! Script addition follows conventions.The new verification script follows the existing naming pattern and is appropriately placed with other verification scripts. Ensure the script file
scripts/verify-smithery-bundle.shhas executable permissions in the repository.src/utils/axe/index.ts (1)
7-7: LGTM! Export follows guidelines.The new export correctly uses the
.tsextension for the internal import, adhering to the project's coding guidelines. The addition ofresolveAxeBinaryprovides a clean public API for dynamic AXe resolution.docs/TROUBLESHOOTING.md (1)
28-33: Excellent troubleshooting guidance!The new section provides clear, actionable steps for users encountering missing AXe errors. The documentation covers all resolution paths (bundled artefacts, environment variables, and installation), and appropriately directs users to verify with the doctor tool.
.github/workflows/ci.yml (1)
35-37: LGTM! Well-positioned CI verification step.The new verification step is logically placed after the Smithery build and before linting. This ensures that bundled artefacts are correctly packaged before proceeding with code quality checks. The step appropriately validates the fix for issue #163.
src/mcp/tools/ui-testing/__tests__/swipe.test.ts (1)
23-23: LGTM! Consistent error messaging updates.The updated error messages provide comprehensive guidance for users, including installation instructions and environment variable configuration. The test structure correctly follows the DI philosophy by using injectable executors without Vitest mocking utilities, as per the project's coding guidelines.
Also applies to: 40-40, 436-436
smithery.config.js (1)
21-25: Verify platform compatibility for bash script execution.The script invokes
bashdirectly viaexecFileSync, which will fail on Windows systems. Whilst the AXe bundling script (bundle-axe.sh) is likely macOS-specific, ensure this limitation is acceptable for all Smithery installation targets.If Smithery installations are expected to support Windows or other non-Unix platforms, consider adding platform detection or documenting macOS-only requirements.
scripts/verify-smithery-bundle.sh (1)
1-26: LGTM!The verification script follows bash best practices with proper error handling (
set -euo pipefail), clear validation logic, and informative error messages. The three-tier validation (AXe binary presence, Frameworks directory existence, framework count) ensures bundle integrity effectively.src/mcp/tools/ui-testing/__tests__/tap.test.ts (2)
13-44: LGTM! Improved error messaging.The updated error messages provide more actionable guidance to users, replacing generic "reinstall" advice with specific installation commands and environment variable configuration options. The test expectations correctly reflect the new AXe resolution behaviour introduced across the PR.
791-965: LGTM! Consistent test coverage for dependency errors.All six test cases correctly validate the DependencyError scenario when AXe is unavailable, ensuring consistent error messaging across various execution paths (success with null path, failure scenarios, exceptions).
src/mcp/tools/ui-testing/__tests__/gesture.test.ts (1)
321-354: LGTM! Consistent error messaging.The test expectations correctly reflect the new AXe not-found error message, maintaining consistency with other UI-testing tools. The updated guidance provides users with actionable steps for resolving AXe availability issues.
src/mcp/tools/ui-testing/__tests__/button.test.ts (1)
321-354: LGTM!The updated error message is more actionable and user-friendly, providing clear installation instructions and Smithery-specific guidance. The test correctly validates the new AXe-not-found messaging behaviour using dependency injection as per the coding guidelines.
src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts (1)
117-150: LGTM!The DependencyError test case correctly validates the updated AXe-not-found messaging, consistent with other UI-testing test files.
docs/investigations/issue-163.md (1)
1-68: Excellent investigation documentation.This investigation document provides valuable context for the fix, including clear root cause analysis and actionable recommendations. This serves as a good architectural decision record for future reference.
src/mcp/tools/ui-testing/__tests__/long_press.test.ts (1)
341-383: LGTM!The test correctly validates the updated AXe-not-found messaging. The mock setup appropriately uses the detailed error message for the test case that specifically validates this error scenario.
src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts (1)
345-378: LGTM!The DependencyError test case correctly validates the updated AXe-not-found messaging. The test structure follows the DI philosophy as per the coding guidelines.
src/mcp/tools/ui-testing/__tests__/type_text.test.ts (1)
313-313: LGTM! Updated error messaging improves user experience.The test expectations now reflect the improved AXe-not-found error message that provides actionable installation guidance (Homebrew, environment variables) and Smithery-specific troubleshooting, which is a significant improvement over the previous bundled-only assumption.
Also applies to: 391-391
src/mcp/tools/doctor/lib/doctor.deps.ts (1)
101-121: LGTM! Dynamic AXe resolution properly integrated into doctor diagnostics.The doctor now correctly delegates to
resolveAxeBinary()to locate AXe from multiple sources (env, bundled, PATH) and attempts version detection. The fallback message when version info is unavailable is appropriate, and the error handling (try-catch with silent ignore) ensures robustness.src/mcp/tools/ui-testing/__tests__/touch.test.ts (1)
137-137: LGTM! Consistent error messaging across UI testing suite.All test expectations have been updated to reflect the improved AXe-not-found error message, providing users with clear installation and configuration guidance. The changes are consistent with updates across the UI-testing test suite.
Also applies to: 187-187, 237-237, 289-289, 380-380, 402-402, 418-418, 456-456, 517-517, 560-560, 603-603, 643-643, 665-665, 686-686, 727-727, 770-770, 813-813
src/utils/axe-helpers.ts (8)
15-22: Well-structured type definitions for multi-source AXe resolution.The new constants and types clearly define the resolution sources and provide type safety for the binary resolution flow.
37-44: LGTM! Proper executable check with platform-appropriate error handling.The
isExecutablehelper correctly usesaccessSyncwithconstants.X_OKto verify execution permissions, with appropriate try-catch for platforms or paths where the check might fail.
46-56: Environment variable resolution correctly prioritises user configuration.The function iterates through the defined environment variables and returns the first valid executable path found. The use of
resolve()ensures absolute paths, which is correct.
77-87: PATH resolution correctly implements cross-platform binary search.The function uses
process.env.PATHand the platform-appropriatedelimiterto search for the AXe binary. The use ofisExecutableensures the binary found on PATH is actually executable.
89-106: Well-designed resolution cascade with clear source tracking.The
resolveAxeBinaryfunction implements a sensible precedence order: environment variables (user override) → bundled (package-provided) → PATH (system installation). Returning both the path and source is excellent for diagnostics and troubleshooting.
111-112: Simplified implementation correctly delegates to new resolver.The
getAxePathfunction now leveragesresolveAxeBinaryand extracts just the path, maintaining backward compatibility whilst benefiting from the improved resolution logic.
133-135: Improved error messaging provides actionable guidance.The updated error message gives users concrete steps: Homebrew installation command, environment variable configuration, and Smithery-specific troubleshooting. This is a significant improvement over the previous bundled-only assumption.
58-75: Comprehensive bundled path resolution with multiple fallback candidates.The function checks multiple potential bundled locations (entry-relative, process.cwd-relative) to account for various deployment scenarios. Using
existsSync()rather thanisExecutable()is appropriate here since the function only needs to verify path existence; execute permissions are managed by the packaging and installation process.
| ## Investigation Log | ||
|
|
||
| ### 2026-01-06 - Initial Assessment | ||
| **Hypothesis:** Smithery packaging omits bundled binaries and server does not fallback to system axe. |
There was a problem hiding this comment.
Minor grammar issue: "fallback" should be "fall back".
The word "fallback" is a noun, but here it's used as a verb. Consider changing to "does not fall back to system axe".
🔎 Suggested fix
-**Hypothesis:** Smithery packaging omits bundled binaries and server does not fallback to system axe.
+**Hypothesis:** Smithery packaging omits bundled binaries and server does not fall back to system axe.📝 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.
| **Hypothesis:** Smithery packaging omits bundled binaries and server does not fallback to system axe. | |
| **Hypothesis:** Smithery packaging omits bundled binaries and server does not fall back to system axe. |
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...ts bundled binaries and server does not fallback to system axe. Findings: Issue repo...
(NOUN_VERB_CONFUSION)
🤖 Prompt for AI Agents
In @docs/investigations/issue-163.md at line 14, Update the sentence "Smithery
packaging omits bundled binaries and server does not fallback to system axe." by
changing the verb "fallback" to the two-word verb form "fall back" so it reads
"server does not fall back to system axe", ensuring correct grammar in the
hypothesis line.
commit: |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| for (const candidate of candidates) { | ||
| if (existsSync(candidate)) { | ||
| return candidate; | ||
| } |
There was a problem hiding this comment.
Bug: The resolveBundledAxePath function doesn't check if the binary is executable. The build process might strip execute permissions, causing failures when the binary is run.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The resolveBundledAxePath function only checks for the existence of the axe binary using existsSync but omits a check for execute permissions. The build process, specifically in smithery.config.js, uses cpSync without the preserve: true option, which can strip the execute permissions set by bundle-axe.sh. Consequently, when tools like doctor.ts attempt to run the binary located by resolveBundledAxePath, the execution will fail, breaking diagnostics for users who installed via Smithery. This is inconsistent with other resolver functions in the same file that do check for executability.
💡 Suggested Fix
Add an isExecutable() check within resolveBundledAxePath to ensure the binary has execute permissions before returning its path. This aligns it with the behavior of resolveAxePathFromEnv and resolveAxePathFromPath. Alternatively, or in addition, add { preserve: true } to the cpSync call in smithery.config.js.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/axe-helpers.ts#L68-L72
Potential issue: The `resolveBundledAxePath` function only checks for the existence of
the `axe` binary using `existsSync` but omits a check for execute permissions. The build
process, specifically in `smithery.config.js`, uses `cpSync` without the `preserve:
true` option, which can strip the execute permissions set by `bundle-axe.sh`.
Consequently, when tools like `doctor.ts` attempt to run the binary located by
`resolveBundledAxePath`, the execution will fail, breaking diagnostics for users who
installed via Smithery. This is inconsistent with other resolver functions in the same
file that do check for executability.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 523137
Fixes issue #163
Note
Strengthens AXe availability and packaging, fixing Smithery installs and aligning detection across the app.
env -> bundled -> PATH) inutils/axe-helpers.tsand re-exports inutils/axescripts/bundle-axe.shandscripts/verify-smithery-bundle.sh; newsmithery.config.jsbuilds and copiesbundled/into.smitherybundle:axe, build Smithery bundle, and verify AXe artifacts; removetsupbuild steps from workflowsWritten by Cursor Bugbot for commit 6e5621b. This will update automatically on new commits. Configure here.