fix(review): restore user-directed model timeout policy - #1895
Conversation
|
Warning Review limit reachedNext included review available in 59 seconds. 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 (4)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…s revert (#1897) #1895 reverted #1892's timeout-minutes addition to opencode-review-dispatch.yml, restoring its blob hash to ade10b3 -- but did not restore the pin this contract test asserts (which #1894 had correctly updated to match #1892's now-reverted state), breaking the required test gate for every open PR in this repository again. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…s revert #1895 reverted #1889/#1892's model-inference wall-clock caps, restoring opencode-review-dispatch.yml's pre-#1892 content. That content change left REVIEW_DISPATCH_BLOB_SHA in test_pr_review_autofix_nvidia_nim_contract.py pinned to the intermediate #1894 resync value (fca5bfc...), not the now-current content (recomputed directly via git hash-object rather than reasoned from history: ade10b3). Same-day stale-pin pattern already seen this session (#1810, #1874) -- a workflow content change updates most-but-not-all of the tests asserting the old content's exact hash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aeba84c2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert_file_not_contains "$workflow_file" 'timeout-minutes: 325' "opencode review target must not cap inference" | ||
| assert_file_contains "$workflow_file" 'timeout-minutes: 12' "opencode evidence preparation fails closed before it ties up the review queue" | ||
| assert_file_contains "$workflow_file" 'timeout-minutes: 15' "opencode model pool returns its shared runner after 900 seconds" | ||
| assert_file_not_contains "$workflow_file" 'timeout-minutes: 205' "opencode model pool step must not cap inference" |
There was a problem hiding this comment.
This assertion does not enforce the stated no-timeout contract: adding timeout-minutes: 15, 30, or any value other than 205 to the Run OpenCode PR Review model pool step still passes this test, and the corresponding Python assertion has the same value-specific gap. Inspect the model-pool step and reject any timeout-minutes key so a future inference cap cannot silently regress the restored policy.
Useful? React with 👍 / 👎.
| assert_file_contains "$workflow_file" 'export STRIX_PROCESS_TIMEOUT_SECONDS=0' "strix disables the scanner process timeout" | ||
| assert_file_contains "$workflow_file" 'export STRIX_TOTAL_TIMEOUT_SECONDS=0' "strix disables the total scanner timeout" |
There was a problem hiding this comment.
These replacement assertions cover only the scanner's inner process and total-timeout variables; deleting the timeout-minutes: 30 assertion leaves the outer job contract guarded only by exact exclusions for 200 and 170. Reintroducing timeout-minutes: 30, 15, or any other value on the synchronous strix job would therefore pass this test while capping the model call, so the test should inspect that job and reject any job-level timeout-minutes key.
Useful? React with 👍 / 👎.
…pline (#1909) Two rules from mistakes this session actually made and corrected, per the per-session lane split agreed with the other concurrent sessions (peer 3 took verification discipline in #1907; peer 2 has gate/merge mechanics; host 1 has close-time diff comparison and noema concurrency; host 2 has CI failure diagnosis). - Narrowing a PR does not carry its delta. #1871 was closed in favor of #1877 plus #1879; both successors were green, but neither carried the coverage/docstring delta, leaving main's required 100% gate broken until #1883 recovered it. "Each piece works" and "the pieces together cover the original's scope" are different questions. - Compare content, not ancestry. main mixes squash and merge commits (last 200: 153 single-parent, 47 two-parent, counted directly), so `git merge-base --is-ancestor` gives false negatives for squashed deltas and false positives for reverted merge-commit deltas. - Never endorse a timeout or retry constant on a model-invocation path without reading docs/product-goal-directive.md section 8, which accepts more than two hours per model and states speed is not a core consideration. #1889/#1890/#1892 each capped a model step at 900s on real multi-hour-hang evidence and were all reverted (#1891, #1895). Every PR number, the section-8 quotes, the parent-count distribution, and the 100% gate values were verified against the repository directly. An earlier draft of the timeout bullet cited a section number that does not exist and attributed a sentence to that file which appears only in #1891's PR body; both were caught by grepping rather than trusting the summary that introduced them, and that failure is recorded in the text. Full suite: 2883 passed, 1 skipped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Correction evidence
The objective distinguishes unexplained 900-second failures (items 4 and 39) from the model-runtime policy (general guidance 8.5–8.6). The latter explicitly forbids a common application/agent/gateway wall-clock cap and permits model execution beyond two hours.
Verification
uv run pytest -q tests/test_opencode_agent_contract.py tests/test_strix_quality_timeout_fixture_budget.py tests/test_strix_backend_unavailable_after_exempted_finding.py tests/test_strix_openai_fallback_api_base.py(80 passed, 4 subtests passed)actionlint .github/workflows/strix.ymlactionlint -shellcheck= .github/workflows/opencode-review-dispatch.ymlbash scripts/ci/test_strix_quick_gate.sh(PASS)git diff --check