Skip to content

feat: add human takeover controls - #2078

Merged
thymikee merged 10 commits into
callstack:mainfrom
szdziedzic:szdziedzic-codex/feat-human-takeover
Aug 29, 2026
Merged

feat: add human takeover controls#2078
thymikee merged 10 commits into
callstack:mainfrom
szdziedzic:szdziedzic-codex/feat-human-takeover

Conversation

@szdziedzic

@szdziedzic szdziedzic commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a human-control hold for a remotely leased simulator or device. The agent session stays open, read-only diagnostics remain available, and mutating commands fail with DEVICE_IN_USE / details.reason: "human_control_active" until all holds on that device end.

agent-device takeover --session remote-session
# The foreground command renews the hold; Ctrl+C releases it.
agent-device takeover status --session remote-session
agent-device takeover release <hold-id> --session remote-session

The remote session must already own a device-scoped lease, for example through connect proxy and open.

Ownership and admission

  • LeaseRegistry owns human-control state using the same backend/provider/device key as lease contention. There is no second device-identity alias set or persisted hold store.
  • Lease-owner takeover operations are ordinary daemon RPCs admitted through request-admission.ts. Their target comes only from the admitted lease; a tenant cannot retarget a hold or modify another owner's hold.
  • Hold renewal, expiry, lease preservation, and final-release refresh are one registry-owned lifecycle. A hold can coexist with the existing session and lease.
  • Pending activation blocks new mutations and drains already-admitted mutations before reporting active. This keeps the guarantee even when a fresh session has no advisory device execution lock.
  • If a tenant RPC or host PUT disconnects while activation is draining, the registry removes only that request's pending hold. Successor and unrelated holds remain intact; admitted mutations are not canceled. Active holds retain their TTL/explicit-release lifecycle.
  • Read/mutation classification comes from existing recordingEffect resolvers, observation-class inventory, and observability semantics, with explicit takeover/heartbeat exemptions. Unknown requests are mutations; no separate per-command humanControlEffect taxonomy remains.
  • Provider-host administration remains a separate capability: PUT /admin/human-control/holds/<id> accepts an exact lease device scope and requires the local daemon token. Tenant credentials do not grant it, and the proxy does not forward /admin/*.

Deliberate scope limits

Holds do not survive daemon restart, matching current lease durability. Controllers must reconnect and re-establish the hold. Local cross-daemon takeover is deferred; a future host-global fence must coexist with the session's existing device claim.

The unrelated macOS OCR/consent handling and workflow prewarming changes have been removed. There is no native or workflow diff against the rebased upstream base. CLI help, remote/security docs, and ADR 0007 describe the new ownership and durability contract; no skills changed.

The final diff spans 55 files because this command crosses CLI/client contracts, daemon admission, lease ownership, and their tests. Registry scope normalization was extracted while keeping mutable lease/hold state in the existing registry. Retaining a separate takeover registry or relying only on advisory locks would reintroduce the ownership and activation races this redesign removes.

Net production growth is 1,001 lines: 339 for registry lifecycle/scope handling, 207 for the host HTTP capability, 157 for CLI/help wiring, 95 for client contracts and methods, and 203 for shared command/admission integration. The growth buys the two authority paths and one lease-owned lifecycle; tests and documentation are excluded from those counts.

Validation

Evidence is stamped to 6d43be22d9e3489b114d63af0158cf8cbceaa93a against upstream base 69001179b04b45bb1529350a94bc0e5b2b1efdc3. The review follow-up and full canonical size report contain the detailed results.

pnpm check:affected --run passed end-to-end in a clean checkout of this exact commit after a frozen-lockfile install and build. Its origin/main points at the actual PR base, avoiding this fork's stale tracking ref without changing the original checkout's refs or any gate rules. The related unit/provider run passed 3,533 tests across 419 files. Node integration passed 90 tests with nine expected live-device skips. All locally runnable catalog gates passed, including the history-backed ratchet, package checks, wire/replay compatibility, documentation, and XCTest selection.

Cancellation regressions: real tenant-RPC and host-PUT clients disconnect during an admitted mutation, both without ttlMs. The pending hold must disappear before the mutation finishes, never activate later, and allow subsequent mutation admission. Both tests were observed red before the fix. A planted unconditional-cleanup violation also made the successor-hold regression fail. The focused lease/takeover/HTTP suite passed 46 tests; it also covers lease refresh, already-aborted activation, and abort-listener cleanup. ADR 0007 and the remote-proxy documentation now state the cancellation contract.

Live remote-proxy proof: built CLI → authenticated loopback proxy → isolated daemon → iPhone 17 Pro simulator (iOS 26.5). With Settings open, takeover reported active; press label=General --settle failed with DEVICE_IN_USE / human_control_active, while snapshot -i succeeded. Ctrl+C released the hold, status returned no holds, and the same press succeeded; a snapshot containing About confirmed navigation to General. The session stayed open throughout. It was then closed, the proxy disconnected, the isolated daemon stopped, and the simulator returned to its original shut-down state. This verifies the remote-proxy protocol on one Mac, not a cloud-provider or tunnel path.

Size: the unmodified canonical report was run on fresh base/head builds with generated package assets and seven startup samples per revision on the same macOS arm64 host. Compared with the base: +14,819 B raw JS, +5,851 B gzipped JS, +5,259 B npm tarball, and +15,514 B npm-unpacked content; native packaged components are unchanged. This supplies the report skipped by the fork-only CI condition.

Published and reported, not yet claimed merge-ready: CI on the pushed head remains authoritative for coverage, provider, and native/device lanes. No internal hosting-service implementation details are included.

Copilot AI lite review requested due to automatic review settings August 27, 2026 11:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “human takeover” mechanism to the agent-device daemon/CLI so a person can temporarily pause state-changing automation on a specific device (while keeping read-only diagnostics and preserving the active lease), with both a foreground CLI workflow and a host-local authenticated admin API.

Changes:

  • Introduces a human-control hold registry (persisted state + TTL) and integrates it into daemon request execution to block mutations with HUMAN_CONTROL_ACTIVE.
  • Adds a host-local /admin/human-control/holds/* HTTP API (daemon-token authenticated, explicitly not forwarded by the proxy) plus a socket-only internal command for CLI control.
  • Documents the workflow and updates tests/fixtures to validate gating, proxy isolation, and lease preservation/refresh behavior.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
website/docs/docs/security-trust.md Documents auth boundary for /admin/human-control/* and persistence location/permissions.
website/docs/docs/remote-proxy.md Adds host-local takeover and admin API guidance; clarifies proxy does not forward /admin/*.
website/docs/docs/commands.md Adds takeover command docs and expected behavior during holds.
test/skillgym/suites/agent-device-smoke-suite.ts Adds SkillGym guidance case for planning the host-local takeover command.
src/utils/cli-command-overrides.ts Adds takeover help/usage overrides and supported flags.
src/utils/tests/args.test.ts Asserts takeover help text and schema behavior is exposed as expected.
src/kernel/errors.ts Adds HUMAN_CONTROL_ACTIVE code, retriable classification, and default hint.
src/daemon/server/http-server.ts Wires in human-control HTTP route handling and blocks human-control RPC over HTTP.
src/daemon/server/daemon-runtime.ts Instantiates HumanControlRegistry, persists it, and protects/refreshes leases around holds.
src/daemon/request-router.ts Threads human-control deps into request execution scopes and handler chain.
src/daemon/request-handler-chain.ts Adds specialized humanControl handler route and dispatch.
src/daemon/request-execution-scope.ts Wraps admitted execution in human-control gating for mutation requests.
src/daemon/lease-registry.ts Adds lease-protection predicate and refresh-by-deviceKey for post-hold recovery.
src/daemon/human-control.ts Implements hold registry (persistence, TTL cleanup, mutation gating, idle wait).
src/daemon/human-control-request.ts Resolves device aliases per request to apply human-control gating consistently.
src/daemon/human-control-http.ts Implements loopback admin HTTP API for holds (list/upsert/remove + token auth).
src/daemon/human-control-contract.ts Defines hold contract types and input parsing/validation.
src/daemon/http-errors.ts Maps HUMAN_CONTROL_ACTIVE to HTTP 423.
src/daemon/handlers/human-control.ts Implements socket-only internal command handler for hold management.
src/daemon/daemon-command-registry.ts Adds humanControl route and human-control “effect” classification per command.
src/daemon/tests/request-handler-catalog.test.ts Ensures command catalog routing includes the humanControl specialized route.
src/daemon/tests/lease-registry.test.ts Tests lease protection/refresh behavior during and after human control.
src/daemon/tests/human-control.test.ts Tests persistence/TTL expiry and mutation blocking semantics.
src/daemon/tests/human-control-request.test.ts Tests daemon request gating behavior and socket management command flow.
src/daemon/tests/human-control-http.test.ts Tests admin HTTP API auth, malformed inputs, and end-to-end gating via HTTP daemon.
src/daemon/tests/daemon-command-registry.test.ts Tests per-command human-control effects (read/mutate/control) and fail-closed behavior.
src/core/command-descriptor/registry.ts Classifies commands with human-control effects and routes internal human-control command.
src/command-catalog.ts Adds internal human_control command and local takeover CLI command classification.
src/cli/parser/cli-help.ts Adds help text describing host-local takeover and admin API, including proxy isolation.
src/cli/commands/takeover.ts Implements the takeover CLI command (foreground hold + status/release + HTTP fallback).
src/cli/commands/router.ts Registers takeover handler in the CLI router.
src/cli.ts Ensures takeover stays host-local (no remote auth/materialization/defaults).
src/tests/takeover-command.test.ts Tests human-facing takeover output rendering.
src/tests/daemon-proxy.test.ts Verifies proxy does not expose /admin/human-control/* routes.
CONTEXT.md Adds domain vocabulary entry for human-control holds and updates daemon registry description.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/daemon/human-control-http.ts Outdated
@thymikee

Copy link
Copy Markdown
Member

We merged a bigger refactor so this will need a rebase

@thymikee

Copy link
Copy Markdown
Member

[P1] Keep the remote lease alive under the same device identity as takeover

Foreground takeover stores device.id, while a proxy lease is keyed as platform:target:device.id. Request admission carries both aliases, so it blocks mutations, but lease protection and release-time refresh compare only the hold’s exact key. The normal proxy lease can therefore expire while takeover is active and will not be refreshed on release, contradicting the stated active-lease guarantee. Make human control use one owning canonical identity or declared alias set shared by lease and request routing, and add a proxy-key-versus-device-id expiry and release-refresh regression.

This head is also not reviewable for readiness: GitHub reports DIRTY/conflicting on the same request/runtime routing surfaces and has no current CI. Rebase onto current main, integrate the ADR-0019 routing changes, then rerun the relevant checks and host/device evidence. Finally, this 35-file, +1,793/-66 slice exceeds the size-review threshold but the PR does not itemize justified growth or explain why a materially smaller design was rejected. Record that design decision before requesting readiness.

@szdziedzic
szdziedzic force-pushed the szdziedzic-codex/feat-human-takeover branch from 89cc568 to 0da824a Compare August 27, 2026 14:56
Copilot AI review requested due to automatic review settings August 27, 2026 14:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.

Comment thread src/core/command-descriptor/registry.ts Outdated
Comment thread website/docs/docs/remote-proxy.md Outdated
@szdziedzic
szdziedzic force-pushed the szdziedzic-codex/feat-human-takeover branch from 0da824a to 676dfa7 Compare August 27, 2026 15:10
Copilot AI review requested due to automatic review settings August 27, 2026 15:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/core/command-descriptor/registry.ts:1481

  • The viewport command uses setViewport(...) in the runtime (a state-changing operation), but its descriptor marks it as ...HUMAN_CONTROL_READ. This would allow viewport resizes during an active human-control hold, contradicting the intended “pause state-changing agent commands” behavior.
    catalog: { group: 'public' },
    frameworkTier: 'extended',
    recordsSessionAction: true,
    recordingEffect: 'mutates-app',
    daemon: { route: 'generic', refFrameEffect: 'may-invalidate', ...HUMAN_CONTROL_READ },
    timeoutPolicy: DEFAULT_TIMEOUT_POLICY,
    batchable: false,
    platformExecution: { kind: 'device-runtime', uses: [viewportRuntimeUse] },

@thymikee

Copy link
Copy Markdown
Member

Not ready at 676dfa74.

[P1] Foreground takeover stores bare device.id, but proxy leases are keyed platform:target:device.id; request admission sees aliases, while lease expiry protection and release refresh compare only the exact lease key. A hold can therefore block commands yet still let the active proxy lease expire. Use one canonical device identity/shared alias set at the human-control and lease boundary, with a real composite-key expiry/refresh regression.

[P1] TTL starts in upsert() before existing mutation work drains. A short TTL can expire and be cleaned up while takeover is pending, admit a new mutation, then return state: active for a hold no longer registered. Model pending drain as non-expirable or arm TTL only after activation, with a planted short-TTL/long-inflight test.

[P2] Remove the newly added implementation narration under current main’s comment rule.

This adds ~1,238 net production lines, including 297/300-line modules, without itemized growth or a rejected-smaller-design rationale; the duplicated identity handling above is evidence for a deeper owner seam. CI is still incomplete and no real device/simulator/emulator takeover evidence is recorded.

@szdziedzic
szdziedzic force-pushed the szdziedzic-codex/feat-human-takeover branch from 676dfa7 to 8a7ce5d Compare August 27, 2026 15:36
Copilot AI review requested due to automatic review settings August 27, 2026 15:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/core/command-descriptor/registry.ts:1478

  • viewport resizes the web viewport (a state-changing operation), but it is currently classified as humanControlEffect: read, which means it will still run during a human-control hold and can race the human operator. This should be treated as a mutation so it is paused during takeover.
    recordsSessionAction: true,
    recordingEffect: 'mutates-app',
    daemon: { route: 'generic', refFrameEffect: 'may-invalidate', ...HUMAN_CONTROL_READ },

src/daemon/human-control-http.ts:35

  • tryHandleHumanControlHttpRoute calls resolveHumanControlRoute, which uses new URL(req.url, ...). If the raw request URL is malformed (e.g. invalid percent-encoding), new URL can throw synchronously and take down the daemon because this is outside the async handler's try/catch. Please guard route parsing so a bad request can't crash the server.
}): boolean {
  const route = resolveHumanControlRoute(params.req);
  if (!route) return false;
  void handleHumanControlRoute(route, params);
  return true;

Comment thread src/core/command-descriptor/registry.ts
@thymikee

Copy link
Copy Markdown
Member

Not ready. The new CLI tests are useful, but the production blockers remain: hold ownership is still stored as a bare device id while leases use a composite identity, and TTL begins before activation so a pending takeover can expire before it becomes active. Also, viewport is classified through HUMAN_CONTROL_READ even though it mutates web state, and malformed absolute/network target URLs can throw from new URL(...) outside the HTTP error boundary. Remove implementation-history narration, itemize the roughly 1.2k net production-line growth and explain why a smaller owning interface was rejected, add practical live takeover evidence, and clear or base-compare the exact-head iOS/macOS failures.

Copilot AI review requested due to automatic review settings August 27, 2026 16:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings August 27, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thymikee

thymikee commented Aug 27, 2026

Copy link
Copy Markdown
Member

Not ready at dd6e53d. The alias/activation, viewport, malformed-URL, and comment-policy fixes are sound. However, macOS has a PR-owned failure: 190 XCTests execute while source-derived selection declares 189 because the new host-permission test is guarded as generic unit-test code instead of macOS-only. Correct its platform classification and rerun. The red iOS replay retains attempt 1's device into attempt 2; establish root cause or same-load base comparison. Post reproducible live takeover commands/outcomes/state details, and split the new 322/308-line production modules or justify a deeper seam.

@thymikee

Copy link
Copy Markdown
Member

The RunnerTests reindent fixes the source-derived selection mismatch, and exact-head repo guards/macOS host count now pass. Still not ready at 28c3881572: macOS replay failed all three attempts, timing out in System Settings step 8 after ~220s; current main was green, and this branch changes broad request-admission/runner behavior, so establish a root cause or same-load base comparison. iOS is still in progress. Reproducible live takeover commands/outcomes/state details remain absent, and the new 322/308-line production modules still need extraction or a deeper-seam rationale.

Copilot AI review requested due to automatic review settings August 27, 2026 17:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@szdziedzic

Copy link
Copy Markdown
Contributor Author

Addressed at c2eaaee4.

  • The host-only XCTest is now guarded by AGENT_DEVICE_RUNNER_UNIT_TESTS && os(macOS). pnpm check:xctest-selection passes with 218 declared tests and host reach 190; a clean macOS runner build also passes.
  • The two oversized production modules are split by responsibility: CLI orchestration is 149 lines and local daemon transport is 168; hold coordination is 193 lines and persisted storage is 53, with shared validation kept in the 163-line contract module. Every touched production module is below 300 lines.
  • The failed macOS replay has a concrete UI root cause. The run artifact screenshot shows the host local-network permission sheet covering System Settings on the final attempt. The JUnit divergence reports 48 successful XCTest snapshots (p50 662 ms, max 862 ms) and wait_deadline_exceeded at step 8 while the app remained on General, so neither request admission nor the runner transport was stalled; the consent sheet intercepted the About navigation. All three retries shared that host permission state. The new head is rerunning the lane.
  • The earlier iOS failure was a separate retry cleanup race: attempt 1 hit its 60-second wrapper timeout while open was still completing, cleanup finished about 1.65 seconds later, and attempt 2 started before the retained claim was released. The exact next PR workflow passed the same gesture replay step.
  • I ran the public CLI against a booted iOS simulator with an isolated state directory. Foreground takeover created and heartbeated one hold; takeover status --json returned it; devices --json remained successful; and the identical open command failed with DEVICE_IN_USE, details.reason: "human_control_active", and retriable: true. Ctrl+C reported release, status returned holds: [], the same open then succeeded, the matching session closed, and daemon stop --clean completed gracefully. The complete reproducible command sequence and outcomes are now in the PR description.

Local validation on this head: full unit gate (8,168 tests / 1,078 files), typecheck, format, build, fallow, XCTest selection, and clean iOS/macOS runner builds all pass.

@thymikee

Copy link
Copy Markdown
Member

The module-size blocker is resolved: the split ownership is coherent and all new production modules are below 300 lines. Still not ready. Colocate focused tests with the new takeover-client, human-control contract, and persistence owners instead of leaving them covered only through aggregate tests. Exact-head macOS and iOS live lanes remain red (System Settings step 8 and Automation lab fixture-state waits); rerun or provide same-load base comparisons before readiness.

Copilot AI review requested due to automatic review settings August 27, 2026 18:17
@thymikee

Copy link
Copy Markdown
Member

The prior P1 remains: frontmost-app alert handling can still scan and operate another process's Local Network prompt. The size audit makes the owning fix more concrete: this adds 103 lines to apple/macos-helper/Sources/AgentDeviceMacOSHelper/main.swift, taking it from 699 to 800 lines despite the extract-before-500 rule. Extract a dedicated, explicitly named CI/host-permission operation constrained to the expected app and permission, with focused proof, rather than extending the generic fallback. The PR also needs truthful production-growth accounting/rejected-smaller-design rationale for ~1.6k net non-test lines and must root-cause or base-compare the currently red macOS System Settings replay and iOS gesture replay. Remove the workflow rationale narration under current comment policy.

Copilot AI review requested due to automatic review settings August 27, 2026 19:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thymikee

Copy link
Copy Markdown
Member

Re-reviewed 307821e22f: the prior cross-process PID/window sweep is gone, and extracting the visual fallback reduces main.swift from 800 to 762 lines. That blocker is materially improved.

Still not ready. The new 116-line Vision/CGEvent fallback has no focused regression or exact-head native result. It OCRs a session screenshot, selects an app window by closest aspect ratio, maps the recognized denial label into global coordinates, and clicks; prove with a focused fixture/coordinate/identity test that the intended Local Network dialog selects exactly the intended denial point and absent/mismatched dialogs return nil, then require the macOS lane to pass. The PR remains roughly +1,592 net production lines and its body does not itemize the growth or explain why smaller/deeper daemon, lease, and descriptor seams were rejected; its claim that all touched production modules are under 300 lines is false (main.swift is 762 and request-handler-chain.ts is 324). Remove the remaining workflow narration at .github/workflows/macos.yml:132-134. Coverage/integration/Repo Guards and native lanes are still incomplete.

@thymikee

Copy link
Copy Markdown
Member

After reviewing the remote path again, I think this should be re-scoped around the ownership boundary that already governs remote devices.

The product behavior is sound: a human-control hold may coexist with an open agent session, mutating agent commands fail with DEVICE_IN_USE / human_control_active, read-only diagnostics remain available, and the agent resumes after release. The current implementation, however, introduces a second device-ownership authority beside LeaseRegistry. That duplication is now responsible for the difficult parts of the change: translating between bare device IDs and provider device keys, separately protecting leases from expiry, refreshing them on release, maintaining another TTL lifecycle, and classifying every command through a second descriptor taxonomy.

Please re-scope the remote implementation so that:

  • Human-control state is owned by LeaseRegistry and keyed through the same backend/provider/device identity used for lease contention — not a new alias set.
  • Lease-owner takeover operations are ordinary daemon RPCs that pass normal tenant and lease admission, in the same gate (request-admission.ts) that admits every other request, and can affect only the admitted lease's device.
  • Hold heartbeat, expiry, lease preservation, and release refresh are one registry-owned lifecycle, updated transactionally inside the registry.
  • The PR states explicitly whether a hold must survive a daemon restart. Lease state today does not; the answer decides whether the persisted hold store carries over or is deleted.
  • Read versus mutation is derived from existing command semantics — recordingEffect where declared (its resolver form already splits e.g. alert get from alert accept), observe-class inventory commands, an explicit exemption for the takeover/heartbeat RPCs themselves — with unknown or unclassified requests treated conservatively as mutations, instead of a second humanControlEffect taxonomy on every descriptor.
  • Activation serializes through the authoritative device execution boundary. Keep the mutation counter/drain state unless an exact-device lock can prove that no admitted mutation completes after activation reports active; today's execution-lock selection is advisory for fresh sessions, so it cannot yet.
  • Any provider-host administrative capability remains explicitly distinct from tenant authority, even if both call the same registry methods.

The local path needs a separate decision: an open session already owns the host-global device claim, so takeover cannot acquire that claim without violating the promised coexistence semantics. Either add a host-global human-control fence that coexists with the session claim, or defer local cross-daemon takeover from this PR — deferral is fine.

Please also remove the macOS OCR consent handling and workflow prewarming from this feature; they are unrelated native/CI changes and need independent focused proof.

The existing CLI UX, error shape, and most behavior tests should carry over. This is a re-ownership of the implementation, not a rejection of the feature.

@thymikee

Copy link
Copy Markdown
Member

Still not ready at exact head 307821e.\n\n- [P1] HumanControlRegistry duplicates LeaseRegistry's ownership/lifecycle authority with its own persisted holds, TTL, mutation ledger and drain wait, then runtime bridges the two using protection predicates and manual lease refresh. Re-own remote takeover under the authoritative lease identity/lifecycle instead of maintaining two clocks and alias translation.\n- [P1] Takeover gating runs outside/before normal lease admission, while socket/admin mutations accept arbitrary deviceKey values without tenant/lease scope. Lease-owner takeover must pass ordinary admission and affect only the admitted lease; keep provider-host admin authority explicitly separate.\n- [P1] The added macOS consent OCR/global-CGEvent workaround has no focused fixture/coordinate/app-identity proof, and exact-head macOS smoke is red in the workflow this PR changed. Remove that unrelated native/CI workaround into independent work.\n- [P2] Current size is +2471/-124 across 45 files, about +1534 net production lines after exclusions. Itemize growth/rejected smaller seams. The body claim that every touched production module is below 300 lines is false (main.swift 762; request-handler-chain.ts 324).\n- Branch blocker: GitHub reports CONFLICTING against current main in the same ownership/admission surfaces. Rebase only after the ownership decision.\n\nThe behavior/test intent is sound, but no readiness label should be applied.

Copilot AI review requested due to automatic review settings August 28, 2026 09:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thymikee

Copy link
Copy Markdown
Member

Re-reviewed new exact head 788fd4c. The one-commit delta only narrows macOS window selection by owner PID; it does not fix the duplicated HumanControlRegistry/LeaseRegistry authority or admission-before-scope findings. The OCR flow still selects by aspect ratio and posts global CGEvents without focused coordinate/app-identity proof. Size is now +2491/-124 while the body remains stale and its under-300-lines claim is false. GitHub remains CONFLICTING and this exact head has zero checks. Still not ready; separate the macOS workaround and redesign ownership/admission before rebasing.

Copilot AI review requested due to automatic review settings August 28, 2026 20:22
@szdziedzic
szdziedzic force-pushed the szdziedzic-codex/feat-human-takeover branch from 788fd4c to 96fd2f9 Compare August 28, 2026 20:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thymikee

Copy link
Copy Markdown
Member

Not ready at 96fd2f9c9bd. The ownership redesign is now sound: holds use LeaseRegistry's canonical device identity, tenant operations pass normal admission, host administration stays loopback/token-only, and pending activation fences/drains mutations.

[P1] Cancellation is still unsafe. putHumanControlHold() records an activating hold, awaits drain, then activates it without receiving or checking the HTTP/RPC abort signal. The RPC server marks disconnects cancelled, but this path ignores that state; host PUT also permits omitted ttlMs, so a disconnected request can later create an indefinite hold that its caller never observed. Add planted tenant-RPC and host-PUT disconnect-during-drain regressions, and remove the exact pending hold on cancellation (or make durable-on-disconnect an explicit, bounded contract).

The PR body still records the remote proxy takeover/block/release/resume run as outstanding. Please provide that exact path's live evidence. Bundle Size is also skipped by the fork-only workflow condition; for this roughly 948 net production-line change, publish the canonical base/head size report before treating the LOC rationale as complete evidence.

Copilot AI review requested due to automatic review settings August 28, 2026 21:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@szdziedzic

Copy link
Copy Markdown
Contributor Author

Addressed the three requests in the latest review in 6d43be22d9e3489b114d63af0158cf8cbceaa93a.

Cancellation

Tenant RPC activation now receives the existing request abort signal; host PUT observes request abort and premature response close. LeaseRegistry removes only the exact pending hold instance on cancellation, preserving successor/unrelated holds and refreshing the protected lease when the last hold ends. Canceling activation does not cancel an already-admitted mutation.

The tenant-RPC and host-PUT regressions disconnect real loopback clients during an outstanding mutation, with no ttlMs. Both were observed failing before the fix, and now require the pending hold to disappear before that mutation ends, remain absent afterward, and allow subsequent mutation admission. A planted unconditional-cleanup violation also made the successor-hold regression fail. Additional tests cover protected-lease refresh, already-aborted activation, and abort-listener cleanup.

Live remote-proxy evidence

Built CLI → authenticated loopback proxy → isolated daemon → iPhone 17 Pro simulator (iOS 26.5). This is the remote-proxy path on one Mac, not a cloud-provider or tunnel test. The existing agent session stayed open throughout.

Action Observed result
connect proxy, then open com.apple.Preferences on the selected simulator Success; device-scoped proxy lease acquired
snapshot -i Settings UI captured
Foreground takeover, then takeover status Active hold reported
press label=General --settle during takeover Exit 1, DEVICE_IN_USE, details.reason: human_control_active, retriable: true
snapshot -i during takeover Success; read-only diagnostics remained available
Ctrl+C in takeover, then takeover status Exit 0; no remaining holds
The same press label=General --settle after release Exit 0; the following snapshot contained About, proving navigation to General
Cleanup Session closed, connection disconnected, isolated daemon stopped, simulator returned to its original shut-down state

After the initial open --platform ios --udid …, subsequent commands used the connection's inferred selector. An exploratory attempt repeating --platform ios exposed a public/internal Apple platform-name mismatch outside this diff; this feature does not change connection-runtime behavior.

Validation at 6d43be22d9e3489b114d63af0158cf8cbceaa93a

pnpm check:affected --run passed end-to-end in a clean checkout of this commit, after a frozen-lockfile install and build, with origin/main identifying the PR's actual base 69001179b04b45bb1529350a94bc0e5b2b1efdc3. This avoids the stale fork-ref issue without changing the original checkout's refs or any gate rules. The related unit/provider run passed 3,533 tests across 419 files; Node integration passed 90 with nine expected live-device skips. All locally runnable catalog gates passed, including the history-backed ratchet, package checks, wire/replay compatibility, and XCTest selection.

ADR 0007 and the remote-proxy documentation now explain cancellation. No skills or native/workflow changes were added. CI is running on the pushed head; this is published and reported, not yet claimed merge-ready.

Size measurement provenance

The report below was generated with the unmodified canonical scripts/size-report.mjs and component reporters used by .github/workflows/size.yml. Base: 69001179b04b45bb1529350a94bc0e5b2b1efdc3; head: 6d43be22d9e3489b114d63af0158cf8cbceaa93a. Both exact commits were built in separate clean checkouts with frozen-lockfile installs, package assets generated on both sides, and seven startup samples per revision on the same macOS arm64 host (Node 24.14.1, pnpm 11.1.3). This is local canonical-method evidence, not a claim that the fork-skipped Ubuntu size job ran.

The full PR adds 14,819 B raw JS, 5,851 B gzipped JS, and 15,514 B npm-unpacked content. Native packaged components are unchanged. Startup medians are descriptive short-run measurements, not a performance guarantee.

Size Report

Metric Base Current Diff
JS raw 2.51 MB 2.53 MB +14.8 kB
JS gzip 841.4 kB 847.3 kB +5.9 kB
npm tarball 968.2 kB 973.5 kB +5.3 kB
npm unpacked 3.35 MB 3.37 MB +15.5 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.67 MB 2.68 MB +15.5 kB
Apple runner source/project 581.1 kB 581.1 kB 0 B
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.6 kB 45.6 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 20.9 ms 21.2 ms +0.3 ms
CLI --help 38.7 ms 37.9 ms -0.8 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/internal/daemon.js +2.7 kB +785 B
dist/src/cli-help.js +899 B +318 B
dist/src/perf-runtime-plan.js -554 B +287 B
dist/src/sdk-batch-runner.js +227 B +77 B

Top changed packed files

Packed file Base Current Diff
dist/src/human-control-contract.js 0 B 6.3 kB +6.3 kB
dist/src/internal/daemon.js 108.5 kB 111.2 kB +2.7 kB
dist/src/takeover.js 0 B 1.9 kB +1.9 kB
dist/src/human-control.js 0 B 1.0 kB +1.0 kB
dist/src/command-schema.js 7.1 kB 8.1 kB +1.0 kB
dist/src/cli-help.js 89.8 kB 90.7 kB +899 B
dist/src/agent-device-client.js 17.6 kB 18.5 kB +892 B
dist/src/client-types.d.ts 55.9 kB 56.5 kB +695 B
dist/src/perf-runtime-plan.js 64.9 kB 64.3 kB -554 B
dist/src/screenshot-runtime.js 14.3 kB 14.5 kB +253 B

@thymikee

Copy link
Copy Markdown
Member

The cancellation P1 is resolved on 6d43be22d9e3: tenant RPC now carries the existing request abort signal, host PUT observes disconnect/early close, and LeaseRegistry removes only the exact pending instance. The real loopback tenant-RPC and host-PUT regressions disconnect during a live mutation with no TTL, prove the pending hold disappears before that mutation ends, preserve successor/unrelated holds, and prove later mutation admission.

The requested evidence is also now present: the remote proxy flow is exercised end-to-end against an iPhone 17 Pro simulator with block/read/release/resume outcomes, and the canonical-method base/head size report is published. The fork-only Size job remains skipped as expected.

No code finding remains in this delta. Exact-head Coverage and Android/iOS smokes were still running at review time, so readiness depends on those lanes finishing green.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 28, 2026
@thymikee
thymikee merged commit d967730 into callstack:main Aug 29, 2026
16 checks passed
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.

3 participants