<!-- task=m-iop-owned-single-request-agent-execution/02+01_preset_binding plan=3 tag=REVIEW_API milestone-task=preset-binding -->

# Code Review Reference - REVIEW_API

> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
> The task is NOT complete until every implementation-owned section below is filled in.
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
> Follow the ownership table at the bottom of this file for which sections you own.

## Overview

date=2026-08-06
task=m-iop-owned-single-request-agent-execution/02+01_preset_binding, plan=3, tag=REVIEW_API

## Archive Evidence Snapshot

- Superseded pair: `plan_local_G06_2.log`, `code_review_cloud_G07_2.log`.
- Archived verdict: FAIL with Required R1-R3 and no Suggested or Nit findings. R1 covers swallowed marked-preset compilation failures, R2 covers discarded fixed stage options and missing defense-in-depth checks, and R3 covers shallow copying of nested option values.
- Fresh reviewer verification passed focused, package, race, vet, full Edge, formatting, and diff-hygiene commands, but the green tests exercised helpers or vacuous values instead of the failing production boundaries.
- Split predecessor 01 is satisfied by `agent-task/archive/2026/08/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log`.

## For the Review Agent

> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.

Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
Review completion means the following steps are finished:

1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
2. Archive `CODE_REVIEW-cloud-G07.md` → `code_review_cloud_G07_3.log` and `PLAN-cloud-G07.md` → `plan_cloud_G07_3.log`.
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/02+01_preset_binding/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
4. If PASS, preserve `milestone-task=preset-binding` in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.

---

## Implementation Item Completion

| Item | Status |
|------|--------|
| REVIEW_API-1 Fail closed at real route admission | [x] |
| REVIEW_API-2 Preserve the approved fixed stage shape | [x] |
| REVIEW_API-3 Deep-copy nested option values | [x] |

## Implementation Checklist

- [x] Propagate marked-preset compilation failures from managed and unmanaged production resolution, omit rejected presets from model lists, and preserve unmarked preset behavior.
- [x] Compile and validate exact fixed plan/work/review role, model, and option values through authorized managed dispatches, with non-vacuous refresh and invalid-shape tests.
- [x] Recursively isolate nested stage option values during construction and cloning, with source-to-binding and clone-to-original mutation tests.
- [x] Run the dependency, formatting, focused, package, race, vet, full Edge regression, and diff-hygiene verification commands exactly as written.
- [x] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.

## Review-Only Checklist

> **[REVIEW AGENT ONLY]** This checklist is used only by the review agent.
> Implementing agents must not modify or check this section.

