<!-- task=m-node-provider-execution-liveness-recovery/01_activity_contract plan=4 tag=REVIEW_REVIEW_API milestone-task=activity-contract -->

# Code Review Reference - REVIEW_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-04
task=m-node-provider-execution-liveness-recovery/01_activity_contract, plan=4, tag=REVIEW_REVIEW_API

## Archive Evidence Snapshot

- Prior plan: `agent-task/m-node-provider-execution-liveness-recovery/01_activity_contract/plan_cloud_G08_3.log`.
- Prior review: `agent-task/m-node-provider-execution-liveness-recovery/01_activity_contract/code_review_cloud_G08_3.log`.
- Prior verdict: FAIL with 4 Required findings, 0 Suggested findings, and 0 Nit findings.
- Passing reviewer checks: generators, client tests, targeted/full/race Go tests, vet, the clean E2E rerun, the real Edge/Node reconnect diagnostic, formatting, and `git diff --check`.
- Failing reviewer evidence: the temporary exact-boundary test rejects safe value `9223372036854`; `make readability-audit` also retains unrelated Edge transport and central AgentOps failures.
- Mandatory carryover: use the exact duration boundary, retain tunnel rejection correlation, enforce direct wire zero, prove queued winner/shared-adapter identity on both request surfaces, and record only evidence actually exercised.

## 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-G08.md` → `code_review_cloud_G08_4.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_4.log`.
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-node-provider-execution-liveness-recovery/01_activity_contract/`. 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=activity-contract` 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_REVIEW_API-1 | [x] |
| REVIEW_REVIEW_API-2 | [x] |
| REVIEW_REVIEW_API-3 | [x] |
| REVIEW_REVIEW_TEST-1 | [ ] |
| REVIEW_REVIEW_VERIFY-1 | [ ] |

## Implementation Checklist

- [x] [REVIEW_REVIEW_API-1] Correct the exact safe duration boundary and preserve validate-before-normalize behavior.
- [x] [REVIEW_REVIEW_API-2] Preserve raw tunnel correlation identity through pre-execution validation errors.
- [x] [REVIEW_REVIEW_API-3] Enforce direct wire-zero ownership while retaining the final queued provider's value.
- [ ] [REVIEW_REVIEW_TEST-1] Add deterministic exact-boundary, direct, queued, and shared-adapter evidence for both request surfaces.
- [ ] [REVIEW_REVIEW_VERIFY-1] Run final verification and record exact, non-overstated evidence.
- [ ] 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_G08_4.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_cloud_G08_4.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-node-provider-execution-liveness-recovery/01_activity_contract/` to `agent-task/archive/YYYY/MM/m-node-provider-execution-liveness-recovery/01_activity_contract/` 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-node-provider-execution-liveness-recovery/` 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

The required provider-pool queued-winner/shared-adapter test matrix has not yet been added. The current focused service test proves only direct normalized and tunnel zero-on-wire behavior. Final verification is therefore incomplete and this implementation must not be finalized until deterministic initial and re-resolved provider-pool coverage is present for both request surfaces.

## Key Design Decisions

- The safe millisecond bound is derived as `math.MaxInt64 / int64(time.Millisecond)`, so it guards only duration conversion overflow.
- The tunnel protobuf mapper builds correlation fields before validating the raw timeout; pre-execution ERROR frames therefore retain the original run and tunnel identifiers.
- Direct service boundaries reset caller-supplied response-stall values to wire zero. Provider-pool paths retain their existing post-admission overwrite from the selected candidate.

## Reviewer Checkpoints

- Verify `math.MaxInt64 / time.Millisecond` is accepted and the next millisecond is rejected by shared, config, normalized-wire, and tunnel-wire boundaries.
- Verify negative and overflow tunnel requests emit exactly one ERROR frame with their original run/tunnel ids before any adapter invocation.
- Verify direct normalized and tunnel DTO values cannot put a non-zero provider-owned timeout on the wire and `RunDispatch` reports the Node default.
- Verify initial and queued provider-pool selection use the final provider record's value on both request surfaces.
- Verify two providers sharing one adapter retain distinct provider ids, served targets, and timeout values after queue re-resolution.
- Verify deterministic barriers replace sleep-based coordination and repeated focused runs are stable.
- Verify evidence names the actual tests and does not claim unexecuted variants.
- Verify no schema, watchdog, timer, cancellation, retry, health, roadmap, commit, or push work entered this slice.

