|
| 1 | +# Claude Review Guide — appium-desktop-driver |
| 2 | + |
| 3 | +## Project Context |
| 4 | + |
| 5 | +This is a **Windows desktop UI automation Appium driver** (`NovaWindows`). It bridges WebDriver protocol to Windows UI Automation (UIA3) via a persistent PowerShell process. An MCP server layered on top exposes tools for AI agent use. |
| 6 | + |
| 7 | +Key stack: TypeScript, Node.js, Appium BaseDriver, PowerShell, koffi FFI (user32.dll), WebdriverIO (MCP client). |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Severity Format |
| 12 | + |
| 13 | +Use this format for findings: |
| 14 | + |
| 15 | +``` |
| 16 | +[BLOCKER] — Must be fixed before merge. Security or correctness issue. |
| 17 | +[HIGH] — Significant bug or reliability issue; should be fixed. |
| 18 | +[MEDIUM] — Non-critical issue worth addressing. |
| 19 | +[LOW] — Minor style, naming, or improvement suggestion. |
| 20 | +[INFO] — Observation or question, no action required. |
| 21 | +``` |
| 22 | + |
| 23 | +--- |
| 24 | + |
| 25 | +## Security Checklist |
| 26 | + |
| 27 | +### PowerShell Injection |
| 28 | +- [ ] User-supplied strings (capability values, element attributes, script arguments) are **never** interpolated raw into PowerShell strings |
| 29 | +- [ ] `executeScript` payloads that build PS commands use proper escaping or parameterized construction |
| 30 | +- [ ] Capability values used in pre/postrun scripts are validated and sanitized |
| 31 | + |
| 32 | +### FFI / native bindings |
| 33 | +- [ ] `user32.dll` calls in `lib/winapi/user32.ts` validate coordinate ranges and handle types before passing to native |
| 34 | +- [ ] koffi struct definitions match actual Windows API signatures |
| 35 | + |
| 36 | +### Secrets & credentials |
| 37 | +- [ ] No API keys, tokens, or passwords in source code or test fixtures |
| 38 | +- [ ] Capability values for app launch do not log sensitive data |
| 39 | + |
| 40 | +--- |
| 41 | + |
| 42 | +## Testing Standards |
| 43 | + |
| 44 | +- Unit tests live in `test/` and use **Vitest** |
| 45 | +- New utility functions in `lib/` should have corresponding unit tests |
| 46 | +- PowerShell condition builders (`lib/powershell/conditions.ts`, `converter.ts`) and XPath evaluator (`lib/xpath/`) are well-covered — changes here need tests |
| 47 | +- E2E tests require a real Windows environment; don't flag missing E2E coverage for pure logic changes |
| 48 | + |
| 49 | +--- |
| 50 | + |
| 51 | +## Architecture Rules |
| 52 | + |
| 53 | +### Session lifecycle |
| 54 | +- `createSession()` must start the PowerShell process cleanly |
| 55 | +- `deleteSession()` must kill the PS process and clear all session state (element cache, capabilities) |
| 56 | +- Any async work initiated during session must be awaited or cancelled on teardown |
| 57 | + |
| 58 | +### Element handles |
| 59 | +- Element IDs are ephemeral — they map to live UIA3 elements that can become stale |
| 60 | +- Code that caches element references must handle `ElementNotFound` / stale element gracefully |
| 61 | + |
| 62 | +### Command routing |
| 63 | +- All new driver commands must be exported from `lib/commands/index.ts` and follow the existing mixin pattern |
| 64 | +- MCP tools in `lib/mcp/tools/` must map cleanly to existing driver commands — avoid duplicating logic |
| 65 | + |
| 66 | +### Error handling |
| 67 | +- Driver errors must be wrapped in Appium error classes (e.g., `NoSuchElementError`, `InvalidArgumentError`) |
| 68 | +- Raw PowerShell stderr should not be surfaced verbatim to the WebDriver client |
| 69 | +- MCP tool errors should return structured error responses, not throw |
| 70 | + |
| 71 | +--- |
| 72 | + |
| 73 | +## Code Style |
| 74 | + |
| 75 | +- TypeScript strict mode is on — no `any` unless unavoidable and justified |
| 76 | +- Prefer `async/await` over raw Promise chains |
| 77 | +- `@/` path alias resolves to `lib/` — use it for imports within the library |
| 78 | +- Avoid adding unnecessary abstraction layers for single-use logic |
0 commit comments