fix(coverage): unblock org-wide OpenCode approval + docs(gaps) corrections - #1438
fix(coverage): unblock org-wide OpenCode approval + docs(gaps) corrections#1438seonghobae wants to merge 69 commits into
Conversation
…ma increment #1297 was already merged and #1345/#1326 were closed unmerged, but all three were still listed as pending candidates for this loop's next pass. Replace with current state and record this pass's actual increment: ContextualWisdomLab/naruon#1486 adds a check_calendar_conflict tool to naruon's noema-general-agent, reusing the existing deterministic conflict policy instead of a second one, and clarifies that naruon's Noema and this repo's central review-bot Noema are separate agents sharing only a name.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSidecar의 stderr 진단 출력 한도를 20줄에서 60줄로 확장했습니다. Provider-family별 catalog 후보 상한을 4에서 8로 변경했습니다. 선행 0이 포함된 잘못된 재시도 설정을 거부하고 관련 테스트와 운영 문서를 갱신했습니다. ChangesSidecar 진단 및 운영 기록
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The change mainly improves failure diagnostics and documentation, but the sidecar still has a configuration edge case where leading-zero zero values can disable retries through an immediate failure; the PR is mergeable with explicit owner awareness and follow-up. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (1 skipped: 1 unsupported.) Full details: Title checkExplanation 제목의 ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Generated by Claude Code |
naruon#1486 confirms the current sidecar pin (30c6d716...) actually reaches a hosted PR-target run, but still fails closed with a new signature: a non-fatal 413 on ZDR-catalog prefetch (falls back to a live feed) followed by bytez discovery returning HTTP 500, which empties the orchestrator/free pool the same way the already-tracked structural gap describes. Also confirms opencode-review's gate correctly fails closed awaiting the async dispatch verdict on fresh heads (naruon#1486, .github#1438) — expected, not a defect.
|
Generated by Claude Code |
…tail The review sidecar's live warm-up preflight already records a bounded error_type/http_status per rejected route, but only into a JSON artifact -- not the CI job's visible console log. That made a real fail-closed incident impossible to diagnose as transient or not without downloading the artifact separately, and led directly to a misdiagnosis this pass corrects in docs/product-technical-gap-baseline.md. Add _log_preflight_rejections (mirrors the existing _log_discovery_errors visibility fix), a matching sanitizer allowlist entry, and widen the failure-path stderr tail from a fixed 20 lines to a named SIDECAR_STDERR_TAIL_LINES=60 so discovery errors plus preflight rejections plus summary lines can no longer silently truncate. Companion to ContextualWisdomLab/contextual-orchestrator#923, which fixes the analogous single-shot discovery fetch. This repo's own completion-warm-up-probe call site is deliberately left single-shot; see that PR's description for the latency/amplification risk that ruled out retrying it blind.
Adversarial re-investigation (triggered by direct feedback that a single provider erroring should never fail-close org-wide review CI) found the "2026-08-30 post-#1486/#1438 wake" entry misdiagnosed the incident on two counts: Bytez can never populate orchestrator/free regardless of HTTP status (_parse_bytez never sets is_free), and the 413 line is the sidecar's own unconditional self-test, not a live ZDR-prefetch fallback -- also present in two earlier entries, flagged here rather than hand-edited there. The actual terminating message was "review sidecar preflight failed" (a live warm-up-probe rejection), not the "no eligible models" path those entries claimed. Records the real root cause (two single-shot HTTP call sites with no retry) and the fix that follows: contextual-orchestrator#923 (discovery retry) and this repo's own #1438 (preflight-rejection visibility + wider stderr tail). Updates §5.1 to track both to merge.
seonghobae
changed the title
main advanced with the owner's own parallel investigation into the same sidecar-preflight incident this branch was fixing, with far more precise evidence than this branch's own analysis had (actual hosted-run preflight/discovery artifacts, not just log-pattern reading). The real root cause turned out to be contextual_orchestrator_review_policy.py's family_cap selecting the same alphabetically-first candidates every run -- 2 of which are permanently-retired NVIDIA model ids returning HTTP 404 forever, not a transient failure -- plus a too-tight gateway smoke-test timeout and a max_tokens/probe-budget desync. All three are already fixed on main (family_cap 4->8, gateway timeout 30s->120s, max_tokens 16->4096). Conflict resolution: - CHANGELOG.md / docs/product-technical-gap-baseline.md: kept both sides' entries; revised this branch's own "correction" entry to defer to main's much more thorough "sidecar-preflight outage: consolidated evidence" entry as the authoritative root cause and fix, rather than presenting this branch's own generic "two call sites, no retry" theory as confirmed. That mechanism is still real (kept, reframed as independent/complementary), just not what caused this specific incident. - scripts/ci/contextual_orchestrator_review_sidecar.sh: kept main's new preflight-report console dump and this branch's SIDECAR_STDERR_TAIL_LINES threading -- complementary, not overlapping. Deliberate follow-up simplification in the same commit: dropped this branch's own _log_preflight_rejections (launcher.py), its sanitizer allowlist entry, and its tests -- main's own `log "sidecar preflight route evidence: ..."` fix already surfaces the same already-bounded-safe preflight_report JSON to the console, achieving the same operator-visibility goal more directly. Keeping both would have been duplicate code solving the same problem twice. Full suite: 1897 passed, 1 skipped, 21 subtests; coverage 100% on all touched files (pre-existing pingora_edge_policy.py:274 gap, owned by #1398, unaffected); interrogate 100%; bash -n clean.
seonghobae
changed the title
…ixes Summarizes the wakeup that found main had advanced with the owner's own deeper investigation into the same bytez/preflight incident, reconciled this branch's three open PRs against it (merged current main/develop into each, dropped the now-redundant preflight-visibility fix, marked all three ready for review), and explicitly defers .github#1347's merge conflict to a dedicated next pass given it overlaps a file main has independently hardened for SSRF.
… finding naruon#1486 and contextual-orchestrator#923 both hit a PR-governance metadata-gate block that looked like a blocking CodeRabbit finding but was actually CodeRabbit's own "approval pending, not reviewed yet" state -- every ContextualWisdomLab repo is below CodeRabbit's 10-GitHub-star automatic-review threshold, so it never reviews a new commit without an explicit @coderabbitai review trigger. Triggered both PRs manually; documents the structural gap and a deferred central-automation candidate fix.
|
@coderabbitai review Generated by Claude Code |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin review on #1438 flagged two real issues in the CodeRabbit gap-baseline entry: bare naruon#1486/contextual-orchestrator#923 references don't create durable cross-repo links (missing the org prefix), and the claim that "every org PR" is affected outran the evidence (only 4 repos were actually checked). Both fixed.
|
_Generated by Claude Code Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… a fix CodeRabbit correctly flagged that "already fixed"/"resolves" overstates the 4->8 family-cap raise: it reduces the odds of the same retired/timed-out candidates being selected every run, but doesn't guarantee against it, and hosted-run confirmation of the fix is still pending. Softened both the changelog bullet and the gap-baseline correction entry to "mitigates ... hosted confirmation remains pending".
|
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
CHANGELOG.md— repository behaviordocs/product-technical-gap-baseline.md— operator or user guidancescripts/ci/contextual_orchestrator_review_sidecar.sh— review and security gate shell pathtests/test_contextual_orchestrator_review_sidecar_contract.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: product-technical-gap-baseline.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: product-technical-gap-baseline.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: contextual_orchestrator_review_sidecar.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: contextual_orchestrator_review_sidecar.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_contextual_orchestrator_review_sidecar_contract.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_contextual_orchestrator_review_sidecar_contract.py"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
c11b68c2e3cb4d14099c0925f4239c1ae13dc674 - Workflow run: 33310753001
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: product-technical-gap-baseline.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: product-technical-gap-baseline.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: contextual_orchestrator_review_sidecar.sh"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: contextual_orchestrator_review_sidecar.sh"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_contextual_orchestrator_review_sidecar_contract.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_contextual_orchestrator_review_sidecar_contract.py"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
_load_changed_files's post-loop PolicyError at the end of the pagination loop can never execute: 31 full 100-item pages would push the file count past 3,000 during page 31's own iteration (30 full pages = exactly 3,000), tripping the len(files) > 3_000 raise inside the loop before the outer range(1, 32) can ever exhaust without an early return or that inner raise. This dead line has been silently failing this repo's org-wide coverage-evidence gate (fail_under=100 on scripts/ci) for every PR reviewed through the central OpenCode/Noema/Strix dispatcher -- confirmed via live Actions logs on multiple unrelated PRs (.github#1161, #1438) all showing "Coverage failure: total of 99 is less than fail-under=100" at this exact line, which in turn blocks opencode-agent from ever posting an APPROVED verdict anywhere in the org. Marked pragma: no cover with a justification, matching this repo's existing convention for provably-unreachable defensive code (see other pragma: no cover sites in scripts/ci/*.py). Verified: full suite 1897 passed, 1 skipped, 21 subtests; coverage TOTAL 100% (9966/9966 statements, 3926/3926 branches); interrogate 100%.
Consolidates a 5-agent investigation into why opencode-agent had not posted a verdict on any of the three tracked PRs: naruon#1486 is stuck on a stale scheduler thread-count snapshot, contextual-orchestrator#923 is missing a cross-repo dispatch credential in its scheduler run, and .github#1438's dispatch ran but was blocked by the pingora_edge_policy.py coverage bug fixed in the preceding commit. Also records hosted-run confirmation that the earlier family_cap sidecar mitigation is now working (3/3 post-fix runs clean), while flagging a separate, still-open "healthz passes then completion request hangs" signature on the same commit.
seonghobae
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e-scoping fixes Documents this pass's three genuine bugs (project-graph-projection workspace mismatch, import_fixtures.py duplicate-check missing workspace filter, bootstrap_db.py missing second legacy constraint name) and the repeat/no-registry-exists finding, plus the false-negative test lesson from the duplicate-check assertion.
…h-quarantine fix, two reasoned deferrals) Documents the fix for base64 quarantine payloads leaking into hybrid search, and the reasoning behind deferring TicketTask workspace scoping (real gap, separate increment) and skipping a real-Postgres smoke test for the reparse path (no lock/constraint semantics a mock would hide).
…he 20 hidden real-Postgres test bugs it masked naruon#1486's Devin finding about 0001's fresh-install migration crash led to installing a local PostgreSQL to verify it -- which revealed naruon's CI backend job has no Postgres service at all, so every @pytest.mark.postgres test has always silently skipped in CI. Fixed the critical migration bug plus 19 hidden workspace_id/ORM-default test gaps (naruon commit b9b02dd0); records the still-open follow-up of actually wiring a Postgres service into naruon's CI.
…ace fixes and two CodeRabbit verifications Documents two more Devin findings fixed on naruon (RESULT_PENDING cursor starvation in NewsdomRecognitionWorker, unlocked reparse-intent TOCTOU race), plus two CodeRabbit findings verified: one correctly deferred (pre-existing raw-SQL migration pattern, out of scope), one confirmed a false positive (workspace mismatch that the query never actually reads).
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
…ling The org's standing directive requires central Strix/OpenCode/Noema scans to get at least a 3-hour floor (observed real runs go well past that). Strix's own budget was 150-minute process / 155-minute total (obfuscated via budget_suffix, per contract, so the literal env var names never appear in workflow logs), bounded by a 170-minute step and 200-minute job -- 155 minutes falls short of the 3-hour floor. Raise process/total/step/job proportionally to the actual maximum job execution time GitHub-hosted runners allow (6 hours): job 200->360 (the platform ceiling itself), step 170->330, total budget 9300->18900s (315 min), process budget 9000->18600s (310 min) -- preserving the original buffer ratios between each layer. Update the matching scripts/ci/test_strix_quick_gate.sh contract assertions in lockstep. Also records this investigation, plus the contextual-orchestrator single-tool-call-limit failover fix (ContextualWisdomLab/contextual-orchestrator#986) and confirmation that Strix already scans the full codebase and the @opencode-agent mention convention is already correct, in docs/product-technical-gap-baseline.md.
…alwisdomlab-commercialization-afow1j
scripts/ci/noema_review_gate.py's call_llm sent its actual review- completion request to the contextual-orchestrator gateway with a bare urllib timeout=120 and no retry of its own -- unlike the sidecar's own preflight self-check (ADR-0005, deliberately kept at 120s with its own bounded retry), this call had no fallback at all. Live reproduction today (naruon#1486, job 99690488248): the gateway's own routing pool rejected 11 of 12 candidates (mostly TimeoutError, two retired-model 404s, one 429) and picked its one remaining "ready" agent; that agent's real completion for a full PR diff ran past 120s with nothing to fall back to, failing the entire required review check with TimeoutError. Replace the bare literal with a named NOEMA_LLM_REQUEST_TIMEOUT_SECONDS = 10800 (3h, matching the org's standing at-least-3-hour floor for central Strix/OpenCode/Noema review latency), and update the tests that pinned the old 120s value. Also fixes 3 pre-existing test failures inherited from origin/main (commit a3f9f9b, #1531): that commit changed opencode-review-dispatch.yml's validate-pr-metadata step from exact-matching head_sha to warn-and-proceed on a head-only advance, but did not update the two tests that pin the workflow's exact git blob SHA (test_pr_review_autofix_nvidia_nim_contract.py, test_opencode_rust_coverage_toolchain_contract.py) or the assertion in test_opencode_agent_contract.py that still looked for the old exact-match line -- main itself was red on these three tests.
…alwisdomlab-commercialization-afow1j
origin/main reverted a3f9f9b (commit 7b1a028, #1540, no rationale given) right after this branch merged it and adapted test_pr_review_autofix_nvidia_nim_contract.py's REVIEW_DISPATCH_BLOB_SHA and test_opencode_agent_contract.py's metadata-step assertion to match its new warn-and-proceed behavior. Restore both to their original exact-match values now that opencode-review-dispatch.yml itself is back to its pre-a3f9f9b6 content (blob 2aa245e).
|
요청하신 7개 항목을 각각 실제 코드/워크플로 추적으로 조사했습니다. 추측이 아니라 확인한 것만 아래에 남깁니다. 1) Contextual-Orchestrator 관련 수정수정 완료 — contextual-orchestrator#986.
기존 2) NVIDIA NIM 단독 사용 금지 → Contextual-Orchestrator 사용중앙 리뷰 파이프라인은 이미 3) Timeout을 최소 3시간으로수정 완료. GitHub-hosted runner(
내부 실질 스캔 예산이 315분(5h15m)이 되어 "최소 3시간"을 여유 있게 충족하고, 단일 GitHub-hosted job이 물리적으로 도달 가능한 최댓값(6시간)에 근접합니다. 관측하셨다는 "6시간 이상"이 정확히 이 워크플로/러너였는지는 확인할 방법이 없었습니다 — self-hosted runner였거나 로컬 실행이었을 가능성이 있어 보입니다. 알려주시면 그 경로를 정확히 다시 추적하겠습니다. 그런데 "120초"는 이 조사 도중 실제로 잡았습니다. 4) OpenCode/Noema를 CodeRabbit/Devin 수준으로결론 유보, 후속 필요. 현재 예산 체계(위에서 본 3h15m~5h25m)는 결코 얕지 않지만, "CodeRabbit/Devin 수준의 실제 리뷰 깊이"는 예산 크기가 아니라 프롬프트·평가 기준·산출물 품질의 문제라 이번 패스의 코드 추적만으로는 결론 낼 수 없었습니다. 실제 PR에서 나온 리뷰 산출물을 CodeRabbit/Devin의 산출물과 나란히 비교하는 별도 품질 평가 트랙이 필요하다고 보고, 5) Strix 전체 코드 보안 리뷰이미 올바르게 구현되어 있음(수정 불필요). 6) Contextual-Orchestrator가 빠르고 능력 좋은 모델로 라우팅부분 확인, 후속 필요. 7)
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1. strix.yml's retry-reserve check silently disabled every retry: raising process_budget_seconds to 18600 without raising the wrapper's own strix_gate_deadline (still 9600) made retry_reserve_seconds (process_budget_seconds + backoff) exceed the deadline unconditionally, so a transient provider outage that used to recover on attempt 2 would now fail closed on attempt 1 every time. Raise strix_gate_deadline to 19200 (same 600s buffer under the new 330-minute step, proportional to the original ratio) and add a regression test (test_retry_deadline_reserves_room_for_at_least_one_retry) asserting the live numbers -- not synthetic ones -- keep a retry reachable. 2. noema_review_gate.py's call_llm can recurse once for a validator- rejected repair; two independent NOEMA_LLM_REQUEST_TIMEOUT_SECONDS (10800s) calls would total 21600s -- exactly the 6h GitHub-hosted job execution ceiling, leaving no room for sidecar provisioning or cleanup and turning a fast failure into a reliable 6h one. Add a shared NOEMA_LLM_TOTAL_BUDGET_SECONDS (19800s) deadline threaded through the repair recursion; each call's own request timeout is now capped to whatever remains of it. New test (test_call_llm_repair_call_shares_the_total_budget_deadline) proves the repair call's timeout shrinks when the first call has already consumed most of the shared budget. 3. strix.yml's timeout comment misattributed the 3-hour floor to the standing operating directive; that directive only accepts scans over two hours per model. The 3-hour floor came from the repo owner's later comment on #1438 -- corrected the citation. All three found by Devin Review on #1438.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1. noema_review_gate.py's timeout comment carried the same misattribution as strix.yml's -- fixed there but missed here -- crediting the 3-hour floor to the standing operating directive (which only accepts scans over two hours per model) instead of the owner's later comment on #1438. Corrected the citation. 2. call_llm clamped an expired shared deadline to a 1-second timeout instead of stopping, so a late validator-rejected repair would still open a network request and burn through the job's already-exhausted reserved cleanup time. Raise TimeoutError immediately when the shared budget is gone, per Devin's suggested fix. New test (test_call_llm_raises_instead_of_sending_a_request_on_an_expired_budget) proves no request is sent once the deadline has passed -- verified genuine RED (the old clamp let a 1-second request through) before GREEN. 3. strix.yml's retry-loop comment still named the old 200-minute job budget after the job became 360 minutes, and did not make clear that the 3-attempt retry cap is conditional on an early failure rather than a guarantee of three full-budget attempts. Updated the comment with both corrections. All three found by Devin Review's second pass on #1438.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review (#1438) noted call_llm's repair call reuses the same absolute deadline, so validation and response processing time between the initial call and its repair also count against the shared budget -- not just network time. That is intended: the deadline is a hard wall-clock ceiling regardless of where the time goes. Documented the intent inline so it reads as deliberate.
…alwisdomlab-commercialization-afow1j Reconciles this branch's fix for noema_review_gate.py's hardcoded 120s timeout (NOEMA_LLM_REQUEST_TIMEOUT_SECONDS=10800 + shared NOEMA_LLM_TOTAL_BUDGET_SECONDS=19800 deadline across the initial call and its one possible repair call) with main's independent fix for the exact same bug (PR #1507, NOEMA_LLM_TIMEOUT_SECONDS=14400 flat), plus main's several other genuine, unrelated hardening fixes in the same file: fail-closed handling of malformed/deeply-nested LLM JSON, UnicodeDecodeError safety, and skipping the repair-retry request when the PR head has moved (new expected_head parameter and StaleHeadDuringRepairRetryError). Kept main's JSON-crash and stale-head logic entirely intact, and kept this branch's shared-deadline design for the timeout itself: a flat 4-hour constant applied independently to both the initial and repair calls could sum past the 6-hour GitHub-hosted job execution ceiling, which the shared budget already accounts for. Removed the now-unused NOEMA_LLM_TIMEOUT_SECONDS constant and updated its references. Fixed 3 additional breaks the merge surfaced in this branch's own tests: two calls to call_llm missing the newly-required expected_head argument, and one test asserting a stale literal 14400 timeout value. Full suite: 2211 passed, 1 skipped, coverage 100%, interrogate 100%.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…gressions found while fixing it Devin's claim (test_stacked_pr_workflow_contract.py never collected by app-ci.yml's backend-scoped pytest) verified real by reading the actual workflow files. Fixed in naruon db97962c. While fixing it, also found a4e01191's stacked-PR trigger change had broken 2 pre-existing contract tests in the same file (stale release/**-branch-list assertions) — also fixed in the same commit.
…icit job timeout, detect a dead sidecar on every attempt Three Devin Review findings against the prior timeout/deadline fix, all verified real: 1. noema_review_gate.py::call_llm relied solely on urllib's timeout= argument, which bounds per-socket-operation inactivity, not the request's total wall-clock duration. A response trickling at least one byte before each such window elapses could keep the shared 5.5-hour budget unenforced indefinitely. The network call now runs on a daemon thread; call_llm enforces the real deadline via Thread.join(timeout=remaining_budget), so a trickling connection is preempted at the actual budget boundary regardless of how the far end paces its response. 2. The noema-review job carried no explicit timeout-minutes, leaving the relationship between its 5.5-hour LLM budget and GitHub's implicit 360-minute default unauditable. Made it explicit. 3. contextual_orchestrator_review_sidecar.sh's gateway-preflight retry loop only checked whether the sidecar process had died on the last configured attempt, so a sidecar that died on attempt 1 still burned the remaining attempts (each up to 120s) before detection -- the original incident this branch exists for shows all 3 attempts took roughly the full 120s each. Moved the dead-sidecar check to run immediately after any failed attempt. Each fix verified genuine RED against the pre-fix code/tests before being restored to GREEN. Full suite: 2214 passed, 1 skipped, 21 subtests, coverage 100%, interrogate 100%.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review found a real regression in the prior daemon-thread deadline fix (e186203): call_llm joined the request thread with timeout=remaining_budget (the full shared 5.5h budget) instead of timeout=request_timeout (this call's own cap, already min()'d against the shared budget). A single trickling response could then consume the entire shared budget, leaving nothing for a validator-rejected repair call -- exactly the failure NOEMA_LLM_REQUEST_TIMEOUT_SECONDS was introduced to prevent. Fixed to join on request_timeout, matching Devin's suggested one-line change. Also corrected a stale comment claiming the noema-review job has no explicit timeout-minutes (this PR already added one in e186203). Verified genuine RED (a 0.05s per-request cap was ignored while 10s of shared budget remained, letting a 0.3s trickle complete normally) before the fix, GREEN after. Full suite: 2215 passed, 1 skipped, 21 subtests, coverage 100%, interrogate 100%.
…estrator#986, root-caused to unmerged .github#1438 fix contextual-orchestrator#986's required noema-review check crashed with an uncaught JSONDecodeError -- traced to the trusted noema_review_gate.py still fetched from .github's main branch, which lacks the malformed-JSON repair-retry fix already sitting in this PR. Not that PR's own fault; commented there with the root cause and re-ran the job once. Reinforces that merging this PR fixes the same crash for every sibling repo, not just this one.
|
Closing as a stale mixed branch after current-main audit. Exact head e6ec629 is BEHIND main@44a3c740, deletes/regresses scheduler coverage evidence added by #1541/#1548, and carries absolute inference/job wall-clock deadlines that conflict with the accepted unbounded Contextual Orchestrator review policy and #1546. The small stderr-tail diagnostic is not sufficient reason to preserve or merge the unsafe branch; no predecessor check/review evidence is transferable. No commits from this branch were pushed or merged by this audit. |
* fix(noema): fail closed on a transport error instead of crashing the required check Live incident on ContextualWisdomLab/naruon#1486: call_llm's opener.open(request) sat outside the surrounding try/except, which only guarded the JSON-decode/validation steps after a successful response. A genuine HTTP 502 from the completion request therefore crashed the whole required noema-review check with an unhandled traceback instead of getting the same one-time repair-retry the malformed-verdict path already has. Widened the try to also cover the request itself, and added urllib.error.URLError alongside RuntimeError to the existing repair-retry except clause. A transient transport failure now gets one retry, then fails closed with a clean RuntimeError on a second failure -- exactly like a malformed verdict already does. Verified genuine RED (the exact HTTPError: Bad Gateway reproduced uncaught) before the fix, GREEN after. Full suite: 2248 passed, 1 skipped, 21 subtests. Confirmed the repo's 99% (11 stmt/7 branch) coverage gap is pre-existing on main in pr_review_fix_scheduler.py/pr_review_merge_scheduler.py, unrelated to this two-file diff -- verified identically present before this change too. Narrowly scoped: nothing here touches the wall-clock-deadline design that #1438 was closed over, or the in-progress #1546 reconciliation (already checked -- #1546's call_llm has this exact same unguarded line). * fix(noema): normalize http.client.HTTPException/OSError into the transport-error retry too Devin Review on #1566 correctly found that the round-1 transport-error fix (RuntimeError, urllib.error.URLError) still missed http.client.IncompleteRead -- raised by response.read() on a truncated body -- since it is neither a RuntimeError nor a URLError. The repo owner independently confirmed the same gap and specified the fix: widen to the bounded transport/read exception families (URLError, http.client.HTTPException including IncompleteRead/RemoteDisconnected, and raw OSError transport failures such as a bare socket timeout reaching opener.open() before urllib wraps it) without swallowing JSON/validator/programming errors, and add RED->GREEN regressions for a truncated-body success-after-retry, a repeated-failure case, and at least one timeout/disconnect family exercising a distinct exception path. Widened the except clause to (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) and simplified the repair-retry re-raise to "re-raise as-is only when it's already our own RuntimeError; otherwise wrap in a clean RuntimeError" -- generalizes the fail-closed contract to any transport exception type rather than needing another isinstance branch added per exception class. Three genuinely distinct exception paths each get their own RED->GREEN success-after-retry and repeated-failure pair, none transferred from another case as substitute proof: - test_call_llm_repairs_once_after_a_truncated_response_then_succeeds / test_call_llm_fails_closed_after_a_repeated_truncated_response (http.client.IncompleteRead from response.read()) - test_call_llm_repairs_once_after_a_socket_timeout_then_succeeds / test_call_llm_fails_closed_after_a_repeated_socket_timeout (raw TimeoutError from opener.open() itself, never wrapped as URLError) Full suite: 2252 passed, 1 skipped, 21 subtests. noema_review_gate.py itself at 100% line/branch coverage; 100% docstring coverage. Repo-wide coverage remains the same pre-existing 99% (11 stmt/7 branch gap in pr_review_fix_scheduler.py/pr_review_merge_scheduler.py) confirmed unrelated to this diff in the prior commit on this branch. Updated docs/product-technical-gap-baseline.md with the full root cause/owner/status writeup for this incident (naruon#1486), including the round-1 and round-2 fixes and the unrelated SIGPIPE test flake found and fixed separately while verifying this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6UJHYbfbGdHfYPjgbVhAr * fix(noema): track retry state independently of the exception's text Devin Review on #1566 found a fourth, distinct bug: gating the retry-vs-fail-closed decision on repair_error's truthiness conflated "is this the second attempt" with "does the caught exception have display text". Several transport exceptions (a bare OSError()/TimeoutError(), or an http.client.HTTPException raised with no message) all stringify to '', so an empty-message failure on the first attempt left repair_error falsy on the recursive call too -- the retry-state signal was lost, and call_llm would retry unboundedly (each recursive call another live gateway request) instead of failing closed after one attempt, eventually crashing on an uncaught RecursionError once the call stack was exhausted. Added an explicit is_retry: bool = False parameter that tracks retry state independently of the exception's text. It (not repair_error) now gates both the prompt-injection branch -- falling back to a generic message when repair_error is empty -- and the except clause's retry-vs-fail-closed decision, and is threaded through as is_retry=True on the recursive call. Verified genuine RED with a bounded-recursion regression test (test_call_llm_fails_closed_after_a_repeated_empty_message_transport_error, which raises a diagnostic AssertionError if call_llm retries more than once instead of letting it recurse to CPython's own limit) before this fix, GREEN after -- paired with test_call_llm_repairs_once_after_an_empty_message_transport_error_then_succeeds for the happy-path case. Full suite: 2254 passed, 1 skipped, 21 subtests. noema_review_gate.py still at 100% line/branch coverage; 100% docstring coverage. Repo-wide 99% remains the same pre-existing gap tracked by #1567, unrelated to this diff. Updated docs/product-technical-gap-baseline.md and CHANGELOG.md with this fourth fix round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6UJHYbfbGdHfYPjgbVhAr --------- Co-authored-by: Claude <noreply@anthropic.com>
…aims CodeRabbit on PR #1661: 1. The hash-lock-freshness verification step's `while IFS= read -r line; do ... done < requirements-opencode-review-ci.txt` silently skips the final line if the file doesn't end with a trailing newline -- `read` returns failure at EOF even though it populated `$line`, so the loop body never runs for that last pin. Fixed with the standard `|| [[ -n ${line:-} ]]` idiom. Verified locally against CodeRabbit's own repro (a requirements file with no trailing newline and a deliberately-stale lock): without the fix the mismatch goes undetected, with it the check correctly fails. 2. "then both fire -- and only one survives the shared concurrency group" overstated cancel-in-progress: false's actual behavior -- it keeps one running job protected plus one replaceable pending job, not a strict single survivor. Corrected in both the doctoring doc and its gap-baseline mirror. 3. "most PRs eventually get through, per the #1438/#1176 evidence" drew an organization-wide majority claim from two examples with no stated denominator or sampling methodology. Softened to "some PRs" with an explicit note that this record doesn't have the basis for a "most" claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
Exact current identity and evidence
18987a7191f070fdcb134d5feb96a07442c39a98main@1ff8268255b061461d9d49b4cab4febf9a8e7bfa.gitignore,CHANGELOG.md,docs/product-technical-gap-baseline.md,scripts/ci/contextual_orchestrator_review_sidecar.sh, and two sidecar tests.scripts/ci/pingora_edge_policy.pyis not in the current diff. The earlier coverage-fix narrative below is historical and must not be treated as this PR's current source delta.50febfe7a9bd74c8c33d1eef6526a33f116f5c2fand cannot satisfy this head.50febfe7, passed discovery/health/gateway preflight, then timed out after 5400 seconds and emittedSTRIX_PROVIDER_UNAVAILABLEwithout an authoritative scan result; attempt 2 was cancelled during provisioning. This is provider/infrastructure evidence recorded on fix(ci): gate Strix's orchestrator/free access on live diversity evidence #1437, not a source finding or current-head pass.Original summary (docs correction + sidecar tail widening — unchanged):
docs/product-technical-gap-baseline.md): the "2026-08-30 post-chore(fuzz): remove dead duplicate fuzz target #1486/fix(coverage): unblock org-wide OpenCode approval + docs(gaps) corrections #1438 wake" entry misdiagnosed the incident on two counts, found by a 7-agent investigation:orchestrator/freeregardless of its HTTP status (_parse_byteznever setsis_free) — its logged HTTP 500 changed nothing about the outcome.request_failed status=413line is the sidecar's own unconditional offline self-test, not a live ZDR-catalog prefetch that "fell back" to anything.main's much more thorough "sidecar-preflight outage: consolidated evidence" entry as the authoritative root cause, rather than presenting this branch's own generic "two call sites, no retry" theory as confirmed for this specific incident (that mechanism is still real, just not what caused this one — seeContextualWisdomLab/contextual-orchestrator#923).SIDECAR_STDERR_TAIL_LINES: widens the sidecar's failure-path stderr tail from a fixed 20 lines to a named constant (60), complementary to (not overlapping with)main's new preflight-evidence log line — so discovery-error diagnostics can't be silently truncated alongside it.Also includes the original §5.1 next-increment-list refresh (#1297 already merged; #1345/#1326 closed unmerged), the naruon-Noema role-clarification entry, and a CodeRabbit 10-star/rate-limit gap entry, all from earlier in this branch's history.
Verification
PYTHONPATH=. python -m coverage run -m pytest tests -q→ 1897 passed, 1 skipped, 21 subtests, coverage TOTAL 100%.python -m interrogate→ 100.0%.bash -n scripts/ci/contextual_orchestrator_review_sidecar.sh→ syntax OK.PYTHONPATH=. python -m pytest tests/test_product_technical_gap_baseline.py -q→ 5 passed (doc contract markers intact).PYTHONPATH=. python -m pytest tests/test_pingora_edge_policy.py -q→ 61 passed.Related
ContextualWisdomLab/contextual-orchestrator#923— companion discovery-side retry fix (independent resilience improvement, not the fix for this specific incident).ContextualWisdomLab/naruon#1486— the PR whose CI run first surfaced this incident.ContextualWisdomLab/.github#1398— the owner's own in-flight, more extensive coverage-evidence/Python-lock work; this PR's dead-code fix is narrowly scoped and orthogonal to it.Summary by CodeRabbit
개선 사항
버그 수정
"00","0000")을 올바르게 거부합니다.문서
테스트