fix(ci): remove elapsed-time job cap on the hourly loop; pin model to orchestrator/free - #1027
Conversation
…efault
The `loop` job in opencode-hourly-loop.yml had no explicit job-level
timeout-minutes on main, so GitHub Actions applied its implicit
360-minute (6h) hard per-job cap. Combined with this workflow's
`concurrency: { group: opencode-hourly-loop, cancel-in-progress: false }`,
a single wedged run (the gateway-start or hourly-loop-agent steps, which
stay deliberately unbounded because model inference is expected to take
2h+ per model) could occupy the concurrency group for up to 6 hours,
during which every subsequent hourly trigger would queue and then be
superseded rather than actually run.
Set an explicit job-level timeout-minutes: 300 (5h) on the `loop` job.
300 was chosen as comfortably above the documented "2+ hours per model"
floor for legitimate long runs, while leaving real margin under the
platform's 360-minute hard ceiling -- generous enough that a genuinely
long legitimate run still completes, tight enough that a truly wedged
run can no longer come close to costing multiple hourly cycles. Placing
it at the job level (rather than per-step) means it covers every step
in `loop`, including "Start the contextual-orchestrator gateway with
auto-discovery" and "Run the hourly loop agent", without needing
per-step values that could conflict with the standing "no timeout on
model/inference calls" policy for those two steps individually.
No contract test pins the previous value: tests/test_hourly_opencode_loop_contract.py
asserts other properties of this workflow file and does not assert
timeout-minutes, so it required no change and still passes.
Addresses a Devin PR review finding on #971 ("Hourly runs can queue
indefinitely") against this same workflow file. Scoped as an independent
fix rather than folded into #971 because this exact job-level timeout
line is itself part of #971's own diff (that branch removes it and adds
narrower per-step timeouts for the deterministic setup steps only,
still leaving no job-level bound) -- see PR body for details.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Warning Review limit reachedNext included review available in 6 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 (6)
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 exact-head owner handoff for This PR correctly identifies a real platform boundary, but There is also a live overlapping valid delta that must not be discarded: protected Required RED before production change:
Smallest GREEN: integrate the #1006/#1013 Current exact-head admission is independently non-passing: CodeQL PR run |
seonghobae
changed the title
seonghobae
changed the title
|
Thanks — this correction is right, and I've verified it against a fresh clone of the current branch head ( What was right: What's now on the branch (I found corrective commits already pushed here after your comment landed, verified them, and updated the PR title/body to match — no additional code push was needed from me):
Confirmed: YAML parses, Honestly out of scope, still true: this does not solve the original concurrency problem — a genuinely wedged run can still occupy the #1006 / #1013: both are still open and unmerged as of this comment ( CodeQL Generated by Claude Code |
|
Fresh successor-carryover check against the current There is also a contradiction inside #1006 that must be repaired during integration rather than copied blindly: the ADR body says Successor RED/GREEN acceptance: add a documentation contract test that requires ADR 0007 body/index status agreement and requires |
Ports #1006's decision record onto this branch's already-corrected fix (the removed job-level model deadline and orchestrator/free pin) so the two overlapping PRs converge on one canonical successor instead of duplicating the same underlying repair: - docs/adr/0007-hourly-loop-orchestrator-free-pool-pin.md: #1006's ADR, carried over unchanged. Its body already reads `Status: Proposed`. - docs/adr/README.md: register the ADR-0007 row as `Proposed`, matching the body -- the Proposed/Accepted mismatch a review comment flagged on #1006 was between an earlier ADR draft and its index row; #1006's current head had already corrected both to `Proposed` before this port, so no repair was needed here beyond carrying the agreement forward. - .github/workflows/opencode-hourly-loop.yml: restore #1006's top-of-file comment block explaining the orchestrator/free vs. auto-discovery distinction and pointing at ADR-0007, plus a note on the no-job-deadline policy this PR already implements. - tests/test_hourly_opencode_loop_contract.py: port #1006's ADR-content test, and add a new test_adr_0007_body_and_index_status_agree_and_stay_proposed_while_open acceptance-criteria test that parses both the ADR body `Status:` line and the docs/adr/README.md index row and fails if they disagree, or if either reads anything but `Proposed` while the decision is open. This is the regression test the review comment asked for so the body/index contradiction it found on #1006 cannot recur silently, here or on any future ADR. #1013 was compared against this diff and #1006's: its only content is the same orchestrator/auto -> orchestrator/free workflow/test edit, already present here and in #1006, with no ADR, no job-deadline fix, and no additional test coverage -- it carries no delta this PR is missing. Verified: tests/test_hourly_opencode_loop_contract.py (4 passed), interrogate 100% on the modified test file, workflow YAML parses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Verified all four points against fresh reads of #1006, #1013, and this PR's actual current heads, and acted on them. Summary:
Integration path taken (deviates from the suggested default of #1006-as-canonical, with justification): I picked this PR (#1027) as canonical instead of #1006, because #1006's workflow file still has Left explanatory comments on #1006 and #1013 noting this PR now supersedes them; I have not closed either, per this org's PR-closure discipline — that's for the queue owner once this PR actually merges. CI / CodeQL Generated by Claude Code |
|
Canonical-successor integration advanced with #980's only remaining valid semantic delta. Fresh evidence before the write:
I therefore ported only the non-obsolete setup-node delta into this canonical branch, not #980's whole stale workflow. New #1027 exact head: Exact workflow acceptance at this head:
The setup-node SHA/tag pair is also independently corroborated by current public GitHub usage/release evidence. This makes #1027 the semantic successor for #980's remaining one-line workflow delta, but #980 should not be closed until this successor actually lands on protected main or another immutable successor proves full inheritance. |
seonghobae
added
area: ci-cd
ChatGPT Codex Connector
…g-fix session Adds a dated entry documenting today's remaining #971 session work not yet covered by an existing entry: the real main-merge conflict resolution (mergeable_state dirty -> blocked, tests/test_review_gateway.py resolved in favor of main's evidence-only-admission suite after main's 0db4e5a dropped max_agents), the two additional ThreadPoolExecutor-atexit-join fixes in endpoint_race.py and batch_routing.py's new _DaemonWorkerPool, the raw-Future callback-settlement regression fix, the _DaemonWorkerPool post-shutdown submit hardening, a verified-false-positive analysis of a proposed provider_routing equality guard, and a recap of the remaining open, large-scope work (bootstrap diversity heuristics, hourly-loop cancel-in-progress gap with a PR #1027 pointer, legacy identifier mixing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Adopt main@212ff437 without force-push; current main delta is disjoint from the hourly-loop workflow/docs/test changes.
# Conflicts: # docs/adr/README.md
Failing check triage: "Full unit and contract suite" (run 33649166922 / job 100311494984)What failed: at Not this PR's defect. This PR's diff only touches A fix already exists upstream, no new fix needed. Real merge conflict found and resolved. Pushed (non-force): merge commit Local verification before push (isolated worktree at the merge result, no other in-flight branch touched):
No code change was needed in this PR's own diff; the fix is the merge itself. Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| { | ||
| "\$schema": "https://opencode.ai/config.json", | ||
| "model": "contextual_orchestrator_gateway/orchestrator/auto", | ||
| "model": "contextual_orchestrator_gateway/orchestrator/free", |
There was a problem hiding this comment.
🟡 Excluded providers can serve hourly work
When discovery labels an OpenAI model free, orchestrator/free admits it through the generic gateway. The hourly agent can use a contractually excluded provider.
Prompt for agents
The hourly workflow requests orchestrator/free from scripts/ci/serve_seeded_gateway.py, which constructs the ordinary TaskOrchestrator. Its free-pool predicate enforces zero price and general-chat modality but does not enforce the review-side provider-account allowlist. contextual_orchestrator/review_gateway.py already defines REVIEW_FREE_POOL_CREDENTIAL_NAMES and excludes OPENAI_API_KEY regardless of discovered price. Route this workflow through a gateway construction path that applies that source-admission policy while preserving loopback authentication, bootstrap KV seeding, auto-discovery, and the required OpenCode API surface. Add an integration contract proving an OpenAI-derived zero-priced model cannot serve the hourly loop.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ## References | ||
|
|
||
| ContextualWisdomLab. (2026). *ADR-0003: Vendored contextual-orchestrator | ||
| review sidecar with governed gateway pools* [Architecture decision record, | ||
| amended 2026-09-02]. `ContextualWisdomLab/.github`, | ||
| `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md`. | ||
| https://github.com/ContextualWisdomLab/.github/blob/main/docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md | ||
|
|
||
| ContextualWisdomLab. (2026). *Pin orchestrator-routed KG extraction to the | ||
| `orchestrator/free` pool* [Proposed architecture decision in | ||
| ContextualWisdomLab/naruon#1525]. https://github.com/ContextualWisdomLab/naruon/pull/1525 |
There was a problem hiding this comment.
🔍 Research grounding remains undocumented
This process-policy ADR lacks the research artifact requested by AGENTS.md. Add applicable literature or record why none governs this operational decision.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Integration update (canonical successor to #1006 / #1013)
A review comment (#1027 comment)
correctly found that #1006 is not a proven no-op successor of this PR: it
carries
docs/adr/0007-hourly-loop-orchestrator-free-pool-pin.md,docs/adr/README.mdregistration, and a more explicit caller-vs-gatewaycontract test that this PR did not have. It also asked for a documentation
contract test requiring ADR-0007's body/index status to agree and to read
Proposedwhile the change is open.I verified all three PRs against their actual current heads and picked this
PR (#1027) as the canonical successor, not #1006, for a concrete reason the
review comment didn't have visibility into yet: #1006's workflow file still
contains
timeout-minutes: 55on theloopjob — it never removed thecaller-authored elapsed-time cap on model/gateway inference that is the
subject of this PR's own root-cause fix (see "Corrected approach" below and
the review comment above it). #1006 is strong on the ADR/decision-record side
and weak on the actual production defect; #1027 was the opposite. Rather than
duplicate effort across three overlapping PRs, I ported #1006's valid,
non-code delta onto this branch (commit
3788cf8):docs/adr/0007-hourly-loop-orchestrator-free-pool-pin.md— carried overunchanged from fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006. Its body already read
Status: Proposedon fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006'scurrent head (
1205e9f); the Proposed/Accepted contradiction the reviewcomment flagged had already been corrected there (both body and
docs/adr/README.mdindex row readProposed) by the time I read it, so nofurther repair to the ADR text itself was needed — just carrying that
agreement onto this branch.
docs/adr/README.md— registered the ADR-0007 row asProposed, matchingthe body.
.github/workflows/opencode-hourly-loop.yml— restored fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006's top-of-filecomment explaining
orchestrator/freevs. gateway auto-discovery andpointing at ADR-0007, plus a note on this PR's own no-job-deadline policy.
tests/test_hourly_opencode_loop_contract.py— ported fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006's ADR-contenttest, and added a new
test_adr_0007_body_and_index_status_agree_and_stay_proposed_while_opentest: it parses the ADR body's
- Status:line and thedocs/adr/README.mdindex row for ADR 0007, fails if they disagree, andfails if either reads anything but
Proposedwhile the decision is open —the exact acceptance criterion the review comment asked for.
#1013 was diffed against both this PR and #1006: its only content is the
same
orchestrator/auto→orchestrator/freeworkflow/test edit, alreadypresent here (and in #1006), with no ADR, no job-deadline fix, and no
additional test coverage. It carries no delta this PR is missing and is fully
redundant once this PR merges.
Verified after the port:
pytest tests/test_hourly_opencode_loop_contract.py -qpasses (4 passed),
interrogateis 100% on the modified test file, and theworkflow YAML parses. I've left comments on #1006 and #1013 explaining this
PR now supersedes them and pointing at this integration; I have not closed
either — that's for the queue owner once this PR actually merges, per this
org's PR-closure discipline.
Still open, not this PR's call: whether
docs/adr/0007-...should also bemigrated so the ADR row is
Accepted— that only happens after this PR mergesto protected
mainunder ordinary exact-head authority (the new contract testenforces that ordering).
Corrected approach (superseding the original PR description below the line)
The original version of this PR added
timeout-minutes: 300at theloopjoblevel to replace
main's pre-existingtimeout-minutes: 55. A review comment(#1027 comment)
correctly identified that any finite job-level timeout that covers the
"Start the contextual-orchestrator gateway" / "Run the hourly loop agent"
steps is a caller-owned elapsed-time termination of model/gateway inference —
exactly what this org's standing policy (model/inference timeouts default to
null; only user cancellation, provider termination, or an explicit auditedadmin timeout may end a call) forbids. Raising 55 → 300 moved the violation
further out; it did not remove it.
main's existing55-minute cap on thissame job was an equivalent, independent instance of the same bug.
What actually changed (now on this branch, verified against the current
head
f0fa0e9):timeout-minutesfrom theloopjob entirely — noreplacement value. The job now relies on GitHub's implicit 360-minute
hosted-runner ceiling as an unavoidable platform fact rather than an
artificial "model timeout policy." Only the already-narrow setup steps
(checkout, pip/npm install, token generation) remain scoped by their own
step timeouts where present; the gateway-start and hourly-loop-agent steps
stay unbounded.
orchestrator/auto→orchestrator/freein all three callsites (
opencode.jsonmodel, themodelscatalog key, and theopencode run --modelflag), plus the workflow's top comment. This closesthe same gap independently carried by fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006 and fix(hourly-loop): route OpenCode hourly maintenance agent to orchestrator/free #1013 (both still open,
unmerged, as of this update — see "Relationship to fix(ci): pin hourly OpenCode loop to orchestrator/free pool #1006 / fix(hourly-loop): route OpenCode hourly maintenance agent to orchestrator/free #1013" below).
.github/opencode/hourly-loop-prompt.mdto drop the "at most 45minutes" self-imposed heuristic and the "highest-leverage gap" informal
ranking, replacing both with fail-closed language: no repository-authored
elapsed-time limit on model work, and no invented ordering when multiple
product gaps are simultaneously admissible.
tests/test_hourly_opencode_loop_contract.py: renamed/updated thefree-pool assertion (forbids
orchestrator/autooutright) and addedtest_hourly_loop_has_no_repository_authored_model_job_deadline, whichasserts the
loopjob header carries notimeout-minutes:and the promptno longer contains the removed heuristics.
CHANGELOG.d/hourly-loop-job-timeout-cap.mdfragment describing thecorrection.
Verified on a fresh clone of this exact branch head: YAML parses
(
yaml.safe_load), andpytest tests/test_hourly_opencode_loop_contract.py -qpasses (2 passed).
Known limitation — NOT fixed by this PR
Removing the job-level timeout does not solve the original concurrency
problem this PR set out to address:
concurrency: { group: opencode-hourly-loop, cancel-in-progress: false }still means a single trulywedged run can occupy the group for up to GitHub's implicit ~360-minute
hard ceiling, during which every other hourly trigger queues up and is then
superseded rather than run. Solving that properly needs checkpointing /
resumability in a durable execution boundary (the reviewer's suggested
direction) — a materially larger, separate piece of work this PR
deliberately does not attempt. Filing that as follow-up work rather than
scope-creeping it into this fix.
Relationship to #1006 / #1013
See the integration update at the top of this description — this PR is now
the canonical successor and carries #1006's ADR/documentation delta forward.
Relationship to #971
(unchanged from the original PR: see discussion on
#971 —
main'sloopjob carriedtimeout-minutes: 55since #834/9c299fa4; #971'sbranch drops the job-level bound and replaces it with narrow per-step
timeouts on setup steps only, leaving the gateway/inference steps unbounded,
which is now also this PR's end state. #971 remains untouched by this PR;
it is draft with unrelated open blockers.)
🤖 Generated with Claude Code
https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Generated by Claude Code