- [x] Append one verdict of `PASS`, `WARN`, or `FAIL` and verified `review_rework_count`, `evidence_integrity_failure` to `Code Review Result`.
- [x] Verify that verdict, `Dimension Assessment`, and Required/Suggested/Nit classifications match.
- [x] Archive active `CODE_REVIEW-*-G??.md` to `code_review_cloud_G07_3.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_cloud_G07_3.log`.
- [x] Verify that the Agent-Ops managed block in `.gitignore` unignores `agent-task/**/*.md` and `agent-task/**/*.log` and ignores `agent-roadmap/current.md`.
- [ ] If PASS, write `complete.log` based on `agent-ops/skills/common/code-review/templates/complete-log-template.md` and leave no active `.md` files.
- [ ] If PASS, move active task directory `agent-task/m-iop-owned-single-request-agent-execution/02+01_preset_binding/` to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/02+01_preset_binding/` and update this checklist at the final archive path.
- [ ] If PASS, preserve and report `milestone-task=preset-binding` for runtime aggregation, without modifying roadmap or directly calling `update-roadmap`.
- [ ] If PASS for split work, remove empty active parent `agent-task/m-iop-owned-single-request-agent-execution/` or verify it was kept due to remaining siblings/files.
- [x] If WARN/FAIL, write the next filesystem state matching code-review verdict and do not write `complete.log`.

## Deviations from Plan

- Removed the obsolete `extractStageOptions(routeDispatch)` helper in `single_request_preset_binding.go`. The plan's Symbol References said "No symbol is renamed or removed," but the same section's REVIEW_API-2 problem statement identified this helper as the defect (`extractStageOptions always returns nil`). Once `resolveStageBinding` sources options from the approved `config.ExecutionSingleRequestStageConfig`, the helper is unreachable dead code that reproduces the flagged bug. It is a private, single-caller helper with no external contract, so deleting it is not a compatibility migration and does not touch any of the four load-bearing symbols the plan directed to keep (`compileSingleRequestBinding`, `compileSingleRequestBindingForUnmanaged`, `resolveStageBinding`, `cloneMapStringAny`), all of which remain.
- `resolveStageBinding` now takes the plan-directed `(role string, stage config.ExecutionSingleRequestStageConfig, …)` signature. The `role` parameter is retained for call-site self-documentation (`resolveStageBinding("plan", sr.Stages.Plan, …)`) per the plan's shown code and is currently unused inside the body; this only surfaces the repo's tolerated info-level `unusedparams` lint (already present in `chat_completion.go`, `hot_path_dispatch.go`) and is clean under `gofmt`/`go vet`.
- One newly added test map literal in `apps/edge/internal/openai/principal_routes_test.go` was reflowed by `gofmt -w` (a multi-line function value); the plan's formatting check (`gofmt -l …`) now reports no paths. No command in the plan's verification list was changed.
- Environmental note (not a code change): `/config` was at 100% disk use, so the first `go test -race` run aborted with `no space left on device` while writing its build `$WORK`. Running `go clean -cache` (18G of derived build cache) freed space; the race command then passed cleanly. A reviewer re-running on a full disk may need the same reclaim. `GOTMPDIR` stays under `/config/workspace` (not `/tmp`, which is `noexec`).
- Environmental note (not a code change): one full-suite `go test ./apps/edge/... -count=1` run flaked once in the unrelated `iop/apps/edge/internal/bootstrap` package (`TestRefreshConfigApplySkipsDisconnectedConfiguredNode: register: request timeout for nonce 1`, a TCP node-registration timeout under concurrent load). It is outside every changed package and code path, passed 3/3 in isolation, and the recorded full-suite re-run is clean. See the `Edge Regression and Hygiene` verification block for details.

## Key Design Decisions

- Fail-closed is enforced at the two real production resolvers, not in a helper. Unmanaged marked presets are rejected in `resolveRouteDispatch` (`route_resolution.go`) by returning `(routeDispatch{}, false)` when `preset.SingleRequest != nil` and unmanaged compilation errors; managed marked presets are rejected in `resolveVirtualPresetModelForPrincipal` (`principal_routes.go`) by propagating any `compileSingleRequestBinding` error as the existing public `ErrRouteNotFound`. Because the advertised-model helpers (`advertisedModels`, `advertisedModelsForPrincipal`) gate listing on those same resolver calls, a marked preset that cannot compile is omitted from `/v1/models` with no new listing policy. Unmarked/legacy presets keep `SingleRequest == nil` and resolve unchanged.
- Immutable admission is treated as all-or-nothing: no path returns a marked dispatch with `SingleRequest == nil`. A managed marked preset either resolves with a fully frozen non-nil `SingleRequest` binding or is not resolvable at all.
- `compileSingleRequestBinding` now performs defense-in-depth shape re-validation (`validateFixedSingleRequestShape`) at admission time rather than trusting only load-time config validation: exactly `["light"]` allowed modes; selector fused to the plan stage; high reasoning on plan/review and none on work; and one ordered, unique `plan→work→review` light route whose per-stage model and options exactly match the frozen policy. Violations map to typed errors without generic fallback — duplicate role → `errSingleRequestBindingDuplicate`, route model diverging from the frozen policy → `errSingleRequestBindingDynamic`, mode/selector/option inconsistencies → `errSingleRequestBindingInconsistent`, missing route/stage → `errSingleRequestBindingMissingStage`, unmanaged/cross-principal binding → `errSingleRequestBindingUnauthorized`.
- Approved options are sourced from the frozen `config.ExecutionSingleRequestStageConfig`, never from dynamic provider dispatch metadata. `NewSingleRequestBinding` takes the defensive deep copy, so a later config refresh mutating `SingleRequest.Stages` options cannot alter an already admitted binding (proven by the refresh-isolation and managed-resolver tests asserting non-empty `reasoning_effort=high` persists after mutation).
- REVIEW_API-3 mirrors the config package's proven recursive copier into a service-private `cloneValueAny`/`cloneReflectValue`, handling nil, interface, pointer, map, slice, and array values while leaving scalars and concrete collection types intact. Both the constructor and `Clone` route every stage option map through it, so nested maps and slices never alias across the caller, binding, or clone boundaries.
- Test fixtures were made shape-valid (selector fused to the plan stage, high reasoning on plan/review) so the binding-authorization sub-cases exercise real resolver rejections instead of tripping shape validation first. The managed and unmanaged fail-closed tests drive rejection through the actual production resolver and `/v1/models`, and each managed invalid variant keeps every canonical reference authorized so the rejection provably originates in immutable compilation rather than authorization — directly closing the archived reviewer gap that prior green tests exercised helpers or vacuous values.

## Reviewer Checkpoints

- The unique packet 01 predecessor evidence still satisfies the split dependency.
- Invalid marked presets fail at the real managed/unmanaged resolver and are omitted from model lists; no path returns a marked dispatch with `SingleRequest == nil`.
- Valid managed admission preserves public identity, authorized canonical models, exact plan/review high options, work option absence, workspace reference, and limits.
- Duplicate roles, dynamic model mismatch, unauthorized bindings, and option inconsistency reject without generic fallback.
- Constructor and clone boundaries recursively isolate nested maps and slices in both mutation directions.
- Unmarked legacy presets and ordinary route resolution remain unchanged.

## Verification Results

### Dependency

Command: `test -f agent-task/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log' | wc -l)" -eq 1`

