Skip to content

refactor(daemon): separate open-target policy from platform mechanics - #2416

Merged
thymikee merged 5 commits into
mainfrom
claude/vigilant-johnson-t2hhxk
Sep 9, 2026
Merged

refactor(daemon): separate open-target policy from platform mechanics#2416
thymikee merged 5 commits into
mainfrom
claude/vigilant-johnson-t2hhxk

Conversation

@thymikee

@thymikee thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member

Summary

Closes #2334.

src/platform-runtime-open-target.ts mixed neutral open-plan policy with Android
platform mechanics (real adb-backed package resolution). This moves
resolveAndroidPackageForOpen / inferAndroidPackageAfterOpen behind the Android
owning seam in packages/platform-android, and simplifies
resolveSessionAppBundleIdForTarget to lazily reach Android mechanics itself
instead of taking an injected resolver callback — removing a dead-code stub
(async () => undefined) at the Apple/macOS call site and keeping exactly one
construction path for the open plan.

session-open-prepare.ts and session-selector-dispatch.ts now import only the
neutral plan/policy surface from platform-runtime-open-target.ts. The two R76
inventory edges are reclassified from leaked-platform-mechanics to
daemon-policy-essential, dropping the daemon-to-root platform-runtime edge count
from 14 to 13. ADR 0022 is updated to record the outcome.

Changes

  • New packages/platform-android/src/open-target-resolution.ts (+ colocated test)
    owns resolveAndroidPackageForOpen / inferAndroidPackageAfterOpen, exported via
    mechanics.ts.
  • platform-runtime-open-target.ts: Android mechanics removed;
    resolveSessionAppBundleIdForTarget drops its injected-callback parameter and
    lazily loads Android mechanics itself (still swallows a load/resolution failure,
    matching prior behavior).
  • platform-runtime-android-application-tools.ts / platform-runtime-apple-application-tools.ts
    / session-selector-dispatch.ts updated to the simplified call signature.
  • scripts/layering/daemon-platform-runtime-inventory.ts (+ test): reclassified
    both edges, added a planted-violation regression test proving a reintroduced
    Android-mechanics import on the selector-dispatch edge stays red.
  • ~7 daemon test files migrated their resolveAndroidPackageForOpen mock from
    platform-runtime-open-target.ts to @agent-device/platform-android/mechanics.
  • Renamed an unrelated private helper in packages/platform-android/src/app-lifecycle.ts
    (resolveAndroidPackageForOpen → requireAndroidPackageForOpen) to remove a
    same-name/different-contract collision with the newly exported function.

