<!-- task=m-node-provider-execution-liveness-recovery/02+01_stall_watchdog plan=4 tag=REVIEW_REVIEW_API milestone-task=activity-contract,stall-watchdog -->

# 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/02+01_stall_watchdog, plan=4, tag=REVIEW_REVIEW_API

## Archive Evidence Snapshot

- The current pair will archive as `agent-task/m-node-provider-execution-liveness-recovery/02+01_stall_watchdog/plan_cloud_G09_3.log` and `agent-task/m-node-provider-execution-liveness-recovery/02+01_stall_watchdog/code_review_cloud_G09_3.log`.
- Prior verdict: FAIL. Required=2, Suggested=0, Nit=0.
- Required fixes: invalidate an expiry after intervening normalized/tunnel progress; release confirmed tunnel admission, run-manager, and credential ownership before publishing the confirmed terminal.
- Fresh reviewer verification passed the focused repeated tests, session lifetime tests, `go test -race -count=3`, vet, Node packages, `go test -count=1 ./...`, `./scripts/e2e-smoke.sh`, reconnect diagnostic, formatting, and `git diff --check`. The touched readability comparison passed; the repository audit retained unrelated worktree ratchet failures.
- Roadmap carryover: preserve `milestone-task=activity-contract,stall-watchdog`; satisfy approved SDD S01/S02 evidence only and do not update roadmap state.

## 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/02+01_stall_watchdog/`. 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 |
|------|---------|
| FIX-1 — stale expiry validity | [x] |
| FIX-2 — confirmed tunnel ownership ordering | [x] |
| TEST-1 — deterministic S01/S02 ordering regressions | [x] |

## Implementation Checklist

