<!-- task=m-iop-hot-path-one-shot-execution/01_preset_schema plan=2 tag=REVIEW_API milestone-task=preset-schema,hot-preset -->

# 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.
> 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-02
task=m-iop-hot-path-one-shot-execution/01_preset_schema, plan=2, tag=REVIEW_API

## Archive Evidence Snapshot

- Current pair will be archived as `agent-task/m-iop-hot-path-one-shot-execution/01_preset_schema/plan_local_G03_1.log` and `agent-task/m-iop-hot-path-one-shot-execution/01_preset_schema/code_review_cloud_G03_1.log`.
- Verdict: FAIL. Required 2, Suggested 0, Nit 0.
- Required: restore the approved top-level `execution_presets[]` selector/per-mode-route/workspace-tool shape; validate every allowed mode and all stage option bounds deterministically.
- Reviewer evidence: focused, race, vet, and `git diff --check` passed. A focused reproducer accepted `allowed_modes: [light,direct]` with light stages and a five-option required light stage, while the approved top-level list shape failed decode with `execution_presets expected a map, got slice`.
- Roadmap carryover: `milestone-task=preset-schema,hot-preset`; SDD S02/S04 remain the acceptance boundary. Runtime generation, model-to-preset one-of, authorization, and request-local binding compilation remain in later children.

## 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-G06.md` → `code_review_cloud_G06_2.log` and `PLAN-cloud-G06.md` → `plan_cloud_G06_2.log`.
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-hot-path-one-shot-execution/01_preset_schema/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
4. If PASS and task group is `m-<milestone-slug>`, preserve the first-line `milestone-task` metadata in `complete.log` and report it for the runtime aggregation event. 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 Restore the preset contract and fail-closed validator | [x] |

## Implementation Checklist

- [x] Replace the preset YAML/types with the approved top-level selector, per-mode routes/stages, canonical model references, and ordered workspace-tool alternatives; normalize identifiers in place.
- [x] Enforce strict preset-field decoding, exact allowed-mode/route correspondence, direct/light stage rules, option and binding bounds, unique identifiers, canonical model resolution, unsupported handler rejection, and deterministic diagnostics.
- [x] Rewrite preset config tests for SDD-shaped valid fixtures and all reviewer fail-open regressions while preserving provider-only compatibility.
- [x] Run focused, fresh, race, vet, and diff verification 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_G06_2.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_cloud_G06_2.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-hot-path-one-shot-execution/01_preset_schema/` to `agent-task/archive/YYYY/MM/m-iop-hot-path-one-shot-execution/01_preset_schema/` and update this checklist at the final archive path.
- [ ] If PASS and task group is `m-<milestone-slug>`, preserve and report `milestone-task` metadata for runtime aggregation, without modifying roadmap or directly calling `update-roadmap`.
- [ ] If PASS for split work, remove empty active parent `agent-task/m-iop-hot-path-one-shot-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

No scope or routing deviation. During this review pass, the plan's Test Strategy edge-case coverage and the "sort descriptor/route names before diagnostics" requirement were completed against the original implementation; both are plan-aligned completions, not new scope:

- `packages/go/config/execution_preset_config_test.go`: added the regression sub-tests the plan Test Strategy lists but the first implementation omitted — duplicate allowed mode, duplicate workspace alternative name, light wrong stage order, light wrong stage count, light missing `read`/`write`/`delete` operation, and custom (non-`heavy`) unregistered mode.
- `packages/go/config/execution_preset_types.go`: the extra-route-key diagnostic now collects and `sort.Strings` route keys before the membership check, so the first reported offending key is deterministic when multiple extra route keys exist.

## Key Design Decisions

- Restored top-level `ExecutionPresets []ExecutionPreset` slice shape matching SDD specifications.
- Implemented strict subtree decoding of `execution_presets` using `mapstructure.Decoder` with `ErrorUnused: true` to fail closed on unrecognized preset fields or malformed map shapes.
- Enforced in-place identifier normalization and deterministic closed validation for selector models, allowed modes, per-mode downstream route stages, option bounds (max 4 per stage), canonical model existence against `cfg.Models`, and workspace tool alternative operations (`read/write/delete` plus `prepare` when `write` does not create parents).
- Promoted `github.com/mitchellh/mapstructure v1.5.0` from the indirect to the direct `require` block in `go.mod` (version unchanged) because `load.go` now imports it directly for the strict `execution_presets` subtree decoder; this matches the plan's Modified Files Summary and keeps the module graph tidy for the new direct import.
- All diagnostics that can produce more than one candidate (registered mode-descriptor names, workspace operation names, and route keys in the extra-key check) iterate in sorted order so error text is deterministic for a given invalid config.

## Reviewer Checkpoints

- The YAML root is the approved `execution_presets[]` list and contains data-only `selector`, `allowed_modes`, `routes.<mode>.stages`, and ordered `workspace_tools` alternatives.
- Every selector/stage model is a normalized canonical `models[].id`; direct/light route keys exactly match the allowlist and required stage order/options are checked without first-mode or required-stage bypasses.
- Strict subtree decoding and deterministic sorted diagnostics reject unsupported fields, handlers, routes, duplicate ids, malformed workspace operations, and dangling references.
- Existing provider-only configs still load, and immutable generation/runtime compilation remain outside this child.

## Verification Results

Paste actual stdout/stderr below each exact command. Record any replacement and reason in `Deviations from Plan`.

### REVIEW_API-1 focused regression verification

```bash
go test -count=1 ./packages/go/config -run 'TestLoadEdgeExecutionPreset(Catalog|RejectsInvalidShape)$'
```

_Actual stdout/stderr:_

```
ok  	iop/packages/go/config	0.042s
```

### Full config package verification

```bash
go test -count=1 ./packages/go/config
```

_Actual stdout/stderr:_

```
ok  	iop/packages/go/config	0.081s
```

### Race verification

```bash
go test -race -count=1 ./packages/go/config
```

_Actual stdout/stderr:_

```
ok  	iop/packages/go/config	1.497s
```

### Vet and diff verification

```bash
go vet ./packages/go/config
git diff --check
```

_Actual stdout/stderr:_

```
(exited 0 with no output)
```

## Code Review Result

### Overall Verdict

FAIL

### Dimension Assessment

- Correctness: Fail — canonical selector/stage references bypass catalog membership when `models[]` is empty, and a `light` preset with no workspace binding alternatives is accepted.
- Completeness: Fail — the workspace operation validator does not enforce the plan-required schema matcher, argument mapping, or result matcher descriptors and does not normalize operation keys into the retained map.
- Test coverage: Fail — the suite omits empty-model-catalog canonical-reference rejection, zero-alternative `light` rejection, and incomplete workspace descriptor cases; a focused reviewer reproducer failed for the first two variants.
- API contract: Fail — accepted configs can violate the approved SDD requirement that stage models resolve through the canonical model catalog and that plan-bearing modes carry declarative workspace tool bindings.
- Code quality: Pass — the change is localized, formatted, and contains no debug output, dead code, or TODOs.
- Implementation deviation: Fail — the plan explicitly requires canonical model resolution and fail-closed binding shapes, but the implementation leaves both guards open without recording a deviation.
- Verification trust: Fail — all claimed commands rerun successfully, but fresh reviewer evidence contradicts the claimed fail-closed production behavior and complete regression coverage.
- Spec conformance (SDD S02/S04 via `milestone-task=preset-schema,hot-preset`): Fail — the decoded schema shape and registered mode rejection are present, but S02/S04 evidence is insufficient while canonical references and `light` binding admission remain fail-open.

### Findings

- **Required** — `packages/go/config/execution_preset_types.go:117` and `packages/go/config/execution_preset_types.go:214`: both canonical-reference checks are conditional on `len(canonicalModelIDs) > 0`, so a preset with `selector.model: missing-model` and no `models[]` catalog loads successfully. The active plan requires every selector/stage model to resolve against `cfg.Models`. Remove the empty-map bypass (the production caller always supplies `seenModelIDs`) and add regression coverage for selector and stage references when the catalog is empty.
- **Required** — `packages/go/config/execution_preset_types.go:239`: `validateWorkspaceTools` returns success when `light` has zero alternatives, and each operation is considered valid with only `tool_name`; the retained operation keys are not normalized or checked for normalized duplicates. The active plan requires fail-closed binding shapes with schema matching, canonical argument locations, deterministic success/error matching, and in-place identifier normalization. Require at least one alternative for `light`, validate every required descriptor field/map, rebuild normalized operation keys with duplicate detection, and add zero-alternative, incomplete-descriptor, and normalized-key regression cases in `packages/go/config/execution_preset_config_test.go`.

### Routing Signals

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

### Next Step

Prepare the smallest fail-closed validator follow-up through the plan skill with the raw findings and fresh reviewer evidence. No milestone-lock or external-execution user-review gate applies.

---

> **[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 |
