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 head now removes the in-runner interruption monitor and prewarms/dismisses the macOS Local Network alert from the workflow. Directionally this may fix the prior XCTest-count and consent-sheet failures, but the helper alert lookup/dismissal has no direct regression and exact-head macOS/iOS evidence is unfinished, so the remedy is not proven yet. Remove the new workflow control-flow rationale at |
|
[P1] Not ready: the new Local Network fallback widens generic |
|
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 |
Summary
agent-device takeoverso a person can temporarily control a selected simulator or device without racing state-changing agent commands; read-only diagnostics remain available.read,mutate, orcontrol) in the central descriptor registry. Request admission enforces that policy once for every handler, while unknown wire commands fail closed.traceandviewportconsistently with their effects, and keeps the public takeover path on client-backed device inventory.This PR touches 43 files (+2,280/-122). Scope expanded beyond the command family because local and remote modes share daemon admission and lease ownership. A per-handler guard would leave specialized or future routes able to bypass enforcement; the descriptor trait plus one request-execution gate makes omissions a type error and keeps enforcement centralized.
Validation
The full unit gate passed 8,168 tests across 1,078 files, followed by the temp-directory leak and smoke-contract checks. Cross-platform integration passed 90 Node scenarios with 9 expected live-device skips and all 161 provider scenarios. Typecheck, formatting, build, the fallow code-quality audit, XCTest selection, and clean iOS/macOS runner builds passed.
A fresh public-CLI run used an isolated
--state-dirand a booted iOS simulator:While active, status returned one persisted hold,
devicessucceeded, andopenfailed withDEVICE_IN_USE,details.reason: "human_control_active", andretriable: true. Ctrl+C reported release, status returnedholds: [], the sameopensucceeded, the matching session closed, and the isolated daemon stopped gracefully with retained-runner cleanup.The prior macOS replay failure was caused by a host local-network permission sheet covering System Settings. Its artifact recorded 48 healthy XCTest snapshots before step 8 timed out on the unchanged General screen, ruling out a stalled daemon/runner or takeover admission failure; all retries shared the same host permission state. The prior iOS gesture replay failure was a separate retry cleanup race: attempt 1 reached its 60-second wrapper timeout while
openwas still finishing, and attempt 2 started before cleanup released the retained claim. The exact next PR iOS workflow passed the same replay step.