```
$ test -f agent-task/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log' | wc -l)" -eq 1
$ echo exit=$?
exit=0
```

PASS. No active `01_preset_config/complete.log` exists; exactly one archived predecessor `agent-task/archive/2026/08/m-iop-owned-single-request-agent-execution/01_preset_config/complete.log` satisfies split index 01 (exit 0).

### Formatting

Command: `test -z "$(gofmt -l apps/edge/internal/openai/route_resolution.go apps/edge/internal/openai/principal_routes.go apps/edge/internal/openai/openai_auth_routes_models_test.go apps/edge/internal/openai/principal_routes_test.go apps/edge/internal/openai/single_request_preset_binding.go apps/edge/internal/openai/single_request_preset_binding_test.go apps/edge/internal/service/single_request_types.go apps/edge/internal/service/single_request_types_test.go)"`

```
$ gofmt -l apps/edge/internal/openai/route_resolution.go apps/edge/internal/openai/principal_routes.go apps/edge/internal/openai/openai_auth_routes_models_test.go apps/edge/internal/openai/principal_routes_test.go apps/edge/internal/openai/single_request_preset_binding.go apps/edge/internal/openai/single_request_preset_binding_test.go apps/edge/internal/service/single_request_types.go apps/edge/internal/service/single_request_types_test.go
$ echo exit=$?
exit=0
```

PASS. `gofmt -l` printed no paths for the eight changed Go files, so all are formatted (exit 0).

### Service Binding

Commands:

- `go test ./apps/edge/internal/service -run 'TestSingleRequestBinding(CloneIsolation|DefensiveCopyOptions)' -count=1`
- `go test ./apps/edge/internal/service -run 'TestSingleRequestBinding' -count=1`

