Skip to content

docs(doctoring): verify item 13's stale-head-cancellation hypothesis is refuted - #1760

Merged
seonghobae merged 10 commits into
mainfrom
docs/item13-concurrency-cancellation-audit
Sep 3, 2026
Merged

docs(doctoring): verify item 13's stale-head-cancellation hypothesis is refuted#1760
seonghobae merged 10 commits into
mainfrom
docs/item13-concurrency-cancellation-audit

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Investigates backlog item 13's specific hypothesis — that Strix/OpenCode Review/Noema fail to cancel a PR's previous-head run on a new push — with a 9-agent workflow (4 independent file audits, 1 direct pull of the item's own cited naruon PR#1528 evidence, 4 adversarial re-verification passes, all refuted: false).
  • Item 13's own hypothesis and cited evidence: refuted. strix.yml, opencode-review.yml, and pr-review-merge-scheduler.yml already reliably cancel a stale prior-head run, via correctly-scoped native concurrency: (fixed after a real prior incident, #1568, for OpenCode Review) or purpose-built same-file jobs that call the GitHub Actions API directly (Strix's cancel-superseded-pr-runs, the merge scheduler's hourly org-queue-sweep). The item's own cited evidence (naruon PR fix(ci): correct required-workflow-bootstrap job-scope check boundary #1528) shows pure queue starvation on one unchanged SHA instead — a Strix run waited 23h22m before starting, a paired OpenCode Review run was still queued 24+ hours later — corroborating the already-recorded plan-level concurrency ceiling (docs/doctoring/actions-plan-concurrency-ceiling-20260903.md) with a concrete example rather than a cancellation bug.
  • But the investigation surfaced a real, separate, confirmed bug: noema-review.yml has no head-SHA component in its concurrency group. GitHub does not guarantee webhook/dispatch delivery order; if an older push's synchronize event is processed after a newer one's, the older run's mere entry into the shared group cancels the newer, still-valid, in-flight review — before the older run's own stale-trigger check ever runs. Confirmed via two independent adversarial re-verification passes that found no refutation. Not fixed in this PR — a change to live, security-critical CI concurrency configuration deserves its own dedicated PR with a regression test, not a same-breath edit to documentation. A fix (SHA-scoping the group, mirroring OpenCode Review's own #1568 fix) has since been developed on ContextualWisdomLab/.github#1661, independently re-verified, and is pending merge.
  • No fix applied in this PR. The user's standing chicken-and-egg bypass-merge authorization was deliberately not used here for the same reason: no fixable bug in item 13's own hypothesis was found, and the one real bug found belongs in its own PR — forcing either into this docs-only PR would mean either inventing a problem the evidence doesn't support, or bundling an unrelated live CI change into documentation.

Test plan

  • Docs-only change; no code paths affected.
  • Every workflow finding was independently re-fetched fresh from raw.githubusercontent.com and re-verified by a separate adversarial pass before being accepted.
  • The cited naruon run/PR evidence was pulled live from the GitHub API, not assumed from the item's description.
  • Every review-comment finding (Devin, CodeRabbit) across multiple rounds was independently re-verified against live sources before being accepted or corrected, including two of this session's own earlier drafts being caught understating/overclaiming — see the record's own "Devin Review" callouts.

🤖 Generated with Claude Code

…is refuted

Investigated with a 9-agent workflow (4 independent audits of strix.yml,
opencode-review.yml, noema-review.yml, pr-review-merge-scheduler.yml + 1
direct pull of the item's own cited naruon PR#1528 evidence + 4 adversarial
re-verification passes, all refuted:false). All four central workflows
already reliably cancel a stale prior-head run on a new push, via correctly
scoped native concurrency or purpose-built same-file API-driven cancellation
jobs. The cited evidence instead shows pure queue starvation on a single
unchanged SHA (23h22m queue wait before a Strix run even started; a paired
OpenCode Review run still queued 24+ hours later) -- corroborating the
already-recorded plan-level concurrency ceiling with a concrete example
rather than a cancellation bug. No fix applied; the standing chicken-and-egg
bypass-merge authorization was not used since no fixable bug was found --
forcing one would mean inventing a problem the evidence doesn't support.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

두 문서가 stale-head 취소 가설에 대한 감사 결과를 기록합니다. 네 개 워크플로는 이미 이전 실행을 폐기하는 경로를 사용합니다. PR #1528의 문제는 취소 누락이 아니라 큐 용량 포화로 분류됩니다.

Changes

Stale-head 취소 감사

Layer / File(s) Summary
감사 방법과 결과
docs/doctoring/item13-stale-head-cancellation-audit-20260903.md
감사 방법과 검증 결과를 기록합니다. 네 개 워크플로의 stale-head 취소 경로를 설명합니다. PR #1528의 큐 대기 증거와 미확인 pr-governance.yml lead를 기록합니다.
기술 기준선 갱신
docs/product-technical-gap-baseline.md
Backlog item 13의 stale-head 취소 가설이 반증되었음을 추가합니다. 관련 현상을 구성 버그가 아닌 큐 용량 문제로 분류합니다.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 2e249

The audit correctly documents a queue-starvation finding overall, but two documents inaccurately describe queued workflow runs as absent. Update the wording to distinguish an existing queued run from a job that never started before merging.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 backlog item 13의 stale-head-cancellation 가설을 검증하고 반박한 문서 변경을 정확하게 요약합니다. 주요 변경 사항과 일치하며 간결하고 구체적입니다.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/item13-concurrency-cancellation-audit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

A peer session's check_run-based waste claim missed that the job's own if:
already restricts that trigger path to CodeRabbit checks -- GitHub Actions
requests no runner for a skipped job, so that vector isn't job-slot waste.
Read pr_governance_gate.sh in full: it evaluates live required-check/
review-thread/CodeRabbit state on every run, not a pure function of head
SHA, so a same-head debounce (the natural reading of the proposed fix)
would be unsafe -- it could leave the gate reporting a stale blocker list
after a check finishes or a review lands. No fix implemented; recorded as
still open rather than forced.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

…13 audit

- CodeRabbit: "with no run at all" contradicted the same sentence's own
  status: queued evidence -- a run existed, its job just never started.
  Reworded to "with no job started" in both the doctoring record and the
  gap-baseline entry.
- Devin (analysis): qualify the bare naruon PR #1528 references with the
  ContextualWisdomLab/ owner prefix so they resolve as cross-repo links from
  this repo, consistent with this cycle's established convention.
- Devin (bug): soften the unverified claim that the five pull_request_target
  firings on one unchanged SHA were "near-certainly labeled/unlabeled" --
  only synchronize was ruled out; the specific event types were not actually
  checked. Say so.
- Devin (bug, most substantive): the audit's noema-review.yml row claimed
  unconditional reliability for native cancel-in-progress, but that
  mechanism cancels whichever run most recently entered the concurrency
  group -- keyed on run-creation order, not head-SHA recency. An older
  push's synchronize event processed after a newer one's could in principle
  cancel the current run instead of the stale one; existing tests
  (tests/test_noema_review_gate.py) prove the *explicit* cancellation step
  can't do this, but don't cover native cancel-in-progress itself. No
  evidence this has ever happened, and no workflow change is made here --
  recorded as an open, unverified risk (footnoted in the table, and as a new
  bullet in "What this resolves, and what it does not") rather than silently
  dismissed or asserted as a confirmed bug, consistent with this record's
  own practice for its other open leads.

Grepped tests/ for the exact strings touched here; only
tests/test_product_technical_gap_baseline.py pins this file's content, and
it still passes. Full suite: 2630 passed, 1 skipped (two pre-existing
Python-3.12-only test files not collectible under this sandbox's 3.11).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4

Copy link
Copy Markdown
Contributor Author

Pushed 5a08116 addressing the outstanding review findings on 087646d:

  • CodeRabbit (wording): "with no run at all" contradicted the same sentence's own status: queued evidence — a workflow run existed, its job just never started. Reworded to "with no job started" in both the doctoring record and the gap-baseline entry.
  • Devin (traceability): qualified the bare PR #1528 references with the ContextualWisdomLab/ owner prefix so they resolve as cross-repo links from this repo.
  • Devin ("Run causes remain unverified"): the record asserted the five pull_request_target firings on one unchanged SHA were "near-certainly labeled/unlabeled" — only synchronize was actually ruled out as their type. Reworded to say the specific event types were not verified.
  • Devin ("Delayed Noema events cancel current reviews", the 🔴 finding): this is real and worth recording, not silently dismissed or hastily "fixed" by redesigning noema-review.yml's concurrency in a docs PR. Native cancel-in-progress cancels whichever run most recently entered the concurrency group — keyed on run-creation order, not head-SHA recency. An older push's synchronize event processed after a newer one's could in principle cancel the current run instead of the stale one. tests/test_noema_review_gate.py proves the workflow's explicit cancellation step can't do this (its .id < $current guard), but that doesn't cover native cancel-in-progress itself, which acts above any step in the workflow. No evidence this has ever occurred and no reproduction was attempted, so it's recorded as an open, unverified risk (a footnote on the noema-review.yml table row, an added caveat paragraph, and a new bullet under "What this resolves, and what it does not") rather than asserted as either a confirmed bug or refuted — consistent with this record's own treatment of its other open lead (the pr-governance.yml re-trigger question).

Verified: grepped tests/ for the strings touched here — only tests/test_product_technical_gap_baseline.py pins this file's content, and it still passes; full suite 2630 passed / 1 skipped (two test files are Python-3.12-only syntax, uncollectible under this sandbox's 3.11, unrelated to this change). No merge conflict, no failing required check — the pending checks are the org's known CI queue backlog (.github#712).


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae added a commit that referenced this pull request Sep 3, 2026
…r audit

Three fixes: (1) two naruon#1741 references were missing the
ContextualWisdomLab/ owner prefix, breaking GitHub's cross-repo timeline
backlink; (2) the item13-stale-head-cancellation-audit doc this record
cross-references lives in a separate, not-yet-merged PR (#1760) -- noted
explicitly so a reader on this branch alone isn't confused by a missing
file; (3) the coalescer fix this record cites as "found and fixing" didn't
name a traceable PR -- it's #1661, commit
c0dc46b, confirmed via the commit-to-PR API, now cited with both.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae and others added 2 commits September 3, 2026 12:10
Three fixes: (1) the naruon PR reference lacked the ContextualWisdomLab/
owner prefix, breaking the cross-repo timeline backlink; (2) the
labeled/unlabeled attribution for the five non-synchronize
pull_request_target firings was speculation presented as near-certain --
softened to state plainly that the specific event types were not verified;
(3) "with no run at all" imprecisely described a workflow run that existed
(status: queued) but whose job never started -- corrected to "no job
started" in both this record and the gap-baseline entry it feeds.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rreach

Devin Review correctly noted the 23h22m/24+ hour waits prove queueing
occurred, not exclusively that a plan-level ceiling is the cause. Added a
precision paragraph: what actually narrows the explanation here is that
Result 1 already ruled out a workflow-configuration bug in this PR's own
review pipeline, not elimination of every other conceivable cause. Does not
change the conclusion, states its actual evidentiary basis correctly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

A dedicated 4-agent follow-up (2 investigate + 2 adversarial verify,
wf_68f78449-bb6) re-examined the two substantive Devin Review claims with
fresh rigor. Result: the noema-review.yml concurrency-ordering hazard is a
CONFIRMED real bug, not the "open, unverified risk" a concurrent edit had
already merged in -- both the investigation and a dedicated refutation
attempt found no flaw in the mechanism (GitHub cancels the currently-active
run in a group unconditionally when a new run enters it, before any of the
new run's own steps execute; noema-review.yml's group has no head-SHA
component, so an out-of-order synchronize event can kill a valid
current-head review before its own stale-check step ever runs). Strong
corroborating evidence: strix.yml and opencode-review.yml both deliberately
avoid this exact pattern already, for exactly this reason.

The strix.yml paths-ignore claim was also fully re-verified: real internal
gap in that file alone, but pr-review-merge-scheduler.yml's scan-pr-queue
(no paths-ignore, same-event trigger) closes it in practice -- confirmed
not a real bug via full adversarial re-check.

Updated the doctoring record's table, caveat, conclusion, and "what this
resolves" section, plus the gap-baseline summary, to state the confirmed
bug plainly rather than hedge it. Not fixing the noema-review.yml bug in
this PR -- a live CI concurrency change deserves its own dedicated PR with
a regression test, not a same-breath edit to documentation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae and others added 2 commits September 3, 2026 12:29
…dict contradiction

- Cite ContextualWisdomLab/naruon#1528 with owner prefix in both remaining
  spots (Devin Review: cross-repo reference format violation)
- Fix the self-contradiction Devin caught: "Result 1 verified all four
  central workflows' cancellation logic is correct" directly contradicted
  the same document's confirmed noema-review.yml bug finding above it.
  Corrected to: three of four are fully correct; the fourth's confirmed
  bug has a different failure signature (wrongful cancellation of a live
  run) than what Result 2's evidence shows (runs stuck queued for
  23h22m/24+ hours with no cancellation at all) -- so the bug still
  doesn't explain this specific wait, without overstating the workflows'
  correctness.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…merged

A peer session pushed 31e46db on #1661 porting
opencode-review.yml's SHA-scoping pattern to noema-review.yml, closing the
confirmed bug this record documents. Independently verified the fix itself
is correct. But their own gap-baseline.md entry (e2869c3) marked item 13
"fully resolved" -- the same premature-credit pattern Devin caught once
already on bebd7c7 (PR #1765): 31e46db is unreachable from main (diverged,
#1661 still open), and main's live noema-review.yml still has the pre-fix
group with no head-SHA component. Recorded the fix's real status here
(proposed, not yet merged) so this record doesn't imply main is safe when
it isn't yet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae added a commit that referenced this pull request Sep 3, 2026
…ctions

The earlier "item 13 confirmed resolved" section (from an early #1760
draft) claimed Noema's native cancel-in-progress correctly retires a
superseded head's run -- directly contradicted by this same file's own
later, more thorough section documenting the confirmed bug that same
mechanism has (cancels by run-creation order, not head-SHA recency).
Added a correction note pointing to the accurate finding rather than
leaving both conclusions standing unreconciled.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pure append-only conflict: this branch's item 13 section and main's
independently-landed codeql-pr.yml required-workflow-hard-limit section
were both appended at the same location. Kept both, item 13 first.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

…vidence

Devin Review caught it: wf_eb15dd2b-ad1 and wf_68f78449-bb6 are internal
Claude Code orchestration run identifiers with no repository path or public
URL -- a future reader has no way to open and inspect them, so citing them
as "the evidence" for this record's findings was misleading. Added an
explicit caveat: they document how the investigation was structured
(agent counts, investigate-vs-verify split), not the evidence itself --
the actual checkable evidence is what each finding cites inline (exact
file paths/line ranges, raw.githubusercontent.com fetches, gh api calls,
named PR/commit references), which any future reader can independently
re-run.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@seonghobae
seonghobae merged commit 2fb81cc into main Sep 3, 2026
9 of 23 checks passed
@seonghobae
seonghobae deleted the docs/item13-concurrency-cancellation-audit branch September 3, 2026 07:17
seonghobae added a commit that referenced this pull request Sep 3, 2026
No conflicts; docs-only additions from main (#1760/#1763/#1765,
item 13/pr-review-merge-scheduler.yml/item 4 doctoring records).
Full suite (2705 tests) passes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants