Skip to content

refactor(macos): derive macOS surface routing from one owner table - #2886

Merged
thymikee merged 3 commits into
mainfrom
refactor/2880-macos-surface-owner
Sep 24, 2026
Merged

thymikee merged 3 commits into
mainfrom
refactor/2880-macos-surface-owner

Conversation

@thymikee

@thymikee thymikee commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds one owner table for "which backend serves a macOS surface":
MACOS_SURFACE_BACKENDS (packages/contracts/src/session-surface.ts,
satisfies Record<SessionSurface, MacOsSurfaceBackend>) with
macOsSurfaceBackend and a table-derived MacOsHelperSurface brand,
re-exported from @agent-device/contracts/session. Every platform-apple
routing site (snapshot, screenshot + --fullscreen refusal, readTextAtPoint,
press, runtime capture, native find admission) and the daemon crop
classifier now read it instead of restating surface !== 'app' locally.
Helper entry points now require a branded, owner-routed surface instead of
an optional one, closing off bypass call sites.

Behavior fix: the crop classifier called frontmost-app macos-app-window
(wrong — the helper captures it) and a surface-less macOS session
macos-helper (wrong — every route treats it as app). Both crop cells
stay rejected, so today's only visible effect is the refusal message's
target name; refusal code and details.reason are unchanged.

Closes #2880

17 files touched (16 plus a CHANGELOG line). Net production lines: +33 (issue estimate 0–15).

Validation

Rebased onto main at efce352650. Tested at d5e451d69f.

  • pnpm check:affected --run: exit 0, all runnable checks passed: format, lint, typecheck, layering, fallow, build, and 480 files / 3589 tests. An earlier run at load average 400 hit 5 s timeouts. Each timed-out file passed alone, and settle-observation failed the same way on origin/main.
  • The focused suites for contracts, platform-apple (interactor, helper, runtime), the daemon crop
    classifier and the operation host all pass: 98 tests. The loader refusal test now asserts the
    message too. Changing the guard's text fails both refusal cases.
  • Earlier, at 086d44eb5e: live macOS desktop/menubar/frontmost-app/Calculator sessions via CLI,
    with --debug route evidence. The frontmost-app capture was confirmed on the helper, and the crop
    refusal text was checked before and after the fix. The surface-less macOS row cannot be reached
    from the CLI and is unit-tested only. The code on those routes has not changed since then.

