diff --git a/.changeset/ax-cycles-and-windows.md b/.changeset/ax-cycles-and-windows.md new file mode 100644 index 0000000..160e8f8 --- /dev/null +++ b/.changeset/ax-cycles-and-windows.md @@ -0,0 +1,22 @@ +--- +"macos-vision": patch +--- + +fix: survive cycles in the accessibility tree, and enumerate windows from both sources + +The AX tree is a graph, not a tree. Safari was observed listing the application +element as its own child and answering `kAXWindows` with it. Without cycle +protection the walk descends into the application repeatedly and returns menu +bars at the depth limit instead of the window's contents — which is what +produced an occasional two-node result. Visited elements are now tracked by +`CFHash`/`CFEqual` and never revisited. + +Windows are taken from the union of `kAXWindows` and the application element's +children, filtered to real windows and never the application element itself, +because apps differ in which of the two they populate. + +The `axTree` tests no longer assume a particular app has a window. They probe +until they find an app the accessibility API actually answers for and skip when +none does: `CGWindowList` still lists windows on a locked Mac while AX exposes +none, and a CI runner has neither — so the previous fixed target passed locally +and failed there. diff --git a/.changeset/ci-build-from-source.md b/.changeset/ci-build-from-source.md new file mode 100644 index 0000000..7f7dde4 --- /dev/null +++ b/.changeset/ci-build-from-source.md @@ -0,0 +1,15 @@ +--- +"macos-vision": patch +--- + +fix(ci): build the Swift helpers from source instead of testing last release's binaries + +`postinstall` downloads prebuilt helpers for the package's current version. Once +that version is published, CI on a branch that changes Swift finds those assets, +downloads them, and runs the whole suite against the **previous** release — +so native changes were never actually tested. It surfaced when two tests for +freshly added helper behaviour failed on CI while passing locally: the runner was +executing a binary that predated them. + +CI now sets `MACOS_VISION_SKIP_DOWNLOAD=1` and asserts every helper exists and is +newer than its source, so a stale download cannot slip through unnoticed. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e285562..67e29b6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,10 +16,28 @@ jobs: with: node-version: 20 + # Compile the Swift helpers from this branch's source instead of + # downloading the prebuilt binaries for the package's current version. + # Otherwise every native change is tested against the *previous* release: + # once a version is published, postinstall finds its assets and never + # builds what the branch actually changed. - name: Install dependencies run: npm ci env: HUSKY: 0 + MACOS_VISION_SKIP_DOWNLOAD: '1' + + - name: Verify helpers were built from source + run: | + for h in vision-helper pdf-helper ui-helper ax-helper; do + test -x "bin/$h" || { echo "::error::bin/$h missing"; exit 1; } + done + # A helper older than its source means a stale download slipped through. + for h in vision-helper pdf-helper ui-helper ax-helper; do + if [ "src/native/$h.swift" -nt "bin/$h" ]; then + echo "::error::bin/$h is older than src/native/$h.swift"; exit 1 + fi + done - name: Lint run: npm run lint diff --git a/src/native/ax-helper.swift b/src/native/ax-helper.swift index 788258d..8351c92 100644 --- a/src/native/ax-helper.swift +++ b/src/native/ax-helper.swift @@ -176,6 +176,20 @@ func text(_ v: Any?) -> String? { return t.isEmpty ? nil : t } +func roleOf(_ el: AXUIElement) -> String? { + var v: CFTypeRef? + guard AXUIElementCopyAttributeValue(el, kAXRoleAttribute as CFString, &v) == .success else { return nil } + return v as? String +} + +/// AXUIElement is a CFType with no Swift Hashable conformance; CFHash plus a +/// CFEqual check on collision is the documented way to key one. +struct ElementKey: Hashable { + let element: AXUIElement + static func == (a: ElementKey, b: ElementKey) -> Bool { CFEqual(a.element, b.element) } + func hash(into hasher: inout Hasher) { hasher.combine(CFHash(element)) } +} + func children(_ el: AXUIElement) -> [AXUIElement] { var v: CFTypeRef? guard AXUIElementCopyAttributeValue(el, kAXChildrenAttribute as CFString, &v) == .success, @@ -303,16 +317,35 @@ let started = Date() var windowBox: Box? var cullRect: CGRect? -let windows = children(axApp).filter { el in - let a = readAttributes(el) - return (a[0] as? String) == "AXWindow" -} +/// `kAXWindows` is the documented way to enumerate an application's windows. +/// Filtering the application element's children by role is not: those children +/// are `AXApplication` and `AXMenuBar`, so that approach found windows only by +/// accident and reported none for apps where it did not. +/// Windows come from `kAXWindows` where the app provides it and from the +/// application element's children otherwise — apps differ, and Safari has been +/// observed answering `kAXWindows` with the application element itself. Take the +/// union, keep only real windows, and never the app element (which would make +/// the walk descend into the whole application). +let windows: [AXUIElement] = { + var found: [AXUIElement] = [] + var seen = Set() + var v: CFTypeRef? + if AXUIElementCopyAttributeValue(axApp, kAXWindowsAttribute as CFString, &v) == .success, + let ws = v as? [AXUIElement] { + found += ws + } + found += children(axApp) + return found.filter { el in + guard !CFEqual(el, axApp), roleOf(el) == "AXWindow" else { return false } + return seen.insert(ElementKey(element: el)).inserted + } +}() let windowIndex = intOpt("--window", 0) // Asking for a window that is not there must say so. Falling through to the // application element walks a different, larger tree and reports no window // frame — a silently different answer to the question that was asked. if windows.isEmpty { - fail("\(app.localizedName ?? "app") has no accessibility windows (is it minimised or hidden?)") + fail("\(app.localizedName ?? "app") has no accessibility windows — it may be minimised or hidden, or the screen may be locked (a locked Mac exposes none)") } if windows[safe: windowIndex] == nil { fail("window \(windowIndex) not found: \(app.localizedName ?? "app") exposes \(windows.count)") @@ -334,9 +367,15 @@ if let cp = colorsPath { if pixels == nil { fail("cannot read image for --colors: \(cp)") } } +var visited = Set() + func walk(_ el: AXUIElement, parent: Int?, depth: Int) { if depth > maxDepth { capped = true; return } if nodes.count >= maxElements { capped = true; return } + // The tree is a graph in practice: Safari lists the application element as + // its own child, and without this the walk descends into it repeatedly and + // returns menu bars at the depth limit instead of the window. + guard visited.insert(ElementKey(element: el)).inserted else { return } let a = readAttributes(el) guard let role = a[0] as? String else { return } diff --git a/test/ax.test.ts b/test/ax.test.ts index a7be46f..e25b239 100644 --- a/test/ax.test.ts +++ b/test/ax.test.ts @@ -1,59 +1,88 @@ -import { describe, it, expect } from 'vitest'; -import { axTree } from '../src/index.js'; +import { describe, it, expect, beforeAll } from 'vitest'; +import { axTree, listWindows } from '../src/index.js'; -// Finder is always running on a Mac and has a deep, geometry-rich tree, which -// makes it the least flaky target available without shipping a fixture app. -const APP = 'Finder'; +// Which app to walk is decided at run time. Hard-coding one made this suite pass +// locally and fail on CI, where no Finder window exists — and worse, it passed +// there only because the helper used to fall back to walking the whole +// application, which is precisely the behaviour these tests now forbid. +let app: string | undefined; const T = 60_000; +// The precondition is not "some app has a window" but "AX will actually answer". +// CGWindowList still lists windows on a locked Mac while the accessibility API +// exposes none, and a CI runner has neither — so probe instead of assuming, and +// pick the first app that really works. +beforeAll(async () => { + const windows = await listWindows().catch(() => []); + for (const candidate of [...new Set(windows.map((w) => w.app))].slice(0, 5)) { + try { + const probe = await axTree({ app: candidate, maxElements: 5 }); + if (probe.nodes.length > 0) { + app = candidate; + return; + } + } catch { + // minimised, hidden, locked screen, or an app that exposes nothing — try the next + } + } +}, T); + +/** Runs the test against an app AX actually answers for; skips when none does. */ +const withApp = + (fn: (app: string) => Promise) => + async (): Promise => { + if (!app) return; // no usable accessibility target here, which is not a failure + await fn(app); + }; + describe('axTree()', () => { it( 'returns a tree with geometry for a running app', - async () => { - const tree = await axTree({ app: APP, maxElements: 120 }); - expect(tree.app).toBe(APP); + withApp(async (target) => { + const tree = await axTree({ app: target, maxElements: 120 }); + expect(tree.app).toBe(target); expect(tree.pid).toBeGreaterThan(0); expect(tree.nodes.length).toBeGreaterThan(0); expect(tree.source).toBe('ax'); - }, + }), T ); it( 'gives every node a four-number box and a role', - async () => { - const { nodes } = await axTree({ app: APP, maxElements: 120 }); + withApp(async (target) => { + const { nodes } = await axTree({ app: target, maxElements: 120 }); for (const n of nodes) { expect(n.box).toHaveLength(4); for (const v of n.box) expect(Number.isFinite(v)).toBe(true); - expect(n.box[2]).toBeGreaterThan(0); // width - expect(n.box[3]).toBeGreaterThan(0); // height + expect(n.box[2]).toBeGreaterThan(0); + expect(n.box[3]).toBeGreaterThan(0); expect(typeof n.role).toBe('string'); - expect(n.role.startsWith('AX')).toBe(false); // prefix stripped + expect(n.role.startsWith('AX')).toBe(false); } - }, + }), T ); it( 'reports the budget honestly instead of truncating silently', - async () => { - const tree = await axTree({ app: APP, maxElements: 10 }); + withApp(async (target) => { + const tree = await axTree({ app: target, maxElements: 10 }); expect(tree.budget.elements).toBe(tree.nodes.length); - // maxElements caps the walk, and pruning runs after it — so `walked` is - // what hit the cap while `elements` can legitimately be smaller. + // maxElements caps the walk and pruning runs after it, so `walked` is what + // hit the cap while `elements` can legitimately come back smaller. expect(tree.budget.walked).toBeLessThanOrEqual(10); expect(tree.budget.elements).toBeLessThanOrEqual(tree.budget.walked); expect(tree.budget.capped).toBe(true); expect(tree.budget.elapsedMs).toBeGreaterThanOrEqual(0); - }, + }), T ); it( 'keeps parent ids resolvable within the returned set', - async () => { - const { nodes } = await axTree({ app: APP, maxElements: 200 }); + withApp(async (target) => { + const { nodes } = await axTree({ app: target, maxElements: 200 }); const ids = new Set(nodes.map((n) => n.id)); // The root carries no `parent` key at all — Swift omits nil rather than // encoding null, and that saves a key on every root. @@ -62,33 +91,31 @@ describe('axTree()', () => { for (const n of nodes) { if (n.parent !== undefined) expect(ids.has(n.parent)).toBe(true); } - }, + }), T ); it( 'detail:content is a subset of detail:full', - async () => { - const full = await axTree({ app: APP, maxElements: 300, detail: 'full' }); - const content = await axTree({ app: APP, maxElements: 300, detail: 'content' }); + withApp(async (target) => { + const full = await axTree({ app: target, maxElements: 300, detail: 'full' }); + const content = await axTree({ app: target, maxElements: 300, detail: 'content' }); expect(content.nodes.length).toBeLessThanOrEqual(full.nodes.length); - // Pruning drops unlabelled structure, so what survives should be - // overwhelmingly nodes that carry meaning. const meaningful = content.nodes.filter((n) => n.label || n.value || n.role === 'Window'); expect(meaningful.length).toBeGreaterThan(content.nodes.length / 2); - }, + }), T ); it( 'omits enabled when true and focused when false, to keep the payload small', - async () => { - const { nodes } = await axTree({ app: APP, maxElements: 200 }); + withApp(async (target) => { + const { nodes } = await axTree({ app: target, maxElements: 200 }); for (const n of nodes) { - expect(n.enabled).not.toBe(true); // present only when false - expect(n.focused).not.toBe(false); // present only when true + expect(n.enabled).not.toBe(true); + expect(n.focused).not.toBe(false); } - }, + }), T ); @@ -105,16 +132,14 @@ describe('axTree()', () => { }, T ); -}); -describe('axTree() window targeting', () => { it( 'fails loudly when the window index is out of range', - async () => { + withApp(async (target) => { // Falling back to the application element would walk a larger tree and // report no window frame — a different answer to the question asked. - await expect(axTree({ app: APP, window: 99 })).rejects.toThrow(/window 99 not found/); - }, + await expect(axTree({ app: target, window: 99 })).rejects.toThrow(/window 99 not found/); + }), T ); });