<!-- task=m-iop-agent-cli-runtime/23+14,16,22_client_process_manager plan=0 tag=API -->

# Code Review Reference - API

> **[IMPLEMENTING AGENT — READ FIRST] This file must be completed.**
> Fill implementation evidence and actual output, then stop with active files in place. Finalization belongs only to the official reviewer.

## Overview

date=2026-07-29
task=m-iop-agent-cli-runtime/23+14,16,22_client_process_manager, plan=0, tag=API

## Roadmap Targets

- Milestone: `agent-roadmap/phase/automation-runtime-bridge/milestones/iop-agent-cli-runtime.md`
- Milestone link: [Milestone 문서](agent-roadmap/phase/automation-runtime-bridge/milestones/iop-agent-cli-runtime.md)
- Task ids:
  - `client-process-manager`: daemon-owned Flutter/Unity start, stop, reconnect, crash recovery, and detail routing
- Completion mode: check-on-pass

## For the Review Agent

> **[REVIEW AGENT ONLY]** Implementing agents must not execute finalization.

Review daemon ownership, durable identity, authorization inheritance, and cleanup together. Append verdict/signals; archive the active pair to `code_review_cloud_G10_0.log` and `plan_cloud_G10_0.log`; on PASS write `complete.log`, archive the task, and report completion metadata without editing the roadmap.

## Implementation Item Completion

| Item | Status |
|------|--------|
| API-1 Client config | [x] |
| API-2 Process ownership | [x] |
| API-3 Persistence/reconcile | [x] |
| API-4 Control operations | [x] |
| API-5 S15 evidence | [x] |

## Implementation Checklist

- [x] Extend only user-local runtime configuration with strict Flutter/Unity process specs, launch/restart policy, and no environment/credential fields.
- [x] Implement one daemon-owned process slot per client kind with PID/start identity, cancellation, reaping, duplicate convergence, and bounded crash restart.
- [x] Persist client lifecycle records and reconcile live, exited, stale, and ambiguous identities without duplicate launch.
- [x] Implement authenticated local-control client operations and Unity-detail-to-Flutter start/focus routing with command-id idempotency.
- [x] Prove fixture process ownership, disconnect/crash/reconnect, duplicate launch, focus routing, and daemon survival under race.
- [x] Update the standalone contract with actual S15 source/test paths and run Darwin cross-build verification.
- [x] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.

## Review-Only Checklist

> **[REVIEW AGENT ONLY]** Implementing agents must not modify this checklist.

- [x] Append one verdict and verified routing signals.
- [x] Verify verdict, dimensions, and finding classifications.
- [x] Archive this review as `code_review_cloud_G10_0.log`.
- [x] Archive the plan as `plan_cloud_G10_0.log`.
- [x] Verify the Agent-Ops managed `.gitignore` block.
- [ ] If PASS, write canonical `complete.log` and leave no active Markdown files.
- [ ] If PASS, move the task to the dated archive and update this checklist there.
- [ ] If PASS, report `client-process-manager` completion metadata without editing the roadmap.
- [ ] If PASS, remove an empty split parent or prove siblings/files remain.
- [x] If WARN/FAIL, materialize the required next state without `complete.log`.

## Deviations from Plan

None.

## Key Design Decisions

- Client launch policy exists only in `UserLocalRuntimeConfig`; strict YAML decoding rejects repo-global client fields, environment/credential fields, unknown client kinds, unsafe paths/argv, unbounded restart policy, and Unity focus configuration.
- `clientprocess.Manager` serializes one durable slot per client kind and records PID plus an OS-derived process start token. Direct children have exactly one waiter and an explicit abort/reap path; adopted children have one identity watcher.
- Durable records use checksum-covered `agentstate.Store` integration-record CAS. Live identities are adopted, exited/stale identities are classified without duplicate launch, and ambiguous identity or CAS state fails closed.
- A successful child exit never cancels the manager context. Crash restart consumes a bounded attempt/backoff policy, and manager closure prevents later start, detail, focus, or connection operations.
- `ClientOperations` reuses S11 authorization, validation, replay, immutable command hashing, durable command ownership, response, and retained-event behavior before invoking the process controller. Unity detail maps to one atomic Flutter start-or-focus call.
- `TestS15ClientLifecycleTrace` is the full-cycle test for this packet: it starts and reaps real helper processes through the production OS backend. Edge E2E and project smoke profiles do not apply because this packet changes no Edge route, external wire, or project execution path.

