docs(loop-lane): define the cross-lane PR hold — label-only, hold-then-explain (9.0.0) by kyle-sexton · Pull Request #2512 · melodic-software/claude-code-plugins

kyle-sexton · 2026-08-12T19:03:06Z

Summary

Adds a Cross-lane PR hold subsection to the loop-lane convention's §2 escalation contract, closing the gap the 2026-07-25 incident exposed: an escalating lane decided PR #1351 must not merge, drafted its explanation first, and applied do-not-merge ~30 minutes later — 3m20s after the merge. Three independent investigations on the issue converged on the same root cause: the server-side enforcement (org-ruleset-required do-not-merge status check, no bypass actors) worked correctly on both sides of the merge boundary; what no convention defined was when to engage it. Holding a PR was undifferentiated from escalating a tracker item, and the two acts fail differently — a PR hold carries a hard real-time deadline.

The clause states three rules:

do-not-merge is the only cross-lane hold; a PR comment is never one (comments are advisory by construction — the same night, an explicit "do not merge as written" comment on docs(review): reconcile stale code-review framing in quality-gate pr.md and README #1326 held nothing).
Hold first, explain second — the label goes on the moment the hold is decided, before the escalation comment is drafted.
Freshness re-read before any hold action — a hold decision drafted against a stale snapshot can target an already-merged PR.

Per the convention's own versioning rule, a change to the escalation contract is a major bump: 8.1.0 → 9.0.0, CHANGELOG entry included.

This implements the recommended remedy from the issue's decision-ready triage summary (Option A, parts 1 and 3 — the convention clause plus the retitle correcting the issue's falsified framing, done on the issue directly). Part 2 (a babysit_merge.py label backstop) is deliberately not included: the ruleset-required check already enforces the hold server-side with no bypass actors and demonstrably held during the incident; a wrapper-level duplicate narrows only the label-apply→check-report race, which the freshness-re-read rule addresses at the behavioral layer. If future evidence shows the ruleset gate insufficient, that is its own scoped change.

Test plan

npx markdownlint-cli2@0.23.2 on both edited files — 0 issues.
Docs-only change to a convention document; no scripts, hooks, or plugin behavior surfaces touched. CI's markdown/link gates cover the rest.

