fix(noema-review): bound both jobs to a job-level timeout-minutes - #1715
Conversation
Neither cancel-closed-pr-runs nor noema-review declared a job-level timeout-minutes, so a stuck run falls back to GitHub's 360-minute platform default -- the same defect class PR #1702 fixed for scan-pr-queue. This file's own poll loops are already bounded by iteration count (unlike opencode-review.yml's pre-#1707 while :; do loop), so no wall-clock-inside-a-loop patch is needed here; the gap is purely the missing job ceiling. cancel-closed-pr-runs gets timeout-minutes: 20 -- its only step is a single-repository, status-filtered gh api --paginate list-and-cancel sweep (up to 3 passes x 5 statuses), no branch update or merge, lighter than scan-pr-queue's own timeout-minutes: 30. noema-review gets timeout-minutes: 210. Its "Prepare Noema model verdict" step calls into two_phase.py's call_llm via the same contextual-orchestrator gateway whose unbounded wait caused the 7-20 hour stuck runs PR #1707 fixed in opencode-review.yml -- noema_review_gate.py's own comment confirms that call "remains governed by contextual-orchestrator rather than a fixed inference timeout," so nothing upstream bounds it either. 210 minutes carries the same ~180-minute (3-hour) allowance PR #1707 set for its analogous model-wait deadline -- comfortably above this org's documented "모델당 두 시간 이상 걸릴 수 있음을 수용한다" policy (docs/product-goal-directive.md #8, which names Noema explicitly) -- plus a 30-minute buffer for this job's other steps (tarball fetch, credential mint, its own superseded-run cleanup sweep, visibility-lookup retries, sidecar provisioning, publication). cancel-in-progress was left as-is: this workflow's only genuinely high-frequency trigger (synchronize) already gets cancel-in-progress: true, and no evidence supports changing the lower-frequency paths. Adds test_cancel_closed_pr_runs_has_a_bounded_runtime and test_noema_review_job_has_a_bounded_runtime_above_the_two_hour_model_allowance, extracting each job's real timeout-minutes value with the same workflow_text()-based contract-test pattern this file and test_required_workflow_queue_contract.py already use. actionlint .github/workflows/noema-review.yml passes clean; tests/test_noema_orchestrator_workflow_contract.py and the full suite (2592 passed, 1 pre-existing skip) pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 3 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 (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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # for this job's other steps (tarball fetch, credential mint, the | ||
| # superseded-run cleanup sweep, visibility-lookup retries, sidecar | ||
| # provisioning, publication), while staying well under GitHub's default. | ||
| timeout-minutes: 210 |
There was a problem hiding this comment.
🔴 Long Noema reviews are terminated
When a valid Noema inference exceeds 210 minutes, timeout-minutes kills the job despite the repository's no-fixed-timeout contract. The required review fails without publishing a verdict, blocking the pull request.
Prompt for agents
Remove the noema-review job-level deadline and its contract test. The deadline applies across Prepare Noema model verdict, whose primary call_llm request intentionally has no fixed inference timeout under docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md. Preserve the separate 20-minute cancel-closed-pr-runs bound, which does not wrap model inference, and replace the new noema-review timeout test with an assertion that this job has no job-level timeout.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # or merge -- lighter than pr-review-merge-scheduler.yml's scan-pr-queue | ||
| # job (PR #1702), which got timeout-minutes: 30 for a comparable | ||
| # single-repo scan that also dispatches a review and updates a branch. | ||
| timeout-minutes: 20 |
There was a problem hiding this comment.
📝 Info: Cleanup timeout scope
The cancel-closed-pr-runs deadline wraps only a bounded three-pass status sweep. It never covers model inference, so the no-fixed-inference-timeout contract does not apply.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review for a04374c79caa3af48e03a39165890f0b1622320f: the 20-minute bound on cancel-closed-pr-runs is a bounded non-model support-job control, but the 210-minute timeout-minutes on noema-review is not admissible under CWL DEVELOPMENT PHILOSOPHY v2026-09-02B. The same diff explicitly states that Prepare Noema model verdict has no fixed inference timeout and is governed by contextual-orchestrator; wrapping that call in a 210-minute job deadline converts elapsed wall clock into model termination authority. “Over two hours per model” is an allowance, not evidence for a 210-minute kill threshold, and the new regression codifies the unsupported threshold rather than proving a production invariant.
RED acceptance: (1) workflow-contract test fails if any job/step timeout introduced by .github can terminate an in-flight Noema reasoning/stream/tool call solely by elapsed time; (2) separately prove the non-model cleanup job remains bounded; (3) cancellation evidence must distinguish explicit user/revision cancellation, provider termination, and configured admin timeout from runner/platform termination.
GREEN: retain the bounded cancel-closed-pr-runs support job if its exact API loop remains non-model and mutation-safe; remove the repository-authored elapsed-time bound that encloses Prepare Noema model verdict; keep application/Agent/Gateway default model timeout null; let provider/user/admin semantics terminate model communication; and preserve runner-release/supersession through the one-shot/exact-run wake architecture rather than a model kill timer. Then rerun focused contract tests plus full exact-head tests, 100% coverage/docstrings, actionlint, security/review/SBOM/provenance on one unchanged repaired head. Queued/pending evidence is non-passing, and the PR body’s suggested bypass is not admissible without exact emergency-policy circular-dependency proof.
|
Concurrency re-fetch: protected |
| # for this job's other steps (tarball fetch, credential mint, the | ||
| # superseded-run cleanup sweep, visibility-lookup retries, sidecar | ||
| # provisioning, publication), while staying well under GitHub's default. | ||
| timeout-minutes: 210 |
There was a problem hiding this comment.
🟡 Four-hour reviews terminate early
When a valid model review exceeds 210 minutes, timeout-minutes cancels the job before the existing four-hour request window closes. The required review fails without publishing a verdict.
Prompt for agents
Reconcile the noema-review job deadline with the four-hour primary request contract in scripts/ci/noema_review_gate.py and docs/product-technical-gap-baseline.md. The job-level boundary must not expire before a valid primary request, subsequent bounded repair, and publication can complete. The newly added source-fix workflow already proposes removing the model job timeout; make the checked-in workflow and permanent contract test directly express the chosen final behavior rather than relying on a later self-modifying commit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - name: Retire one-shot repair artifacts and verify scope | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| rm scripts/ci/source_fix_pr1715_no_model_job_timeout.py | ||
| rm .github/workflows/source-fix-pr1715-no-model-job-timeout.yml | ||
| allowed='^(.github/workflows/noema-review.yml|tests/test_noema_orchestrator_workflow_contract.py|CHANGELOG.md|docs/product-technical-gap-baseline.md|scripts/ci/source_fix_pr1715_no_model_job_timeout.py|.github/workflows/source-fix-pr1715-no-model-job-timeout.yml)$' | ||
| bad="$(git status --short | sed -E 's/^.. //' | grep -Ev "$allowed" || true)" | ||
| test -z "$bad" | ||
| test ! -e scripts/ci/source_fix_pr1715_no_model_job_timeout.py | ||
| test ! -e .github/workflows/source-fix-pr1715-no-model-job-timeout.yml | ||
| remote_head="$(git ls-remote origin refs/heads/fix/noema-review-job-timeout-minutes | cut -f1)" | ||
| test "$remote_head" = "$GITHUB_SHA" |
There was a problem hiding this comment.
🔍 Reviewed head remains transitional
The one-shot workflow defers the final contract to a later bot commit. Merge evidence generated before that push validates a tree the workflow intends to replace.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def replace_once(text: str, old: str, new: str, label: str) -> str: | ||
| """Replace one literal block and fail closed when branch contents moved.""" | ||
| count = text.count(old) | ||
| if count != 1: | ||
| raise SystemExit(f"PR1715 {label}: expected one literal block, found {count}") | ||
| return text.replace(old, new, 1) |
There was a problem hiding this comment.
🔍 Repair helper bypasses coverage gate
No test executes the new scripts/ci helper or its replacement boundaries. This conflicts with the repository's mandatory 100% coverage convention.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ording #1715 (merged to main, pulled in by this branch's merge) added test_noema_review_job_has_a_bounded_runtime_above_the_two_hour_model_allowance, which pins the literal Korean phrase "모델당 두 시간 이상 걸릴 수 있음을 수용한다" from docs/product-goal-directive.md as the documented justification for noema-review's >120min job timeout. That phrase only ever existed in main's own stale pre-2026-08-30 copy of the directive file; this branch's owner-directed rewrites (recorded across this session) reworded the same substantive 2+-hour-per-model commitment to "OpenCode·Strix·Noema의 모델당 2시간 이상을 수용한다" without changing its meaning. Update the test's pinned string to match the current, accurate directive text instead of reverting the directive's own wording to satisfy a test -- the directive is the authoritative verbatim record per this repo's own doctoring convention. Verified: this test file (10 passed), full suite (2612 passed, 1 skipped, 21 subtests, 0 failed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YgmaEKnPhrsbSyQBierFH
…1727) autofix's and noema-review's jobs each received a job-level timeout-minutes earlier the same day (#1714: 25min, #1715: 210min) while fixing a real, separate problem -- several central jobs had no timeout-minutes at all, letting a genuinely stuck job occupy a shared runner for up to GitHub's 360-minute default. That fix was correct for jobs that do bookkeeping or poll for a verdict a *different* process prepares (opencode-review.yml's poll_deadline_epoch), but wrong for these two: each job's body IS a synchronous model call (`opencode run` in autofix; two_phase.py's call_llm in noema-review), so a job-level bound directly caps the model's own reasoning/tool-use time once elapsed -- exactly the fixed inference-time cap docs/product-goal-directive.md #8 prohibits ("Model timeout은 application·Agent·Gateway 공통 상한 없이 기본 null이다"). Caught by Devin's automated review on .github#1661, which flagged a leftover debris file from this org's own autonomous self-repair loop (scripts/ci/source_fix_pr1715_no_model_job_timeout.py) that had correctly identified this bug and was mid-fix when it was reconciled away as apparent already-served-its-purpose debris -- it was not; its fix had not landed. This restores that fix by hand, per this org's "land it as a normal direct fix, not another self-modifying generator script" convention. Removes timeout-minutes: 25 from autofix and timeout-minutes: 210 from noema-review entirely (no replacement bound, matching the policy's default). Inverts the two contract tests that asserted a bound was present into tests asserting one is absent. Re-verified opencode-review.yml, pr-review-merge-scheduler.yml, and strix.yml's existing job-level timeouts against the same question and confirmed sound -- only these two needed reverting. See docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…zation The earlier entry called source_fix_pr1714/1715_no_model_job_timeout.py "harmless debris" and removed them; they were actually the org's autonomous repair loop mid-fix on a real bug (job-level timeouts capping synchronous model calls in autofix/noema-review) that had not yet landed. Devin's review on this PR caught it. Records the correction and the real fix (.github#1727, merged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Audited
noema-review.ymlfresh off currentmain(already has #1702/#1704/#1707 merged). Confirmed:while :; dopoll bounded only by transport-failure count) and item 3 (concurrency/cancel-in-progress) are sound here — every loop is bounded by a fixed iteration count, andcancel-in-progress: truealready covers the workflow's only high-frequency trigger (synchronize).cancel-closed-pr-runsnornoema-reviewdeclares a job-leveltimeout-minutes, so a stuck run falls back to GitHub's 360-minute platform default — the same defect class PR fix(scheduler): bound scan-pr-queue to timeout-minutes: 30 #1702 fixed forscan-pr-queue. This also silently disables both of this file's own manual cleanup sweeps (its real supersession mechanism, per the file's own PR fix(noema): fail closed on malformed LLM JSON instead of crashing #1507 comment) under exactly the load conditions that motivated fix(scheduler): bound scan-pr-queue to timeout-minutes: 30 #1702/fix(opencode-review): bound verdict-polling loop by wall clock #1707.Fix
cancel-closed-pr-runs:timeout-minutes: 20. Its only step is a single-repo, status-filteredgh api --paginatelist-and-cancel sweep (≤3 passes × 5 statuses), no branch update/merge — lighter thanscan-pr-queue'stimeout-minutes: 30.noema-review:timeout-minutes: 210. Its "Prepare Noema model verdict" step callstwo_phase.py'scall_llmthrough the same contextual-orchestrator gateway whose unbounded wait caused the 7–20 hour stuck runs fix(opencode-review): bound verdict-polling loop by wall clock #1707 fixed —noema_review_gate.py's own comment confirms that call has no fixed inference timeout. 210 min = the same ~180-min (3-hour) allowance fix(opencode-review): bound verdict-polling loop by wall clock #1707 used for its analogous model-wait deadline (comfortably abovedocs/product-goal-directive.md§8's documented "over two hours per model" policy, which names Noema explicitly), plus a 30-min buffer for the job's other steps.cancel-in-progressleft unchanged — no evidence it's safe to flip on the lower-frequency paths.Two new contract tests (
test_cancel_closed_pr_runs_has_a_bounded_runtime,test_noema_review_job_has_a_bounded_runtime_above_the_two_hour_model_allowance) extract each job's realtimeout-minutesvia the sameworkflow_text()pattern this file already uses.Test plan
python3 -m pytest tests/test_noema_orchestrator_workflow_contract.py tests/test_required_workflow_queue_contract.py tests/test_noema_review_gate.py tests/test_noema_review_handoff.py tests/test_noema_two_phase_handoff.py -q→ 250 passedpython3 -m pytest tests -q→ 2592 passed, 1 pre-existing skipactionlint .github/workflows/noema-review.yml→ cleanNote for merge
This may need bypass-merge given the ongoing org-wide Actions capacity incident referenced in #1707/#1702 — not merging this myself.
🤖 Generated with Claude Code