## Reviewer Checkpoints

- Config remains user-local, strict, and environment/credential free.
- PID/start identity and one waiter prevent duplicate/adopted-process races.
- Ambiguous recovery blocks instead of launching.
- Unity detail can reach Flutter only through daemon start/focus.
- All helper children are reaped while daemon context remains live.

## Verification Results

### Focused and race suites

```bash
go test -count=1 ./packages/go/agentconfig ./apps/agent/internal/clientprocess ./apps/agent/internal/localcontrol
go test -count=1 -race ./apps/agent/internal/clientprocess ./apps/agent/internal/localcontrol ./packages/go/agentstate
```

```text
ok  	iop/packages/go/agentconfig	0.033s
ok  	iop/apps/agent/internal/clientprocess	0.484s
ok  	iop/apps/agent/internal/localcontrol	0.074s
ok  	iop/apps/agent/internal/clientprocess	6.669s
ok  	iop/apps/agent/internal/localcontrol	1.399s
ok  	iop/packages/go/agentstate	1.305s
```

### S15 and Darwin

```bash
go test -count=1 -race ./apps/agent/internal/clientprocess ./apps/agent/internal/localcontrol -run 'TestS15|TestUnityDetail'
GOOS=darwin GOARCH=arm64 go test -c -o /tmp/clientprocess-darwin.test ./apps/agent/internal/clientprocess
```

```text
ok  	iop/apps/agent/internal/clientprocess	4.162s
ok  	iop/apps/agent/internal/localcontrol	1.033s
```

The Darwin cross-build produced no stdout/stderr and exited 0.

The fresh verbose S15 trace was:

```text
=== RUN   TestS15ClientLifecycleTrace
    manager_test.go:430: S15 trace flutter=8664/linux:28131471 duplicate=8664 unity=8671/linux:28131472 restart=8687/linux:28131478 focus=1 daemon=live
--- PASS: TestS15ClientLifecycleTrace (0.11s)
PASS
ok  	iop/apps/agent/internal/clientprocess	0.114s
```

### Static verification

```bash
go vet ./packages/go/agentconfig ./apps/agent/internal/clientprocess ./apps/agent/internal/localcontrol
git diff --check
```

Both commands produced no stdout/stderr and exited 0.

Additional local-profile verification:

```bash
go test -count=1 ./packages/go/...
go test -count=1 ./apps/agent/...
```

```text
ok  	iop/packages/go/agentconfig	0.038s
ok  	iop/packages/go/agentguard	0.016s
ok  	iop/packages/go/agentpolicy	0.012s
ok  	iop/packages/go/agentprovider/catalog	0.090s
ok  	iop/packages/go/agentprovider/cli	30.360s
?   	iop/packages/go/agentprovider/cli/internal/testutil	[no test files]
ok  	iop/packages/go/agentprovider/cli/status	40.096s
ok  	iop/packages/go/agentruntime	0.652s
ok  	iop/packages/go/agentstate	0.155s
ok  	iop/packages/go/agenttask	1.353s
ok  	iop/packages/go/agentworkspace	10.988s
ok  	iop/packages/go/audit	0.008s
?   	iop/packages/go/auth	[no test files]
ok  	iop/packages/go/config	0.077s
?   	iop/packages/go/events	[no test files]
ok  	iop/packages/go/hostsetup	0.012s
?   	iop/packages/go/jobs	[no test files]
?   	iop/packages/go/metadata	[no test files]
ok  	iop/packages/go/observability	0.023s
?   	iop/packages/go/policy	[no test files]
ok  	iop/packages/go/streamgate	0.896s
?   	iop/packages/go/version	[no test files]
ok  	iop/apps/agent/cmd/agent	0.030s
ok  	iop/apps/agent/internal/bootstrap	0.009s
ok  	iop/apps/agent/internal/clientprocess	0.636s
ok  	iop/apps/agent/internal/command	0.017s
ok  	iop/apps/agent/internal/host	0.005s
ok  	iop/apps/agent/internal/localcontrol	0.211s
ok  	iop/apps/agent/internal/projectlog	4.048s
```

