<!-- task=m-node-provider-execution-liveness-recovery/13+10_recovery_observability plan=3 tag=REVIEW_REFACTOR milestone-task=ops-evidence -->

# Plan - REVIEW_REFACTOR

## For the Implementing Agent

Filling every implementation-owned section of `CODE_REVIEW-cloud-G04.md` is mandatory. Execute this plan without changing its owner or write boundary, run every verification command, paste actual notes and stdout/stderr into the review stub, leave both active files in place, and report ready for review. Final verdict, archive renames, `complete.log`, and task-directory archival belong only to the code-review skill. If blocked, record only the exact blocker, attempted commands/output, and resume condition in implementation-owned evidence fields; do not ask the user, call user-input tools, create control-plane stop files, classify the next state, archive logs, or write `complete.log`.

## Background

The liveness projection implementation passed its focused, package, race, vet, and runtime checks, but the completed evidence packet omitted two explicit regression boundaries. The follow-up is test-only: prove explicit ownership for an application-installed sink of the same concrete zap type, and extend production-handler evidence beyond successful redispatch to rejection, immediate terminal, and recovery failure outcomes.

## Archive Evidence Snapshot

- Prior pair: `agent-task/m-node-provider-execution-liveness-recovery/13+10_recovery_observability/plan_cloud_G08_2.log` and `agent-task/m-node-provider-execution-liveness-recovery/13+10_recovery_observability/code_review_cloud_G08_2.log`; official verdict `FAIL` with Required R1 and R2, no Suggested or Nit findings.
- Required findings: R1 found that `repeated_construction_shares_collectors` installs only `*capturingObservationSink`, leaving the explicit same-concrete-type `*zapFilterObservationSink` forwarding boundary unproved. R2 found that the production Chat/Responses x normalized/tunnel handler matrix proves only successful redispatch, while rejection, immediate terminal/not-selected, and recovery failure remain synthetic-only.
- Affected implementation boundary: `apps/edge/internal/openai/liveness_recovery_observability_test.go`; production source, contracts, and specs were judged behaviorally consistent and are not reopened.
- Verification evidence: fresh focused count loops, package tests, race tests, vet, fake-provider smoke, the two-process reconnect diagnostic, and `git diff --check` passed. The prior active predecessor check failed only because the predecessor had already moved; `agent-task/archive/2026/08/m-node-provider-execution-liveness-recovery/10+09_stall_recovery/complete.log` now supplies the exact archived PASS dependency evidence.
- Roadmap carryover: preserve approved SDD S06 and `milestone-task=ops-evidence`; completion still requires bounded Edge eligibility/result metric and raw-free structured-log evidence across the actual OpenAI handler surfaces.

## Finding Resolution Map

| Finding | Mode | Exact Fix / Dependency Evidence | Changed or Satisfied Precondition |
|---------|------|---------------------------------|-----------------------------------|
| R1 | `direct-fix` | Add a server-level explicit `*zapFilterObservationSink` ownership regression in `apps/edge/internal/openai/liveness_recovery_observability_test.go`. | The same concrete type will be installed through `SetObservationSink`, so forwarding is tested by ownership rather than inferred type. |
| R2 | `direct-fix` | Extend `TestOpenAILivenessRecoveryObservability` in `apps/edge/internal/openai/liveness_recovery_observability_test.go` with production-handler rejection, immediate terminal, and recovery dispatch-failure fixtures for Chat/Responses and normalized/provider-tunnel paths. | Every required outcome will traverse the real handler/runtime observation pipeline instead of only the synthetic sink harness. |

## Analysis

### Files Read