## Verification Results

### `go version && go env GOMOD`

Exit 0: `go version go1.26.2 linux/arm64`; module `/config/workspace/iop-s1/go.mod`.

### `flutter --version`

Exit 0: Flutter 3.41.5 stable, Dart 3.11.3.

### `make proto`

Exit 0. `protoc` regenerated checked-in Go bindings; generated-file state remains part of the pre-existing worktree changes.

### `make proto-dart`

Exit 0. `protoc-gen-dart` regenerated checked-in Dart bindings; generated-file state remains part of the pre-existing worktree changes.

### `make client-test`

Exit status not recorded: the combined verification command stopped after dependency resolution before a client-test result was captured. This task does not modify client code; rerun is required for finalization.

### `go test -count=1 ./packages/go/execution ./packages/go/streamgate ./packages/go/config`

Exit 0: execution, streamgate, and config packages passed.

### `go test -count=1 ./apps/node/internal/node -run 'StallTimeout|ProviderTunnelRequest'`

Exit 0. `TestOnProviderTunnelRequestRetainsValidatedStallTimeout` exercises zero, positive, and exact safe-boundary adapter-visible values; `TestOnProviderTunnelRequestInvalidStallTimeoutKeepsCorrelation` exercises negative/overflow single correlated ERROR frames and zero adapter calls.

### `go test -count=1 ./apps/edge/internal/service -run 'ResponseStallTimeout|ProviderStallTimeout'`

Exit 0. `TestDirectDispatchUsesZeroWireStallTimeout` captures normalized and tunnel protobuf requests and verifies wire zero plus default dispatch metadata. Initial/queued/shared-adapter provider-pool variants remain unimplemented.

### `go test -count=10 ./apps/edge/internal/service -run 'ResponseStallTimeout|ProviderStallTimeout'`

Exit 0. The currently implemented focused service tests passed ten repetitions; this is not evidence for the missing queued/shared-adapter variants.

### `go test -count=1 ./packages/go/execution ./apps/node/...`

Exit 0: execution and all `apps/node/...` packages passed.

### `go test -count=1 ./packages/go/streamgate ./apps/edge/internal/openai ./apps/edge/internal/service ./apps/edge/internal/controlplane`

Not run separately to completion; required before finalization.

### `go test -count=1 ./packages/go/execution ./packages/go/config ./apps/edge/internal/service ./apps/edge/internal/configrefresh ./apps/node/internal/node ./apps/node/internal/router`

Not run separately to completion; required before finalization.

### `go test -race -count=1 ./packages/go/execution ./apps/node/internal/node`

Not run; required before finalization.

### `go vet ./packages/go/execution ./packages/go/config ./apps/edge/internal/service ./apps/edge/internal/configrefresh ./apps/node/internal/node ./apps/node/internal/router`

Not run; required before finalization.

### `go test -count=1 ./...`

Invocation was started but no terminal result was captured; required before finalization.

### `./scripts/e2e-smoke.sh`

Not run; required before finalization.

### `IOP_DEV_RECONNECT_BIND_TIMEOUT=45 ./scripts/dev/edge-node-reconnect-diagnostic.sh`

Not run; required before finalization.

### `make readability-audit`

Not run; required before finalization.

### `gofmt -l packages/go/execution packages/go/config apps/edge/internal/service apps/node/internal/node`

The changed files were formatted with `gofmt -w`; the required listing command was not run separately.

### `git diff --check`

Exit 0: no whitespace errors reported.

### `git diff --stat`

Exit 0 for the tracked-file stat. Several planned common-package files are currently untracked in this pre-existing worktree; generated bindings are pre-existing modified files and were regenerated through Make targets.

---

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

