<!-- task=m-iop-owned-single-request-agent-execution/14+05,12,13_observation_timing plan=2 tag=REVIEW_API milestone-task=cleanup-observation -->

# 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-07
task=m-iop-owned-single-request-agent-execution/14+05,12,13_observation_timing, plan=2, tag=REVIEW_API

## Archive Evidence Snapshot

- Closed pair: `agent-task/m-iop-owned-single-request-agent-execution/14+05,12,13_observation_timing/plan_local_G06_1.log` and `agent-task/m-iop-owned-single-request-agent-execution/14+05,12,13_observation_timing/code_review_cloud_G07_1.log`.
- Verdict: FAIL with Required R1-R3, zero Suggested findings, and zero Nit findings.
- Affected behavior: service observer/clock injection, semantic stage timing across internal tools, actual tool outcome/duration emission, cleanup timing, terminal closure, and lifecycle-level evidence.
- Fresh reviewer verification: packets 05/12/13 were uniquely complete; focused race test passed in 1.046s; service package passed in 6.211s; vet and `git diff --check` passed. Those commands did not exercise an attached production timing accumulator.
- Roadmap carryover: `milestone-task=cleanup-observation`; approved SDD scenario S07 still requires linked raw-free stage/tool/cleanup/total timing and outcomes.

## 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_2.log` and `PLAN-cloud-G07.md` → `plan_cloud_G07_2.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/14+05,12,13_observation_timing/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
4. If PASS and task group is `m-iop-owned-single-request-agent-execution`, 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 Wire and correct lifecycle timing ownership | [x] |
| REVIEW_API-2 Replace manual-only evidence with lifecycle integration tests | [x] |

## Implementation Checklist

- [x] Attach one failure-isolated timing accumulator to every admitted service request before the accepted event and executor launch, with a safe real-clock default.
- [x] Emit each semantic provider stage, actual Node tool, cleanup gate, and terminal exactly once with closed success/error/cancel values while excluding tool time from stage pure time.
- [x] Prove the real service/handle lifecycle for success, error, cancellation, tool pause/resume, cleanup success/failure, terminal races, observer failure, and sentinel exclusion with deterministic tests.
- [x] Run dependency, focused race, package, vet, and whitespace verification with cache bypass where supported.
- [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_2.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_cloud_G07_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-owned-single-request-agent-execution/14+05,12,13_observation_timing/` to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/14+05,12,13_observation_timing/` and update this checklist at the final archive path.
- [ ] If PASS and task group is `m-iop-owned-single-request-agent-execution`, 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-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

None.

## Key Design Decisions

- `Service.StartSingleRequest` snapshots the observer and clock once after admission and passes them into observed request construction. The accumulator defaults a nil clock to `singleRequestRealClock` and a nil observer to the existing noop sink.
- Observation follows semantic plan/work/review identity. An internal tool pauses the active stage, emits its own closed outcome when the Node call settles, and resumes the same stage; `reviewing -> repairing` remains one review observation.
- The cleanup gate starts and finishes its observation exactly once under the existing cleanup ownership. Terminal close defensively closes any remaining semantic stage before emitting the one terminal total.
- Lifecycle integration tests use `Service.StartSingleRequest`, a manual clock, and the existing typed workspace wire fixtures. They cover success, tool pause/resume, cleanup failure, provider error, caller cancellation, observer panic isolation, terminal races, and closed/sentinel-safe DTO assertions.

## Reviewer Checkpoints

- Confirm `Service.StartSingleRequest` snapshots the observer/clock and creates timing before the accepted event and executor launch.
- Confirm one semantic plan/work/review event spans internal-tool pauses and normal transitions close the previous stage exactly once.
- Confirm each actual tool and cleanup gate emits one closed outcome/duration across success, error, and cancellation.
- Confirm terminal total includes cleanup/acknowledgement resolution and terminal races emit once.
- Confirm observer error/panic and invalid/secret-bearing values cannot alter lifecycle outcomes or escape the allowlist.

## Verification Results

Paste actual stdout/stderr for every command; record replacements under deviations.

### 1. Packet 05 dependency

`test -f agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log' | wc -l)" -eq 1`

```text
exit 0 (no stdout/stderr)
```

### 2. Packet 12 dependency

`test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,06,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,06,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`

```text
exit 0 (no stdout/stderr)
```

### 3. Packet 13 dependency

`test -f agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log' | wc -l)" -eq 1`

```text
exit 0 (no stdout/stderr)
```

### 4. Focused race test

`go test -race ./apps/edge/internal/service -run 'TestSingleRequestObservation' -count=1`

```text
ok  	iop/apps/edge/internal/service	1.072s
```

### 5. Package regression

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

```text
ok  	iop/apps/edge/internal/service	6.243s
```

### 6. Vet

`go vet ./apps/edge/internal/service`

```text
exit 0 (no stdout/stderr)
```

### 7. Whitespace

`git diff --check`

```text
exit 0 (no stdout/stderr)
```

---

> **[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:
  - Correctness: Fail
  - Completeness: Fail
  - Test Coverage: Fail
  - API Contract: Pass
  - Code Quality: Fail
  - Implementation Deviation: Fail
  - Verification Trust: Fail
  - Spec Conformance: Fail
- Findings:
  - Required R4 — `apps/edge/internal/service/single_request.go:466` closes the semantic stage as soon as cancellation/failure wins, while the actual tool observation is deferred until `apps/edge/internal/service/single_request_tool_loop.go:131`. `apps/edge/internal/service/single_request_observation.go:550` then increments the tool count and resumes stage timing after that stage has already emitted and closed. A fresh real-service tool-failure reproducer reported `failed stage ToolCount=0, want 1`, so error/cancel tools are not linked to the stage that owned them and can leave a phantom resumed timer. Defer the stage close while a tool is in flight (or otherwise settle the tool first), emit the tool once, include it in the owning stage count, and resume timing only when that semantic stage remains active; add deterministic real-lifecycle tool error and cancellation regressions.
  - Required R5 — `apps/edge/internal/service/single_request.go:762` classifies typed sentinel errors by searching for snake-case strings such as `internal_tool_failed` and `workspace_cleanup`, but the actual sentinel messages use spaces. The fresh tool-failure reproducer therefore emitted terminal `ErrorClass="provider"` instead of `internal_tool_failed`, and cleanup failures likewise cannot map to `workspace_cleanup`. Replace raw string inspection with `errors.Is` classification over the existing typed sentinels and assert exact terminal classes for provider, validation, budget, tool failure, cleanup failure, timeout, and cancellation paths.
  - Required R6 — `apps/edge/internal/service/single_request_observation.go:521`, `:564`, `:592`, `:618`, and `:632` do not attach one request-local opaque correlation to all stage/tool/cleanup/terminal/request DTOs: stage events receive only a shared stage constant while every other lifecycle event is empty. The fresh reproducer reported `stage="single_request.stage.plan" tool="" terminal=""`, so concurrent request events cannot satisfy SDD S07's linked raw-free timing/outcome evidence. Generate one bounded opaque correlation per accumulator without deriving it from caller `request_id`, preserve it on every DTO, and prove within-request equality, cross-request separation, and sentinel/raw-input exclusion.
- Routing Signals:
  - `review_rework_count=2`
  - `evidence_integrity_failure=true`
- Next Step: Create and implement the routed follow-up plan for R4-R6 under the same task path; no user-review gate applies.