```
$ go test ./apps/edge/internal/service -run 'TestSingleRequestBinding(CloneIsolation|DefensiveCopyOptions)' -count=1
ok  	iop/apps/edge/internal/service	0.024s

$ go test ./apps/edge/internal/service -run 'TestSingleRequestBinding' -count=1
ok  	iop/apps/edge/internal/service	0.026s
```

PASS. Nested constructor and clone isolation tests (`TestSingleRequestBindingDefensiveCopyOptions`, `TestSingleRequestBindingCloneIsolation`) and the full `TestSingleRequestBinding*` validity set pass freshly under `-count=1`.

### Route Admission

Commands:

- `go test ./apps/edge/internal/openai -run 'Test(UnmanagedSingleRequestPresetFailsClosed|ManagedSingleRequestPresetFailsClosed|VirtualPresetModelAuthorizationMatrix)' -count=1`
- `go test ./apps/edge/internal/openai -run 'TestSingleRequestPresetBinding' -count=1`
- `go test ./apps/edge/internal/openai -run 'Test(SingleRequestPresetBinding|UnmanagedSingleRequestPresetFailsClosed|ManagedSingleRequestPresetFailsClosed|VirtualPresetModelAuthorizationMatrix)' -count=1`

```
$ go test ./apps/edge/internal/openai -run 'Test(UnmanagedSingleRequestPresetFailsClosed|ManagedSingleRequestPresetFailsClosed|VirtualPresetModelAuthorizationMatrix)' -count=1
ok  	iop/apps/edge/internal/openai	0.031s

$ go test ./apps/edge/internal/openai -run 'TestSingleRequestPresetBinding' -count=1
ok  	iop/apps/edge/internal/openai	0.037s

$ go test ./apps/edge/internal/openai -run 'Test(SingleRequestPresetBinding|UnmanagedSingleRequestPresetFailsClosed|ManagedSingleRequestPresetFailsClosed|VirtualPresetModelAuthorizationMatrix)' -count=1
ok  	iop/apps/edge/internal/openai	0.040s
```

PASS. Managed and unmanaged marked presets fail closed through the real production resolvers and are omitted from `/v1/models`; the valid managed authorization matrix, fixed plan/work/review options, invalid defense-in-depth cases, public identity echo, and refresh isolation all pass freshly under `-count=1`.

### Package and Race Regression

Commands:

- `go test ./apps/edge/internal/service ./apps/edge/internal/openai -count=1`
- `go test -race -count=1 ./apps/edge/internal/openai ./apps/edge/internal/service`

```
$ go test ./apps/edge/internal/service ./apps/edge/internal/openai -count=1
ok  	iop/apps/edge/internal/service	5.887s
ok  	iop/apps/edge/internal/openai	7.859s

$ go test -race -count=1 ./apps/edge/internal/openai ./apps/edge/internal/service
ok  	iop/apps/edge/internal/openai	11.608s
ok  	iop/apps/edge/internal/service	6.950s
```

PASS. Both changed packages pass without cached results and under the race detector.

### Edge Regression and Hygiene

Commands:

- `go vet ./apps/edge/...`
- `go test ./apps/edge/... -count=1`
- `git diff --check`

```
$ go vet ./apps/edge/...
$ echo exit=$?
exit=0

$ go test ./apps/edge/... -count=1
ok  	iop/apps/edge/cmd/edge	0.171s
ok  	iop/apps/edge/internal/authprojection	0.058s
ok  	iop/apps/edge/internal/bootstrap	0.448s
ok  	iop/apps/edge/internal/configrefresh	0.101s
ok  	iop/apps/edge/internal/controlplane	6.615s
ok  	iop/apps/edge/internal/edgecmd	0.107s
ok  	iop/apps/edge/internal/edgevalidate	0.067s
ok  	iop/apps/edge/internal/events	0.052s
ok  	iop/apps/edge/internal/input	0.084s
ok  	iop/apps/edge/internal/input/a2a	0.065s
ok  	iop/apps/edge/internal/node	0.046s
ok  	iop/apps/edge/internal/openai	7.930s
ok  	iop/apps/edge/internal/opsconsole	0.037s
ok  	iop/apps/edge/internal/service	5.914s
ok  	iop/apps/edge/internal/transport	4.768s

$ git diff --check
$ echo exit=$?
exit=0
```