---

> **[IMPLEMENTING AGENT — BEFORE SAVING]** Fill every implementation-owned section and leave review-only content unchanged.

## Section Ownership

| Section | Owner | Note |
|---------|-------|------|
| Header, Overview, Roadmap Targets, Review Instructions | Fixed | Implementer must not modify |
| Implementation Item Completion, Implementation Checklist | Implementing agent | Check status only |
| Review-Only Checklist | Review agent | Implementer must not modify |
| Deviations, Decisions, Verification Results | Implementing agent | Record actual evidence |
| Reviewer Checkpoints | Fixed | Reviewer verifies them |
| Code Review Result and finalization | Review agent | Append/execute only after review |

## Code Review Result

- Overall Verdict: FAIL
- Dimension Assessment:
  - Correctness: Fail
  - Completeness: Fail
  - Test coverage: Fail
  - API contract: Fail
  - Code quality: Pass
  - Implementation deviation: Fail
  - Verification trust: Pass
  - Spec conformance: Fail
- Findings:
  - Required — `apps/agent/internal/localcontrol/client_operations.go:106` and `apps/agent/internal/clientprocess/manager.go:266`: accepted-but-incomplete ledger recovery reclaims ownership and dispatches the same `command_id` again, but the process manager only writes `LastCommandID` and never uses it to converge a replay. A daemon failure after `Focus` or the focus branch of `StartOrFocusFlutter` applies the subprocess side effect but before `FinishCommand` commits causes recovery to run the focus command and increment `FocusCount` a second time. Add durable command receipt/pending-outcome handling at the process boundary, return the prior completed result without another mutation, fail closed when a non-idempotent outcome is ambiguous, and add an accepted-incomplete restart regression using the real manager boundary.
  - Required — `apps/agent/internal/clientprocess/manager.go:100` and `apps/agent/internal/clientprocess/store.go:113`: `NewManager` creates slots only for client kinds present in the current configuration, so a live checksum-covered record whose client policy was removed is never loaded, watched, reconciled, or made available for exact stop. The durable validator also accepts contradictory projections such as `stopped` with a live identity and `connected` with `Connected=false`, allowing an invalid record to enter adoption/start paths. Load and reconcile durable ownership for both canonical client kinds independently of current launch configuration, distinguish unconfigured launch policy from retained ownership, reject inconsistent lifecycle/identity projections, and add removed-config plus malformed-record regressions.
  - Required — `apps/agent/internal/clientprocess/manager.go:161`, `apps/agent/internal/clientprocess/manager.go:396`, `apps/agent/internal/clientprocess/manager.go:573`, and `apps/agent/internal/clientprocess/process.go:29`: lifecycle closure is not fenced with slot mutation. A `Start` that passes `ensureOpen` before `Close` marks the manager closed can acquire the slot after `Close` has stopped it and launch a child, while the OS backend ignores the manager context. For adopted children, watcher cancellation closes `waitDone` without proving process exit, so cleanup can clear durable ownership after only sending a signal to a still-live identity. Fence launch/focus/connection mutation under the slot lock against closure, make launch context cancellation abort and reap any child, separate watcher shutdown from proven process exit, and add deterministic concurrent-close and adopted-child cancellation cleanup tests.
- Routing Signals:
  - `review_rework_count=1`
  - `evidence_integrity_failure=false`
- Next Step: Invoke the plan skill with these raw findings and route the smallest complete follow-up for durable command convergence, ownership reconciliation, and lifecycle fencing.