- `apps/edge/internal/openai/server.go`
- `apps/edge/internal/openai/filter_observation_sink.go`
- `apps/edge/internal/openai/filter_observation_sink_test.go`
- `apps/edge/internal/openai/liveness_recovery_observability.go`
- `apps/edge/internal/openai/liveness_recovery_observability_test.go`
- `apps/edge/internal/openai/stream_gate_filters.go`
- `apps/edge/internal/openai/stream_gate_stall_recovery_test.go`
- `apps/edge/internal/openai/provider_test_support_test.go`
- `packages/go/streamgate/filter_observation.go`
- `packages/go/streamgate/recovery_coordinator.go`

### SDD Criteria

- SDD: `agent-roadmap/sdd/operational-observability-provider-management/node-provider-execution-liveness-recovery/SDD.md`
- Status: approved; lock released; no unresolved user review.
- First-line milestone task id: `ops-evidence`.
- Target Acceptance Scenario: S06.
- Evidence Map driver: S06 requires Node/Edge metric label guards, structured-log capture, and raw-free evidence across liveness/fence/health/commit/recovery axes.
- Effect on this packet: REVIEW_REFACTOR-1 proves the log ownership boundary without changing projection semantics; REVIEW_REFACTOR-2 makes the Edge commit/eligibility/result evidence deterministic across the actual OpenAI handler variants and outcomes. The final verification repeats focused, package, race, vet, and repository-native runtime checks.

### Verification Context

- No separate handoff was supplied; the verdict-appended active pair and the exact archived predecessor `complete.log` provided the recovery context.
- Source paths read are listed under `Files Read`; contract/spec synchronization in the prior packet was inspected and judged complete, so this follow-up does not reopen those files.
- Fresh commands already reproduced the implementation packet's focused count loops, package suite, race suite, vet, fake-provider smoke, reconnect diagnostic, and diff check. Source inspection—not command failure—identified the two coverage gaps.
- Preconditions: the predecessor dependency is satisfied by `agent-task/archive/2026/08/m-node-provider-execution-liveness-recovery/10+09_stall_recovery/complete.log`; SDD S06 is approved; the task remains under `milestone-task=ops-evidence`.
- Constraints: preserve request-local projection, global collector ownership, generic-log suppression semantics, StreamGate decisions, HTTP/SSE behavior, and the existing shared dirty worktree. Do not edit production source, contracts, specs, or unrelated test files.
- Gap and confidence: the missing tests are directly visible at the prior test lines 574-596 and 632-684. Existing `scriptedPoolRunService`, `stallMatrixFailureAttempt`, `stallMatrixSuccessAttempt`, `stallMatrixServer`, and `runStallMatrixHandler` fixtures provide deterministic repository-native coverage with high confidence.
- External Verification Preflight: not applicable; both runtime checks are repository-native local scripts using the current checkout, and the fake-provider smoke requires no external host or credential.

### Test Coverage Gaps

- Explicit same-type sink ownership: production code tracks constructor ownership with `obsSinkIsDefault`, but no server-level test installs `newZapFilterObservationSink(...)` through `SetObservationSink`. Gap assigned to REVIEW_REFACTOR-1.
- Production-handler result outcomes: Chat/Responses x normalized/provider-tunnel successful redispatch is covered, but plan rejection, immediate terminal, and recovery dispatch failure are absent from the handler matrix. Gap assigned to REVIEW_REFACTOR-2.
- Production behavior changes: none. Existing source behavior remains the verification subject.

### Symbol References

None. No production symbol is renamed or removed.

### Split Judgment

This is one indivisible test packet because both findings validate the same `Server.observationSink()` ownership/projection boundary through one fixture family. The dependent subtask directory encodes predecessor index 10; it is satisfied by archived PASS evidence at `agent-task/archive/2026/08/m-node-provider-execution-liveness-recovery/10+09_stall_recovery/complete.log`.

### Scope Rationale

Only `apps/edge/internal/openai/liveness_recovery_observability_test.go` may change. Production source already implements the reviewed ownership and projection behavior, and the OpenAI contract plus StreamGate/OpenAI specs already describe it. Other test fixtures are reusable read-only dependencies; expanding into Node health projection, provider overlay, recovery policy, public HTTP behavior, or unrelated shared-worktree changes is excluded.

