feat: support precise location settings

[anupamchugh]

Summary

• add settings location set <lat> <lon> for precise location coordinates
• route coordinates through CLI/client/daemon/dispatch settings flow
• implement iOS simulator simctl location set and Android emulator adb emu geo fix
• update command docs and targeted tests

Fixes #94.

Verification

• pnpm exec vitest run src/platforms/android/__tests__/index.test.ts src/platforms/ios/__tests__/index.test.ts src/utils/__tests__/args.test.ts src/__tests__/cli-client-commands.test.ts
• pnpm check:quick
• pnpm check:unit

[thymikee]

Review

Read end-to-end (CLI → client → daemon → dispatch → platform), checked against #94, and skimmed CI. The feature works at the unit level and docs are updated, but there are a few things worth addressing before merge plus a couple of follow-ups for code health.

Does it fix #94?

Mostly. Acceptance criteria status:

• Set latitude/longitude for current session device — covered for iOS sim + Android emulator
• Altitude — issue explicitly calls this out as optional ("and optionally altitude if supported"); PR doesn't accept it. simctl location set accepts the extended lat,lon,alt,hAccuracy,vAccuracy,course,speed,timestamp form, so it's a missed opportunity. OK as a follow-up if scoped tightly.
• iOS simulator + Android emulator — yes
• Document physical device support — Android: explicit UNSUPPORTED_OPERATION with a hint (good). iOS: relies on the pre-existing ensureSimulator(device, 'settings') guard, so physical devices fall back to a generic message — fine, but worth a one-liner in the docs.
• Skill updated — issue acceptance mentions "Docs and skill updated". PR updates commands.md and quick-start.md but I don't see a corresponding skill file change. Worth confirming whether the skill manifests need a refresh too.

Possible bug — iOS simctl argument shape

await runSimctl(device, ['location', device.id, 'set', String(latitude), String(longitude)]);

xcrun simctl location <device> set is documented to take a single coordinate argument formatted <latitude>,<longitude> (with optional trailing ,alt,hAccuracy,...), not two separate positional args. The iOS unit test mocks xcrun and just records argv, so it doesn't catch this — it confirms what we passed, not what simctl accepts. Please verify on a real booted simulator that two-arg form actually applies the coordinate (it's possible recent xcrun is permissive, but I'd want to see it work end-to-end). If simctl rejects it, the fix is [\${lat},${lon}`]` as a single token.

Code quality / maintenance

These are the bigger-picture concerns; happy to take them as follow-ups rather than blockers if the simctl shape checks out.

1. Coordinate validation is duplicated four times with the same finite/range logic and slightly different error strings:

   • src/cli/commands/generic.ts → readCoordinate
   • src/core/dispatch.ts → readLocationCoordinate
   • src/platforms/ios/apps.ts → requireLocationCoordinates
   • src/platforms/android/settings.ts → requireLocationCoordinates

   Extract one helper (next to the type, or in utils/) and reuse it. Right now changing the bounds or message means touching four sites and remembering all of them.

2. PermissionSettingOptions is now a misnomer. It carries latitude/longitude despite the name. Either rename to something neutral (SettingOptions) or split into a discriminated union that mirrors SettingsUpdateOptions. Otherwise future readers will reasonably assume non-permission fields don't belong here, and it'll keep accreting unrelated payload.

3. Positional encoding between daemon and dispatch is getting fragile. In snapshot-settings.ts:

   [setting, state, latitude, longitude, '', appBundleId ?? '']

   The empty string at index 4 exists purely to push appBundleId to index 5 to match dispatch.ts's expectations. And in dispatch.ts:

   const [setting, state, target, mode] = positionals;

   target/mode now do double-duty as either permissionTarget/permissionMode or latitude/longitude based on setting. Each new sub-command will need another branch in the appBundleId index lookup and another reinterpretation of the slots. A structured payload (named object) for settings sub-commands would be more robust — worth a follow-up to flatten before this grows further.

4. CLI test only covers happy path. Add a negative test for invalid lat/lon coming from the CLI (out-of-range, non-numeric) — the validation lives in readCoordinate but isn't exercised by cli-client-commands.test.ts.

5. CI is red: "Fallow Code Quality" and "deploy-preview" jobs are failing. Worth a look before merge — they may be unrelated infra noise but the failures should at least be triaged.

Suggested follow-ups (non-blocking)

• Consolidate coordinate validation into one helper.
• Rename PermissionSettingOptions → SettingOptions (or split per-sub-command).
• Replace the positional-string protocol between daemon and dispatch with a structured payload.
• Add altitude support to round out Expose precise location coordinates (simctl location / adb emu geo) #94.

Verdict

Functionally close to landing once the simctl argument format is verified on a real simulator (or fixed if needed). The validation duplication and PermissionSettingOptions naming are real maintenance debt but acceptable as follow-ups. No regressions to existing location on/off, permission, or other settings paths in the diff — the new branch is gated behind state === 'set' everywhere.

---

Generated by Claude Code

[anupamchugh]

Addressed the review items in the latest branch updates.\n\nChanges pushed:\n- Fixed iOS simctl location argv to use the documented single lat,lon token.\n- Consolidated coordinate validation into a shared helper.\n- Renamed PermissionSettingOptions to SettingOptions.\n- Removed the daemon/dispatch placeholder positional for location set.\n- Added negative CLI coordinate tests.\n- Updated the agent-device skill routing note for location workflows.\n- Triaged CI: deploy preview now skips fork PRs, iOS smoke replays clean daemon state first, and Fallow baselines were refreshed for the PR branch.\n\nLocal checks passed:\n- pnpm check:quick\n- pnpm check:fallow --base 7d47c6796aafa5957cd27d2fbc17c4b8dcf68a73\n- Targeted Vitest command covering CLI, iOS, Android, and snapshot settings: 204 tests passed.\n\nGitHub Actions are currently blocked as action_required for this fork PR and need repository admin/maintainer approval before they can run.

[thymikee]

Re-review (commit a6fb0d3)

Re-read the affected files end-to-end. The author addressed everything substantive from the prior review. Summary by point:

Confirmed fixes

• iOS simctl format — src/platforms/ios/apps.ts:443 now passes a single comma-joined token: [\location`, device.id, `set`, `${latitude},${longitude}`]. Matches the documented simctl location set ,` shape. ✅
• Validation consolidated — New src/utils/location-coordinates.ts exposes readLocationCoordinate and requireLocationCoordinates. All four prior copies are gone; CLI (generic.ts), dispatch (dispatch.ts), iOS (apps.ts), and Android (settings.ts) all import from the shared module. The min/max derivation from the label inside the helper is a nice touch. ✅
• PermissionSettingOptions → SettingOptions — renamed in permission-utils.ts and propagated to iOS, Android, and interactor-types.ts. Names now reflect what the type actually carries. ✅
• Daemon/dispatch placeholder positional — snapshot-settings.ts no longer emits the empty-string slot; both permission and location set produce 5 positionals ([setting, state, A, B, appBundleId]) with a brief comment documenting the layout. The slot reuse (target/mode doubling as either permissionTarget/permissionMode or latitude/longitude) still exists, but it's better contained now and the comment explicitly flags it. Acceptable. ✅
• Negative CLI tests — added in cli-client-commands.test.ts, covering both an out-of-range latitude (91) and a non-numeric longitude (not-a-number). ✅
• Skill update — skills/agent-device/SKILL.md adds a router note pointing at the installed settings help for location workflows. Consistent with the rest of the file's "router only" pattern. ✅
• CI — all required checks green on the latest run; deploy-preview is now correctly skipped for fork PRs. ✅

Remaining nits (non-blocking, defer)

1. dispatch.ts:handleSettingsCommand — the appBundleId ternary has two identical branches:

   setting === 'permission'
     ? (positionals[4] ?? context?.appBundleId)
     : setting === 'location' && state === 'set'
       ? (positionals[4] ?? context?.appBundleId)  // same RHS
       : (positionals[2] ?? context?.appBundleId);

   Collapse to a single check (extendedLayout = permission || location-set). Trivial.

2. Diagnostic emission still uses target/mode in the settings_apply log payload, which now means coordinates show up under those labels for location-set. Not security-sensitive, just slightly misleading in traces. Could swap to a discriminated payload when convenient.

3. Altitude (issue Expose precise location coordinates (simctl location / adb emu geo) #94 optional bullet) — still not implemented. Worth tracking as a follow-up issue rather than expanding scope here.

Verdict

LGTM to merge once a maintainer is comfortable with the remaining nits being follow-ups. The simctl shape was the only correctness concern and it's fixed; the rest of the diff is cleaner than v1 (shared helper, accurate type name, no padding string), tests cover the validation surface, and CI is clean.

---

Generated by Claude Code