Validation

  • pnpm check:layering — green (13 daemon-to-root edges, R76 holds; new planted
    Android-mechanics-reintroduction test passes).
  • pnpm check:di-seams — green.
  • pnpm lint, pnpm typecheck, pnpm format:check — green.
  • pnpm check:affected --run — unit project: 2118/2118 tests pass; replay-compat
    fails only because this sandbox is a shallow clone with no tags (pre-existing
    environment limitation, unrelated to this change).
  • Full unit-core sweep of src/daemon/** + packages/platform-android/**:
    3298/3300 pass; the 2 failures (app-log-session-resource.test.ts,
    durable-capture-resource.test.ts) reproduce identically on unmodified main.
  • Adversarial review pass (fable model): no correctness bugs found; addressed its
    low-severity findings (exception-swallowing parity, the naming collision above,
    and the planted-violation test).

🤖 Generated with Claude Code

https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD

Generated by Claude Code

@github-actions

github-actions Bot commented Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.51 MB 4.51 MB +593 B
Package (unpacked) 4.51 MB 4.51 MB +593 B
Package (download) 1.34 MB 1.34 MB +269 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.8 ms 28.2 ms +0.4 ms
CLI --help 76.9 ms 78.5 ms +1.5 ms

@thymikee thymikee mentioned this pull request Sep 9, 2026
Closed
@thymikee

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Please preserve the no-target fast return in inferOpenedAppBundleId. It now loads Android mechanics before the helper can return for an absent target, so a loader failure can turn a fresh targetless Android open into an error instead of leaving the optional package identity unset. Keep loading inside the best-effort operation and add a targetless regression with the loader rejecting. This also overlaps #2408 on #2334; these are alternative implementations, not two changes to merge. Checks are green on a0ad24c.

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Good catch, fixed in 8cd9a0f. The move from loadOpenTarget() (a near-infallible root module) to loadAndroidMechanics() (real adb-backed host binding) meant inferOpenedAppBundleId's unconditional load was no longer safe to leave unguarded — a load failure would throw instead of leaving the app-bundle identity unset. Wrapped the load+delegate in try/catch so it degrades to the current bundle id on failure, matching the pre-refactor behavior, and added a regression test that mocks loadAndroidMechanics to reject on a targetless open and asserts the call still resolves instead of throwing.

Noted on the #2408 overlap — that's a separate implementation of the same issue, not something to merge alongside this one.

Generated by Claude Code

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

The "Smoke Tests" (iOS) check failed on 8cd9a0f: RunnerTests.testAlertAcceptDoesNotActivateAReplacementWithASharedButton, a native XCUITest for alert-handling UI behavior. This isn't this PR's failure — the diff here only touches TypeScript Android package-resolution/open-target code (src/platform-runtime-open-target.ts, packages/platform-android/**, daemon selector-dispatch/open-prepare); it doesn't touch iOS runner or alert-handling code at all. Re-ran the failed job once (run 34340690624) to rule out a one-off flake; will follow up if it fails again on the re-run.

Generated by Claude Code

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Re-run confirms it was a flake — Smoke Tests passed on the second attempt. All checks are green and the PR is mergeable.

Generated by Claude Code

@thymikee

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

The loader failure is handled correctly at 8cd9a0f, and CI is green. I found no remaining correctness blocker; skipping the unnecessary import when identity is already known would be a small optimization, not a required fix. This and #2408 are alternatives for #2334, so choose one before marking it ready to merge.

@claude
refactor(daemon): separate open-target policy from Android mechanics
358b3aa 
Move resolveAndroidPackageForOpen/inferAndroidPackageAfterOpen behind
the Android owning seam in packages/platform-android. resolveSessionAppBundleIdForTarget
now lazily reaches Android mechanics itself instead of taking an
injected resolver function, so open-prepare and selector-dispatch
import only the neutral open plan/result surface from
platform-runtime-open-target.ts. Reclassifies the two R74 inventory
edges to daemon-policy-essential and updates ADR 0022.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD
@claude
refactor: address adversarial review findings on open-target seam
919becb 
Restore try/catch around the Android-mechanics lazy load so a module
load failure still resolves to undefined instead of throwing. Rename
the unrelated private resolveAndroidPackageForOpen in app-lifecycle.ts
to requireAndroidPackageForOpen to remove the naming collision with
the new exported function. Add a planted-violation regression test
for reintroducing Android mechanics on the selector-dispatch edge.
Tighten ADR/inventory wording that overstated which files consume the
neutral resolver.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD
@claude
fix(platform-android): keep the mechanics facade lazy for the new ope…
8235280 
…n-target exports

resolveAndroidPackageForOpen/inferAndroidPackageAfterOpen were re-exported
statically from mechanics.ts, which eagerly evaluates open-target-resolution.ts
on import and tripped the eager-closure-budgets gate (177 -> 178 modules).
Wrap them as lazy async functions, matching the existing pattern used for
listAndroidAppsWithAdb/captureAndroidLogcatWithAdb in the same file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD
@claude
fix(android-tools): keep inferOpenedAppBundleId best-effort on a mech…
03f3564 
…anics load failure

Loading Android mechanics moved from the near-infallible root
platform-runtime-open-target.ts to the real adb-backed mechanics
module, but the wrapper call stayed unguarded. A loader failure now
throws instead of leaving the app-bundle identity unset, even for a
targetless open that never needed the loaded module. Wrap the load
and delegate in try/catch so it degrades to the current bundle id,
matching the pre-refactor behavior, and add a regression test with
the loader rejecting on a targetless open.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD
@thymikee

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

This now conflicts with main at 8cd9a0f after the lifecycle changes landed. Resolve the conflict and rerun the affected checks once we choose between this and #2408.

@claude
perf(android-tools): skip loading Android mechanics when app-bundle i…
696df77 
…dentity is known

inferOpenedAppBundleId always loaded Android mechanics before
delegating, even when currentAppBundleId already made the delegate's
own fast-return a no-op. Check it first so the load is skipped
entirely once the identity is already known, and add a regression
test asserting the loader is never called in that case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0167UVzrdzVMCZqXgxtzWoTD
@thymikee
thymikee force-pushed the claude/vigilant-johnson-t2hhxk branch from 8cd9a0f to 696df77 Compare September 9, 2026 14:43
@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main at 9d7d60c (696df77). The ADR conflict was in §2.2/§4: merged #2333's "done" writeup (from the landed lifecycle-participation change) with this PR's open-target-planning writeup rather than picking one side. No code conflicts.

Also applied the optional optimization you mentioned: inferOpenedAppBundleId now checks currentAppBundleId before loading Android mechanics at all, so a known identity skips the load entirely instead of loading-then-short-circuiting inside the delegate. Added a regression test asserting the loader is never called in that case.

CI is re-running on 696df77.

Generated by Claude Code

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

"Smoke Tests" (Linux) failed on 696df77: the "Install Linux desktop dependencies" step (apt-get install) timed out after 6 minutes, before any test ran — pure CI infrastructure flake, unrelated to this diff. Re-ran the failed job once (run 34365510670).

Generated by Claude Code

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Re-run confirms it was infra flake — all checks green on 696df77, rebased onto latest main, mergeable.

Generated by Claude Code

@thymikee

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

The conflict resolution at 696df77 preserves the lazy Android boundary and the targetless/current-identity behavior. The regressions exercise those paths and all checks are green. This is the version I would choose for #2334; it overlaps with #2408, so choose this one and supersede the other before merging both accidentally.

@thymikee Claude

thymikee commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Closed #2408 as superseded by this PR. This one is ready to merge — head 696df77, all checks green, mergeable, rebased onto latest main.

Generated by Claude Code

@thymikee
thymikee merged commit 342e98c into main Sep 9, 2026
18 of 19 checks passed
@thymikee
thymikee deleted the claude/vigilant-johnson-t2hhxk branch September 9, 2026 15:35
@thymikee thymikee mentioned this pull request Sep 9, 2026
Open
8 tasks
@github-actions

github-actions Bot commented Sep 9, 2026

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

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor(daemon): separate open-target planning from platform mechanics

2 participants

@thymikee @claude