feat: add human takeover controls - #2078
Conversation
There was a problem hiding this comment.
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.
|
We merged a bigger refactor so this will need a rebase |
|
[P1] Keep the remote lease alive under the same device identity as takeover Foreground This head is also not reviewable for readiness: GitHub reports |
89cc568 to
0da824a
Compare
0da824a to
676dfa7
Compare
There was a problem hiding this comment.
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
viewportcommand usessetViewport(...)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] },
|
Not ready at [P1] Foreground takeover stores bare [P1] TTL starts in [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. |
676dfa7 to
8a7ce5d
Compare
There was a problem hiding this comment.
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
viewportresizes the web viewport (a state-changing operation), but it is currently classified ashumanControlEffect: 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
tryHandleHumanControlHttpRoutecallsresolveHumanControlRoute, which usesnew URL(req.url, ...). If the raw request URL is malformed (e.g. invalid percent-encoding),new URLcan 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;
|
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, |
|
Not ready at |
|
The RunnerTests reindent fixes the source-derived selection mismatch, and exact-head repo guards/macOS host count now pass. Still not ready at |
|
Addressed at
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. |
|
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 |
|
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. |
|
Re-reviewed 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 ( |
|
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 Please re-scope the remote implementation so that:
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. |
|
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. |
|
Re-reviewed new exact head |
788fd4c to
96fd2f9
Compare
|
Not ready at [P1] Cancellation is still unsafe. 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. |
|
Addressed the three requests in the latest review in CancellationTenant RPC activation now receives the existing request abort signal; host PUT observes request abort and premature response close. The tenant-RPC and host-PUT regressions disconnect real loopback clients during an outstanding mutation, with no Live remote-proxy evidenceBuilt 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.
After the initial Validation at
|
| 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 |
|
The cancellation P1 is resolved on 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. |
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.The remote session must already own a device-scoped lease, for example through
connect proxyandopen.Ownership and admission
LeaseRegistryowns 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.request-admission.ts. Their target comes only from the admitted lease; a tenant cannot retarget a hold or modify another owner's hold.recordingEffectresolvers, observation-class inventory, and observability semantics, with explicit takeover/heartbeat exemptions. Unknown requests are mutations; no separate per-commandhumanControlEffecttaxonomy remains.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
6d43be22d9e3489b114d63af0158cf8cbceaa93aagainst upstream base69001179b04b45bb1529350a94bc0e5b2b1efdc3. The review follow-up and full canonical size report contain the detailed results.pnpm check:affected --runpassed end-to-end in a clean checkout of this exact commit after a frozen-lockfile install and build. Itsorigin/mainpoints 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 --settlefailed withDEVICE_IN_USE/human_control_active, whilesnapshot -isucceeded. Ctrl+C released the hold, status returned no holds, and the same press succeeded; a snapshot containingAboutconfirmed 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.