[codex] Require OpenCode inline suggested diffs - #8
Merged
Merged
Conversation
seonghobae
enabled auto-merge
June 21, 2026 09:16
Contributor
OpenCode Review Overview
|
Contributor
There was a problem hiding this comment.
OpenCode Agent approved this PR.
The changes to the OpenCode review workflow are well-structured and maintain existing functionality while adding new features for handling inline comments and failed checks. No security, privacy, or regression risks were identified.
- Result: APPROVE
- Reason: No blockers or regressions detected in the changes.
- Head SHA:
a143c04f91ccc2ce3058cb373a16f6eddf501221 - Workflow run: 27899790694
- Workflow attempt: 1
This was referenced Jul 2, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
) * fix(noema-review): bound both jobs to a job-level timeout-minutes 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> * test(noema): encode no model wall-clock timeout repair * ci(noema): materialize PR1715 timeout-authority repair --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
…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>
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
…regation) Read scripts/ci/noema_review_gate.py's call_llm() directly: Noema issues exactly one structured-output request per review with no tool use, code execution, or iterative exploration -- the only depth lever (_required_probe_count) raises the minimum cited-probe count on that same single response, not the number of reasoning steps or context gathered. This is a materially different architecture from CodeRabbit and Devin, both of which visibly explore surrounding context. Item 24's gap is therefore architectural (single-shot vs. multi-step/agentic), not a prompt-wording problem, and lines up with this doc's own general-guideline #8 already anticipating multi-step, effort-scaled test-time compute for these three reviewers. Item 23: searched scripts/ci/ and tests/ for any tracking of verdict accuracy or cross-reviewer disagreement and found none -- no mechanism exists, in any form, to recompile Noema's own review failure cases. Neither implemented this tick: both are new capabilities on an already-shipping reviewer (changed safety surface for tool use/execution against untrusted diffs; added load on the same shared orchestrator/free gateway the still-open Strix concurrency finding above is about), not bug fixes, so both stay deferred per this session's standing throttle policy pending queue relief or explicit instruction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
…ee pieces resolved Fixed-timeout replacement was already unnecessary (items 4/39, above). TRINITY role-effort allocation already exists via default_role_effort_catalog(), correctly gated behind ADR 0021's opt-in. The one real gap -- _select_agent's deterministic top-1 pick starving losing model-group members of both traffic and continued observations -- is now fixed via contextual-orchestrator#1034 (Thompson Sampling), independently re-verified: hand-derived win-probability math, 5x stability runs, 100% interrogate, full suite clean (3354 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
…cked job Devin Review caught a real deadlock risk in the cancel-in-progress:false redesign (f9d4373): the "Cancel superseded Noema runs after live-head validation" logic lived as a STEP inside the same noema-review job that carries the job-level concurrency group. With cancel-in-progress:false, a new push's entire run -- including that cleanup logic -- cannot even start until the group frees up, which (since the active run is protected) only happens when the older run finishes on its own. Noema inference deliberately has no wall-clock deadline (docs/product-goal-directive.md #8), so a long-running older-head review could block the current head's review from ever starting -- the cleanup mechanism meant to prevent exactly that was trapped behind the same non-preemptable group it needed to unblock. Fixed by extracting the cancellation logic into a new job, cancel-superseded-noema-runs, with no concurrency block of its own -- it can run immediately regardless of the noema-review job's group state, live-reverify the current head, and cancel a genuinely superseded active run via a direct API call, freeing the group for the current push's own review. Mirrors strix.yml's cancel-superseded-pr-runs and opencode-review.yml's cancel-superseded-opencode-review-runs, both already shaped this way (though opencode-review.yml's is still nested inside its own workflow-level concurrency block -- flagged separately). Moved concurrency itself from workflow-level to job-level (scoped only to noema-review) to match; actions: write moved with the cancellation logic to the new job, no longer needed on noema-review itself. Full suite (2704 tests) passes; also hardened several test boundary extractions that happened to be more fragile than intended once concurrency moved past permissions: in file order (found while fixing this: one test's strict equality check surfaced that a pre-existing unrelated comment between cancel-in-progress and the job's if: condition made the naive "next if:" boundary capture way more than intended). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
REQUEST_CHANGESfindings through GitHub reviewcommentsso each finding'ssuggested_diffis inside the inline review thread comment.REQUEST_CHANGESbody when GitHub rejects inline anchors, instead of silently approving or copying unanchored diffs into the overview.Contract
Line-specific OpenCode findings with
suggested_diffmust publish as GitHub review comments withpath,line,side: RIGHT, and a comment body containing the fenced diff. If the anchor cannot be created, the workflow leaves the diff out of the PR-level body and explains the anchor failure.Validation
git diff --check.github/workflows/opencode-review.ymlcommentspayload exists, inline comment body contains#### Suggested diff, andformat_request_changes_body()no longer contains a fenced diff block.