Cross-checked the factual infrastructure claim in both files ("the workflow behind it re-evaluates on labeled/unlabeled") against .github/workflows/do-not-merge.yml — the labeled/unlabeled event types are present, matches the claim.
Checked for a distinction between this PR-level do-not-merge hold and the separate --block-labels veto-label config in babysit_merge.py — these are different mechanisms (org-ruleset status check vs. an in-script tier veto), so no inconsistency in treating do-not-merge as the literal, org-enforced hold label.
Checked the version bump: no other file in the repo pins loop-lane to 8.1.0, so no consumer references need updating alongside this bump.
Checked no lane body currently implements a do-not-merge apply/comment ordering that this new clause would immediately contradict (work-items/work-loop and attend-queue don't touch the do-not-merge label today), so this is genuinely a new convention rather than a fix that also needed a code-side change in the same PR.
Confirmed CHANGELOG structure/style (heading format, bullet style, issue-reference style) matches the file's existing entries (e.g. 8.1.0, 4.0.0).
The PR description's npx markdownlint-cli2 claim is author-claimed and unverified by me — I don't have network access in this environment to independently run it.

No correctness or maintainability issues found. The new clause is internally consistent with the rest of the convention (correctly distinguishes tracker-item escalation from PR holds, doesn't conflict with the existing "no lane creates labels" rule since it only applies a pre-existing label), and the factual claims about CI enforcement check out against the actual workflow file.

…n-explain (9.0.0) Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

github-actions · 2026-08-12T19:54:51Z

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

…n-explain (9.0.0) (#2512) ## Summary Adds a **Cross-lane PR hold** subsection to the loop-lane convention's §2 escalation contract, closing the gap the 2026-07-25 incident exposed: an escalating lane decided PR #1351 must not merge, drafted its explanation first, and applied `do-not-merge` ~30 minutes later — 3m20s **after** the merge. Three independent investigations on the issue converged on the same root cause: the server-side enforcement (org-ruleset-required `do-not-merge` status check, no bypass actors) worked correctly on both sides of the merge boundary; what no convention defined was *when to engage it*. Holding a PR was undifferentiated from escalating a tracker item, and the two acts fail differently — a PR hold carries a hard real-time deadline. The clause states three rules: - **`do-not-merge` is the only cross-lane hold; a PR comment is never one** (comments are advisory by construction — the same night, an explicit "do not merge as written" comment on #1326 held nothing). - **Hold first, explain second** — the label goes on the moment the hold is decided, before the escalation comment is drafted. - **Freshness re-read before any hold action** — a hold decision drafted against a stale snapshot can target an already-merged PR. Per the convention's own versioning rule, a change to the escalation contract is a major bump: `8.1.0` → `9.0.0`, CHANGELOG entry included. This implements the recommended remedy from the issue's decision-ready triage summary (Option A, parts 1 and 3 — the convention clause plus the retitle correcting the issue's falsified framing, done on the issue directly). Part 2 (a `babysit_merge.py` label backstop) is deliberately **not** included: the ruleset-required check already enforces the hold server-side with no bypass actors and demonstrably held during the incident; a wrapper-level duplicate narrows only the label-apply→check-report race, which the freshness-re-read rule addresses at the behavioral layer. If future evidence shows the ruleset gate insufficient, that is its own scoped change. ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited files — 0 issues. - Docs-only change to a convention document; no scripts, hooks, or plugin behavior surfaces touched. CI's markdown/link gates cover the rest. ## Related Fixes #1409 --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

claude · 2026-08-12T20:39:51Z

Claude finished @kyle-sexton's task in 37s —— View job

Code review

Gather context (PR body, changed files, prior review threads)
Invoke /review:code-review skill per custom instructions
Apply skill's skip gate

Result: skipped (skip-gate condition 1 — PR is closed, not open).

Per plugins/review/skills/code-review/SKILL.md's skip gate: "Before deep review, stop early when any of these hold ... 1. PR is closed or not open ... (say so in the summary and post nothing else)." This PR is MERGED, so no deep review or inline comments were produced for this run.

For reference, the merged PR already carries a completed review from this lane (posted 2026-08-12T19:51:50Z, no correctness/maintainability issues found) plus two Codex review comments on docs/conventions/loop-lane/README.md (server-enforcement scope and the "no gate reads comments" claim) that were not addressed before merge — those remain open follow-up items if anyone wants to act on them post-merge, but per the skip gate this run does not re-litigate them.

`verify-security-review-evidence.sh` runs under `set -euo pipefail` and called its scope helper bare: pr_touches_security_paths "$base_ref" local in_scope=$? The helper signals OUT-of-scope by RETURNING NON-ZERO. Under `set -e` a bare call with a non-zero return aborts the shell immediately, so the script died right there — exit 1, no message — and the "guard not applicable" branch below it was unreachable dead code. Every out-of-scope pull request FAILED the guard instead of being waved through. Observed on run 31637054526 (PR #2512): the diff touches only `docs/conventions/loop-lane/*.md`, the lane correctly SKIPPED, and `security-review-evidence` failed beside it with a step log containing nothing but `##[error]Process completed with exit code 1`. The empty log is the signature — `set -e` aborts before any echo can run. The obvious one-line fix (`|| in_scope=$?`) clears the red check but leaves something worse, and ShellCheck says so via SC2310, which this repo enables deliberately in `.shellcheckrc` ("catches the bash trap where errexit silently stops working"). The helper runs `python3`, and a crashing interpreter also exits 1 — so with out-of-scope encoded as "returns 1", a BROKEN scope check is indistinguishable from a NEGATIVE one, and the guard would skip itself and silently pass a pull request it exists to check. Fail-open is the wrong direction for a security guard. Separate the channels instead: the helper prints its verdict (`in-scope` / `out-of-scope`) on stdout and reserves a non-zero exit for a genuine fault. The caller consumes it through command substitution, which keeps `set -e` live for the helper, and treats any unrecognised verdict as a fault rather than a pass. Verified: self-tests 6 pass / 0 fail, including a regression that models the bare call in a separate `bash -c` process (a `( … )` subshell will not do — bash suppresses `set -e` for the whole dynamic extent of a command whose status is tested, so `$( … )` inside `[[ … ]]` makes the bug unreproducible in the very harness meant to catch it), plus static guards that fail if either older shape returns. `shellcheck --rcfile .shellcheckrc` clean on both files, with SC2310 no longer firing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeEvWwrK8KdQTFb5n9CrUP

…PR (#2523) ## Summary `scripts/verify-security-review-evidence.sh` runs under `set -euo pipefail` and called its scope helper bare: ```bash pr_touches_security_paths "$base_ref" local in_scope=$? ``` The helper signals OUT-of-scope by **returning non-zero**. Under `set -e`, a bare call with a non-zero return aborts the shell immediately — so the script died on that line, exit 1, no message, and the `"diff does not touch security-relevant paths — guard not applicable"` branch below was unreachable dead code. Every out-of-scope pull request FAILED the guard instead of being waved through. Observed on [run 31637054526](https://github.com/melodic-software/claude-code-plugins/actions/runs/31637054526) (PR #2512): the diff touches only `docs/conventions/loop-lane/*.md`, `security-review / security-review` correctly **SKIPPED**, and `security-review-evidence` failed beside it with a step log containing nothing but `##[error]Process completed with exit code 1`. The empty log is the signature — `set -e` aborts before any `echo` can run. Reproduced standalone: ``` $ cat setE.sh set -euo pipefail f() { return 1; } main() { f "x"; local rc=$?; echo "REACHED with rc=$rc"; } main $ bash setE.sh; echo "exit=$?" exit=1 # "REACHED" never prints ``` ## Fix — and why not the one-liner The obvious fix is `pr_touches_security_paths "$base_ref" || in_scope=$?`. It clears the red check and introduces something worse, which **ShellCheck flags directly via SC2310** — a check this repo enables on purpose in `.shellcheckrc` ("catches the bash trap where errexit silently stops working inside if/while/&&/||"). The helper runs `python3`, and a crashing interpreter also exits 1. With out-of-scope encoded as "returns 1", a **broken** scope check is indistinguishable from a **negative** one — the guard would skip itself and silently pass a PR it exists to check. Fail-open is the wrong direction for a security guard. So the two channels are separated instead: the helper prints its verdict (`in-scope` / `out-of-scope`) on **stdout** and reserves a non-zero **exit** for a genuine fault. The caller consumes it via command substitution, which keeps `set -e` live for the helper, and treats any unrecognised verdict as a fault rather than a pass. ## Verification Run in this worktree — commands I actually ran, with real output: - `bash scripts/verify-security-review-evidence.sh.test.sh` -> **6 pass, 0 fail**, `All checks passed.` - `shellcheck --rcfile .shellcheckrc scripts/verify-security-review-evidence.sh scripts/verify-security-review-evidence.sh.test.sh` -> **clean**, with SC2310 no longer firing on the guard (it fired on the `||` variant, which is how the fail-open hole was found). New regression cases: - a non-zero-returning helper called bare under `set -e` kills the script — modelled in a **separate `bash -c` process** deliberately. A `( … )` subshell will not do: bash suppresses `set -e` for the whole dynamic extent of a command whose status is being tested, and `$( … )` inside `[[ … ]]` is exactly that context, which made the bug unreproducible in the very harness meant to catch it. My first attempt at this test failed for that reason, not because the fix was wrong. - static guards that fail if either older shape (bare call, or `||`-suppressed call) returns - a static guard that fails if the unrecognised-verdict catch-all is removed **Not verified here:** that the three currently-red PRs (#2512, #2504, #2499) go green. They will only pick this up once their branches carry it — the evidence workflow runs from the PR head. ## Related Third distinct defect found in this one file today, which is itself the finding: it infers another workflow's behaviour by reading that workflow's logs. #2517 (grep matched the lane action's echoed source) only became reachable once the `cursor[bot]` outage was fixed; this one only became reachable once #2517 let execution past the grep. No linked issue — filing was interrupted; the detail is captured here instead. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

kyle-sexton mentioned this pull request Aug 12, 2026

loop-lane: cross-lane PR hold was undefined — escalating lane explained-then-held, and the do-not-merge label landed 3m20s after #1351 merged #1409

Closed

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread docs/conventions/loop-lane/README.md

Comment thread docs/conventions/loop-lane/README.md

This was referenced Aug 12, 2026

security-evidence guard fails every successful in-scope PR by matching the lane's echoed script source #2517

Closed

fix(ci): stop the security-evidence guard matching the lane's own source #2518

Merged

docs(loop-lane): define the cross-lane PR hold — label-only, hold-the…

ea33415

…n-explain (9.0.0) Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

kyle-sexton force-pushed the cursor/loop-lane-pr-hold-63e1 branch from 4ac537e to ea33415 Compare August 12, 2026 19:53

kyle-sexton and others added 3 commits August 12, 2026 16:06

Merge branch 'main' into cursor/loop-lane-pr-hold-63e1

7b87e74

chore: retrigger CI after main merge

f6054b1

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>

Merge branch 'main' into cursor/loop-lane-pr-hold-63e1

01552db

kyle-sexton merged commit 85ac4cd into main Aug 12, 2026
37 of 38 checks passed

kyle-sexton deleted the cursor/loop-lane-pr-hold-63e1 branch August 12, 2026 20:27

claude Bot mentioned this pull request Aug 12, 2026

fix(disk-hygiene): scan transcript head for guard-launch failures #2478

Merged

1 task

kyle-sexton mentioned this pull request Aug 12, 2026

fix(ci): stop the security-evidence guard failing every out-of-scope PR #2523

Merged

Navigation Menu

Uh oh!

docs(loop-lane): define the cross-lane PR hold — label-only, hold-then-explain (9.0.0) - #2512

docs(loop-lane): define the cross-lane PR hold — label-only, hold-then-explain (9.0.0)#2512
kyle-sexton merged 4 commits into
mainfrom
cursor/loop-lane-pr-hold-63e1

kyle-sexton commented Aug 12, 2026

Uh oh!

cursor Bot commented Aug 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

claude Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kyle-sexton commented Aug 12, 2026

Summary

Test plan

Related

Uh oh!

cursor Bot commented Aug 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

github-actions Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented Aug 12, 2026 •

edited

Loading

github-actions Bot commented Aug 12, 2026 •

edited

Loading

claude Bot commented Aug 12, 2026 •

edited

Loading