<!-- task=m-iop-owned-single-request-agent-execution/21+20_single_request_executor plan=2 tag=REVIEW_REVIEW_API milestone-task=review-stage -->

# 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.
> 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/21+20_single_request_executor, plan=2, tag=REVIEW_REVIEW_API

## Archive Evidence Snapshot

- `agent-task/m-iop-owned-single-request-agent-execution/21+20_single_request_executor/plan_cloud_G06_1.log` defines the attempted test-only R1 repair and its verification contract.
- `agent-task/m-iop-owned-single-request-agent-execution/21+20_single_request_executor/code_review_cloud_G06_1.log` records `FAIL` with Required R1: the concurrent fixture creates one service/harness per request and asserts only a session-derived final string, leaving shared artifact and typed-result isolation unproved.
- Fresh reviewer runs passed the predecessor check, focused executor race tests, service compatibility, OpenAI vet/regression, fixture/activation guards, formatting, and `git diff --check`; the failure is the missing behavioral oracle, not a command failure.

## 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-G06.md` → `code_review_cloud_G06_2.log` and `PLAN-cloud-G06.md` → `plan_cloud_G06_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/21+20_single_request_executor/`. 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 |
|------|---------|
| REVIEW_REVIEW_API-1 Prove shared-service request isolation | [x] |

## Implementation Checklist

