docs(gap-baseline): record noema-review serving-timeout root cause

docs/product-technical-gap-baseline.md

@@ -2343,6 +2343,185 @@ sync: `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s default,
 contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md`'s
 "today" reference. Landed in the same PR (`#1463`) as the streaming revert,
 not split out, since the revert is unsafe without it.
+## 2026-08-31 noema-review serving-call timeout: retry×failover budget mismatch, root-caused
+
+**A distinct failure mode from the 2026-08-30 preflight-outage entries above**: those covered the
+sidecar's own *startup* preflight (`healthz`, the launcher's `_preflight_with_fallback`) rejecting
+candidates or the running server's virtual-model route returning 502. This entry covers a case where
+preflight succeeds — `contextual-orchestrator#956`'s `noema-review` run (`run 33439328660` / job
+`99643408237`) reached `[contextual-orchestrator-sidecar] gateway chat/completions preflight
+confirmed (attempt 1/3)` and reported `free_account_diversity: 3`, `ready_count: 2` of 12 probed
+routes — and the *serving* request still fails with a raw `TimeoutError: timed out` at
+`scripts/ci/noema_review_gate.py:656`'s `opener.open(request, timeout=120)`.
+
+Root cause (verified against job logs, not speculation; full arithmetic in
+`contextual-orchestrator#974`'s PR body): `noema_review_gate.py`'s single serving request carries a
+fixed 120s socket timeout with **zero retries on the caller side**, but the vendored
+`contextual_orchestrator_review_launcher.py` constructs its *serving* `ModelClient`/`TaskOrchestrator`
+with no overrides — general-purpose defaults `ModelClient(timeout=90, max_retries=2)` and
+`TaskOrchestrator`'s default `tool_retry_attempts=1`. Three multiplying layers, each individually
+correct for their general-purpose callers, combine into a budget the external 120s caller has no
+visibility into: (1) `ModelClient._send_with_retry`, up to 3 HTTP attempts per `chat()` call ≈ 271.5s
+worst case for one call alone — already 2.26x the external budget; (2) `_invoke`'s same-agent tool
+retry (`tool_retry_attempts=1` ⇒ 2 full `chat()` calls per agent) ≈ 543s worst case per agent; (3)
+`_invoke`'s cross-candidate failover over the review sidecar's up-to-12-route catalog with no combined
+ceiling ≈ 6519s worst case if every candidate fails. Even without layers 2-3, layer 1 alone
+(`2×90=180s`) already exceeds 120s. This also explains why the *preflight* correctly reports a route
+"ready": the preflight client is deliberately bounded (`timeout=10s, max_retries=0`) and uses a tiny
+~60-character probe, while the real review's diff+context payload (up to 60,000 + 24,000 chars) under
+the *serving* client's un-tuned defaults can legitimately need more than one 90s attempt, triggering
+the retry cascade above.
+
+**Fix, in flight across both repos, not yet landed as of this entry:**
+
+- `contextual-orchestrator#974` adds an opt-in, additive `deadline_seconds` parameter to
+  `TaskOrchestrator.route_once()` (threaded into `_invoke()` as an absolute deadline) bounding the
+  *combined* wall-clock across same-agent retry and cross-candidate failover to a caller-chosen
+  ceiling. Left `None` (the default), every existing caller's behavior is unchanged — confirmed by the
+  full existing test suite passing unmodified. This repository alone cannot fix the symptom: the
+  values that need to change are constructed entirely inside this org's `.github` repo.
+- `ContextualWisdomLab/.github#1415` is the companion change to
+  `scripts/ci/contextual_orchestrator_review_launcher.py`'s serving-client construction (bounding
+  `ModelClient`'s serving timeout/retries and `TaskOrchestrator`'s `tool_retry_attempts`).
+- **Correction 1 (owner direction, 2026-08-31, on this entry's own PR #1530):** a small 130-150s margin
+  on `noema_review_gate.py`'s socket timeout, as originally proposed above, is too tight — raise it to
+  a 3-hour budget instead, matching the standing directive's own accepted tolerance (§8) for central
+  review taking multiple hours per model.
+- **Correction 2 (supersedes Correction 1, same day):** the repo owner's fuller cross-PR architectural
+  contract, posted on `contextual-orchestrator#979`, states the principle more precisely: *"Do not
+  impose fixed wall-clock deadlines on inference, initial ping, readiness/health, provider discovery,
+  or OpenRouter ZDR-list retrieval; use explicit cancellation and evidence-backed transport failure
+  instead."* Not "a longer fixed number" but no fixed client-side deadline at all. `.github#1508`
+  ("fix(noema): preserve long-running substantive reviews", open, in flight) already implements exactly
+  this for `noema_review_gate.py`: its diff changes
+  `opener.open(request, timeout=120)` → `opener.open(request)` (the timeout argument dropped entirely,
+  not raised) and the same PR's earlier `curl -sS --max-time 120 ...` gateway-preflight probe drops
+  `--max-time 120` the same way, so the review runs "for the enclosing GitHub job lifetime" (bounded
+  only by the workflow's own `timeout-minutes`) instead of racing any client-imposed number, 120s or
+  10800s alike. `#1415`'s own serving-timeout work on the same file should reconcile with `#1508`
+  rather than reintroduce a bounded number — flagged as a comment on both PRs; not resolved here.
+  `contextual-orchestrator#974`'s opt-in `deadline_seconds` bound is under the same tension: it is a
+  fixed wall-clock deadline mechanism, which `#971` ("fix(routing): select concrete free model groups",
+  open, in flight) addresses instead by *removing* fixed wall-clock deadlines from inference, initial
+  completion probes, and equivalent-endpoint races and by keying routing identity on `model_group`
+  rather than provider grouping — also flagged as a comment on `#974`, not resolved here.
+- A secondary, unrelated diagnosability gap noted alongside this fix (not yet fixed): `main()`'s
+  top-level handler in `noema_review_gate.py` only catches `RuntimeError`; `TimeoutError` is an
+  `OSError` subtype, so this failure surfaces as a raw uncaught Python traceback instead of the gate's
+  own clean, redacted error path.
+
+Standing-down comment with this analysis posted on `contextual-orchestrator#956` (not a defect in that
+PR's own diff); tracked for re-check once #974/#1415 land. `#1519` in this repo hit a related-but-
+distinct symptom the same day — a required `strix` check cancelled by this repo's own per-repository
+Strix concurrency group (a sibling PR's scan superseding a queued/pending run,
+`.github/workflows/strix.yml`'s documented `cancel-in-progress: false` behavior) with no automatic
+re-dispatch observed 8+ hours later despite the workflow's own comment describing one; manually
+re-triggered via the Actions API as a stopgap, not yet root-caused as a scheduler bug.
+
+**Correction 3 (owner decision, 2026-09-01): `contextual-orchestrator#979` closed as policy-incompatible,
+not merged.** `#979` was one contributor to the free-pool preflight budget waste this whole entry
+documents — a hand-maintained denylist (`_NVIDIA_NIM_RETIRED_MODEL_IDS` checked against
+`_NVIDIA_NIM_PROVIDER_NAMES = {"nvidia_nim", "nvidia_nim_sub"}`) excluding two catalog-listed-but-
+HTTP-404-retired NVIDIA NIM model ids from `is_routable_discovered_model`. Despite a same-session
+investigation (confirmed via `git merge-tree` against `#971`) concluding this denylist was *not* a
+routing-identity/provider-family abstraction — it operates on the raw pre-`ModelAgent` eligibility
+gate, before any grouping or routing selection — the owner's ruling on the closing comment overrides
+that read: *"That is a provider-specific, hand-authored routing rule and conflicts with the required
+provider-neutral `model_group` contract and the prohibition on keyword/list heuristics."* The
+underlying problem (two dead ids burning preflight budget every run, confirmed live on `#957`/`#961`)
+remains real and unfixed as of this entry.
+
+**Required replacement design, per the same comment, quoted in full:** *"The replacement must record
+structured runtime serving evidence for an exact deployment identity (endpoint + credential/account +
+concrete model/model-group member), mark a terminal completion 404 unavailable through the shared
+measured-routing path, and allow later successful evidence to recover it. No provider-name or
+model-id literals. The text+image regression may be retained only with a provider-neutral fixture.
+`#971` remains the implementation parent."* i.e.: no hand-maintained list at all — a terminal 404 on a
+concrete deployment identity should feed the same `ModelGroupRouter`/measured-routing evidence
+ledger `#971` is introducing (`model_group_name_for()`), marking that identity unavailable until a
+later successful call recovers it, self-healing rather than requiring a human edit for every future
+retirement. Relayed as a comment on `#971` (named the implementation parent, still open and actively
+iterating) rather than reopening `#979` or opening a replacement PR — per the harness's own instruction
+after a PR closes without merging, do not reopen or replace it unless explicitly asked.
+
+## 2026-09-01 owner directive: broadcast review-quality and governance instruction
+
+The owner posted an identical instruction as a comment on 14 open PRs simultaneously
+(`contextual-orchestrator#955/#956/#957/#961`, `.github#1437/#1443/#1478/#1482/#1492/#1495/#1498/#1530`,
+plus two more), a broadcast rather than 14 independent asks. Recording the full instruction and this
+session's response once here rather than 14 times.
+
+**Instruction (paraphrased from the Korean original, preserved in full on the PR comments themselves):**
+1. Fix/resolve everything related to contextual-orchestrator, one way or another.
+2. **Bypass merge is explicitly authorized when needed, for chicken-and-egg situations** — the first
+   time this standing session has received merge-bypass authorization from the owner directly, though
+   scoped to genuine bootstrap deadlocks, not general impatience with review latency.
+3. Using NVIDIA NIM *alone* is not allowed; must route through contextual-orchestrator.
+4. Timeout should be **at least 3 hours** — "something like 120 seconds is bewildering." Reaffirms and
+   strengthens the 2026-08-31 entry above; combined with that entry's finding that `.github#1508`
+   already removes the client-side timeout *entirely* (not merely extends it), "at least 3 hours" reads
+   as a floor compatible with "no fixed deadline," not a contradiction of it.
+5. Make OpenCode and Noema actually review at CodeRabbit/Devin's depth.
+6. Make Strix's security review thorough and run it over the **whole codebase**, not just the diff;
+   the owner has observed Strix legitimately run 6+ hours to catch a vulnerability.
+7. contextual-orchestrator should route review/repair traffic to fast, capable models to cut latency,
+   not just free ones.
+8. A secondary note: internet documentation describes OpenCode's own slash-command trigger as `/oc`,
+   worth checking against this org's `@opencode-agent` convention.
+
+**This session's response, item by item:**
+
+- **(2) Bypass merge — assessed, not exercised.** Checked `.github#1443` (the PR the owner's first
+  comment named specifically) for a genuine chicken-and-egg deadlock: its `opencode-review-target`
+  job depends on `coverage-evidence`, which was still queued (not yet started) at check time, and the
+  org has had severe cross-repo CI-runner queue congestion all session (`contextual-orchestrator#961`'s
+  CI queued 1+ hour before even starting; `.github#1519`'s Strix scan cancelled twice by concurrency-
+  group contention). This reads as ordinary congestion-induced delay, not a structural loop where the
+  check can never produce a verdict — #1443 is not itself a draft PR, so it doesn't trip the very bug
+  it fixes, and nothing about its diff touches the review-*dispatch* mechanism (only the verdict-check
+  bash and a later dispatch-step draft guard). Not bypassing it on this reading; keeping it watched and
+  will reassess if it's still stuck once the CI queue visibly clears elsewhere.
+- **(3) NVIDIA NIM direct-usage audit — clean for active paths, one dormant item flagged.** Full-repo
+  audit (`.github`, current `main`): every workflow site injecting `NVIDIA_NIM_API_KEY`/`_SUB` feeds
+  only `contextual_orchestrator_review_sidecar.sh`'s own loopback (`127.0.0.1:<port>`) credential set —
+  `noema-review.yml`, `pr-review-autofix.yml`, `strix.yml` (which additionally hard-rejects any
+  `STRIX_MODEL_REQUESTED` other than `contextual-orchestrator/orchestrator/free`), and
+  `opencode-review-dispatch.yml` (`OPENCODE_MODEL_CANDIDATES` hardcoded to the single gateway
+  candidate). No direct `integrate.api.nvidia.com` call reachable from any of these in CI. **Dormant,
+  not currently reachable, but retains direct-call capability**: `opencode.jsonc`'s checked-in
+  `nvidia-nim` provider block (`baseURL: https://integrate.api.nvidia.com/v1`) and
+  `run_opencode_review_model_pool.sh`'s `is_nvidia_nim_candidate()` mapping — inert only because the
+  one real invocation hardcodes the gateway candidate and `test_strix_quick_gate.sh` asserts no
+  workflow ever supplies an `nvidia-nim/*` candidate. Same latent-armed-code pattern as the
+  already-removed `scripts/ci/select_nvidia_nim_model.py` (orphaned direct-NIM resolver, removed for
+  having no callers) — worth a small follow-up PR removing both, not done in this pass.
+- **(8) `/oc` vs `@opencode-agent` — resolved, not a bug.** `docs/automation/review-agent-comment-
+  invocation.md` documents `@opencode-agent` as this org's own deliberately-engineered cross-repo
+  mention router (SHA-256-bound invocation keys, exact-name artifact ledger, five-minute organization
+  sweep), built specifically because GitHub's `issue_comment` event does not fire for a workflow file
+  that lives only in the central `.github` repo when the comment is posted on a sibling repo's PR.
+  `/oc` (if that is a real vendor-native OpenCode slash-command) would be a *different* integration
+  model — a single-repo GitHub App receiving `issue_comment` natively — solving a structurally
+  different problem than this org's cross-repo dispatch need. Intentional divergence, not a
+  mismatch to fix.
+- **(While handling the broadcast) `.github#1482` merge conflict resolved.** One real textual conflict
+  in `scripts/ci/noema_review_gate.py::inspect_and_review` between this PR's pre-POST duplicate-
+  submission re-check and `main`'s newly-landed (`#1508`) `changed_paths` threading into `call_llm` —
+  kept both, re-check now runs after the `changed_paths`-aware call. One test needed a new mock
+  (`fetch_changed_file_paths`) it hadn't needed before the merge. Pushed, full suite validating.
+- **(1)(4)(5)(6)(7) — acknowledged, not yet implemented; recorded as the next concrete work items,
+  not claimed done.** (4) is tracked and already substantively addressed via the 2026-08-31 entry
+  above (`#1508`/`#971`, in flight). (5), (6), and (7) are real, large, contract-tested-file changes
+  (`ci-review-prompt.md`, `code-reviewer-prompt.md`, `opencode.jsonc`, `strix.yml`'s
+  `timeout-minutes: 200`/`170` job/step caps and its diff-only path-filter scope) that deserve
+  dedicated, careful sessions rather than a rushed edit inside this already-large response — editing
+  a contract-pinned prompt or workflow file without matching test updates breaks CI immediately per
+  this repo's own convention (`tests/` asserts exact prose of several of these files). Next session
+  picking up this thread should start here: read `ci-review-prompt.md`/`code-reviewer-prompt.md` in
+  full against a CodeRabbit/Devin finding-depth bar, read `strix.yml`'s current scan-scope path
+  filters (currently diff-changed-files-gated, per the file's own header comments) against "whole
+  codebase," and read contextual-orchestrator's `orchestrator/free` selection policy for a
+  speed/capability-weighted route distinct from the existing cost-only ranking.
 
 ## 5. 실행 루프와 고객의 다음 행동