review-all-impls

Metadata

Description

TASK <id>: VERDICT concur
Rationale: <2-4 sentences on diff + tests + scores>

TASK <id>: VERDICT concerns
Items:
  - <file:line> — <specific issue>
  - <file:line> — <specific issue>
Rationale: <why these matter; whether they block integration or are follow-ups>

OVERALL: <ship | iterate | escalate>
- ship: every task concur, integration smoke passes
- iterate: ≥1 task has concerns that should be addressed before SYN smoke runs
- escalate: cross-impl pattern (e.g. all four impls misuse the same primitive) needs human attention

## Description
Cross-model peer review of all five impl tasks (I1-I4 + INT). Each impl ran on codex:gpt-5.5; this review runs on claude:opus per the user's modulation 2026-05-02 (pattern C: opus reviews codex's work, including the eval verdict, and emits a calibrated cross-model verdict).

Originally planned as one review per impl, consolidated into a single combined review because the 10-task subtask cap on the design agent ran out. The consolidation is acceptable because the reviewer sees the full delta as one coherent change before issuing a verdict.

## What to read
For each of fix-nex-cursor-corruption, fix-supervisor-restart-backoff, fix-tui-supervisor-coexistence, fix-chat-dir-race, integrate-nex-chat-end-to-end:
- `git log --oneline main..<impl-branch>` — commits on the impl agent's worktree branch
- `git diff main..<impl-branch>` — full diff
- `wg show <task-id>` — Validation checklist + log entries + Evaluations section (LLM eval + FLIP scores)

Then look at the SYSTEM-LEVEL signal:
- `wg show smoke-tui-nex-end-to-end` once that's run (the simulated-human end-to-end is the ultimate truth)

## What to produce (via wg log on review-all-impls)

For EACH of the five tasks:

### Form A — concur
```
TASK <id>: VERDICT concur
Rationale: <2-4 sentences on diff + tests + scores>
```

### Form B — concerns
```
TASK <id>: VERDICT concerns
Items:
  - <file:line> — <specific issue>
  - <file:line> — <specific issue>
Rationale: <why these matter; whether they block integration or are follow-ups>
```

Then a final OVERALL:
```
OVERALL: <ship | iterate | escalate>
- ship: every task concur, integration smoke passes
- iterate: ≥1 task has concerns that should be addressed before SYN smoke runs
- escalate: cross-impl pattern (e.g. all four impls misuse the same primitive) needs human attention
```

## Operating constraints
- READ ONLY — no source mods.
- Independence — form your verdict from the diff + tests + scores, not from the impl agent's self-assessment.
- Calibrated — disagree with the eval verdict if warranted (flag as a separate concern).
- Specific — every concerns item cites file:line.

## Validation
- [ ] All five tasks reviewed (one verdict each)
- [ ] OVERALL summary produced
- [ ] At least 2 file:line citations per non-concur task
- [ ] No source modifications

Depends on

• done fix-nex-cursor-corruption
• done fix-supervisor-restart-backoff
• done fix-tui-supervisor-coexistence
• done fix-chat-dir-race
• done integrate-nex-chat-end-to-end
• done .assign-review-all-impls

Required by

• done smoke-tui-nex-end-to-end

Log

