Skip to content

fix(playbooks): repo-sweep next stops on a conflicting sweep PR - #5386

Merged
kyle-sexton merged 8 commits into
mainfrom
fix/5206-repo-sweep-conflicting-pr
Sep 29, 2026
Merged

kyle-sexton merged 8 commits into
mainfrom
fix/5206-repo-sweep-conflicting-pr

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #5206

Summary

/playbooks:repo-sweep next pushed a commit per step without noticing that the sweep PR had become mergeable: CONFLICTING. GitHub runs no pull_request workflows on a conflicting PR, so those step commits got no CI.

Fix

  • state.sh requests mergeable in its existing gh pr list call and prints mergeable CONFLICTING for an open PR that conflicts. MERGEABLE and UNKNOWN print nothing. No new exit code.
  • reference/next.md section 1 stops before any step when that line is present and asks the user to merge the base branch into the sweep branch, push, and rerun next. UNKNOWN never stops.
  • SKILL.md lists the new line; evals/evals.json has a case for the stop.
  • state.test.sh covers CONFLICTING (line printed, normal exit code), MERGEABLE and UNKNOWN (no line), and a merged PR (exit 11, no line).
  • playbooks 0.14.1 -> 0.14.2 with a CHANGELOG entry.

Verification

  • bash plugins/playbooks/skills/repo-sweep/scripts/state.test.sh: all passed
  • scripts/check-changelog-parity.sh --check --check-order: passed
  • scripts/validate-plugins.sh: all manifests and the catalog validated
  • evals.json parses as JSON

Related

Found in melodic-software/.github#153, where the last six step commits got no CI run.

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits September 29, 2026 14:36
Read mergeable from the existing gh pr list call and print
"mergeable CONFLICTING" after pr-state for an open PR only. No new exit code.

Refs #5206

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #5206

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 29, 2026 19:47
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T19:59:12.567776Z cfe84cc Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 52s —— View job


Code review (/review:code-review)

  • Gather context (PR diff, changed files)
  • Run /review:code-review per custom instructions
  • Post findings as inline PR comments
  • Post summary comment

Reviewed plugins/playbooks/ changes (state.sh, next.md, SKILL.md, evals.json,
state.test.sh, plugin.json, CHANGELOG.md) diffed between ebf8289 (pre-fix base) and
cfe84cc (HEAD), which matches the PR's 7 changed files exactly.

state.sh and state.test.sh look correct. Traced the script: mergeable CONFLICTING is
printed only for an OPEN PR (after the state == OPEN gate, line 74) and before the exit-1
"no markers" check, so it can appear with exit 0, 1, 12, or 13, never 11/14/15 —
consistent with the new tests. The "line sits between pr-state and playbook"
(state.test.sh:138-140) and "no line for MERGEABLE/UNKNOWN" assertions check out against
the script logic. I could not execute bash state.test.sh myself in this sandbox (Bash denied
running it), so the author's "all passed" claim in the PR body is unverified by me — the
line-by-line trace above is my own independent check, not a rerun of their command.

