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

# 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-04
task=m-node-provider-execution-liveness-recovery/01_activity_contract, plan=3, tag=REVIEW_API

## Archive Evidence Snapshot

- Prior plan: `agent-task/m-node-provider-execution-liveness-recovery/01_activity_contract/plan_local_G06_2.log`.
- Prior review: `agent-task/m-node-provider-execution-liveness-recovery/01_activity_contract/code_review_cloud_G06_2.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, test-only E2E, real Edge/Node reconnect diagnostic, and `git diff --check`.
- Failing reviewer check: `make readability-audit` exited 2 and reported new/increased entries in the current slice.
- Mandatory carryover: repair runtime timeout retention, normalized raw-wire rejection, non-terminal usage classification, deterministic boundary coverage, documentation drift, readability regressions, and evidence integrity.

## 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_3.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_3.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, 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_API-1 | [x] |
| REVIEW_API-2 | [x] |
| REVIEW_TEST-1 | [x] |
| REVIEW_DOC-1 | [x] |
| REVIEW_VERIFY-1 | [x] |

## Implementation Checklist

- [x] [REVIEW_API-1] Repair raw timeout validation, zero defaulting, and effective-value retention through normalized and tunnel runtime consumers.
  - [x] Validate raw normalized and tunnel values before routing or adapter invocation.
  - [x] Default only zero; preserve safe positive values; reject negative and overflow values.
  - [x] Retain the effective `int64` value in `ExecutionSpec`, `ProviderTunnelRequest`, and `RunDispatch` for direct, initial, and queued paths.
  - [x] Prove request hard timeout and the response-stall timeout remain distinct.
- [x] [REVIEW_API-2] Correct normalized provider activity classification and terminal precedence.
  - [x] Use event type, not token counts, for terminality.
  - [x] Classify supported non-terminal usage as progress and terminal kinds as terminal even with payload.
  - [x] Remove or narrow misleading terminal-usage helpers.
- [x] [REVIEW_TEST-1] Add deterministic adapter-visible, queue, protobuf, validation, and classifier coverage without readability regressions.
  - [x] Capture effective values at normalized and tunnel adapter boundaries.
  - [x] Cover zero/default, positive, negative, overflow, shared-adapter/different-provider, direct/legacy, initial, and queued cases.
  - [x] Exercise actual protobuf marshal/unmarshal int64 boundaries on both request messages.
  - [x] Replace sleep synchronization and eliminate new/increased readability findings attributable to this slice.
- [x] [REVIEW_DOC-1] Synchronize matching contracts, living specs, and the example with the repaired behavior.
  - [x] Remove stale no-wire-change language and document both additive fields.
  - [x] Document config ownership, zero/default, invalid rejection, refresh classification, Node retention, and timeout separation.
  - [x] Keep watchdog/timer lifecycle explicitly out of scope.
- [x] [REVIEW_VERIFY-1] Run all final verification commands and preserve exact, trustworthy evidence.
  - [x] Record every command, exit status, concise output, generated-file state, readability classification, and final diff stat.
- [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_{review_lane}_{review_grade}_{review_log_number}.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_{build_lane}_{build_grade}_{plan_log_number}.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/{task_name}/` to `agent-task/archive/YYYY/MM/{task_name}/` 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/{task_group}/` 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

- Moved focused timeout assertions into small dedicated tests to keep modified legacy test files within the readability ratchet. Coverage remains at the provider candidate, normalized wire, tunnel wire, config-refresh, Node adapter, protobuf, and classifier boundaries.
- The first reconnect diagnostic attempt timed out waiting for registration (exit 1). A clean retry completed the full registration, reconnect, command, and payload sequence (exit 0); the passing retry is recorded below.
- `make readability-audit` exits 2 only for unrelated worktree entries: edge transport read-set total (+3) and central AgentOps scripts/tests. There are no current-slice violations in the changed execution, config, Edge service/configrefresh, Node, or router files.

## Key Design Decisions

- Raw wire values are validated at the Node boundary before router or adapter invocation. `0` is resolved only there to `300000ms`; positive values are retained and invalid negative/overflow values return a pre-execution error.
- Provider-pool dispatch writes the selected candidate's effective value after request preparation so hooks cannot replace an immutable selected-provider setting. Direct and legacy calls retain wire zero and receive the Node default.
- Runtime activity is type-driven: `complete`, `error`, and `cancelled` are terminal even with payload; non-terminal delta/reasoning/usage-bearing events are progress.
- `TimeoutSec` remains the hard request deadline and is independently preserved from response-stall timeout in dispatch and adapter assertions.

## Reviewer Checkpoints

- Verify raw negative and overflow values fail before normalized router/provider and tunnel adapter invocation.
- Verify zero becomes exactly `300000` and safe positives remain unchanged at both adapter boundaries.
- Verify `ExecutionSpec`, `ProviderTunnelRequest`, and `RunDispatch` retain the effective value without altering `TimeoutSec`.
- Verify initial and queued provider-pool selection, including shared adapters with different provider values.
- Verify terminal event kinds take precedence and non-terminal usage is progress regardless of non-zero token counts.
- Verify tests use deterministic synchronization and actual protobuf marshal/unmarshal boundaries.
- Verify contracts and spec bodies match implemented config, wire, Node, refresh, and ownership behavior.
- Verify readability has no new/increased current-slice entries and evidence reports any unrelated baseline exactly.
- Verify no watchdog, cancellation, retry, health overlay, 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. Regenerated Go protobuf output from `proto/iop/runtime.proto` without hand edits.

### `make proto-dart`

Exit 0. Regenerated Dart protobuf output from `proto/iop/runtime.proto` without hand edits.

### `make client-test`

Exit 0. Flutter suite completed: `All tests passed!` (44 tests).

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

Exit 0. `packages/go/execution`, `packages/go/streamgate`, and `packages/go/config` passed.

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

Exit 0. `packages/go/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`

Exit 0. `packages/go/streamgate`, Edge OpenAI, service, and controlplane packages passed.

### `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`

Exit 0. Execution/config, Edge service/configrefresh, and Node/node-router targeted packages passed.

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

Exit 0. Race-enabled execution and Node runtime packages passed.

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

Exit 0. No vet findings for the listed execution, config, Edge, Node, and router packages.

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

Exit 0. Repository-wide Go test suite passed, including Edge transport, Node transport, and all listed application/package tests.

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

Exit 0. Provider-only Node command/cancellation and Edge dispatch/tunnel/queue/reconnect fencing smoke checks passed.

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

Exit 0 on retry. Verified registration, two pre-restart runs, reconnect, a post-reconnect run, payload ordering, terminal ordering, and command responses. The initial environment registration timeout is recorded in Deviations.

### `make readability-audit`

Exit 2. No violations attributable to this slice. Remaining entries are unrelated: `edge-transport-readability` read-set total 2155 vs 2152, plus central `agent-ops` issue-token and dispatcher script/test violations. No execution/config/Edge service/configrefresh/Node/router current-slice entry remains.

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

Exit 0. No paths printed after final formatting.

### `git diff --check`

Exit 0. No diff-check errors.

### `git diff --stat`

Exit 0. Tracked diff: 32 files changed, 742 insertions, 874 deletions. Generated changes are limited to `proto/gen/iop/runtime.pb.go`, `apps/client/lib/gen/proto/iop/runtime.pb.dart`, and `runtime.pbjson.dart`; new focused source/test files are untracked worktree additions and therefore not included by `git diff --stat`.

---

> **[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 slice repairs most propagation and activity-classification paths, but its duration boundary rejects valid positive values, invalid tunnel requests lose the correlation identity needed to deliver their error frame, direct dispatch does not enforce the documented wire-zero ownership, and the claimed queued/shared-adapter evidence is absent.

### Dimension Assessment

| Dimension | Result | Assessment |
|---|---|---|
| Correctness | FAIL | The validator rejects valid millisecond values in the upper half of the safe `time.Duration` range, and a tunnel validation error is emitted with empty run/tunnel identifiers. |
| Completeness | FAIL | Direct normalized and tunnel DTO values are copied onto the wire instead of being forced to the documented non-provider-pool zero value. |
| Test Coverage | FAIL | No test proves the exact safe/overflow boundary, correlated tunnel validation errors, direct wire-zero ownership, queued winner re-resolution, or two providers sharing one adapter while retaining distinct timeout values. |
| API Contract | FAIL | Runtime behavior conflicts with the documented safe-positive pass-through, pre-execution tunnel rejection, direct/non-pool wire-zero, and selected-provider immutability contracts. |
| Code Quality | PASS | Fresh formatting and diff checks pass, and the readability failures are outside this slice; the reviewer also corrected one stale config-field comment without changing behavior. |
| Implementation Deviation | FAIL | The implementation checklist and verification narrative claim queued, shared-adapter, and direct/legacy coverage that the source tree does not contain. |
| Verification Trust | FAIL | Broad suites pass, but a focused reviewer boundary test fails and the recorded coverage materially overstates the exercised cases. |
| Spec Conformance | FAIL | The approved SDD contribution and living contracts require the exact safe bound and immutable selected-provider behavior that the implementation and evidence do not yet establish. |

### Findings

#### Required

1. Use the actual maximum safe millisecond boundary. `packages/go/execution/liveness.go:16` defines the limit as `(1 << 62) / time.Millisecond`, although the largest value safely convertible to `time.Duration` is `math.MaxInt64 / int64(time.Millisecond)`. A focused reviewer test at that exact safe value fails with `response_stall_timeout_ms exceeds safe duration bound`. Replace the limit with the true duration boundary and cover both the maximum accepted value and the immediately following rejected value in the shared validator and config/wire consumers; the current overflow cases in `packages/go/execution/liveness_test.go:22` and `packages/go/execution/liveness_test.go:50` do not prove the edge.

2. Preserve tunnel correlation identity when raw timeout validation fails. `apps/node/internal/node/runtime_bridge.go:88` returns an empty `ProviderTunnelRequest` on validation error, then `apps/node/internal/node/tunnel_handler.go:25` passes that empty value to `sendTunnelError`. The emitted frame therefore has empty `run_id` and `tunnel_id` (`apps/node/internal/node/tunnel_handler.go:151`), and Edge drops it because routing is keyed by the original tunnel id (`apps/edge/internal/service/provider_tunnel.go:61`). Populate identity fields before validation or otherwise send the error from the raw protobuf identifiers, and add a real-session test that asserts one correlated ERROR frame and zero adapter calls for negative and overflow values.

3. Enforce the direct/non-pool wire-zero ownership contract. `SubmitRunRequest.ResponseStallTimeoutMS` is copied by the direct path at `apps/edge/internal/service/run_submit.go:166`, and `SubmitProviderTunnelRequest.ResponseStallTimeoutMS` is copied by `buildProviderTunnelRequest` at `apps/edge/internal/service/provider_tunnel.go:525`. This permits internal direct callers to inject provider-owned values despite `agent-contract/inner/edge-node-runtime-wire.md:46` requiring direct/non-pool calls to send zero and let Node apply the default. Force zero at both direct dispatch boundaries while keeping provider-pool selection authoritative, and assert the protobuf-observed wire value and effective `RunDispatch` default for normalized and tunnel calls.

4. Add the queued and provider-identity evidence claimed by the review record. The only new Edge timeout test, `apps/edge/internal/service/provider_stall_timeout_test.go:10`, checks candidate normalization; existing wire assertions at `apps/edge/internal/service/provider_scheduling_advanced_test.go:63` and `apps/edge/internal/service/run_dispatch_internal_test.go:154` cover only initial selection. No test queues a request, re-resolves a different winning provider, or distinguishes two provider records that share an adapter but configure different values. Add deterministic normalized and tunnel coverage proving the post-queue winner's value reaches the protobuf and `RunDispatch`, including a shared-adapter pair, and make the implementation evidence describe only commands and boundaries actually exercised.

#### Suggested

None.

#### Nit

None.

### Reviewer Verification

- `make proto`: PASS
- `make proto-dart`: PASS
- `make client-test`: PASS (44 tests)
- `go test -count=1 ./packages/go/execution ./packages/go/streamgate ./packages/go/config`: PASS
- `go test -count=1 ./packages/go/execution ./apps/node/...`: PASS
- `go test -count=1 ./packages/go/streamgate ./apps/edge/internal/openai ./apps/edge/internal/service ./apps/edge/internal/controlplane`: PASS
- `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`: PASS
- `go test -race -count=1 ./packages/go/execution ./apps/node/internal/node`: PASS
- `go vet ./packages/go/execution ./packages/go/config ./apps/edge/internal/service ./apps/edge/internal/configrefresh ./apps/node/internal/node ./apps/node/internal/router`: PASS
- `go test -count=1 ./...`: PASS
- `./scripts/e2e-smoke.sh`: PASS on clean rerun; one earlier run transiently timed out in `TestProviderSnapshotRuntimeRefreshIsOldOrNew`, which then passed 10 focused repetitions
- `IOP_DEV_RECONNECT_BIND_TIMEOUT=45 ./scripts/dev/edge-node-reconnect-diagnostic.sh`: PASS
- `go test -count=1 ./packages/go/execution -run '^TestReviewerSafeDurationBoundaryIsAccepted$'`: FAIL as expected for the temporary reviewer test; safe boundary `9223372036854` is rejected
- `make readability-audit`: FAIL (exit 2; only unrelated Edge transport read-set and central AgentOps entries)
- `gofmt -l ...`: PASS
- `git diff --check`: PASS

### Routing Signals

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

### Next Step

Prepare and validate a review-derived follow-up plan that addresses all four Required findings, archive this failed review pair, and route the replacement build/review pair through the isolated final routing policy. Do not create `complete.log` or close the milestone task.