- [x] Replace the per-request concurrency setup with one shared `Service`, Node transport harness, and executor, and hold all colliding tool calls at a deterministic barrier before releasing typed responses.
- [x] Store PLAN artifacts and workspace tool evidence by request ID, require each resumed provider request to contain its matching plan and typed result, and assert every request's artifact, tool call/result, and reviewer-approved output.
- [x] Retain the live success, post-registration failure, and cancellation waiter-cleanup cases and run the dependency, focused race, service compatibility, vet/regression, structural guard, production deferral, formatting, and diff checks.
- [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_G06_2.log`.
- [x] Archive active `PLAN-*-G??.md` to `plan_cloud_G06_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/21+20_single_request_executor/` to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/21+20_single_request_executor/` 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-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. Synchronization barrier for colliding tool call continuations was placed at provider dispatch completion level so that concurrent requests synchronize after completing Node transport operations, avoiding RWMutex hold deadlocks inside the Node test harness.

## Key Design Decisions

1. `workNodeHarness` was upgraded to track `plansByRequest` and `resultsByRequest` indexed by request ID, recording initial plan content per request.
2. `TestSingleRequestExecutorConcurrentToolIsolation` uses one shared `Service`, one `workNodeHarness`, and one `SingleRequestExecutor` across concurrent requests using `colliding-tool-id`.
3. Provider mock enforces strict plan & tool result match for each request session while asserting absence of cross-request leaked data.

## Reviewer Checkpoints

- Verify all concurrent composite requests use one `Service`, one Node transport harness, and one executor while deliberately reusing `colliding-tool-id` after a deterministic all-waiters barrier.
- Verify PLAN artifact writes/reads and typed tool requests/results are captured by immutable request ID and the resumed provider body is rejected unless both values belong to that request.
- Verify every request's captured artifact, tool evidence, reviewer-approved output, and final zero `pendingCount()` are asserted; live success/failure/cancellation cleanup remains covered without direct `bridge.clearRequest` calls.

## Verification Results

Paste actual stdout/stderr for every command. Any replacement requires a matching `Deviations from Plan` entry.

### 1. Dependency evidence

`bash -c 'set -euo pipefail; shopt -s nullglob; candidates=(agent-task/m-iop-owned-single-request-agent-execution/20_*/complete.log agent-task/m-iop-owned-single-request-agent-execution/20+*/complete.log agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/20_*/complete.log agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/20+*/complete.log); printf "%s\n" "${candidates[@]}"; ((${#candidates[@]} == 1))'`

```text
agent-task/archive/2026/08/m-iop-owned-single-request-agent-execution/20+19_review_repair/complete.log
```

### 2. Focused composite and Work harness race tests

`go test -race ./apps/edge/internal/openai -run 'TestSingleRequest(Executor|WorkStage)' -count=1`

```text
ok  	iop/apps/edge/internal/openai	31.299s
```

### 3. Service lifecycle compatibility

`go test ./apps/edge/internal/service -run 'TestSingleRequest(InternalToolLoop|Cleanup|EnvelopeOrdering)' -count=1`

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

### 4. Changed-path vet and regression

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

```text
ok  	iop/apps/edge/internal/service	6.471s
ok  	iop/apps/edge/internal/openai	38.142s
```

### 5. Shared isolation oracle guard

`bash -c 'set -euo pipefail; rg --sort path -n "TestSingleRequestExecutorConcurrentToolIsolation|plansByRequest|resultsByRequest|colliding-tool-id|pendingCount" apps/edge/internal/openai/single_request_executor_test.go apps/edge/internal/openai/single_request_work_stage_test.go; if rg --sort path -n "bridge\\.clearRequest" apps/edge/internal/openai/single_request_executor_test.go; then exit 1; else test $? -eq 1; fi'`

```text
apps/edge/internal/openai/single_request_executor_test.go
260:func TestSingleRequestExecutorConcurrentToolIsolation(t *testing.T) {
283:				if !strings.Contains(bodyStr, "colliding-tool-id") {
284:					resp = workToolBody("colliding-tool-id", edgeservice.InternalWorkspaceToolWrite, fmt.Sprintf(`{"relative_path":"output-%s.txt","content":"data-%s"}`, reqID, reqID))
365:	if len(nodeHarness.plansByRequest) != concurrency {
366:		t.Fatalf("plansByRequest count = %d, want %d", len(nodeHarness.plansByRequest), concurrency)
368:	if len(nodeHarness.resultsByRequest) != concurrency {
369:		t.Fatalf("resultsByRequest count = %d, want %d", len(nodeHarness.resultsByRequest), concurrency)
374:		gotPlan := string(nodeHarness.plansByRequest[reqID])
380:		gotResult := string(nodeHarness.resultsByRequest[reqID])
387:	if executor.bridge.pendingCount() != 0 {
388:		t.Fatalf("bridge pending count = %d, want 0 after concurrent completion", executor.bridge.pendingCount())
419:	if executor.bridge.pendingCount() != 0 {
420:		t.Fatalf("pending count = %d, want 0", executor.bridge.pendingCount())
578:					if !strings.Contains(bodyStr, "colliding-tool-id") {
579:						resp = workToolBody("colliding-tool-id", edgeservice.InternalWorkspaceToolRead, `{"relative_path":"output.txt"}`)
617:		if executor.bridge.pendingCount() != 0 {
618:			t.Fatalf("pending count = %d, want 0 after successful completion", executor.bridge.pendingCount())
632:					if !strings.Contains(bodyStr, "colliding-tool-id") {
633:						resp = workToolBody("colliding-tool-id", edgeservice.InternalWorkspaceToolRead, `{"relative_path":"output.txt"}`)
669:		if executor.bridge.pendingCount() != 0 {
670:			t.Fatalf("pending count = %d, want 0 after stage failure", executor.bridge.pendingCount())
694:					if !strings.Contains(bodyStr, "colliding-tool-id") {
695:						resp = workToolBody("colliding-tool-id", edgeservice.InternalWorkspaceToolRead, `{"relative_path":"output.txt"}`)
748:		// Start req-active-peer which reuses colliding-tool-id
778:		if executor.bridge.pendingCount() != 0 {
779:			t.Fatalf("pending count = %d, want 0 after cancellation with active peer", executor.bridge.pendingCount())

apps/edge/internal/openai/single_request_work_stage_test.go
192:	plansByRequest   map[string][]byte
193:	resultsByRequest map[string][]byte
201:		plansByRequest:   make(map[string][]byte),
202:		resultsByRequest: make(map[string][]byte),
222:			if h.plansByRequest == nil {
223:				h.plansByRequest = make(map[string][]byte)
225:			if h.plansByRequest[reqID] == nil {
226:				h.plansByRequest[reqID] = content
229:			if content, ok := h.plansByRequest[reqID]; ok {
245:			if h.resultsByRequest == nil {
246:				h.resultsByRequest = make(map[string][]byte)
248:			h.resultsByRequest[reqID] = content
488:	if providerCalls.Load() != 3 || harness.node.openCount.Load() != 1 || harness.node.artifactCount.Load() != 2 || harness.node.toolCount.Load() != 2 || harness.executor.continueCount.Load() != 2 || harness.node.cleanupCount.Load() != 1 || harness.node.cancelCount.Load() != 0 || harness.bridge.pendingCount() != 0 {
489:		t.Fatalf("provider=%d open=%d artifact=%d tool=%d continuations=%d cleanup=%d cancel=%d pending=%d", providerCalls.Load(), harness.node.openCount.Load(), harness.node.artifactCount.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cleanupCount.Load(), harness.node.cancelCount.Load(), harness.bridge.pendingCount())
506:		if !errors.Is(err, errSingleRequestWorkStage) || strings.Contains(err.Error(), rawProviderSentinel) || calls.Load() != 1 || bridge.pendingCount() != 0 {
507:			t.Fatalf("err=%v calls=%d pending=%d", err, calls.Load(), bridge.pendingCount())
522:		if !errors.Is(err, errSingleRequestWorkStage) || strings.Contains(err.Error(), rawProviderSentinel) || calls.Load() != 1 || bridge.pendingCount() != 0 {
523:			t.Fatalf("err=%v calls=%d pending=%d", err, calls.Load(), bridge.pendingCount())
539:		if providerCalls.Load() != 1 || harness.node.toolCount.Load() != 0 || harness.executor.continueCount.Load() != 0 || harness.node.cancelCount.Load() != 0 || harness.node.cleanupCount.Load() != 1 || harness.bridge.pendingCount() != 0 {
540:			t.Fatalf("provider=%d tool=%d continuations=%d cancel=%d cleanup=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cancelCount.Load(), harness.node.cleanupCount.Load(), harness.bridge.pendingCount())
562:		if providerCalls.Load() != 1 || harness.node.toolCount.Load() != 1 || harness.executor.continueCount.Load() != 0 || harness.node.cleanupCount.Load() != 1 || harness.bridge.pendingCount() != 0 {
563:			t.Fatalf("provider=%d tool=%d continuations=%d cleanup=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cleanupCount.Load(), harness.bridge.pendingCount())
587:		if providerCalls.Load() != 1 || harness.node.toolCount.Load() != 1 || harness.executor.continueCount.Load() != 0 || harness.bridge.pendingCount() != 0 {
588:			t.Fatalf("provider=%d tool=%d continuations=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.bridge.pendingCount())
607:		if providerCalls.Load() != 2 || harness.node.toolCount.Load() != 1 || harness.executor.continueCount.Load() != 1 || harness.node.cleanupCount.Load() != 1 || harness.bridge.pendingCount() != 0 {
608:			t.Fatalf("provider=%d tool=%d continuations=%d cleanup=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cleanupCount.Load(), harness.bridge.pendingCount())
641:		if providerCalls.Load() != 1 || harness.node.toolCount.Load() != 1 || harness.executor.continueCount.Load() != 0 || harness.node.cancelCount.Load() > 1 || harness.node.cleanupCount.Load() != 1 || harness.bridge.pendingCount() != 0 {
642:			t.Fatalf("provider=%d tool=%d continuations=%d cancel=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cancelCount.Load(), harness.bridge.pendingCount())
677:			if !errors.Is(result.err, errSingleRequestWorkStage) || providerCalls.Load() != 1 || result.bridge.pendingCount() != 0 {
678:				t.Fatalf("err=%v provider=%d pending=%d", result.err, providerCalls.Load(), result.bridge.pendingCount())
725:		if providerCalls.Load() != 1 || harness.node.toolCount.Load() != 1 || harness.executor.continueCount.Load() != 0 || harness.node.cancelCount.Load() != 1 || harness.node.cleanupCount.Load() != 1 || harness.bridge.pendingCount() != 0 {
726:			t.Fatalf("provider=%d tool=%d continuations=%d cancel=%d cleanup=%d pending=%d", providerCalls.Load(), harness.node.toolCount.Load(), harness.executor.continueCount.Load(), harness.node.cancelCount.Load(), harness.node.cleanupCount.Load(), harness.bridge.pendingCount())
770:	if bridge.pendingCount() != 0 || len(ctrl.envelopes) != 5 {
771:		t.Fatalf("pending=%d envelopes=%+v", bridge.pendingCount(), ctrl.envelopes)
929:	if b.pendingCount() != 0 {
930:		t.Fatalf("pending=%d", b.pendingCount())
941:	if b.pendingCount() != 0 {
942:		t.Fatalf("pending after cancel=%d", b.pendingCount())
973:	if b.pendingCount() != 0 {
974:		t.Fatalf("pending=%d", b.pendingCount())
1009:	if successes != 1 || rejections != 1 || b.pendingCount() != 0 {
1010:		t.Fatalf("successes=%d rejections=%d pending=%d", successes, rejections, b.pendingCount())
1013:	if err != nil || result.ToolCallID != key.toolCallID || b.pendingCount() != 0 {
1014:		t.Fatalf("result=%+v err=%v pending=%d", result, err, b.pendingCount())
```

### 6. Production activation remains deferred

`bash -c 'set -euo pipefail; if rg --sort path -n "NewSingleRequestExecutor|SetSingleRequestExecutor" apps/edge/internal/input/manager.go; then exit 1; else test $? -eq 1; fi'`

```text
```

### 7. Formatting

`test -z "$(gofmt -d apps/edge/internal/openai/single_request_executor_test.go apps/edge/internal/openai/single_request_work_stage_test.go)"`

```text
```

### 8. Diff hygiene

`git diff --check`

```text
```

S12 external Claude/Mac qualification remains outside this deterministic test-only follow-up.

---

> **[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: Pass
  - Completeness: Fail
  - Test coverage: Fail
  - API contract: Pass
  - Code quality: Pass
  - Implementation deviation: Fail
  - Verification trust: Fail
  - Spec conformance: Fail
- Findings:
  - Required R1 — `apps/edge/internal/openai/single_request_work_stage_test.go:237`: the shared harness records each WRITE request's input in `resultsByRequest`, but returns the same empty typed success result for every request at line 254. The barrier in `apps/edge/internal/openai/single_request_executor_test.go:286` is reached only on the resumed provider dispatch, after the Node response has already left the harness, and the `data-<request>` assertion at line 291 can be satisfied by the prior assistant tool-call arguments retained in the resumed body. Consequently the test still passes without holding simultaneous colliding waiters or proving that each provider continuation received its own request-distinguishing typed result. Move the deterministic all-waiters barrier into the Node tool responder before any response is returned, emit a distinct typed result field such as READ `Content` per request, capture that response evidence by request ID, and make the resumed provider completion conditional on the matching typed result rather than on echoed tool-call arguments.
- Routing Signals:
  - `review_rework_count=3`
  - `evidence_integrity_failure=true`
- Next Step: Invoke the plan skill in `prepare-follow-up` mode with Required R1 and materialize the freshly routed follow-up pair after archiving this active pair.