Two findings posted inline on reference/next.md, both on the new step 3 (lines 34-37):

  1. The new stop is unreachable for 2 of the 3 exit codes it names (line 34). Step 2's own
    bullets already terminate the flow for exit 12 ("Stop.") and, in the no-done-unverified
    case, exit 13 ("Otherwise go to section 5.") — both before step 3 is ever reached. The exit-13
    gap matters most: it's the "all steps done, ready to finalize" moment, where section 5 suggests
    /source-control:pull-request ready on a PR that's still CONFLICTING, without ever showing
    the new stop-and-merge instruction.
  2. Unqualified claim about GitHub's platform behavior (line 35). "GitHub runs no
    pull_request workflows on a conflicting PR" is stated as bare fact, with none of the
    four-part verification record (claim, basis, as-of date, recheck trigger) this repo's own
    rule (.claude/rules/skill-bodies-state-current-rules.md) requires for a volatile external
    claim in a skill body — a convention this same skill already follows elsewhere (SKILL.md:47,
    catalogs/hygiene.md:82). It's also not obviously true as a platform-wide fact (GitHub's
    pull_request trigger doesn't itself gate on mergeable status); the PR's one cited case
    (melodic-software/.github#153) is more likely explained by that repo's own branch-protection
    config.

No other correctness issues found in state.sh's field ordering, the eval addition, the version
bump, or the CHANGELOG entry.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (diff, changed files)
  • Run /review:security-review criteria over the diff
  • Post findings (or confirm none)

No security issues found.

Reviewed all 7 changed files (plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh) against the CI security-review criteria: injection surfaces, auth/access-control gaps, unsafe secret/token handling, and instruction-surface deletions.

The functional change is narrowly scoped:

  • state.sh adds mergeable to the existing gh pr list --json field list.
  • state.sh#L75 emits a hardcoded literal "mergeable CONFLICTING" gated on select(.mergeable == "CONFLICTING") — it never interpolates the raw JSON value into output, so there's no command/shell injection surface even though mergeable originates from PR/API state. This mirrors the script's existing pattern of validating or hardcoding anything that reaches downstream shell commands (e.g. the safe_branch/step-id regex checks already in place).
  • reference/next.md adds a new stop condition (an instruction that increases caution, not one that removes or softens an existing guardrail), so the instruction-surface-deletion lens doesn't apply here.
  • The remaining changes (CHANGELOG.md, plugin.json version bump, SKILL.md table entry, evals.json test case, state.test.sh coverage) are documentation/test-only with no security-relevant surface.

No secrets, credentials, or authorization logic are touched by this PR.

Comment thread plugins/playbooks/skills/repo-sweep/reference/next.md Outdated
Comment thread plugins/playbooks/skills/repo-sweep/reference/next.md Outdated
…anches

Exit 12 and exit 13 branch out of step 2 before the old step 3 ran, so a
conflicting sweep PR at the finalize point skipped the stop. Fold the check
into step 2 and attach the verification record for the GitHub behavior.

Refs #5206

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cfe84cc9a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/playbooks/skills/repo-sweep/reference/next.md Outdated
Comment thread plugins/playbooks/skills/repo-sweep/reference/next.md Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 21s —— View job


Code review (/review:code-review)

  • Gather context (PR diff, changed files)
  • Run /review:code-review per custom instructions
  • Post findings as inline PR comments
  • Post summary comment

Reviewed at HEAD 9638991 (7 changed files: plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh). This head already folds in the two prior findings (unreachable-for-exit-12/13 stop, and the missing four-part verification record), both confirmed fixed by tracing 4a49054 against the earlier commit: the mergeable check now runs before the per-exit-code list, and the claim carries a basis URL, as-of date, and recheck trigger.

One finding posted inline on reference/next.md:19 — the (exit 0, 12, or 13) parenthetical is still incomplete. Tracing state.sh: the mergeable CONFLICTING line prints at line 75, right after the OPEN gate and before the checklist-markers check that can exit 1. So an OPEN, CONFLICTING PR whose body lacks checklist markers prints the mergeable line and then exits 1 — a fourth reachable code the parenthetical omits (10/14/15 never reach the mergeable field; 11 returns before it, so those three are correctly excluded). It's low-severity: the stop in this step fires unconditionally on the printed line, before the exit-code list is even consulted, so behavior doesn't change today. But it's the same category of exit-code-reachability inaccuracy already caught and fixed twice on this exact line in this PR, so flagging it for completeness.

I could not execute state.sh / state.test.sh myself in this sandbox (Bash denied running them), so the author's "all passed" claim is unverified by me — my check above is an independent code trace against state.sh's actual control flow, not a rerun of their command.

No other correctness issues found in state.sh's field ordering/gating, state.test.sh's new CONFLICTING/MERGEABLE/UNKNOWN/merged-PR cases (all match the script's logic on inspection), the evals.json addition, the SKILL.md table entry, or the CHANGELOG/version bump (0.14.2 → 0.14.3, consistent across CHANGELOG.md and plugin.json).
· branch fix/5206-repo-sweep-conflicting-pr

🤖 Generated with Claude Code

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 23s —— View job


Security review (/review:security-review)

  • Gather context (PR diff, changed files)
  • Run /review:security-review per custom instructions
  • Post findings (or confirm none)

No security issues found.

Reviewed the full cumulative diff (gh pr diff 5386, 7 files: plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh) at HEAD 9638991 against the CI security criteria: injection surfaces, auth/access-control gaps, unsafe secret/token handling, and instruction-surface deletions.

  • state.sh#L39 adds mergeable to the existing gh pr list --json field list.
  • state.sh#L75 emits a hardcoded literal "mergeable CONFLICTING" gated on select(.mergeable == "CONFLICTING"); the raw JSON value is never interpolated into shell output, so there's no injection surface even though mergeable originates from PR/API state — consistent with the script's existing pattern of validating or hardcoding anything that reaches downstream commands.
  • reference/next.md#L14-26: the earlier revision of this PR (cfe84cc) had the new stop placed after the per-exit-code branches, so it was unreachable for exit 12/13 — flagged in two prior review rounds (Claude, Codex, both inline comments) as a logic gap that would let a completed-but-conflicting sweep proceed toward pull-request ready. That's fixed on this head: the mergeable CONFLICTING check is now step 2, evaluated before the exit-code branches (step 2 sub-bullets), so it fires ahead of the exit-12/13 "Stop"/"go to section 5" paths. Confirmed by re-reading the current file, not just trusting the review-comment resolution.
  • The previously-flagged missing four-part verification record for the "GitHub runs no pull_request workflows on a conflicting PR" claim is also now present (basis: the linked events-that-trigger-workflows doc section, as-of 2026-09-29, recheck: that section changes), satisfying .claude/rules/skill-bodies-state-current-rules.md.
  • This diff only adds a stop condition; it doesn't delete, narrow, or soften any standing instruction, so the instruction-surface-deletion lens doesn't apply.
  • CHANGELOG.md, plugin.json (version bump), SKILL.md, evals.json, and state.test.sh are documentation/test-only with no security-relevant surface.

No GitHub Actions workflow files are touched by this PR, so zizmor's advisory lane has nothing to review here.
· branch fix/5206-repo-sweep-conflicting-pr

Comment thread plugins/playbooks/skills/repo-sweep/reference/next.md Outdated
state.sh prints mergeable CONFLICTING before the marker check, so exit 1
can carry it too. Say any exit code instead of listing three. Merges
origin/main and bumps playbooks to 0.14.4.

Refs #5206

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 30s —— View job


Code review (/review:code-review)

  • Gather context (PR diff, changed files)
  • Run /review:code-review per custom instructions
  • Post findings as inline PR comments
  • Post summary comment

No new findings.

Reviewed the full diff at HEAD eec24d1 (7 changed files: plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh), focusing on the last commit (eec24d1, on top of the previously-reviewed 9638991), which changes exactly one line in
reference/next.md:19:

(exit 0, 12, or 13) → (any exit code).

This resolves the exit-code-reachability finding from the prior review round (the parenthetical
previously omitted exit 1, which state.sh can also carry the mergeable CONFLICTING line with,
since the print at state.sh:75 sits before the markers check that can exit 1). Generalizing to "any exit code" sidesteps the enumeration problem entirely rather than just adding the missing code, and traces correctly against state.sh's control flow: the print at line 75 runs right after the OPEN gate (line 74, else exit 11) and before every remaining exit path (0, 1, 12, 13), so "any exit code" is accurate — the only codes that can never carry the line (10, 11, 14, 15) all return before line 75 is reached.

Also re-confirmed the two earlier findings (unreachable-for-exit-12/13 stop; missing four-part
verification record) are still fixed on this head — the mergeable check runs ahead of the
per-exit-code branches (next.md:19-40), and the claim carries basis/as-of/recheck (next.md:22-25), satisfying .claude/rules/skill-bodies-state-current-rules.md.

No other changes since the last-reviewed head (SKILL.md, evals.json, state.test.sh,
plugin.json, CHANGELOG.md are unchanged by eec24d1 itself; the CHANGELOG.md/plugin.json
diff versus origin/main is a version bump, 0.14.3 → 0.14.4, consistent between the two files).
evals.json parses as valid JSON (jq empty, verified). I could not execute
state.test.sh in this sandbox (Bash denied running it), so the author's "all passed" claim in
the PR body is author-claimed and unverified by me — the trace above is an independent code read,
not a rerun of their command.
· branch fix/5206-repo-sweep-conflicting-pr

🤖 Generated with Claude Code

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 20s —— View job


Security review (/review:security-review)

  • Gather context (PR diff, changed files)
  • Run /review:security-review per custom instructions
  • Post findings (or confirm none)

No security issues found.

Reviewed at HEAD eec24d1 (7 files: plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh) against the CI security criteria: injection surfaces, auth/access-control gaps, unsafe secret/token handling, and instruction-surface deletions.

Since the last security review (9638991), the only functional change is in reference/next.md: the exit-code parenthetical changed from (exit 0, 12, or 13) to (any exit code), fixing the exit-1 gap a prior review round flagged. That's a correctness fix, not a security change — it broadens (not narrows) the conflict-stop condition, so it stays on the safe side of the instruction-surface-deletion lens (an addition, never a removal or softening of a guardrail).

Re-confirming the two structural points already established in this PR's prior two security-review rounds, since they still hold on this head:

  • state.sh#L39 adds mergeable to the existing gh pr list --json field list.
  • state.sh#L75 emits a hardcoded literal "mergeable CONFLICTING" gated on select(.mergeable == "CONFLICTING") — the raw JSON value is never interpolated into shell output, so there's no injection surface even though mergeable originates from PR/API state.
  • The four-part verification record for the "GitHub runs no pull_request workflows on a conflicting PR" claim (basis, as-of date, recheck trigger) is present and unchanged on this head, satisfying .claude/rules/skill-bodies-state-current-rules.md.

CHANGELOG.md, plugin.json (version bump 0.14.3 → 0.14.4), SKILL.md, evals.json, and state.test.sh are documentation/test-only with no security-relevant surface.

No GitHub Actions workflow files are touched by this PR, so zizmor's advisory lane has nothing to review here.
· branch fix/5206-repo-sweep-conflicting-pr

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 13s —— View job


Security review (/review:security-review)

  • Gather context (PR diff, changed files)
  • Run /review:security-review per custom instructions
  • Post findings (or confirm none)

No security issues found.

This run is triggered by the merge commit 48c7c35 ("chore: merge origin/main into fix/5206-repo-sweep-conflicting-pr"), which lands after four prior review rounds already covered the full cumulative diff (through eec24d1) with no findings. I diffed eec24d1..48c7c35 across all 7 files this PR touches:

  • plugin.json: version bump 0.14.4 → 0.14.5 only — re-bumped because origin/main had already claimed 0.14.4 for an unrelated merged PR (fix(playbooks): run the repo-sweep ai-slop step audit-only #5379), so this PR's version had to move to avoid a collision.
  • CHANGELOG.md: reflowed to insert the merged-in 0.14.4 entry ahead of this PR's (now 0.14.5) entry, matching the version bump above.
  • SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh: byte-identical to eec24d1 — no new lines, no logic change.

No other files outside this PR's 7-file scope changed by this commit. So the merge introduces no new security-relevant surface at all — it's a version/changelog-ordering fix for a collision with main, nothing else.

Re-confirming the two structural points from the prior review rounds, since the underlying files are unchanged and I re-read them directly against this head:

  • state.sh#L39 adds mergeable to the existing gh pr list --json field list.
  • state.sh#L75 emits a hardcoded literal "mergeable CONFLICTING" gated on select(.mergeable == "CONFLICTING") — the raw JSON value from PR/API state is never interpolated into shell output, so there is no command/injection surface.
  • reference/next.md only adds a stop condition (with the four-part verification record for the GitHub-behavior claim already in place: basis, as-of date, recheck trigger), so the instruction-surface-deletion lens doesn't apply — this diff never removes or softens a standing guardrail.

CHANGELOG.md and plugin.json are documentation/version-only. No GitHub Actions workflow files are touched by this PR, so zizmor's advisory lane has nothing to review here.
· branch fix/5206-repo-sweep-conflicting-pr

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 29s —— View job


Code review (/review:code-review)

  • Gather context (PR diff, changed files)
  • Run /review:code-review per custom instructions
  • Post findings as inline PR comments
  • Post summary comment

No findings.

Reviewed the full diff at HEAD 48c7c35 (gh pr diff 5386, 7 files: plugin.json, CHANGELOG.md, SKILL.md, evals.json, reference/next.md, scripts/state.sh, scripts/state.test.sh) against this lane's criteria: correctness and alignment with CLAUDE.md/AGENTS.md, architecture, error handling, test coverage, maintainability (security is out of scope for this lane; a separate security lane runs on this PR).

48c7c35 is a merge of origin/main into this branch (git show --stat 48c7c35) and touches none of the plugins/playbooks/skills/repo-sweep/** files — the content under review is unchanged from eec24d1, which this lane already reviewed twice with no findings after two rounds of fixes.

Traced reference/next.md:19-26 against scripts/state.sh independently to confirm the three issues raised across this PR's earlier review rounds (mine, Codex's, and my own follow-ups) are still fixed on this head:

  • Unreachable stop for exit 12/13 (originally flagged by me and Codex): the mergeable CONFLICTING check is step 2's own second sentence, evaluated immediately after printing pr-state and before "Otherwise act on its exit code" — so it now runs ahead of every per-exit-code branch, including 12's "Stop" and 13's "go to section 5". Confirmed against state.sh: the print at line 75 sits right after the OPEN gate (line 74) and before every later exit path.
  • Missing four-part verification record (flagged by me and Codex): next.md:22-25 now carries basis (a linked upstream doc section), an as-of date (2026-09-29), and a recheck trigger (that section changes), satisfying .claude/rules/skill-bodies-state-current-rules.md.
  • Incomplete exit-code parenthetical (flagged by me in a later round, since state.sh can also print the line with exit 1): resolved by generalizing to (any exit code) rather than re-enumerating, which is accurate — the only exit codes that can never carry the line (10, 11, 14, 15) all return before the print statement.

state.test.sh's new cases (CONFLICTING → line printed, normal exit; MERGEABLE/UNKNOWN → no line; merged+CONFLICTING → exit 11, no line) match state.sh's control flow on inspection. evals.json case 7 is valid JSON and matches the new behavior. SKILL.md's table entry reflects the correct field order (pr-state → mergeable CONFLICTING → playbook). CHANGELOG.md (0.14.4 → 0.14.5) and plugin.json's version bump are consistent.

I could not execute state.test.sh in this sandbox, so the author's "all passed" claim in the PR body is author-claimed and unverified by me — the above is an independent code trace, not a rerun of their command.
· branch fix/5206-repo-sweep-conflicting-pr

@kyle-sexton
kyle-sexton merged commit 281131d into main Sep 29, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the fix/5206-repo-sweep-conflicting-pr branch September 29, 2026 21:14
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.

playbooks: repo-sweep next does not detect a sweep PR that CI has stopped testing

1 participant