Owner-table typecheck evidence (#2880)

Run with tsc -b packages/xml packages/kernel packages/contracts packages/platform-apple after adding a fifth
member 'window' to SESSION_SURFACES.

Unclassified (no MACOS_SURFACE_BACKENDS entry): production and the test literal maps fail.

packages/contracts/src/session-surface.ts(23,12): error TS2741: Property 'window' is missing in type '{ readonly app: 'xctest'; readonly 'frontmost-app': 'macos-helper'; readonly desktop: 'macos-helper'; readonly menubar: 'macos-helper'; }' but required in type 'Record<"app" | "desktop" | "frontmost-app" | "menubar" | "window", MacOsSurfaceBackend>'.
packages/contracts/src/session-surface.ts(27,10): error TS7053: Element implicitly has an 'any' type because expression of type '"app" | "desktop" | "frontmost-app" | "menubar" | "window"' can't be used to index type '{ readonly app: "xctest"; ... }'.
packages/contracts/src/session-surface.ts(31,26): error TS2536: Type 'S' cannot be used to index type '{ readonly app: "xctest"; ... }'.
packages/contracts/src/session-surface.test.ts(10,7): error TS2741: Property 'window' is missing in type '{ app: "xctest"; ... }' but required in type 'Record<"app" | "desktop" | "frontmost-app" | "menubar" | "window", MacOsSurfaceBackend>'.
packages/platform-apple/src/__tests__/interactor-macos-surface.test.ts(64,7): error TS2741: Property 'window' is missing ...
packages/platform-apple/src/runtime.test.ts(404,7): error TS2741: Property 'window' is missing ...

Classified as window: 'xctest': production compiles. Only the test literal maps fail, because they
restate the expected table on purpose.

packages/contracts/src/session-surface.test.ts(10,7): error TS2741: Property 'window' is missing in type '{ app: "xctest"; 'frontmost-app': "macos-helper"; desktop: "macos-helper"; menubar: "macos-helper"; }' but required in type 'Record<"app" | "desktop" | "frontmost-app" | "menubar" | "window", MacOsSurfaceBackend>'.
packages/platform-apple/src/__tests__/interactor-macos-surface.test.ts(64,7): error TS2741: Property 'window' is missing ...
packages/platform-apple/src/runtime.test.ts(404,7): error TS2741: Property 'window' is missing ...

The daemon crop test's literal map (src/, root tsc -p tsconfig.json) has the same
Record<SessionSurface, MacOsSurfaceBackend> shape. I did not run that step for this experiment.

Remaining risk: the fix is latent until a macOS crop cell is flipped to
accepted.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.81 MB 4.81 MB +235 B
Package (unpacked) 4.80 MB 4.81 MB +235 B
Package (download) 1.44 MB 1.44 MB +48 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.2 ms 26.9 ms -0.3 ms
CLI --help 81.5 ms 79.7 ms -1.8 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 086d44e. The routing logic looks sound. HelperRoutedSurface is derived from the owner table, so a fifth SESSION_SURFACES member should fail typecheck until it is classified. #2880 asks the PR description to show that typecheck output (once unclassified, once classified as xctest). Could you paste it into the PR body?

Could the owner table, macOsSurfaceBackend, the crop fix and the literal-map tests do the job at about half the size, without the brand type? Or does the brand need to be this explicit to stop new unrouted helper calls? A short note on why the smaller design was rejected is enough.

Not blocking: the crop refusal now names a different target for frontmost-app and for a surface-less session, with no CHANGELOG line (add one only if refusal text counts as user-visible). The refusal case at src/platform-runtime-operation-host.test.ts:88 checks only toBeInstanceOf(TypeError), not the message.

I did not run typecheck or tests, and I did not repeat the live macOS runs; I rely on the reported results.

Smoke Tests was still running at review time. If it is the macOS job, it covers this diff directly (interactor.ts, interactions.ts, runtime-snapshot.ts), so a failure there needs a look at its log. The iOS, Android and Linux jobs do not reach these macOS-only sites.

@thymikee
thymikee force-pushed the refactor/2880-macos-surface-owner branch from 086d44e to d5e451d Compare September 24, 2026 16:33
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the 11:36Z review. Rebased onto main at efce352650, which includes #2902. New head: d5e451d69f.

  • Typecheck output for a fifth surface (Derive macOS surface routing from one owner table (the crop classifier misreads frontmost-app and a missing surface) #2880): in the PR body under "Owner-table typecheck evidence". Unclassified 'window': session-surface.ts fails with TS2741/TS7053/TS2536, and the three test literal maps fail with TS2741. Classified as xctest: production compiles, and only the test literal maps fail.
  • Half the size without the brand? I measured it. The brand costs 3 production lines (unique symbol, the intersection, and the doc line). The cast in macOsHelperSurface stays either way, because TS cannot narrow through a table lookup. The derived HelperRoutedSurface union alone already makes drift unrepresentable: with the brand removed and window: 'xctest' added, a restated surface !== 'app' narrowing passed to runMacOsScreenshotAction fails (Type '"window"' is not assignable to type 'HelperRoutedSurface'). With the four current surfaces, that restatement compiles. So the brand does not add drift safety. It makes macOsHelperSurface() the only way to construct a helper surface, which is the Derive macOS surface routing from one owner table (the crop classifier misreads frontmost-app and a missing surface) #2880 goal ("read the owner, do not restate surface !== 'app'"). Dropping it fails only the two @ts-expect-error bare-literal cases in helper.test.ts. There is no "half the size" design: the rest of the diff is call-site rewiring and table tests, and both designs need them. I kept the brand. Say so if you want it gone: the change is 3 lines plus those 2 test directives.
  • Refusal test at platform-runtime-operation-host.test.ts:88: now also asserts the message. Mutation check: changing the guard's text fails both cases.
  • CHANGELOG: added a Changed (macos) line for the frontmost-app --crop-on refusal target name (code and details.reason unchanged). I left out the surface-less row because the CLI cannot reach it.
  • Smoke Tests (iOS): the failure was wait for Automation lab → APP_NOT_RUNNING (wait_capture_stalled). That is the deep-link-launch signature test(ios): wait out the launch an accepted deep-link confirmation releases #2902 fixes, and test(ios): wait out the launch an accepted deep-link confirmation releases #2902 was not on this branch's base. The iOS/Android/Linux jobs do not reach the macOS sites this PR changes. The rebase brings the fix in.

Gate on d5e451d69f: exit 0, all runnable checks passed: format, lint, typecheck, layering, fallow, build, and 480 files / 3589 tests. An earlier run at load average 400 hit 5 s timeouts. Each timed-out file passed alone, and settle-observation failed the same way on origin/main.

…wner

Add a SessionSurface-keyed MACOS_SURFACE_BACKENDS table in contracts with
macOsSurfaceBackend and a table-derived MacOsHelperSurface brand. The
platform-apple snapshot, screenshot, readText, press, runtime capture and
native find sites, and the daemon crop classifier, now read it.

The crop classifier now reports frontmost-app as macos-helper (the helper
captures it) and a surface-less macOS session as macos-app-window (every
route serves it through the runner). Helper entry points require an
owner-routed surface.

Fixes #2880
@thymikee
thymikee force-pushed the refactor/2880-macos-surface-owner branch from d5e451d to f828155 Compare September 24, 2026 16:39
@thymikee

Copy link
Copy Markdown
Member Author

Routing now derives from the one owner table, checked against f828155. The satisfies Record<SessionSurface, ...> constraint plus the indexing sites cover a fifth surface at compile time, so the 086d44e ask is closed.

CI is green on all 18 checks, and the delta since that review is one test-only commit in src/platform-runtime-operation-host.test.ts.

I did not rerun the fifth-surface tsc experiment myself; the closure above is inferred from the constraint and the indexing sites, so does that inference hold if a surface is added without adding an indexing site, or is the satisfies clause alone enough to fail the build? The daemon crop test's literal map under the root tsconfig is a test map, not production, and the author has already said it wasn't exercised.

Live macOS validation is carried over from 086d44e. The routing code is byte-identical since then per the range-diff, so nothing here needed a fresh run at this commit. The surface-less macOS row still has no CLI-reachable path and is covered by unit tests only — is that gap intentional, or worth one integration test so it isn't unit-only forever?

Nothing here blocks merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 24, 2026
@thymikee
thymikee merged commit fa1082d into main Sep 24, 2026
18 checks passed
@thymikee
thymikee deleted the refactor/2880-macos-surface-owner branch September 24, 2026 17:42
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-24 17:44 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Derive macOS surface routing from one owner table (the crop classifier misreads frontmost-app and a missing surface)

1 participant