diff --git a/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md index aa3be579..d1e3432e 100644 --- a/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md +++ b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md @@ -1,44 +1,43 @@ - + -# Code Review Reference - TEST - -> **[IMPLEMENTING AGENT — READ FIRST]** Start only after child 07 PASS. Fill implementation evidence, leave active files in place, and report ready for review. Finalization is review-only. +# Code Review Reference - REVIEW_TEST ## Overview date=2026-08-12 -task=m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun, plan=0, tag=TEST +task=m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun, plan=1, tag=REVIEW_TEST ## Archive Evidence Snapshot -- Preserve `run-20260812T044800Z-412e05fc80df` and `run-20260812T050756Z-18293db0b83f`; do not reuse them. -- Dependency: archived PASS `07+06_caller_write_contracts/complete.log`. +- Prior logs: `plan_cloud_G09_0.log`, `code_review_cloud_G10_0.log`; verdict FAIL/R1. +- Old preflight-only id: `run-20260812T053517Z-6b4f4de67128`; never reuse. ## Implementation Item Completion | Item | Status | |---|---| -| TEST-1 runtime recovery | [ ] | -| TEST-2 ready=9 preflight | [ ] | -| TEST-3 one scored run | [ ] | +| REVIEW_TEST-1 bootstrap test fix | [ ] | +| REVIEW_TEST-2 release qualification/deploy | [ ] | +| REVIEW_TEST-3 ready preflight | [ ] | +| REVIEW_TEST-4 scored run | [ ] | ## Implementation Checklist -- [ ] [TEST-1] Prove child 07 PASS, inspect and restore the existing managed dev runtime without changing source. -- [ ] [TEST-2] Run a fresh public CLI preflight and record its emitted id with ready=9. -- [ ] [TEST-3] Run exactly one fresh public CLI scored matrix and record its run id and nine-cell terminal/web results. +- [ ] [REVIEW_TEST-1] Apply R1 test-only lifecycle/timeout fix and pass high-count local/remote regressions. +- [ ] [REVIEW_TEST-2] Complete clean release tests, four-binary rebuild/redeploy, and 4-node healthy verification. +- [ ] [REVIEW_TEST-3] Produce a fresh public ready=9 preflight id. +- [ ] [REVIEW_TEST-4] Execute exactly one public scored run and record nine-cell terminal/workspace results. - [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output. ## Review-Only Checklist -- [ ] Append verdict/routing signals after fresh verification. -- [ ] Verify no direct caller/provider invocation or scored retry occurred. -- [ ] Archive active files and write/move `complete.log` only on PASS. -- [ ] Preserve milestone-task metadata. +- [ ] Append final verdict and routing signals. +- [ ] Freshly verify R1, release identity, ready=9, one run, and web gates. +- [ ] Archive/finalize only after all requirements pass. ## Deviations from Plan -_Record actual deviations or state none._ +_Record actual deviations or none._ ## Key Design Decisions @@ -46,28 +45,29 @@ _Record actual decisions._ ## Reviewer Checkpoints -- Runtime recovery used the existing managed deployment and did not expose secrets. -- Preflight is ready=9 before run. -- Exactly one new scored run id exists, with 9 success and all web gates passing. +- Test cleanup never invokes a second Runtime Start. +- All fake register calls share the established handshake timeout. +- Deployed source is clean release HEAD; four Nodes are connected/healthy. +- Fresh preflight is ready=9; exactly one fresh scored run succeeds 9/9. ## Verification Results -### Dependency and runtime recovery +### R1 high-count tests -_Paste exact commands/output with secret values omitted by command construction._ +_Paste exact output._ + +### Release qualification and deployment + +_Paste exact output/identity summary._ ### Public preflight -Command: `python3 scripts/agent_comparison_benchmark.py preflight --manifest scripts/fixtures/agent-comparison-benchmark-iop-one-shot.json` - -_Paste actual stdout/stderr and exit status._ +_Paste exact output and id._ ### Public scored run -Command: `python3 scripts/agent_comparison_benchmark.py run --manifest scripts/fixtures/agent-comparison-benchmark-iop-one-shot.json` - -_Paste actual stdout/stderr and exit status._ +_Paste exact output, id, status, and artifact gate summary._ ## Section Ownership -Implementation status/checklist/evidence is implementation-owned. Review checklist and verdict are review-only. +Implementation status/checklist/evidence is implementation-owned. Review checklist/verdict is review-only. diff --git a/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/PLAN-cloud-G10.md b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/PLAN-cloud-G10.md new file mode 100644 index 00000000..80c17360 --- /dev/null +++ b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/PLAN-cloud-G10.md @@ -0,0 +1,151 @@ + + +# Bootstrap qualification fix and comparison rerun + +## For the Implementing Agent + +Implement the selected R1 fix exactly, run every verification, then resume the clean release deployment, ready=9 preflight, and exactly one scored run. Fill `CODE_REVIEW-cloud-G10.md`, keep active files in place, and do not create `complete.log` or archive files. + +## Background + +The first live-run plan correctly stopped twice: a preflight found the initial clean release lacked the feature, then the feature-bearing clean release exposed two failure modes in one bootstrap test. The reviewer closed both to stale test-only timeout/cleanup code; production runtime behavior is not changed. + +## Archive Evidence Snapshot + +- `plan_cloud_G09_0.log` / `code_review_cloud_G10_0.log`: FAIL with R1 after no scored run. +- Preflight-only evidence `run-20260812T053517Z-6b4f4de67128`: all cells `model_missing` on pre-feature clean release; preserve, do not reuse. +- Release branch `release/dev-936` now contains clean feature merge `b244b6e224a9d44c09a1e7af49c0788f597c89fe`. + +## Finding Resolution Map + +| ID | Evidence | Root Cause | Selected Fix | Mode | Changed precondition | Acceptance | +|---|---|---|---|---|---|---| +| R1 | remote full suite register timeout; package run duplicate-Start unexpectedly succeeded | obsolete 2s register bound plus cleanup duplicate Start cancels active lifetime | use `fakeNodeHandshakeTimeout` for all fake register calls and cleanup with one Stop | direct-fix | test no longer mutates lifecycle during cleanup and uses the established remote-safe bound | exact test x20, package x10, full suite, rebuild/deploy, ready=9, one scored run | + +## Analysis + +### Files Read + +- `apps/edge/internal/bootstrap/runtime_refresh_node_test.go` +- `apps/edge/internal/bootstrap/runtime.go` +- `apps/edge/internal/transport/server.go` +- `apps/edge/internal/transport/integration_test.go` +- `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/code_review_cloud_G10_0.log` +- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-one-shot-agent-model-comparison/SDD.md` + +### SDD Criteria + +SDD remains READY; milestone-task ids and S04-S08 are unchanged. The live evidence gate remains ready=9 followed by nine successful terminal/workspace results. + +### Verification Context + +Remote runner is `toki@toki-labs.com:/Users/toki/agent-work/iop-dev`, release `dev-936`. No scored run has started. Four Nodes were restored connected before the new release test failed. Token files remain process-local and values must not be printed. + +### Test Coverage Gaps + +R1 is covered by high-count exact/package runs. External endpoint and workspace output remain covered only by the resumed public preflight/run. + +### Symbol References + +None; no production or public symbol changes. + +### Split Judgment + +Keep the fix and resumed deployment in this follow-up because the fixed qualification is the ordered precondition for the same all-cell execution invariant. Child 07 remains satisfied by its archived `complete.log`; no new sibling dependency is introduced. + +### Scope Rationale + +Exclude production Runtime behavior, routes, model aliases, task content, scoring, and roadmap mutation. Only the broken test lifecycle/timeout, evidence pointer files, and active review may change. + +### Final Routing + +- evaluation_mode=`isolated-reassessment`; finalizer=`finalize-task-policy.sh pair` +- build closures all true; scores `2/2/2/2/2` => G10; route `grade-boundary`; `PLAN-cloud-G10.md` +- review closures all true; scores `2/2/2/2/2` => G10; route `official-review`; `CODE_REVIEW-cloud-G10.md` +- large_indivisible_context=false; risks=`temporal_state,boundary_contract,variant_product` (3); review_rework_count=1; evidence_integrity_failure=false; capability gap absent. + +## Implementation Checklist + +- [ ] [REVIEW_TEST-1] Apply R1 test-only lifecycle/timeout fix and pass high-count local/remote regressions. +- [ ] [REVIEW_TEST-2] Complete clean release tests, four-binary rebuild/redeploy, and 4-node healthy verification. +- [ ] [REVIEW_TEST-3] Produce a fresh public ready=9 preflight id. +- [ ] [REVIEW_TEST-4] Execute exactly one public scored run and record nine-cell terminal/workspace results. +- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output. + +### [REVIEW_TEST-1] Close bootstrap test nondeterminism + +**Problem:** `runtime_refresh_node_test.go:517-524` restarts Runtime in cleanup, and register requests in the file retain a shorter timeout than the declared fake-node handshake contract. + +**Solution:** Replace the duplicate Start cleanup with the standard Stop-only form used by neighboring tests. Replace all four fake register `2*time.Second` bounds with `fakeNodeHandshakeTimeout`; do not add retries or production changes. + +**Modified Files and Checklist:** + +- [ ] `apps/edge/internal/bootstrap/runtime_refresh_node_test.go`: exact R1 test-only changes. + +**Test Strategy:** Fresh high-count exact and package tests prove both observed modes are absent. + +**Verification:** `go test -count=20 -run '^TestRefreshConfigApplyNoChangeSkipsNodePush$' ./apps/edge/internal/bootstrap` and `go test -count=10 ./apps/edge/internal/bootstrap` exit 0. + +### [REVIEW_TEST-2] Release qualification and deployment + +**Problem:** The current deployed d40e release is clean but lacks the benchmark feature; b244 source has not passed qualification or been deployed. + +**Solution:** Commit/push R1, merge it into `release/dev-936`, sync the runner, run full pre/post-build suites, rebuild four binaries, config check/dry-run, deploy Edge and all four Nodes, then require connected/healthy state. + +**Modified Files and Checklist:** + +- [ ] `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md`: exact release/deployment evidence. + +**Test Strategy:** Apply dev-runtime deploy contract; any failed required stage stops scored execution. + +**Verification:** clean source/build identity matches release HEAD and CP reports 4 connected Nodes with healthy providers. + +### [REVIEW_TEST-3] Fresh public preflight + +**Problem:** The old preflight belongs to the pre-feature release. + +**Solution:** With protected environment values, call public CLI preflight and store only emitted id. + +**Modified Files and Checklist:** + +- [ ] `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/preflight_id.log`: exact id. + +**Test Strategy:** Require `ready=9 registration_required=0 implementation_gap=0`. + +**Verification:** public preflight exits 0. + +### [REVIEW_TEST-4] One scored run + +**Problem:** No valid generated benchmark output exists. + +**Solution:** Invoke public run exactly once after ready=9, store id, and inspect immutable evidence. + +**Modified Files and Checklist:** + +- [ ] `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/run_id.log`: exact id. + +**Test Strategy:** Require 9 success and all web artifact gates PASS; do not retry on failure. + +**Verification:** public run exits 0 with required counts. + +## Dependencies and Execution Order + +Child 07 PASS remains satisfied. Execute REVIEW_TEST-1 through REVIEW_TEST-4 strictly in order. + +## Modified Files Summary + +| File | Item | +|---|---| +| `apps/edge/internal/bootstrap/runtime_refresh_node_test.go` | REVIEW_TEST-1 | +| `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/CODE_REVIEW-cloud-G10.md` | REVIEW_TEST-2, REVIEW_TEST-3, REVIEW_TEST-4 | +| `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/preflight_id.log` | REVIEW_TEST-3 | +| `agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/run_id.log` | REVIEW_TEST-4 | + +## Final Verification + +1. High-count commands in REVIEW_TEST-1 exit 0. +2. Full dev-runtime pre/post suites and build identity checks pass. +3. Public preflight exits 0 with ready=9. +4. Exactly one public run exits 0 with 9 success and all artifact gates. + +After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`. diff --git a/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/code_review_cloud_G10_0.log b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/code_review_cloud_G10_0.log new file mode 100644 index 00000000..fcd4613a --- /dev/null +++ b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/code_review_cloud_G10_0.log @@ -0,0 +1,115 @@ + + +# Code Review Reference - TEST + +> **[IMPLEMENTING AGENT — READ FIRST]** Start only after child 07 PASS. Fill implementation evidence, leave active files in place, and report ready for review. Finalization is review-only. + +## Overview + +date=2026-08-12 +task=m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun, plan=0, tag=TEST + +## Archive Evidence Snapshot + +- Preserve `run-20260812T044800Z-412e05fc80df` and `run-20260812T050756Z-18293db0b83f`; do not reuse them. +- Dependency: archived PASS `07+06_caller_write_contracts/complete.log`. + +## Implementation Item Completion + +| Item | Status | +|---|---| +| TEST-1 runtime recovery | [ ] | +| TEST-2 ready=9 preflight | [ ] | +| TEST-3 one scored run | [ ] | + +## Implementation Checklist + +- [ ] [TEST-1] Prove child 07 PASS, inspect and restore the existing managed dev runtime without changing source. +- [ ] [TEST-2] Run a fresh public CLI preflight and record its emitted id with ready=9. +- [ ] [TEST-3] Run exactly one fresh public CLI scored matrix and record its run id and nine-cell terminal/web results. +- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output. + +## Review-Only Checklist + +- [ ] Append verdict/routing signals after fresh verification. +- [ ] Verify no direct caller/provider invocation or scored retry occurred. +- [ ] Archive active files and write/move `complete.log` only on PASS. +- [ ] Preserve milestone-task metadata. + +## Deviations from Plan + +Clean release source was required before a scored run. The first rebuilt release lacked the feature source and public preflight `run-20260812T053517Z-6b4f4de67128` correctly stopped with 9 `model_missing` cells. After committing/merging the feature into `release/dev-936`, the mandatory pre-build suite exposed a new deterministic test defect, so no scored run was started. + +## Key Design Decisions + +Preserve both preflight-only runs. Do not bypass the failed pre-build suite or deploy `+dirty` source. Repair the exact bootstrap test lifecycle/timeout defect, rerun the full release qualification, then resume preflight and the single scored run. + +## Reviewer Checkpoints + +- Runtime recovery used the existing managed deployment and did not expose secrets. +- Preflight is ready=9 before run. +- Exactly one new scored run id exists, with 9 success and all web gates passing. + +## Verification Results + +### Dependency and runtime recovery + +```text +release source before merge: d40e4494e767e3fec7796c670f58130a9a194e80 +first rebuilt Edge source: d40e4494e767 (clean) +ports: 18082/18083/18084/19093/19101 listening +Control Plane: 4/4 nodes connected; 8 providers healthy/available; in_flight=0 queued=0 + +fresh release b244b6e224a9d44c09a1e7af49c0788f597c89fe pre-build test: +--- FAIL: TestRefreshConfigApplyNoChangeSkipsNodePush (2.02s) + runtime_refresh_node_test.go:575: register: request timeout for nonce 1 + +isolated probes: +3 x exact test: PASS +full bootstrap package: +--- FAIL: TestRefreshConfigApplyNoChangeSkipsNodePush (0.00s) + runtime_refresh_node_test.go:518: expected already started error +``` + +### Public preflight + +Command: `python3 scripts/agent_comparison_benchmark.py preflight --manifest scripts/fixtures/agent-comparison-benchmark-iop-one-shot.json` + +```text +error: preflight blocked run_id=run-20260812T053517Z-6b4f4de67128 status=registration_required ready=0 registration_required=9 implementation_gap=0 +``` + +The immutable preflight JSON classifies every cell as `model_missing`. This came from the clean d40e release not yet containing the feature; it is not accepted as ready evidence. + +### Public scored run + +Command: `python3 scripts/agent_comparison_benchmark.py run --manifest scripts/fixtures/agent-comparison-benchmark-iop-one-shot.json` + +Not run. The mandatory clean-release pre-build suite failed before the scored-run gate. + +## Code Review Result + +- Verdict: FAIL +- Required: 1 +- Suggested: 0 +- Nit: 0 +- Dimension Assessment: + - Correctness: Fail — bootstrap qualification is nondeterministic under its own test lifecycle. + - Completeness: Fail — ready=9 and scored execution are not yet produced. + - Test coverage: Fail — the intended handshake timeout constant is not used by register calls and cleanup asserts an unsupported duplicate Start lifecycle. + - API contract: Pass — public preflight stopped without substitution. + - Code quality: Warn — one test contains a destructive duplicate Start in cleanup. + - Plan deviation: Pass — the new source defect was preserved and routed instead of bypassed. + - Verification trust: Pass — both distinct failure modes were reproduced on the declared remote release runner. +- Findings: + - R1 Required + - Evidence: `apps/edge/internal/bootstrap/runtime_refresh_node_test.go:517-524` calls `Start` again in cleanup; `Runtime.newLifetimeContext` cancels the live lifetime before the second transport bind. The same test's registration at `:569-575` still uses `2*time.Second` while `fakeNodeHandshakeTimeout=5*time.Second` exists for this remote/race stall class. Remote full suite produced register timeout; isolated package produced duplicate-Start success. + - Root Cause: stale test-only lifecycle and timeout code; the duplicate Start can cancel/rebind the active runtime, and the register handshake retains the obsolete short bound. + - Selected Fix: replace the cleanup with one `rt.Stop()` call and replace all fake-node register `2*time.Second` bounds in this file with `fakeNodeHandshakeTimeout`. Add no production lifecycle change. Accept with exact test `-count=20`, package `-count=10`, full dev-runtime suite, then rebuild/redeploy and resume ready=9 plus one scored run. + - Disposition: direct-fix. +- Routing Signals: review_rework_count=1, evidence_integrity_failure=false. +- Next Step: materialize REVIEW_TEST follow-up and continue automatically. + +## Section Ownership + +Implementation status/checklist/evidence is implementation-owned. Review checklist and verdict are review-only. diff --git a/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/PLAN-cloud-G09.md b/agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/plan_cloud_G09_0.log similarity index 100% rename from agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/PLAN-cloud-G09.md rename to agent-task/m-iop-one-shot-agent-model-comparison/08+07_comparison_rerun/plan_cloud_G09_0.log diff --git a/apps/edge/internal/bootstrap/runtime_refresh_node_test.go b/apps/edge/internal/bootstrap/runtime_refresh_node_test.go index ee154f6a..f464b4e0 100644 --- a/apps/edge/internal/bootstrap/runtime_refresh_node_test.go +++ b/apps/edge/internal/bootstrap/runtime_refresh_node_test.go @@ -183,7 +183,7 @@ nodes: regResp, err := toki.SendRequestTyped[*iop.RegisterRequest, *iop.RegisterResponse]( &fakeNode.Communicator, &iop.RegisterRequest{Token: "tok-refresh"}, - 2*time.Second, + fakeNodeHandshakeTimeout, ) if err != nil { t.Fatalf("register: %v", err) @@ -402,7 +402,7 @@ nodes: regResp, err := toki.SendRequestTyped[*iop.RegisterRequest, *iop.RegisterResponse]( &fakeNode.Communicator, &iop.RegisterRequest{Token: "tok-conn"}, - 2*time.Second, + fakeNodeHandshakeTimeout, ) if err != nil { t.Fatalf("register: %v", err) @@ -514,13 +514,8 @@ nodes: t.Fatalf("Start: %v", err) } defer func() { - if err := rt.Start(context.Background()); err == nil { - t.Fatal("expected already started error") - } else { - // runtime is running, stop it - if err := rt.Stop(); err != nil { - t.Fatalf("Stop: %v", err) - } + if err := rt.Stop(); err != nil { + t.Fatalf("Stop: %v", err) } }() @@ -569,7 +564,7 @@ nodes: regResp, err := toki.SendRequestTyped[*iop.RegisterRequest, *iop.RegisterResponse]( &fakeNode.Communicator, &iop.RegisterRequest{Token: "tok-noc-change"}, - 2*time.Second, + fakeNodeHandshakeTimeout, ) if err != nil { t.Fatalf("register: %v", err) @@ -878,7 +873,7 @@ nodes: regResp, err := toki.SendRequestTyped[*iop.RegisterRequest, *iop.RegisterResponse]( &fakeNode.Communicator, &iop.RegisterRequest{Token: "tok-rt"}, - 2*time.Second, + fakeNodeHandshakeTimeout, ) if err != nil { t.Fatalf("register: %v", err)