The three API repairs are present and pass focused review, but the required deterministic provider-pool matrix is still absent and the final verification checklist is intentionally incomplete. The implementation therefore does not yet provide the evidence required to close the activity contract.

### Dimension Assessment

| Dimension | Result | Assessment |
|---|---|---|
| Correctness | PASS | The exact duration bound, correlation-preserving tunnel rejection, and direct wire-zero ownership are implemented and pass focused tests. |
| Completeness | FAIL | `REVIEW_REVIEW_TEST-1`, `REVIEW_REVIEW_VERIFY-1`, and the mandatory implementation-evidence item remain incomplete. |
| Test Coverage | FAIL | The Edge focused inventory contains only candidate normalization and direct dispatch tests; it has no initial/queued/shared-adapter provider-pool matrix for either normalized or tunnel dispatch. |
| API Contract | PASS | The reviewed source matches the exact safe-boundary, Node validation, tunnel correlation, and direct/non-pool zero-on-wire requirements. |
| Code Quality | PASS | Fresh `gofmt -l` and `git diff --check` checks are clean for the reviewed slice. |
| Implementation Deviation | FAIL | The plan requires deterministic queued winner and shared-adapter identity evidence on both request surfaces, but the implementation explicitly stops before adding it. |
| Verification Trust | PASS | The implementation record accurately distinguishes executed passing checks from missing or incomplete checks, and fresh focused results agree with the recorded claims. |
| Spec Conformance | FAIL | SDD S01 and the selected-provider evidence contract cannot be closed without deterministic proof that queue re-resolution preserves the final provider identity and timeout on both wire paths. |

### Findings

#### Required

1. Add the provider-pool evidence required by the plan. `apps/edge/internal/service/provider_stall_timeout_test.go:19-100` tests only candidate normalization and direct normalized/tunnel dispatch, and fresh `go test ./apps/edge/internal/service -list 'ResponseStallTimeout|ProviderStallTimeout|StallTimeout'` lists only `TestProviderCandidateResponseStallTimeout` and `TestDirectDispatchUsesZeroWireStallTimeout`. Add deterministic initial and queued cases for both normalized and tunnel surfaces, with two provider records sharing one adapter but retaining distinct provider ids, served targets, and timeout values after re-resolution. Use queue-state barriers rather than fixed sleeps, assert the protobuf request and `RunDispatch`, and keep the repeated focused run stable.

2. Complete the mandatory verification and implementation evidence. `agent-task/m-node-provider-execution-liveness-recovery/01_activity_contract/CODE_REVIEW-cloud-G08.md:61-67` leaves the test, verification, and evidence checklist items unchecked, while multiple required Final Verification entries are explicitly not run or lack a terminal result. After adding the missing matrix, run every plan command, record exact exit status/output without overstating coverage, and mark only the evidence-backed items complete.

#### Suggested

None.

#### Nit

None.

### Reviewer Verification

- `go test -count=1 ./packages/go/execution ./packages/go/config`: PASS
- `go test -count=1 ./apps/node/internal/node -run 'StallTimeout|ProviderTunnelRequest'`: PASS
- `go test -count=1 ./apps/edge/internal/service -run 'ResponseStallTimeout|ProviderStallTimeout'`: PASS
- `go test -count=10 ./apps/edge/internal/service -run 'ResponseStallTimeout|ProviderStallTimeout'`: PASS for the two currently implemented tests only
- `go test ./apps/edge/internal/service -list 'ResponseStallTimeout|ProviderStallTimeout|StallTimeout'`: PASS; inventory confirms only the candidate and direct-dispatch tests
- `gofmt -l packages/go/execution packages/go/config apps/edge/internal/service apps/node/internal/node`: PASS
- `git diff --check`: PASS

### Routing Signals

- `review_rework_count=3`
- `evidence_integrity_failure=false`

### Next Step

Prepare and validate the smallest review-derived follow-up plan for the missing deterministic provider-pool matrix and complete verification, archive this failed review pair, and continue through isolated final routing. Do not create `complete.log` or update the roadmap.