PASS. `go vet ./apps/edge/...` emits no diagnostics; the full Edge profile passes without cached results; `git diff --check` reports no whitespace errors.

Transient-flake note: one earlier full-suite run of `go test ./apps/edge/... -count=1` produced a single failure in the unrelated `iop/apps/edge/internal/bootstrap` package — `TestRefreshConfigApplySkipsDisconnectedConfiguredNode: register: request timeout for nonce 1` — a TCP node-registration timeout under concurrent full-suite load. It is not in any changed package or code path. The test passed 3/3 in isolation (`go test ./apps/edge/internal/bootstrap -run 'TestRefreshConfigApplySkipsDisconnectedConfiguredNode' -count=1`) and the full-suite re-run above is clean. A reviewer re-running the full profile may occasionally hit the same timing flake; re-run to confirm.

---

> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
> If anything is blank, go back and fill it in before saving this file.
> Leave review-agent-only sections unchanged.

## Section Ownership

| Section | Owner | Note |
|---------|-------|------|
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
| Code Review Result | Review agent appends | Not included in stub |

## Code Review Result

- **Overall Verdict:** FAIL

### Dimension Assessment

| Dimension | Assessment | Evidence |
|-----------|------------|----------|
| Correctness | Fail | The admission boundary accepts a marked preset with an extra route and does not consistently classify duplicate stage roles before positional validation. |
| Completeness | Fail | REVIEW_API-2 requires exactly one light route and typed duplicate rejection for every duplicate-role shape; both cases remain incomplete. |
| Test coverage | Fail | Existing defense-in-depth tests cover only the `plan,plan,review` duplicate ordering and do not cover extra route keys. |
| API contract | Fail | The marked-preset contract requires inconsistent or duplicate fixed-shape inputs to fail closed without fallback. |
| Code quality | Pass | The implementation is focused, formatted, and contains no debug code or unrelated edits in the reviewed boundary. |
| Implementation deviation | Fail | `validateFixedSingleRequestShape` does not fully implement the plan's exact-one-route and duplicate-error mapping requirements. |
| Verification trust | Fail | All recorded commands pass freshly, but focused reviewer reproducers contradict the claimed complete fixed-shape production path. |
| Spec conformance | Fail | SDD S02 requires immutable fixed-light admission and rejection of dynamic/inconsistent binding shapes; the extra-route variant is admitted. |

### Findings

- **Required R2** — `apps/edge/internal/openai/single_request_preset_binding.go:91`: `validateFixedSingleRequestShape` checks positional role equality while it is still discovering duplicates, so duplicate sequences such as `work,work,review` and `plan,review,review` return `errSingleRequestBindingInconsistent` instead of the plan-required `errSingleRequestBindingDuplicate`. It also reads the `light` route without requiring `len(preset.Routes) == 1`, so a marked preset containing a valid light route plus an extra route compiles successfully despite the exact fixed-light contract. Require exactly one route key, scan the full light-stage role list for duplicates before positional validation, and add table-driven regression cases for duplicate roles at each position plus an extra route.

### Routing Signals

- `review_rework_count=2`
- `evidence_integrity_failure=true`

### Next Step

- Invoke the plan skill in `prepare-follow-up` mode for `m-iop-owned-single-request-agent-execution/02+01_preset_binding`, map R2 to a direct fixed-shape validation and regression-test change, rerun isolated routing, then archive this pair and materialize the routed follow-up pair. Do not write `complete.log`.
