fix(strix): remove the 300s LLM_TIMEOUT cap - #1658
Conversation
…o-timeout env vars strix.yml already sets STRIX_MEMORY_COMPRESSOR_TIMEOUT=0, STRIX_PROCESS_TIMEOUT_SECONDS=0, and STRIX_TOTAL_TIMEOUT_SECONDS=0 right next to LLM_TIMEOUT, and the job's own concurrency-block comment says inference "has no wall-clock deadline" and central scans "may take more than two hours per model" per docs/product-goal-directive.md §8 (no uniform LLM timeout ceiling; default unlimited). LLM_TIMEOUT was still hardcoded to 300 seconds, contradicting both the neighboring env vars and that documented intent. scripts/ci/test_strix_quick_gate.sh already asserts `export LLM_TIMEOUT=0` (line 302) and fails on origin/main today because of this exact mismatch — confirmed directly against origin/main (fb02129, .github/workflows/strix.yml:786) before this fix, independent of any single PR's diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YgmaEKnPhrsbSyQBierFH
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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 |
|
Fresh verification immediately before merge:
No substantive failing test, security finding, meaningful CHANGES_REQUESTED, malformed provenance, merge conflict, or unrelated policy defect is being bypassed. Merge is bound to the exact head SHA; predecessor evidence is not transferred. |
Non-destructive reconciliation of protected main@69e80bdf37bfbae813851c1b0e6b8a0cfb4a704c into the PR1656 queue-pressure repair. The protected-main delta is the non-overlapping one-line Strix LLM_TIMEOUT=0 fix from #1658; preserve all PR1656 runnerless closed-event cleanup and permanent regressions.
…ueue facts Merged current main (through #1658/#1656/#1651) into this branch to resolve the "behind" state again -- clean merge, no conflicts. The owner left fresh, directly-observed control-plane facts as a comment on this PR while main kept moving underneath it, with an explicit instruction to preserve them in docs/product-technical-gap-baseline.md rather than let them get lost in the PR thread, and not to rewrite the directive text around them. Recorded as a new dated entry: - #1658 (69e80bd): Strix's LLM_TIMEOUT=0 -- already independently confirmed during the prior merge pass on this same PR. - #1656 (6a25bc1): ten PR-close workflows stop allocating a runner-backed job for echo-only cancel-closed-pr-runs steps -- a second, distinct contributor to the organization-wide queuing problem, separate from the floating-ubuntu-latest-image root cause this file already tracks. - #1651 (2792b96): the Bytez exact-zero-meterPrice fail-closed fix, cross-referenced to its own ADR-0003 update and doctoring record rather than restated here. - The queue-depth data point (1,252 before, 1,280/1,282 after) and the owner's own reading of it: existing queued runs aren't retroactively removed by a source-level fix, so the right acceptance signal going forward is future stale/no-op admission rate and current-head throughput, not an instantaneous depth drop. This supplements, and does not restate or supersede, the adjacent 2026-09-01 floating-runner-image entry. Verified: tests/test_product_technical_gap_baseline.py's 5 contract tests pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6UJHYbfbGdHfYPjgbVhAr
QUEUE_SATURATION_CHICKEN_EGG: exact head 754f9f1 is mechanically mergeable; the repair has prior full-suite 100% branch/docstring evidence, no review submission or inline finding exists on the Ready successor, and current-head workflows are queued behind a 1,367-run Actions backlog. This restores protected-main test signal needed by later control-plane PRs without weakening any required-check definition.
Syncing with current main pulled in #1651, #1656, and #1658 on top of #1654, all of which left pre-existing tests broken (see #1663's commit history for full root-cause detail on each). Ported the same fix already validated and opened as its own PR (#1663) rather than re-deriving it here: - scripts/ci/current_head_run_coalescer.py + 5 test files: removed two provably-unreachable dead-code checks (_run_matches_head_identity already subsumes the later event-type check; select_duplicate_queued_run_ids re-derived workflow_id behind a guard _run_identity_matches already enforces), added eight regression tests for genuinely-reachable but previously-untested branches in _run_pr_scope_is_safe and the sibling- authority loop, and fixed seven stale assertions (a retry-with-backoff loop now absorbs two test fixtures' synthetic sentinel exit codes; literal-text and renamed-field contract drift). - Three runner-image occurrence-count/job-presence assertions updated for #1656's removal of ten no-op cancel-closed-pr-runs jobs. - One literal LLM_TIMEOUT=300 assertion updated for #1658's removal of the 300s cap (now exports 0/unlimited directly). Full suite: 2605 passed, 100% branch coverage, 100% docstrings.
While validating the merge with main on this PR, the full suite surfaced tests/test_opencode_live_draft_state_regression.py::test_draft_exemption_fails_closed_when_live_head_moved failing. Reproduced identically on a clean, unmodified checkout of origin/main (5c561a6) alone via a disposable worktree, confirming this is pre-existing on protected main and unrelated to this PR's own diff (scripts/ci/noema_review_gate.py's standalone-import fallback). Root cause: #1697 ("retire stale draft/head dispatches without false failure") deliberately inverted opencode-review.yml's check order -- the draft/closed exemption now runs before the head-moved check, and a still-draft PR whose live head has also moved now exits 0 quietly instead of failing closed with exit 1 -- fixing a real production false-failure (contextual-orchestrator run 33548447878/job 100066104033). #1697 updated its own new tests in test_opencode_required_verdict_regression.py to match, but this file's independent _run_step harness covering the identical production script text was never updated, so it kept asserting the superseded pre-#1697 contract. Same pattern as this repo's own previously-documented "stale test assertions left by a merged PR" class of fix (see CHANGELOG's #1654/#1656/ #1658 entry): no production behavior changed here, only the test's assertion and docstring, which now match the intentional, already-reviewed #1697 contract and cross-reference its sibling coverage. Verified: tests/test_opencode_live_draft_state_regression.py 19/19 passed; interrogate 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
… main exact-head-path-policy failed on this branch's own copy of scripts/ci/test_strix_quick_gate.sh for two unrelated stale assertions, neither touching this PR's actual Strix evidence-hardening diff: 1. The LLM_TIMEOUT assertion (line 302) still expected the round-6 'export LLM_TIMEOUT=300' value this PR itself introduced on 2026-09-01 to match #1601's contemporary state. Main later reverted strix.yml back to 'export LLM_TIMEOUT=0' via #1658 ("remove the 300s LLM_TIMEOUT cap") without ever having carried the 300 assertion on main's own copy of this file, so a same-line 3-way merge always kept this branch's now-stale text with no conflict to surface it. Restored the assertion to match main's (and strix.yml's) current, unchanged content. 2. The scheduler-heartbeat cron assertion (line 1562) still expected the pre-#1704 'cron: "*/30 * * * *"' quarter-hourly schedule. #1704 ("lengthen scan-pr-queue's own heartbeat, don't drop it") lengthened pr-review-merge-scheduler.yml's repository-local scan to hourly ('cron: "30 * * * *"') for the same Actions-capacity reason as #1630, and added/updated the matching pytest contract (tests/test_actions_queue_saturation_scheduler_cadence.py, tests/test_required_workflow_queue_contract.py) but missed this repo's separate, duplicate shell-harness assertion of the same contract. Confirmed this exact failure reproduces identically on fresh main (same stale assertion, same actual hourly cron) -- it predates and is unrelated to this PR's diff. Updated the assertion to match #1704's now-current cron and added the mirroring assert_file_not_contains for the retired quarter-hourly string, same pattern #1704 already established in its own pytest contract. Verified on the merged head (origin/main merged in via the preceding merge commit, mergeable_state was "behind" only, no conflicts): - bash scripts/ci/test_strix_quick_gate.sh (full harness): PASS, 0 failures (previously 2: the LLM_TIMEOUT and cron assertions above). - PYTHONPATH=. python3.12 -m coverage run -m pytest tests -q: 2644 passed, 1 skipped, 21 subtests. - coverage report --show-missing: 100% on scripts/ci. - interrogate: 100% (RESULT: PASSED, minimum: 100.0%, actual: 100.0%). - python -m compileall on the five exact-head-path-policy test files, bash -n scripts/ci/strix_quick_gate.sh, git diff --exit-code: all clean after this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Description
.github/workflows/strix.yml'sRun Strix (quick)step sets four inference-related timeout env vars together (line 786-789). Three are already0:STRIX_MEMORY_COMPRESSOR_TIMEOUT,STRIX_PROCESS_TIMEOUT_SECONDS,STRIX_TOTAL_TIMEOUT_SECONDS.LLM_TIMEOUTwas left hardcoded at300(5 minutes) — contradicting both its three siblings and the job's own concurrency-block comment a few lines above:docs/product-goal-directive.md§8 is explicit on this: no uniform application/agent/gateway-wide LLM timeout ceiling; default unlimited, with admin-configurable per-model overrides only.scripts/ci/test_strix_quick_gate.shalready assertsexport LLM_TIMEOUT=0(line 302,"strix disables the model client inference timeout"). Confirmed this assertion currently fails onmainitself (fb02129), independent of any other PR's diff — a real, pre-existing base-branch contract violation, not something introduced by this change.Fix
One line:
export LLM_TIMEOUT=300→export LLM_TIMEOUT=0, matching the three sibling env vars on the following lines.Type of change
Checklist
origin/maindirectly (git show origin/main:.github/workflows/strix.yml | grep LLM_TIMEOUT→export LLM_TIMEOUT=300) before making this changegit diff origin/main→1 file changed, 1 insertion(+), 1 deletion(-))LLM_TIMEOUTreferences in the file need changing (theassert_file_not_contains ... "LLM_TIMEOUT:"check in the same test guards against a different pattern — a YAMLenv:mapping key — which this change does not introduce)🤖 Generated with Claude Code
https://claude.ai/code/session_019YgmaEKnPhrsbSyQBierFH
Generated by Claude Code