### Final Routing

- `evaluation_mode=isolated-reassessment`; `finalizer=finalize-task-policy.sh`; `finalizer_mode=pair`.
- Build closures: `scope_closed=true`, `context_closed=true`, `verification_closed=true`, `evidence_trusted=true`, `ownership_closed=true`, `decision_closed=true`; capability gap not observed.
- Build grade scores: scope coupling 1, state/concurrency 1, blast/irreversibility 0, evidence/diagnosis 1, verification complexity 1; grade G04.
- Build route: base `local-fit`, promoted by `recovery-boundary` because `review_rework_count=1` and `evidence_integrity_failure=true`; lane `cloud`; canonical filename `PLAN-cloud-G04.md`.
- Review closures: `scope_closed=true`, `context_closed=true`, `verification_closed=true`, `evidence_trusted=true`, `ownership_closed=true`, `decision_closed=true`; capability gap not observed.
- Review grade scores: scope coupling 1, state/concurrency 1, blast/irreversibility 0, evidence/diagnosis 1, verification complexity 1; grade G04.
- Review route: `official-review`, lane `cloud`, adapter `codex`, model `gpt-5.6-sol`, reasoning effort `xhigh`; canonical filename `CODE_REVIEW-cloud-G04.md`.
- `large_indivisible_context=false`; positive loop risks: `temporal_state`, `boundary_contract`, `variant_product` (count 3); `risk_boundary_matched=false`; `recovery_boundary_matched=true`.

## Dependencies and Execution Order

1. Predecessor subtask `10+09_stall_recovery` is complete at `agent-task/archive/2026/08/m-node-provider-execution-liveness-recovery/10+09_stall_recovery/complete.log`.
2. Implement REVIEW_REFACTOR-1 before REVIEW_REFACTOR-2 so the explicit sink ownership oracle is isolated from the handler outcome matrix.
3. Run the complete final verification only after both test additions pass their focused commands.

## Implementation Checklist

- [ ] REVIEW_REFACTOR-1 adds a server-level regression that installs the exact `*zapFilterObservationSink` concrete type through `SetObservationSink`, proves original private-liveness/ExactReplay rows are forwarded, proves metrics remain active, and proves safe replacement logging stays exclusive to the constructor-owned default sink.
- [ ] REVIEW_REFACTOR-2 extends the production Chat/Responses x normalized/provider-tunnel observability matrix with deterministic redispatch, plan-rejection, immediate-terminal, and recovery-dispatch-failure outcomes, asserting exact metric family/label/count evidence, exact safe-log fields, and no generic high-cardinality private-liveness rows.
- [ ] Run every focused, package, race, vet, fake-provider auxiliary smoke, two-process Edge/Node diagnostic, predecessor, and diff command in Final Verification with fresh output.
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.

### [REVIEW_REFACTOR-1] Prove explicit same-type zap sink ownership

**Problem**

`apps/edge/internal/openai/liveness_recovery_observability_test.go:574-596` proves global collector reuse and generic custom-sink ownership, but line 591 installs only a capturing sink:

```go
574 t.Run("repeated_construction_shares_collectors", func(t *testing.T) {
...
591     conf.SetObservationSink(&capturingObservationSink{})
592     s2 := conf.observationSink().(*openAILivenessObservationSink)
593     if s2.suppressDefault {
594         t.Fatal("explicitly installed sink must not be suppressed")
595     }
596 })
```

That does not catch a regression that classifies default ownership by concrete type and suppresses an application-installed `*zapFilterObservationSink`.

**Solution**

Add a focused subtest that builds a server and isolated collectors/logger, installs the exact factory result through `SetObservationSink`, then emits a complete private liveness/ExactReplay cycle through the server-created wrapper:

