diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 834706ad7c..30c9e9a517 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -187,21 +187,22 @@ jobs: noema-review: name: noema-review runs-on: ubuntu-24.04 - # Bound this job well short of GitHub's 360-minute platform default. Its - # "Prepare Noema model verdict" step calls into two_phase.py's call_llm - # via the same contextual-orchestrator gateway whose unbounded wait was - # confirmed to stall runs for 7-20 hours in opencode-review.yml before - # PR #1707's fix -- and noema_review_gate.py's own comment says that - # step "remains governed by contextual-orchestrator rather than a fixed - # inference timeout", so nothing upstream of this job bounds it either. - # 210 minutes gives that step the same ~180-minute (3-hour) allowance - # PR #1707 set for its analogous model-wait deadline -- comfortably - # above this org's documented "accommodate over 2 hours per model" - # policy (docs/product-goal-directive.md #8) -- plus a 30-minute buffer - # 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 + # No job-level timeout-minutes here, deliberately. This job's "Prepare + # Noema model verdict" step calls two_phase.py's call_llm synchronously + # via the contextual-orchestrator gateway and blocks on the model's own + # response -- a job-level wall-clock bound here would cap the model's + # reasoning/tool-use time directly, which docs/product-goal-directive.md + # #8 prohibits ("Model timeout은 application·Agent·Gateway 공통 상한 없이 + # 기본 null이다"; "OpenCode·Strix·Noema의 모델당 2시간 이상을 수용한다"). An + # earlier version of this job set timeout-minutes: 210, reasoning it gave + # that step "the same ~180-minute allowance" PR #1707 gave an unrelated + # step -- that reasoning was wrong: #1707's poll_deadline_epoch bounds a + # step that polls GitHub for whether a *separately triggered* review + # process has posted a verdict yet (an async external wait), not a step + # that itself runs the model synchronously. Any fixed cap on a job whose + # body IS the synchronous model call is exactly the fixed inference-time + # cap the policy forbids. See + # docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md. if: >- github.event_name == 'repository_dispatch' || ( diff --git a/.github/workflows/pr-review-autofix.yml b/.github/workflows/pr-review-autofix.yml index 3da5a98ec9..505384ccfd 100644 --- a/.github/workflows/pr-review-autofix.yml +++ b/.github/workflows/pr-review-autofix.yml @@ -23,16 +23,22 @@ permissions: jobs: autofix: runs-on: ubuntu-latest - # Bound the job well short of GitHub's 360-minute platform default. Setup - # (checkout, OIDC token exchange, OpenCode CLI install, context collection) - # is API/IO-bound and normally finishes in a few minutes; the one - # `opencode run` call (12 agent steps, single fixed model, no - # multi-provider fallback pool unlike opencode-review-dispatch.yml's - # review job) is the dominant cost, followed by fast local validation - # and a single git commit/push. 25 minutes gives that single LLM run - # generous per-step room while still failing a hung invocation well - # before the platform cap. - timeout-minutes: 25 + # No job-level timeout-minutes here, deliberately. This job's dominant + # cost is `opencode run` (up to two invocations: the main autofix pass, + # and a base-merge conflict-resolution pass) -- a job-level wall-clock + # bound here would cap the model's own reasoning/tool-use time, which + # docs/product-goal-directive.md #8 prohibits ("Model timeout은 + # application·Agent·Gateway 공통 상한 없이 기본 null이다"; "OpenCode·Strix· + # Noema의 모델당 2시간 이상을 수용한다"). An earlier version of this job set + # timeout-minutes: 25, reasoning it gave the model call "generous room" -- + # that reasoning was wrong: any fixed job-level cap on a job whose body IS + # the synchronous model call terminates the model's work once elapsed, + # which is exactly the fixed inference-time cap the policy forbids, not a + # wall-clock bound on a step that merely waits on a separate async + # verdict (contrast opencode-review.yml's poll_deadline_epoch, which + # bounds a step polling for a verdict prepared by a different process, + # not the model call itself). See + # docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md. env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true TARGET_REPOSITORY: ${{ github.event.client_payload.target_repository }} diff --git a/docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md b/docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md new file mode 100644 index 0000000000..3e80cfb35b --- /dev/null +++ b/docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md @@ -0,0 +1,91 @@ +# Removing job-level timeout-minutes from autofix and noema-review + +## What was wrong + +Earlier the same day, `pr-review-autofix.yml`'s `autofix` job (#1714) and +`noema-review.yml`'s `noema-review` job (#1715) each received a job-level +`timeout-minutes` (25 and 210 respectively) as part of fixing a real, +separate problem: several central `.github` workflow jobs had **no** +`timeout-minutes` at all, so a genuinely stuck job (a hung transport, a +runner fault) could occupy a shared runner for up to GitHub's 360-minute +platform default, contributing to the org-wide Actions capacity incident +documented elsewhere in `docs/product-technical-gap-baseline.md`. + +That fix was correct for jobs whose steps do bookkeeping (cancel stale runs, +publish a status) or that poll for a verdict a *separate* process prepares +(`opencode-review.yml`'s `poll_deadline_epoch`, which bounds a step polling +GitHub for whether a repository-dispatch-triggered review process has posted +a receipt yet -- the model call itself happens in a different workflow, +`opencode-review-dispatch.yml`, which correctly stayed unbounded). + +It was **wrong** for `autofix` and `noema-review`, because in both of those +jobs the model call itself runs synchronously, in-job: + +- `autofix`'s "Run OpenCode review autofix" step runs `opencode run "$(cat + "$prompt_file")" ...` directly and blocks on its output (and a second + `opencode run` for base-merge conflict resolution, later in the same job). +- `noema-review`'s "Prepare Noema model verdict" step runs + `python3 .github/actions/noema-review/two_phase.py ...`, which itself + calls the model (`NOEMA_LLM_API_URL`, `NOEMA_LLM_MODEL=orchestrator/free`) + and blocks until it returns. + +A job-level `timeout-minutes` on either job does not merely bound "how long +this job waits for something external" -- it bounds the model's own +reasoning/tool-use time directly, because the model call is the job's +dominant, synchronous body. That is exactly the fixed inference-time cap +`docs/product-goal-directive.md` #8 prohibits: "Model timeout은 +application·Agent·Gateway 공통 상한 없이 기본 null이다" (no common upper bound +across the application/agent/gateway stack; defaults to null), and "정확성을 +우선하고 OpenCode·Strix·Noema의 모델당 2시간 이상을 수용한다" (prioritize +accuracy; accommodate over two hours per model for OpenCode/Strix/Noema -- +"over two hours" describes a floor on tolerance, not a ceiling to round up +to and hard-code). + +Both original PR descriptions and in-file comments justified the added +timeouts by analogy to `opencode-review.yml`'s `poll_deadline_epoch` fix +(#1707) -- e.g. "gives that step the same ~180-minute allowance PR #1707 set +for its analogous model-wait deadline." That analogy was the actual mistake: +`poll_deadline_epoch` bounds a step that polls for a verdict a *different, +separately triggered* process prepares (an async external wait with no +model call in the bounded step itself); `autofix`'s and `noema-review`'s +jobs are not analogous, because their bounded step **is** the model call. + +## What changed + +- `.github/workflows/pr-review-autofix.yml`: removed `timeout-minutes: 25` + from the `autofix` job. No replacement bound -- the job has no other + timeout mechanism, matching the policy's "기본 null" default. +- `.github/workflows/noema-review.yml`: removed `timeout-minutes: 210` from + the `noema-review` job. `cancel-closed-pr-runs` (pure GitHub API + bookkeeping, no model call) keeps its unrelated `timeout-minutes: 20`. +- `tests/test_pr_review_autofix_writer_security_contract.py`: + `test_autofix_job_has_a_bounded_runtime` (asserted a timeout WAS present, + 5-60 minutes) replaced with `test_autofix_job_has_no_job_level_timeout` + (asserts one is absent). +- `tests/test_noema_orchestrator_workflow_contract.py`: + `test_noema_review_job_has_a_bounded_runtime_above_the_two_hour_model_allowance` + (asserted a timeout WAS present, 120-360 minutes) replaced with + `test_noema_review_job_has_no_job_level_timeout` (asserts one is absent). + `test_cancel_closed_pr_runs_has_a_bounded_runtime` is untouched -- that + job has no model call, so its bound is correct as-is. + +## Why this was caught, and what stayed the same + +Devin's automated review on `ContextualWisdomLab/.github#1661` flagged a +leftover debris file, `scripts/ci/source_fix_pr1715_no_model_job_timeout.py` +-- part of this org's own autonomous self-repair loop, which had correctly +identified this exact bug and was in the middle of fixing it when its +generated PR was reconciled away as apparent "already-served-its-purpose +debris" without checking whether its fix had actually landed. It had not. +This doctoring entry and the accompanying fix restore, by hand (per this +org's "land it as a normal direct fix, not another self-modifying generator +script" convention), the fix that debris script was attempting. + +`opencode-review.yml`'s `poll_deadline_epoch` (#1707), `pr-review-merge-scheduler.yml`'s +`scan-pr-queue` timeout (#1702), and `strix.yml`'s `cancel-superseded-pr-runs` +/ `publish-manual-pr-evidence-status` timeouts (#1713) were all re-checked +against the same question -- "does the bounded job's own step body run the +model synchronously, or does it wait on a separate async actor / do pure +bookkeeping?" -- and confirmed sound: none of them bound a step that itself +runs a model call. `strix.yml`'s main `strix` job (which does run the model) +correctly remains unbounded, as before. diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index f6e97f745b..4e8b0e32fb 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -390,27 +390,34 @@ def test_cancel_closed_pr_runs_has_a_bounded_runtime() -> None: assert timeout < 360 -def test_noema_review_job_has_a_bounded_runtime_above_the_two_hour_model_allowance() -> None: - """noema-review must not fall back to GitHub's 360-minute platform default. - - Its "Prepare Noema model verdict" step calls into two_phase.py's - call_llm via the contextual-orchestrator gateway, which - noema_review_gate.py's own module comment says "remains governed by - contextual-orchestrator rather than a fixed inference timeout" -- so - nothing upstream of this job bounds that call. docs/product-goal-directive.md - section 8 documents that "중앙 OpenCode, Strix, Noema는 모델당 두 시간 - 이상 걸릴 수 있음을 수용한다" (central OpenCode, Strix, and Noema accept - that a model call may legitimately take over two hours), so the bound - must clear two hours (120 minutes) without falling back to GitHub's - 360-minute job default. +def test_noema_review_job_has_no_job_level_timeout() -> None: + """noema-review must not carry a job-level timeout-minutes. + + Its "Prepare Noema model verdict" step calls two_phase.py's call_llm + synchronously via the contextual-orchestrator gateway and blocks on the + model's own response -- a job-level wall-clock bound here directly caps + the model's reasoning/tool-use time once elapsed, which + docs/product-goal-directive.md #8 prohibits ("Model timeout은 + application·Agent·Gateway 공통 상한 없이 기본 null이다"). An earlier + version of this job set timeout-minutes: 210, reasoning it gave that + step "the same ~180-minute allowance" opencode-review.yml's + poll_deadline_epoch gives an unrelated step -- that reasoning was + itself the mistake: poll_deadline_epoch bounds a step that polls GitHub + for whether a *separately triggered* review process has posted a + verdict yet (an async external wait), not a step that itself runs the + model synchronously. Any fixed cap on a job whose body IS the + synchronous model call is exactly the forbidden inference-time cap. See + docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md. """ workflow = workflow_text("noema-review.yml") job = workflow.split(" noema-review:\n", 1)[1] match = re.search(r"^ timeout-minutes: (\d+)$", job, flags=re.MULTILINE) - assert match is not None, "noema-review must declare a job-level timeout-minutes" - timeout = int(match.group(1)) - assert 120 < timeout < 360 + assert match is None, ( + "noema-review must not declare a job-level timeout-minutes -- its " + "body is a synchronous model call, so any job-level bound caps " + "model inference time, which this org's model-timeout policy forbids" + ) assert ( "모델당 두 시간 이상 걸릴 수 있음을 수용한다" diff --git a/tests/test_pr_review_autofix_writer_security_contract.py b/tests/test_pr_review_autofix_writer_security_contract.py index b6e246a183..3f6119424f 100644 --- a/tests/test_pr_review_autofix_writer_security_contract.py +++ b/tests/test_pr_review_autofix_writer_security_contract.py @@ -96,21 +96,31 @@ def test_read_only_steps_do_not_prefer_mutation_credentials() -> None: assert "OPENCODE_APPROVE_TOKEN" not in header -def test_autofix_job_has_a_bounded_runtime() -> None: - """The autofix job must not fall back to GitHub's 360-minute platform default. - - Without a job-level timeout-minutes, a stuck OpenCode CLI invocation (a - rate-limited provider, a hung agent loop) could occupy a shared runner for - up to six hours. The job runs a single `opencode run` call against one - fixed model with a bounded 12-step agent budget -- not the multi-provider - fallback pool that justifies opencode-review-dispatch.yml's much longer - review job -- so it needs a much shorter bound than that job's default. +def test_autofix_job_has_no_job_level_timeout() -> None: + """The autofix job must not carry a job-level timeout-minutes. + + This job's body IS a synchronous `opencode run` call (up to two + invocations: the main autofix pass and a base-merge conflict-resolution + pass) -- a job-level wall-clock bound here directly caps the model's own + reasoning/tool-use time once elapsed, which + docs/product-goal-directive.md #8 prohibits ("Model timeout은 + application·Agent·Gateway 공통 상한 없이 기본 null이다"). An earlier version + of this job set timeout-minutes: 25, reasoning it gave the model call + "generous room" -- that reasoning was itself the mistake: any fixed cap + on a job whose body is the model call is exactly the forbidden + inference-time cap, not a bound on a step that merely waits on a + separate async verdict (contrast opencode-review.yml's + poll_deadline_epoch, which bounds a step polling GitHub for a verdict a + *different* process prepares, not the model call itself). See + docs/doctoring/autofix-and-noema-review-model-job-timeout-removal.md. """ workflow = _workflow_text() job = workflow.split(" autofix:\n", maxsplit=1)[1] job_header = job.split(" steps:\n", maxsplit=1)[0] match = re.search(r"^ timeout-minutes: (\d+)$", job_header, flags=re.MULTILINE) - assert match is not None, "autofix must declare a job-level timeout-minutes" - autofix_timeout = int(match.group(1)) - assert 5 <= autofix_timeout <= 60 + assert match is None, ( + "autofix must not declare a job-level timeout-minutes -- its body is " + "a synchronous model call, so any job-level bound caps model " + "inference time, which this org's model-timeout policy forbids" + )