• 2026-05-02T23:58:21.224816122+00:00 Task paused
• 2026-05-03T00:51:27.907169360+00:00 Task published
• 2026-05-03T04:44:29.270325847+00:00 Spawned by coordinator --executor claude --model opus
• 2026-05-03T04:44:51.984431582+00:00 Starting cross-model peer review of 5 impl tasks (I1-I4 + INT). Reading each branch's diff, validation, and eval scores.
• 2026-05-03T04:53:15.301462733+00:00 Reviewed all 5 impls. fix-supervisor-restart-backoff has no commit on main and no diff in scoped files — confirmed via git show + grep for backoff logic. The other 4 impls landed cleanly with passing regression tests.
• 2026-05-03T04:53:29.241786460+00:00 TASK fix-nex-cursor-corruption: VERDICT concur Rationale: 11 LOC core fix (is_safe_launcher_field_char filters control chars + U+2580–U+259F block elements + line separators) applied to handle_paste and 3 launcher KeyCode::Char handlers in src/tui/viz_viewer/event.rs. Two regression tests pin the behavior: launcher_paste_filters_rendered_cursor_glyph_from_endpoint and launcher_char_input_filters_block_elements_from_add_new_fields — both pass. Scope respects task description (event.rs only). Eval LLM=0.79, FLIP=0.44 — FLIP low because intent_fidelity counts 'manual repro performed' which agent skipped in noninteractive session (acceptable substitute via regression tests).
• 2026-05-03T04:53:58.072597106+00:00 TASK fix-supervisor-restart-backoff: VERDICT concerns Items: - src/commands/service/coordinator_agent.rs:892 — restart_timestamps.push_back(Instant::now()) fires on EVERY successful spawn including clean restarts after TUI handoffs. The intended fix would gate this push with an exit_status==1 + recent session-lock-busy check so genuine lock contention is what counts toward the rate limit, not normal handoff cycles. - src/commands/service/coordinator_agent.rs:629-661 — MAX_RESTARTS_PER_WINDOW rate limiter therefore trips on non-contention restarts; integrate-nex-chat-end-to-end's agent flagged that 3 normal TUI handoff cycles will trigger the 10-min pause. The per-cycle .tui-driven sentinel deferral added by fix-tui-supervisor-coexistence partially masks this in steady state but does not address the root cause. Rationale: This task has zero commits and zero diff — the evaluator correctly flagged 'no implementation artifacts found in assigned worktree; branch has 0 commits ahead of main' and returned LLM=0.04 / FLIP=0.01 / intent_fidelity=0.01. Despite that failing evaluator verdict the task transitioned PendingEval → Done. The intended fix never landed. The integration agent created follow-up 're-implement-fix' to re-attempt; that's the right path. This blocks the SYN smoke only if a scenario exercises ≥MAX_RESTARTS_PER_WINDOW handoffs — current integrate_nex_chat_end_to_end.sh does only 1 spawn so it does not catch this gap.
• 2026-05-03T04:54:11.349002423+00:00 TASK fix-tui-supervisor-coexistence: VERDICT concur Rationale: Replaces the prior 'request-release-then-SIGTERM-stuck-handler' approach with a cooperative .tui-driven sentinel: TUI writes pid to chat_dir/.tui-driven before requesting handler release; supervisor checks the sentinel before each respawn and defers 5s if alive (src/commands/service/coordinator_agent.rs:751-758, src/session_lock.rs:290-353). Sentinel staleness is handled via pid_is_alive — killed TUI with leftover sentinel will not deadlock the supervisor. TUI clears the sentinel on close_tab/abandon/archive/PTY-spawn-failure paths (state.rs). 3 unit tests pass: tui_driver_sentinel_round_trip, stale_tui_driver_sentinel_is_not_alive, test_tui_sentinel_defers_supervisor_respawn_only_while_alive. Scope respects task (session_lock.rs, coordinator_agent.rs, state.rs). Eval LLM=0.74, FLIP=0.69. Minor concern (not blocking): the new flow drops the SIGTERM fallback for genuinely stuck handlers — observer_mode now waits 5s for cooperative release and then proceeds — fine for healthy handlers, slightly worse for wedged ones; trade-off acceptable per design.
• 2026-05-03T04:54:19.900809466+00:00 TASK fix-chat-dir-race: VERDICT concur Rationale: Minimal, surgical fix in src/chat_sessions.rs:335-337 — fs::create_dir_all(&chat_dir) inserted into register_coordinator_session immediately after the UUID is resolved, before alias registration. Ensures the on-disk dir exists before any IPC writer calls chat_dir_for_uuid and tries to write inbox/outbox. Regression test register_coordinator_session_creates_missing_uuid_chat_dir builds the exact pre-condition (registry has session row but dir is missing) and asserts post-call dir.is_dir(). Test passes. tests/integration_chat.rs:61 adds '--route claude-cli' to wg init for fresh-init env; defensive but unrelated to the race itself. Scope respects task (chat_sessions.rs + tests/integration_chat.rs). Eval LLM=0.79, FLIP=0.93 — strongest scores in the cohort.
• 2026-05-03T04:54:32.442270790+00:00 TASK integrate-nex-chat-end-to-end: VERDICT concur Rationale: 11 LOC composition glue in src/commands/spawn_task.rs:181-183 — adds .chat-N → chat-N strip mirroring the existing .coordinator-N → coordinator-N strip, so the handler resolves the same registered alias the IPC writers use (no UUID-vs-literal split-brain). Regression test dot_chat_id_strips_leading_dot_for_chat_ref pins the behavior. Pre-existing chat_native_endpoint_full_pipeline.sh smoke updated for the new chat_ref preview. New permanent smoke tests/smoke/scenarios/integrate_nex_chat_end_to_end.sh added with comprehensive owners list (5 fixes + smoke-gate-is); asserts no literal .chat-0 dir exists alongside UUID dir, sends 2 messages, requires ≥2 inbox/outbox entries, requires ≤2 supervisor spawns. Live-validated against lambda01/qwen3-coder per agent's log. Eval LLM=0.91, FLIP=0.73. Agent transparently flagged the missing fix-supervisor-restart-backoff and created the follow-up 're-implement-fix' rather than papering over it.
• 2026-05-03T04:55:12.502840173+00:00 OVERALL: escalate Four of five impls (fix-nex-cursor-corruption, fix-tui-supervisor-coexistence, fix-chat-dir-race, integrate-nex-chat-end-to-end) are concur — clean diffs, scoped to declared files, regression tests present and passing, evaluator scores 0.69–0.93. The integration agent did the right thing in flagging gaps explicitly rather than masking them. The escalation trigger is fix-supervisor-restart-backoff: it transitioned PendingEval → Done with an evaluator score of 0.04 / intent_fidelity 0.01 and an explicit 'no implementation artifacts found' finding. Two distinct issues: 1. The intended exit-status-1 + lock-busy backoff was never implemented; the rate-limit bug at coordinator_agent.rs:892 is unfixed and three normal TUI handoff cycles can still trip the 10-min pause once fix-tui-supervisor-coexistence's sentinel deferral expires. 2. A workgraph state-machine question: how did a 0.04 eval pass the PendingEval → Done gate? Either the threshold is too lenient or the FLIP/LLM verdicts were not consulted by the gate. Worth a separate human review of the agency.flip_threshold / done-gate logic. The integration smoke (integrate_nex_chat_end_to_end.sh) does NOT exercise the missing backoff — only 2 messages → 1 spawn, well below MAX_RESTARTS_PER_WINDOW. Smoke-tui-nex-end-to-end (still open) might trigger it depending on the simulated-human flow. Recommended action: do not block SYN smoke on this task — the unrelated four fixes integrate cleanly and provide value. But before declaring 'nex chat in TUI is shipped', land the supervisor backoff via the already-created 're-implement-fix' follow-up, and have a human inspect the eval-to-done threshold.
• 2026-05-03T04:55:24.357647343+00:00 Task pending eval (agent reported done; awaiting `.evaluate-*` to score)
• 2026-05-03T04:57:47.180810543+00:00 PendingEval → Done (evaluator passed; downstream unblocks)