- [x] [FIX-1] Reject a consumed watchdog expiry after intervening normalized or tunnel progress while preserving exactly-once terminal/fence behavior.
- [x] [FIX-2] Close confirmed tunnel admission, run-manager, and credential ownership before publishing the confirmed stall terminal; retain unconfirmed ownership until provider return.
- [x] [TEST-1] Add deterministic normalized/tunnel stale-expiry and confirmed-terminal ownership-order regressions and rerun the S01/S02 repeated/race evidence.
- [x] Run every command in Final Verification and record literal output in `CODE_REVIEW-cloud-G08.md`.
- [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_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/02+01_stall_watchdog/` to `agent-task/archive/YYYY/MM/m-node-provider-execution-liveness-recovery/02+01_stall_watchdog/` 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

No implementation deviation.

The reconnect diagnostic was retried once after confirming no leftover Edge/Node diagnostic process. Both attempts timed out while waiting for Node registration. This is recorded below as a local diagnostic-environment blocker; no runtime, config, or diagnostic-script change was made because it is outside this task's scope.

## Key Design Decisions

- Each consumed watchdog expiry captures the observer activity epoch before it asks a sink to fence. Provider progress drains an unread timer tick, increments the epoch, and rearms the timer. Both normalized and tunnel sink claims reject an expiry whose epoch is no longer current.
- The package-private before/after stall-claim seams are used only by deterministic handler-level race tests. They force a consumed old expiry to wait while accepted provider progress resets the observer, without scheduler sleeps.
- Confirmed tunnel stalls run local cleanup before terminal visibility. Unconfirmed stalls still defer cleanup until the provider actually returns.

## Reviewer Checkpoints

- Reproduce a consumed watchdog expiry followed by progress that wins the normalized sink authority; the old tick must not stall and the reset timer must still stall after a full threshold.
- Reproduce the same stale-expiry ordering for tunnel progress, including a blocked accepted send, with no frame after terminal.
- Confirm provider terminal, caller cancel, hard deadline, and session disconnect still beat or invalidate a pending stall as specified.
- Confirm the exact `defaultAttemptCloseGrace=5s` boundary and exactly-once terminal/fence behavior remain unchanged.
- At confirmed tunnel terminal visibility, assert adapter admission is zero, the run handle is deregistered, and credential material is zeroed; for unconfirmed, assert all remain owned until provider return.
- Confirm normalized Failure metadata, normalized protobuf metadata, and tunnel ERROR metadata remain Node-owned, independently cloned, and omit `recovery_eligible` and secret fields.
- Confirm touched readability values stay no greater than baseline without modifying baseline/read-set files.
- Confirm the active header ids remain `activity-contract,stall-watchdog` and the implementation supplies approved SDD S01/S02 evidence only.

## Verification Results

> Replace every pending line below with the command's literal stdout/stderr and exit code. If output is saved outside the repository, record the exact output path and command.

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

```text
go version go1.26.2 linux/arm64
/config/workspace/iop-s1/go.mod
```

Exit code: 0.

### `go test -count=10 ./apps/node/internal/node -run 'Test((Run|Tunnel)WatchdogStaleExpiryYieldsToProgress|TunnelConfirmedFenceClosesOwnershipBeforeTerminal|RunWatchdogLifecycle|TunnelWatchdogLifecycle|TunnelSinkStallClaimSerializesAcceptedFrame|TunnelCredentialFailureReleasesAdmission)$'`

```text
exit 0
```

### `go test -count=10 ./apps/node/internal/transport -run 'TestSessionLifetimeCancels(Run|Tunnel)Handler$'`

```text
exit 0
```

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

```text
ok  \tiop/packages/go/execution\t1.256s
ok  \tiop/apps/node/internal/node\t10.530s
ok  \tiop/apps/node/internal/transport\t20.354s
```

Exit code: 0.

### `go vet ./packages/go/execution ./apps/node/internal/node ./apps/node/internal/transport`

```text
exit 0
```

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

```text
exit 0
```

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

```text
exit 0
```

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

```text
[e2e] verifying provider-only Node command and cancellation boundary
ok  \tiop/apps/node/internal/node\t0.129s
[e2e] verifying Edge dispatch, provider tunnel, queue, and reconnect fencing
ok  \tiop/apps/edge/internal/service\t5.546s
ok  \tiop/apps/edge/internal/transport\t0.542s
[e2e] provider-only Edge-Node smoke PASSED
```

Exit code: 0.

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

```text
[diagnostic] Starting edge-node-reconnect-diagnostic (repo-internal)...
[diagnostic] Starting edge.sh...
[diagnostic] Starting node.sh...
[diagnostic] Awaiting node registration...
[diagnostic] Timeout waiting for node registration
[edge] config=/tmp/iop-reconnect-diag-RHVjhI/edge.yaml
IOP Edge console listening on 127.0.0.1:39802
Console target node= adapter=mock target=mock-stream session=diagnostic-correlation background=false
Start node.sh on another host, then type a message here.
Commands: /nodes, /node <id|alias>, /session <id>, /background on|off, /capabilities, /transport, /exit
edge> [diagnostic] Cleaning up...
```

Exit code: 1. A retry after confirming no leftover diagnostic processes reached the same Node-registration wait and failed. Resume condition: a local diagnostic environment in which `scripts/dev/node.sh` can register with the temporary Edge within the script timeout.

### `make readability-audit || test $? -eq 2`

```text
python3 scripts/readability_audit.py --check --input-mode worktree --output build/readability-audit.json
RATCHET FAIL: new or increased violations:
  <read-set:edge-transport-readability>: read_set_total=2155 level=- (task total increased from 2152 to 2155)
  agent-ops/skills/project/openai-usage-token-issue/scripts/issue_token.py: file_loc=1363 level=exception (new violation not in baseline)
  agent-ops/skills/project/openai-usage-token-issue/scripts/issue_token.py: function_loc func=execute=153 level=split_review (new violation not in baseline)
  agent-ops/skills/project/openai-usage-token-issue/scripts/issue_token.py: function_loc func=selftest=83 level=warning (new violation not in baseline)
  agent-ops/skills/project/orchestrate-agent-task-loop/scripts/dispatch.py: file_loc=7227 level=exception (value increased from 7215)
  agent-ops/skills/project/orchestrate-agent-task-loop/scripts/dispatch.py: function_loc func=run_review=134 level=split_review (value increased from 122)
  agent-ops/skills/project/orchestrate-agent-task-loop/tests/test_dispatch.py: file_loc=12872 level=split_review (value increased from 12738)
  agent-ops/skills/project/orchestrate-agent-task-loop/tests/test_dispatch.py: function_loc func=DispatcherConvergenceSimulationTest.test_review_finalization_mismatch_keeps_dispatcher_running=92 level=warning (new violation not in baseline)
  apps/node/internal/node/liveness_watchdog.go: file_loc=516 level=warning (new violation not in baseline)
  apps/node/internal/node/liveness_watchdog_test.go: file_loc=854 level=warning (new violation not in baseline)
readability-audit: 490 files, 224649 LOC, 6728 functions, 537 violations
make: *** [Makefile:79: readability-audit] Error 4
```

`make readability-audit` exit code: 2; the planned `make readability-audit || test $? -eq 2` command exit code: 0.

### Touched readability baseline comparison

```bash
python3 - <<'PY'
import json

audit = json.load(open('build/readability-audit.json', encoding='utf-8'))
baseline = json.load(open('scripts/readability_baseline.json', encoding='utf-8'))
targets = {
    ('apps/node/internal/node/run_handler.go', 'Node.OnRunRequest'),
    ('apps/node/internal/node/tunnel_handler.go', 'Node.OnProviderTunnelRequest'),
    ('apps/node/internal/transport/session.go', 'newSession'),
}
base_functions = {(x['path'], x.get('function')): x['value'] for x in baseline['function_thresholds']}
current_functions = {(x['path'], x.get('function')): x['value'] for x in audit['violations'] if x.get('metric') == 'function_loc'}
bad = {key: current_functions.get(key, 0) for key in targets if current_functions.get(key, 0) > base_functions[key]}
base_sets = {x['task_id']: x['value'] for x in baseline['task_read_set_totals']}
current_sets = {x['task_id']: x['total_loc'] for x in audit['task_read_sets']}
if current_sets['node-core-readability'] > base_sets['node-core-readability']:
    bad['node-core-readability'] = current_sets['node-core-readability']
if bad:
    raise SystemExit(f'touched readability regression: {bad}')
print('touched readability regression: none')
PY
```

```text
touched readability regression: none
```

Exit code: 0.

### `test -z "$(gofmt -l apps/node/internal/node/liveness_watchdog.go apps/node/internal/node/liveness_watchdog_test.go)"`

```text
exit 0
```

### `git diff --check`

```text
exit 0
```

---

> **[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: Fail
  - Code quality: Pass
  - Implementation deviation: Fail
  - Verification trust: Fail
  - Spec conformance: Fail
- Findings:
  - Required — `apps/node/internal/node/liveness_watchdog.go:184`: `awaitAttempt` receives the timer signal before `captureExpiry`, but `captureExpiry` records the observer's current epoch rather than the epoch that armed the consumed signal. If provider progress resets the observer after the channel receive and before line 185, the old tick is relabeled with the new epoch and `claimFence` accepts it, so valid progress can still be followed immediately by `response_stalled`. Fresh reviewer reproduction consumed `observer.expired()`, called `observe(DispositionProgress)`, then showed `captureExpiry` plus `claimFence` succeeding; the existing handler tests block only inside `beforeStallClaim`, after the expiry epoch was already captured. Make the expiry signal carry its armed epoch or validate the timer event against monotonic last-progress state, and add normalized/tunnel handler regressions that force progress specifically between expiry receive and expiry capture before proving the reset timer can stall.
  - Required — `agent-task/m-node-provider-execution-liveness-recovery/02+01_stall_watchdog/CODE_REVIEW-cloud-G08.md:163`: the required 45-second reconnect diagnostic exits 1 before Node registration. Fresh review reproduced the failure with both the normal path and a prebuilt `/tmp` Node binary. A diagnostic run with a 300-second registration ceiling then passed all three message runs, Node/Edge payload ordering, commands, and reconnect; live secret-safe logs showed cold Edge/Node Go builds, rather than runtime registration, consumed the 45-second window. Revalidate the follow-up verification setup so local build latency is isolated from runtime registration (for example, an explicit prebuild plus a cold-build-tolerant registration ceiling), then record a zero-exit full diagnostic without weakening its message, terminal, command, or reconnect assertions.
- Routing Signals: `review_rework_count=3`, `evidence_integrity_failure=false`
- Next Step: Archive this pair and create the routed follow-up PLAN/CODE_REVIEW pair through plan `prepare-follow-up` mode.
