fix(opencode): stop two echo-only jobs from serializing the review critical path - #1910
Merged
Merged
Conversation
…itical path opencode-review.yml chained five jobs in series: required-workflow-bootstrap -> admit-current-head -> coverage-source-tree -> coverage-evidence -> opencode-review-target. The middle two exist solely to hold branch-protection contexts; each one's entire body is a single `echo`, and neither declares `outputs:`, so both `needs:` edges through them ordered work without carrying any data. Ordering is not free. A job is not created until its `needs:` complete, so under a saturated queue every link waits out the whole queue again. Measured on naruon#1528 (run 33581213805), where each job's created_at equals the previous job's completed_at: required-workflow-bootstrap waited 7h57m, ran 4s coverage-source-tree waited 9h40m, ran 4s coverage-evidence waited 13h01m, ran 5s opencode-review waited 12h13m That is ~22h41m of queue time spent to print two sentences, with the actual review held behind it. Both edges are removed and the two context holders now depend on admit-current-head directly, dropping serial depth from 5 to 3 and the number of queue waits from 4 to 2. Safety, verified rather than assumed: - coverage-evidence had no `if:` and relied entirely on transitive skipping through coverage-source-tree. Its admission gate is now stated explicitly, so an unadmitted head still skips it. Dropping the edge without this would have run a required context on unadmitted heads. - opencode-review-target never reads coverage-evidence at runtime; the only reference was the `needs:` line itself. The consumer of that context is opencode-review-dispatch.yml via scripts/ci/opencode_coverage_identity.py, which resolves it against the check-runs API on its own schedule. - No test asserts this ordering. scripts/ci/test_strix_quick_gate.sh:1203 names both jobs but as set membership, not sequence. - Branch protection evaluates required contexts independently; all of required-workflow-bootstrap, coverage-evidence and opencode-review still report. Scope is deliberately limited to opencode-review.yml. opencode-review-dispatch.yml has jobs of the same two names whose edge is a real data dependency -- its coverage-source-tree uploads the materialized PR merge tree and its coverage-evidence downloads it -- and must not be parallelized. Credit to peer session review for catching that name collision, and to a Codex audit for finding the echo-only jobs in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 5 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 (1)
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 |
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
… dimension This entry counted runner slots and concluded reclaiming 1 of 33 could not matter in a capacity-bound queue. Wrong. The two echo-only jobs sit in series on the review critical path, and a job is not created until its needs: complete, so each link waits out the whole queue again. Measured on naruon#1528: 9h40m and 13h01m of queue wait for 4s and 5s of runtime, ~22h41m total holding the real review behind it, on every PR across 76 repositories. Slot count made that look like 3%; critical-path latency is the dimension that governs how long a PR actually takes to clear. Fix shipped as #1910 (serial depth 5 -> 3, queue waits 4 -> 2). Also records the two non-obvious safety conditions: coverage-evidence had no if: of its own and relied on transitive skipping, and opencode-review-dispatch .yml has same-named jobs whose edge is a real artifact dependency -- job names are unique only within a workflow file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…tion #1910 implements the fix, correctly scoped to opencode-review.yml only: five serial links to three, queue waits per PR from four to two, with the explicit admission if: carried onto coverage-evidence and coverage-evidence dropped from opencode-review-target's needs after confirming that job never reads the context at runtime. Adds the cross-family (Codex) reproduction of all three points, including the artifact name this record had not cited (opencode-coverage-source). Records the implementing session's own honest note: their change was safe because they scoped it narrowly, not because they had checked for the name collision. That generalizes better than the specific fix -- a job name is unique only within one workflow file, and the same name in another file can carry the opposite safety property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 5, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…1904) * docs(items15-17): measure Detect changed scope gate-job runner waste Took jobs-per-PR as the metric for the 60-job ceiling complaint and measured a real baseline: one completed .github PR head produced 57 check runs across 2 attempts (~28/attempt), with "Detect changed scope" the most repeated job name at 5 per attempt. The obvious reading -- 5 duplicate gates, 5 wasted runners -- is wrong, and this entry records the corrected version. Whether a gate is waste depends on its consumer count: security-scan.yml amortizes one gate across 4 gated jobs (legitimate; self-gating would trade 1 runner for 4 redundant API calls), while sast-semgrep.yml and strix.yml each gate exactly one consumer, so each burns two runner allocations where one suffices. Real opportunity: 2 runner slots per PR, org-wide (both are ruleset-required workflows dispatched into ~74 repos). Records the load-bearing constraint any fix must preserve -- the ruleset ignores on: filters, so the job-level gate cannot become a trigger-level skip. Not fixed here: these are live org-wide required workflows and the org CI cannot complete runs at all right now, so the edit belongs in its own PR that can actually be validated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(items15-17): two echo-only jobs sit serially on the review critical path Extends the gate-job measurement with a second, larger finding. A peer session's read-only Codex pass spotted that opencode-review.yml's coverage-source-tree job does nothing but echo a string; verified here against origin/main, which shows the problem is bigger than one job. opencode-review.yml chains five jobs serially -- bootstrap -> admit-current-head -> coverage-source-tree -> coverage-evidence -> opencode-review-target -- and two of those links only print a sentence. A job is created only after its needs: predecessor finishes, so under queue saturation each link pays a full fresh queue wait. Quantified with this session's own item-13 audit data for naruon#1528 (run 33581213805): the two echo-only links waited ~9h40m and ~13h1m respectively, contributing roughly 22h41m of pure queue latency to one PR while holding the actual review behind them. Both jobs are load-bearing as reported branch-protection contexts and cannot simply be deleted, but their needs: edges are ordering, not data dependency -- neither produces an output the next consumes. Records the parallelization option and explicitly flags it as needing a check that nothing depends on these contexts completing in order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(items15-17): close the order-dependency question, add the skip guard A peer session independently re-pulled run 33581213805 and confirmed the serialization mechanism rather than inferring it: each job's created_at is exactly its predecessor's completed_at, so a job is not queued until its needs: predecessor finishes. Execution was 4 and 5 seconds against 9h40m and 13h1m of waiting. Closes the order-dependency question this entry left open: no test asserts the needs: chain order, the merge scheduler reads only a context name and its exact-head conclusion (CANONICAL_CHECK_NAME), and neither job declares outputs. Adds a safety condition the first draft missed: coverage-evidence has no if: of its own and is skipped only transitively via coverage-source-tree's admission guard, so cutting that edge without moving the guard would let a required context run on an unadmitted head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(items15-17): warn that two files define these job names, one unsafe Two sessions independently reasoned about "the coverage jobs" without checking that the name resolves to two different jobs in two workflow files. opencode-review.yml (required, pull_request_target) holds the echo-only placeholders this entry analyses. opencode-review-dispatch.yml (privileged, repository_dispatch) defines jobs with the same names that do the real work: coverage-source-tree materializes the PR merge tree and uploads it as an artifact, coverage-evidence downloads that artifact and runs with a 300 minute timeout. There the edge is a hard data dependency, and cutting it would break coverage measurement outright. Caught by opening scripts/ci/test_strix_quick_gate.sh, whose assertions describe coverage-source-tree as materializing and uploading a merge tree -- contradicting "it only echoes" and exposing the second file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(items15-17): record the implementation and cross-family confirmation #1910 implements the fix, correctly scoped to opencode-review.yml only: five serial links to three, queue waits per PR from four to two, with the explicit admission if: carried onto coverage-evidence and coverage-evidence dropped from opencode-review-target's needs after confirming that job never reads the context at runtime. Adds the cross-family (Codex) reproduction of all three points, including the artifact name this record had not cited (opencode-coverage-source). Records the implementing session's own honest note: their change was safe because they scoped it narrowly, not because they had checked for the name collision. That generalizes better than the specific fix -- a job name is unique only within one workflow file, and the same name in another file can carry the opposite safety property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
6 tasks
seonghobae
pushed a commit
that referenced
this pull request
Sep 5, 2026
Resolves the AGENTS.md and CLAUDE.md conflicts against #1914. Both sides were purely additive, so both are kept; verified by section count rather than by the absence of markers -- AGENTS.md 5 -> 6 sections with every main section retained, CLAUDE.md 7 -> 7 with this PR's bullet living inside an existing section. This ends the push freeze on this branch deliberately, and voids the experiment it was running. A dirty head cannot merge however good its review, so freezing a conflicted PR to protect a review run protects nothing; the merge conflict outranks it. New: "absence of data flow is not evidence that an edge is safe to cut." Three edges here share one surface signature -- upstream job declares no outputs, nothing downstream references its needs.*, sits above the real work -- and they are not equally safe. Detect changed scope is the documented path-filtering safety mechanism; coverage-source-tree and coverage-evidence were genuinely ordering-only and #1910 was right to parallelise them; required-workflow-bootstrap has the same signature as the second and is a pull_request_target trust boundary, rejecting untrusted fork PRs and verifying the immutable policy source before anything downstream starts. Cutting it lets three downstream jobs run concurrently with the fork check. The discriminator is never the edge's shape: read every step of the upstream job and ask what becomes possible if it has not run. Recorded with the cheaper alternative that keeps the guarantee -- noema-review.yml enforces the same boundary as a per-job if:, and a job whose if: is false is never created. That makes two controls in this repository that read as waste, signature 2's trusted_ref pin being the first, so the entry names the structure: on pull_request_target a control's cost is visible in the queue while its benefit is invisible until it is gone, and optimization pressure points at the security gates. Also records a false negative hit while resolving this very conflict: git merge-tree plus a grep for ^<<<<<<< returned 0 while a real merge conflicted in both files. The authoritative check is a real merge in a scratch worktree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
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.
The problem
opencode-review.ymlchained five jobs in series:required-workflow-bootstrap→admit-current-head→coverage-source-tree→coverage-evidence→opencode-review-targetThe middle two exist only to hold branch-protection contexts. Each one's entire body is a single
echo, and neither declaresoutputs:— so bothneeds:edges through them ordered work without carrying any data.Ordering is not free here. A job is not created until its
needs:complete, so under a saturated queue every link waits out the whole queue again. Measured onnaruon#1528(run33581213805), where each job'screated_atequals the previous job'scompleted_at— which is what proves the mechanism:required-workflow-bootstrapcoverage-source-treecoverage-evidenceopencode-reviewThat is roughly 22h41m of queue time spent to print two sentences, with the actual review held behind it. Multiplied across every PR in the 76 repositories the ruleset injects this workflow into.
The change
Both echo jobs now depend on
admit-current-headdirectly. Serial depth 5 → 3; queue waits per PR 4 → 2. No job is removed and no context stops reporting.Safety, verified rather than assumed
coverage-evidencehad noif:— it relied entirely on transitive skipping throughcoverage-source-tree. Its admission gate is now stated explicitly, so an unadmitted head still skips it. Dropping the edge without this would have run a required context on unadmitted heads; this was the real hole in the first draft of the idea.opencode-review-targetnever readscoverage-evidenceat runtime. The only reference was theneeds:line itself. The actual consumer isopencode-review-dispatch.ymlviascripts/ci/opencode_coverage_identity.py, which resolves the context against the check-runs API on its own schedule and is therefore order-independent by construction.scripts/ci/test_strix_quick_gate.sh:1203names both jobs, but as set membership (checks fast-approval ignores), not sequence.required-workflow-bootstrap,coverage-evidenceandopencode-reviewall still report.Scope limit — deliberate, and load-bearing
Changes are confined to
opencode-review.yml.opencode-review-dispatch.ymlhas jobs of the same two names whose edge is a genuine data dependency and must not be parallelized: itscoverage-source-tree(timeout-minutes: 12) uploads the materialized PR merge tree, and itscoverage-evidence(timeout-minutes: 300) downloads it. Verified directly:upload-artifact=True/download-artifact=Truerespectively. Breaking that edge would silently destroy coverage measurement.Provenance
The echo-only jobs were first surfaced by an independent Codex audit run against the repo with no knowledge of my earlier conclusion — my own analysis in #1905 had wrongly classified them as load-bearing gate jobs by pattern-matching the documented
changed-scopedesign without opening them. A peer session then measured the queue cost and caught the same-name collision with the dispatch workflow. #1905's "3% of slots, not worth shipping" reasoning is superseded by this data: slot count was the wrong dimension — critical-path latency is the one that matters.Test plan
if:on all three gated jobsgit diff --statconfirms a single file changed (+22/−2)🤖 Generated with Claude Code