```go
explicit := newZapFilterObservationSink(logger)
srv.SetObservationSink(explicit)
sink := srv.observationSink()
// Emit the real private-liveness eligibility and ExactReplay lifecycle rows.
// Assert original generic rows are present, the expected counters increment,
// and constructor-owned safe replacement logging is not claimed by this sink.
```

Keep the constructor-default case in the same focused test so the two ownership modes are contrasted by the explicit `SetObservationSink` call, not by Go type inspection. Reuse the current observation builders and `assertExactObservationFields`; do not alter production code.

**Modified Files and Checklist**

- [ ] Modify `apps/edge/internal/openai/liveness_recovery_observability_test.go` only.
- [ ] Install the value returned by `newZapFilterObservationSink(logger)` through `SetObservationSink`.
- [ ] Assert original private filter and ExactReplay lifecycle observations reach `filterObservationLogMessage` on the explicit sink path.
- [ ] Assert one eligibility and one final result metric are still projected.
- [ ] Assert safe replacement logs remain owned only by the constructor-default path and contain the exact six-field allowlist there.

**Test Strategy**

Write the regression in `apps/edge/internal/openai/liveness_recovery_observability_test.go` under `TestOpenAILivenessObservationSink`, named `explicit_same_type_zap_sink_preserves_originals`. Use an isolated Prometheus registry, zap observer core, the real zap sink factory, `Server.observationSink()`, and existing private-liveness/ExactReplay observation builders. Assert exact generic observation kinds/fields, exact eligibility/result counter values, and default-vs-explicit safe-log ownership.

**Verification**

Run `go test -count=20 ./apps/edge/internal/openai -run '^TestOpenAILivenessObservationSink$'`; all iterations must pass and execute the new subtest.

### [REVIEW_REFACTOR-2] Complete the production handler outcome matrix

**Problem**

`apps/edge/internal/openai/liveness_recovery_observability_test.go:632-684` enumerates both endpoints and paths but scripts only a confirmed stall followed by successful redispatch:

```go
632 func TestOpenAILivenessRecoveryObservability(t *testing.T) {
633     for _, tc := range []struct {
634         endpoint string
635         path     string
636         wantPath string
637     }{
...
647         service := newScriptedPoolRunService(
648             stallMatrixFailureAttempt(tc.path, "attempt-sentinel", "provider-sentinel", "unavailable"),
649             stallMatrixSuccessAttempt(tc.endpoint, tc.path, false, "replacement-sentinel", "provider-replacement", "recovered-sentinel"),
650         )
```

The synthetic sink harness exercises other result classifiers, but it does not prove those rows emerge from the production Chat/Responses handler runtimes or retain bounded metrics/logging across execution paths.

**Solution**

Turn the handler test into a Cartesian table over Chat/Responses, normalized/provider-tunnel, and four deterministic outcomes:

```go
for _, surface := range surfaces {
    for _, outcome := range []string{"redispatched", "plan_rejected", "terminal", "dispatch_failed"} {
        t.Run(surface.endpoint+"/"+surface.path+"/"+outcome, func(t *testing.T) {
            // Script the existing production fixtures for this outcome.
            // Run the actual endpoint handler and gather the isolated registry.
            // Assert the exact metric rows/counts and exact safe-log allowlist.
        })
    }
}
```

Use `stallMatrixFailureAttempt` plus `stallMatrixSuccessAttempt` for redispatch, zero recovery budget for plan rejection, an unconfirmed-fence stall fixture for immediate terminal, and a second `scriptedPoolAttempt{err: ...}` for recovery dispatch failure. Validate gathered metric family names and label key sets, exactly one eligibility row and exactly one result row per request, the expected closed values, and the six-field safe log allowlist. Generic logs may retain unrelated/terminal observations, but must contain no row attributed to the private liveness filter or the consumed ExactReplay lifecycle.

**Modified Files and Checklist**

- [ ] Modify `apps/edge/internal/openai/liveness_recovery_observability_test.go` only.
- [ ] Cover Chat and Responses handlers with normalized and provider-tunnel initial paths for all four outcomes.
- [ ] Reuse `scriptedPoolRunService` and stall matrix helpers; add only local test helpers required for deterministic fixture construction and registry/log assertions.
- [ ] Assert exact metric family names, exact label key sets, expected closed label values, exactly one eligibility metric, and exactly one final result metric per request.
- [ ] Assert every safe log has exactly `phase`, `execution_path`, `provider_health`, `commit_state`, `eligibility`, and `recovery_result`, with no sentinel/raw/high-cardinality value.
- [ ] Assert constructor-default generic logs contain no private-liveness or consumed ExactReplay row while preserving unrelated terminal visibility.
- [ ] Preserve existing HTTP/SSE response and dispatch-count assertions so observability remains side-effect free.

**Test Strategy**

Extend `TestOpenAILivenessRecoveryObservability` in `apps/edge/internal/openai/liveness_recovery_observability_test.go`. The test name remains stable for the existing count-loop command. Fixtures are `newScriptedPoolRunService`, `stallMatrixFailureAttempt`, `stallMatrixSuccessAttempt`, `stallMatrixServer`, `runStallMatrixHandler`, isolated Prometheus registries, and zap observer cores. Each subtest must identify the endpoint/path/outcome, assert the real handler response and dispatch count, gather exact metric descriptors/counts, and inspect exact structured-log fields.

**Verification**

Run `go test -count=10 ./apps/edge/internal/openai -run '^TestOpenAILivenessRecoveryObservability$'`; all iterations and every endpoint/path/outcome subtest must pass.

## Modified Files Summary

| File | Item |
|------|------|
| `apps/edge/internal/openai/liveness_recovery_observability_test.go` | REVIEW_REFACTOR-1, REVIEW_REFACTOR-2 |
| `agent-task/m-node-provider-execution-liveness-recovery/13+10_recovery_observability/CODE_REVIEW-cloud-G04.md` | REVIEW_REFACTOR-1, REVIEW_REFACTOR-2 |

## Final Verification

Fresh Go output is required; cached output is not acceptable.

1. `test -f agent-task/archive/2026/08/m-node-provider-execution-liveness-recovery/10+09_stall_recovery/complete.log` — archived predecessor PASS evidence exists.
2. `go test -count=20 ./apps/edge/internal/openai -run '^TestOpenAILivenessObservationSink$'` — every iteration passes and the explicit same-type zap sink subtest executes.
3. `go test -count=10 ./apps/edge/internal/openai -run '^TestOpenAILivenessRecoveryObservability$'` — every iteration passes and the complete endpoint/path/outcome matrix executes.
4. `go test -count=1 ./packages/go/streamgate ./apps/edge/internal/openai ./apps/edge/internal/service ./apps/edge/internal/controlplane` — all selected packages pass under the Edge/platform-common profiles.
5. `go test -race -count=3 ./packages/go/streamgate ./apps/edge/internal/openai -run 'LivenessObservation|LivenessRecovery|Observation'` — passes with no race report.
6. `go vet ./packages/go/streamgate ./apps/edge/internal/openai ./apps/edge/internal/service ./apps/edge/internal/controlplane` — exits 0 with no diagnostics.
7. `IOP_VLLM_MODE=fake ./scripts/e2e-openai-vllm.sh` — credential-free Chat streaming/non-streaming Edge -> Node -> fake-provider smoke passes.
8. `IOP_DEV_RECONNECT_BIND_TIMEOUT=45 ./scripts/dev/edge-node-reconnect-diagnostic.sh` — separate Edge/Node registration, two same-session messages, reconnect, post-reconnect message, payload equality, status endpoints, and exactly-once terminal ordering pass.
9. `git diff --check` — exits 0 with no whitespace errors.

After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.
