Skip to content

feat(bot): unblock release pipeline (title-lint, freshen, tiered conflict resolution) - #2847

Merged
bpamiri merged 14 commits into
developfrom
claude/compassionate-cohen-4b3192
Jun 3, 2026
Merged

feat(bot): unblock release pipeline (title-lint, freshen, tiered conflict resolution)#2847
bpamiri merged 14 commits into
developfrom
claude/compassionate-cohen-4b3192

Conversation

@bpamiri

@bpamiri bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the two recurring stalls in the wheels-bot release pipeline, each at the right layer.

Commit-message CI failurespr.yml now lints the PR title (the squash subject) instead of every commit. The repo squash-merges, so intermediate commit headers never land in develop — yet the old per-commit check failed on throwaway bot commits (e.g. #2845's 105-char intermediate, while its valid title was ignored). Adds the edited trigger so a corrected title re-runs the check, and guards fast-test so title-only edits don't re-run the suite. Also corrects the _shared-rails.md rail that capped the subject when commitlint measures the whole header (type(scope): prefix included).

Merge conflicts from in-flight work → two new workflows, cloning the proven bot-address-review.yml scaffold (App-token auth, skip-check idempotency):

  • bot-freshen.yml — on push to develop (+30-min backstop), behind-but-clean bot PRs are brought current via non-destructive update-branch; DIRTY ones are dispatched to the resolver. (freshen-decide.sh maps merge state → action.)
  • bot-resolve-conflicts.yml + /resolve-conflicts — a deterministic classifier (classify-conflicts.sh) gates resolution: auto-resolve content/docs conflicts (markdown/MDX anywhere, CHANGELOG, .ai/, docs/) then push; escalate anything touching code with a conflict:needs-human label + comment — never auto-resolved.

Explicitly NOT automated

  • Merging — PRs are brought to a green, conflict-free, ready state; the maintainer performs the final squash-merge.
  • Code-conflict resolution — always escalated to a human (a green suite doesn't prove a code merge preserved intent).
  • No force-push (freshen uses merge-from-base, not rebase).

Required manual follow-up

  • Flip the repo to squash-only (deferred — repo-settings change, intentionally not done by automation): gh api -X PATCH repos/wheels-dev/wheels -F allow_merge_commit=false. Makes "the PR title is what lands" a guarantee rather than a convention, closing the merge-commit path where an unlinted intermediate header could land verbatim.

Test plan

  • Local suites, all green: tools/test-commit-title.sh, tools/test-classify-conflicts.sh, tools/test-freshen-decide.sh.
  • actionlint clean on bot-freshen.yml, bot-resolve-conflicts.yml, and pr.yml.
  • This PR's own CI exercises the new title-lint and the Lucee 7 + SQLite fast-test.
  • Post-merge live smoke (workflow_dispatch/gh workflow run only see workflows once they're on develop):
    • Freshen: land a change on develop touching a file an open bot PR also touched cleanly → confirm the PR auto-updates via update-branch.
    • Resolver, resolve path: force a docs-only conflict on a bot PR → gh workflow run bot-resolve-conflicts.yml -f pr-number=<n> → confirm it resolves + pushes.
    • Resolver, escalate path: force a .cfc conflict → confirm conflict:needs-human label + comment, and nothing pushed.

Implementation notes

  • Two design refinements surfaced during the build: the rail capped the wrong unit (subject vs. full header — the actual cause of fix(controller): populate protectedControllerMethods so global helpers aren't URL-invokable #2845's failure), and the /resolve-conflicts safety gate was reconciled to mirror the classifier exactly (so a .md under vendor/ resolves, and non-markdown files under content trees escalate — no gate/classifier disagreement).
  • The design spec and step-by-step plan live at docs/superpowers/specs/2026-06-03-bot-pipeline-unblock-design.md and docs/superpowers/plans/2026-06-03-bot-pipeline-unblock.md (added in the final commit, kept as design records per maintainer request).

🤖 Generated with Claude Code

bpamiri and others added 9 commits June 3, 2026 09:49
Squash-merge makes the PR title the landing subject, so per-commit
linting failed on throwaway intermediate commits (e.g. #2845's 105-char
bot commit) while the valid title was ignored. Lint the title; add the
edited trigger so corrections re-run; guard fast-test against title edits.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
The rail told the bot to cap the subject, but commitlint limits the full
type(scope): subject header. That mismatch produced >100-char headers
(#2845). Also state that the PR title is the linted gate under squash-merge.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
All-content conflicts (md/mdx, CHANGELOG, .ai, docs, web content trees)
are safe to auto-resolve; anything touching code escalates. Empty/unknown
input escalates. Keeps risk classification out of model judgement.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Maps mergeStateStatus to update / dispatch-resolver / skip so the freshen
sweep workflow stays a thin, tested wrapper.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Reconciles content/docs conflict markers on a bot PR branch after a
deterministic gate confirms no code is involved. Integrates both sides,
completes the merge, never pushes (the workflow does), never touches code.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Merges develop into a DIRTY bot PR, runs the deterministic classifier, then
either escalates code conflicts (label + comment, no push) or resolves
content/docs conflicts via /resolve-conflicts and pushes after verifying no
markers remain. Existing PR checks validate the pushed result.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
On push to develop (+30-min backstop), updates behind-but-clean bot PR
branches via non-destructive update-branch and dispatches the conflict
resolver for DIRTY ones. Bot-authored, non-draft PRs only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
The classifier marked *.md/*.mdx as resolvable anywhere, but the command gate
refused anything under vendor/cli/app/config/tests — so a docs conflict like
vendor/wheels/migrator/CLAUDE.md was dispatched then aborted as a bug. Mirror
the command gate to the classifier's set (md/mdx, CHANGELOG, .ai/, docs/),
drop the redundant content-tree arm so non-markdown content files escalate,
and align the docs. Also normalise test-script exec bits and an SC2164.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@github-actions github-actions Bot added the docs label Jun 3, 2026

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: This PR adds three valuable bot-pipeline features — PR-title linting, stale-branch freshening, and tiered conflict resolution — and the underlying design is sound. However, the freshen sweep contains a correctness bug that makes the feature silently non-functional from day one: the author filter uses \"app/wheels-bot\" (a string that can never match any GitHub login, because logins can't contain slashes) instead of \"wheels-bot[bot]\". Every other workflow in this repo uses the correct form. Requesting changes on that single issue; everything else is a minor nit.


Correctness

bot-freshen.yml line 50 — author filter is the wrong identity string

# .github/workflows/bot-freshen.yml, line 50
--jq '.[] | select(.isDraft==false) | select(.author.login=="app/wheels-bot") | .number')

GitHub App bot accounts carry the login <appname>[bot], not app/<appname>. GitHub logins cannot contain slashes, so \"app/wheels-bot\" will never match any real PR author. The prs variable will always be empty, the loop body never runs, and the workflow prints No open bot PRs. and exits 0 on every invocation. The freshen feature is completely non-functional.

Every other workflow in the repo uses the correct form:

File Evidence
bot-address-review.yml github.event.comment.user.login == 'wheels-bot[bot]'
bot-review-a.yml select(.user.login == \"wheels-bot[bot]\")
bot-update-docs.yml github.event.pull_request.user.login == 'wheels-bot[bot]'
bot-tdd-gate.yml if [[ \"$PR_AUTHOR\" == \"wheels-bot[bot]\" ]]

Fix:

--jq '.[] | select(.isDraft==false) | select(.author.login=="wheels-bot[bot]") | .number')

Conventions

_shared-rails.md — updated line is 290+ characters wide

The corrected bullet (line 44 post-merge) is accurate, but the single line runs to ~290 characters which makes it hard to read in a terminal or side-by-side diff. Consider wrapping:

- **Header ≤ 100 chars, not ALL-CAPS.** commitlint measures the whole header —
  `type(scope): subject` including the prefix — not just the subject.
  A 90-char subject under a `docs(web/guides): ` prefix is a 108-char header
  and FAILS. Count the prefix. Sentence-case is fine.

NIT — not blocking.


Tests

Test coverage for the shell scripts is thorough: test-classify-conflicts.sh covers resolve/escalate paths, the empty-input edge case, and the no-trailing-newline read-loop edge case; test-freshen-decide.sh covers all six status strings including empty; test-commit-title.sh exercises pass/fail and the 100-char boundary. Well done.


Docs

.ai/wheels/wheels-bot.md is updated with the new automation section. No CHANGELOG entry, which is fine for bot infrastructure.


Security

pr.yml stores ${{ github.event.pull_request.title }} in env: before referencing it as $PR_TITLE in the shell — the recommended mitigation for GitHub Actions context-injection. Clean.


Commits

All nine commits conform to commitlint.config.js (valid types, headers ≤ 100 chars, not ALL-CAPS, DCO Signed-off-by present on all commits). ci(pr): and chore(bot): are correct — ci is a type, not a scope.


One additional observation (not blocking)

bot-resolve-conflicts.yml includes a Set up Wheels test environment step (lines ~131–135) that starts a full Lucee 7 server and checks for Playwright — copied from bot-address-review.yml but never used, since the resolve path only needs git and text editing (the allowedTools list confirms: Bash(gh:*),Bash(git:*),Read,Edit,Write,Grep,Glob). This adds a few minutes of CI time per conflict-resolution run. Worth stripping in a follow-up or in this PR before merge.

… wrap rail

Review response on #2847:
- bot-freshen.yml: the `app/wheels-bot` filter was already correct for the
  `gh pr list --json author` surface (verified: selects PRs 2845/2843/2842/2839/
  2834, excludes dependabot + humans), contrary to the review's "can never match"
  claim. Hardened to also match the REST `wheels-bot[bot]` form so it is robust
  across gh API surfaces / versions.
- bot-resolve-conflicts.yml: drop the unused `Set up Wheels test environment`
  step — the resolve path only uses git/gh/text edits, so the Lucee server was
  dead weight that burned CI minutes per run.
- _shared-rails.md: wrap the two over-long header-rail bullets for readability.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri

bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in cc23f0149.

Author filter (the "changes requested" item) — verified, then hardened

The claim that app/wheels-bot "can never match" doesn't hold for the surface this code queries. gh pr list --json author and the REST /pulls endpoint use different representations for the same App bot:

PR gh pr list --json author.author.login REST /pulls.user.login
#2845 (this bot) app/wheels-bot wheels-bot[bot]
#2836 (dependabot) app/dependabot dependabot[bot]

bot-freshen.yml uses gh pr list --json author, so app/wheels-bot is the correct match on that surface. The cited workflows (bot-review-a.yml, bot-tdd-gate.yml, …) read .user.login from the REST/event surface, which is why they use wheels-bot[bot]. Live proof the original filter already selected the right PRs:

$ gh pr list --base develop --json number,isDraft,author \
    --jq '.[] | select(.isDraft==false) | select(.author.login=="app/wheels-bot") | .number'
2845  2843  2842  2839  2834     # bot PRs only — dependabot (#2836) and humans excluded

That said, depending on one surface's representation is brittle, so I've hardened the filter to match both forms (app/wheels-bot or wheels-bot[bot]) with a comment explaining the two surfaces — correct today and resilient if gh's representation changes.

bot-resolve-conflicts.yml unused test-env step — removed

Correct — the resolve path only uses git/gh/text edits (per its allowedTools), so the Set up Wheels test environment step (Lucee server + Playwright check) was dead weight. Removed; saves the per-run CI minutes.

_shared-rails.md line width — wrapped

Both header-rail bullets wrapped to ~78 cols. Content unchanged.

No changes needed for the tests / security / commits sections (coverage, the env:-based context-injection mitigation, and commit hygiene called out as clean are unchanged).

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

wheels-bot[bot]
wheels-bot Bot previously approved these changes Jun 3, 2026

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: This is a follow-up review on PR #2847 (head SHA cc23f014). The previous review requested changes on what it described as a correctness bug — the app/wheels-bot author filter being an unmatchable string. That finding was disputed, and the dispute is correct: gh pr list --json author uses the GraphQL API, which represents GitHub App authors as app/<slug>, not <slug>[bot] (that form is the REST/webhook surface). The fix — accepting both forms — is the right outcome: it is now empirically verified, well-commented, and robust across API surfaces and gh CLI versions. All other findings from the previous review (unused test-env step, over-long rail lines) are also addressed. I'm approving.


Previous findings — resolution confirmed

Author filter (bot-freshen.yml line 54) — correctly hardened to:

select(.author.login=="app/wheels-bot" or .author.login=="wheels-bot[bot]")

The app/<slug> form is the GitHub GraphQL representation; <slug>[bot] is REST/webhook. The inline comment documents this clearly. The previous review's claim that "logins can't contain slashes" applies to the REST API, not GraphQL, and the gh CLI uses GraphQL for pr list. Accepting both forms is the most robust solution and the commit message confirms empirical verification against real PR numbers. Finding closed.

Unused Set up Wheels test environment step — removed. The /resolve-conflicts allowed-tools list (Bash(gh:*),Bash(git:*),Read,Edit,Write,Grep,Glob) has no CFML server dependency. The step was indeed dead weight. Closed.

Long lines in _shared-rails.md — wrapped. Both the Header ≤ 100 chars bullet and the PR title is the linted gate bullet now wrap at a readable width. Closed.


Conventions

fast-test skip condition is slightly broader than the PR description states (pr.yml line 34)

if: github.event.action != 'edited'

The PR body says "guards fast-test so title-only edits don't re-run the suite," but edited fires for title OR body changes — not exclusively title changes. A PR body edit (without any code push) will also skip fast-test. In practice this is harmless: a body-only edit carries no code change, and the prior test run (from the last synchronize) is still valid. The behavior is reasonable even if the description is imprecise. NIT — not blocking.


Correctness

No remaining correctness issues. The classify-conflicts.sh conservative-by-default design (empty input → escalate) is sound. The verify-then-push guard in bot-resolve-conflicts.yml (check for unresolved --diff-filter=U output before committing, then [ -f .git/MERGE_HEAD ] safety net) is a solid double-check. The clean-merge notice path correctly omits a push (nothing to push if the merge was already clean when it arrived at this workflow).


Security

PR title injection remains clean: PR_TITLE: ${{ github.event.pull_request.title }} is stored in env: and consumed as echo "$PR_TITLE" | npx commitlint. The env-var indirection is the correct mitigation.


Commits

All ten commits conform to commitlint.config.js. Types, scopes, and header lengths are valid. DCO sign-off is present on the latest commit. No issues.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's review is sound. No sycophancy, no false positives. One imprecision and one minor missed observation, neither of which blocks merge. Converging on approve.

Sycophancy

None detected. A's approval is backed by specific, line-cited verification of each prior finding. The correctness and security sections are substantive, not boilerplate.

False positives

None detected.

  • Author filter claim — A's reasoning is accurate. gh pr list --json author queries the GraphQL API, which represents GitHub App authors as app/<slug>, not <slug>[bot] (the REST/webhook form). The hardened filter accepting both is the right outcome. A cites the live proof from the PR author's own comment, which is solid.
  • env-var injection — A confirms PR_TITLE: ${{ github.event.pull_request.title }} stored in env: and consumed via echo "$PR_TITLE" | npx commitlint is the correct mitigation. Accurate.
  • double-check guard — A confirms the --diff-filter=U check before commit and the [ -f .git/MERGE_HEAD ] safety net are correct. Accurate.

Missed issues

Minor — DCO phrasing (commits section): A says "DCO sign-off is present on the latest commit." The actual state is that all 10 commits carry Signed-off-by: Peter Amiri <peter@alurium.com>. A's underlying conclusion is correct; the phrasing implies spot-checking only one. Not blocking, just imprecise.

Minor — Idempotency marker format (Escalate step, bot-resolve-conflicts.yml ~line 116): The escalation comment embeds the marker in visible body text —

(Labelled `conflict:needs-human`; marker: wheels-bot:conflict-attempted:$PR_NUMBER)

Every other bot comment in this repo uses an invisible HTML comment (<!-- wheels-bot:marker -->) for the idempotency token. The plain-text form probably works if the skip-check action does a substring search through the body, but it is inconsistent with the convention and exposes the internal token to readers. A did not flag it. Worth a follow-up cleanup, but not a correctness blocker given the marker-pattern in the skip-check step would still match the string.

Verdict alignment

A's APPROVE is consistent with the findings. All three prior findings (author filter, unused test-env step, long rail lines) are resolved and confirmed against the actual diff. The remaining edited trigger NIT is correctly categorised as non-blocking.

Convergence

Aligned. Both A and B reach the same recommendation: the PR is clean, the implementation is sound, and no further changes are warranted. The two observations above are note-for-follow-up quality, not request-changes. The human can mark ready and merge.

Per Reviewer B round-1 follow-up on #2847: the escalate step embedded the
wheels-bot:conflict-attempted marker in visible comment text, inconsistent with
the repo convention (invisible `<!-- ... -->` markers) and exposing the internal
token to readers. Move it into an HTML comment. The skip-check `marker-pattern`
still substring-matches it, so idempotency is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri

bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed B's round-1 marker-format note in b874d3040: moved the wheels-bot:conflict-attempted token out of the visible escalation-comment text and into an <!-- … --> HTML comment, matching the repo's marker convention. The skip-check marker-pattern substring-matches it either way, so idempotency is unchanged.

B's other note (DCO phrasing) was a critique of Reviewer A's wording, not the code — no change there.

The brainstorming design spec and step-by-step implementation plan that
produced this PR's changes, kept as design records per maintainer request.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: Third review on PR #2847 (head SHA b874d3040). Prior findings (author-filter, unused test-env step, long rail lines) are all closed. The b874d30 fix correctly moves the escalation idempotency marker into an HTML comment, consistent with the marker conventions in wheels-bot.md. One minor gap remains: the resolve path never posts the conflict-attempted marker after a successful push, leaving a theoretical repeat-dispatch cycle if the AI safety-check ever aborts with no marker posted. Commenting, not requesting changes.


Correctness

Resolve path: missing conflict-attempted idempotency marker after a successful push

After the escalate path, the marker <!-- wheels-bot:conflict-attempted:$PR_NUMBER --> is embedded in the PR comment at bot-resolve-conflicts.yml line 121. The skip-check gate (lines 60-67) finds it on re-dispatch and skips. After the resolve path (lines 124-135, 137-152), no equivalent comment is ever posted, so the skip-check always returns skip=false.

Practical effect in the happy path: if freshen re-dispatches before the pushed commit is visible, the re-run does git merge origin/develop and exits cleanly with a "No conflicts" notice. Harmless but wasteful.

Edge case worth noting: /resolve-conflicts instructs Claude to run git merge --abort and post a "gand classifier disagreed" comment if it finds any file outside the low-risk set (resolve-conflicts.md, hard safety rule section). After that abort, the verify step (lines 142-151) sees no unresolved files (--diff-filter=U is empty post-abort), skips the commit, and runs git push origin HEAD as a no-op since the branch is unchanged. The workflow exits 0. The PR stays DIRTY. Freshen re-dispatches. The skip-check finds no marker. The cycle repeats.

The classifier and the command's low-risk definition are intentionally mirrored (per the PR body: "the /resolve-conflicts safety gate was reconciled to mirror the classifier exactly"), so this loop should never trigger in practice. But it is a missing safety net if that invariant is ever broken.

Suggested fix: add a step after the successful push that posts the marker:

- name: Mark conflict-attempted (resolve succeeded)
  if: steps.gate.outputs.skip == 'false' && steps.classify.outputs.decision == 'resolve'
  env:
    GH_TOKEN: ${{ steps.app-token.outputs.token }}
  run: |
    gh pr comment "$PR_NUMBER" --repo "$REPO" --body \
      "<!-- wheels-bot:conflict-attempted:$PR_NUMBER -->"

Or update /resolve-conflicts to instruct Claude to include the marker in the abort comment, so the skip-check blocks re-dispatch if the gate/classifier invariant breaks.


Conventions

fast-test skip condition skips on body edits as well as title edits (pr.yml line 34)

The condition github.event.action != 'edited' fires for title OR body changes. A body-only edit also skips fast-test. Behavior is harmless (body-only edits carry no code change), but the PR description's phrasing ("title-only edits don't re-run the suite") is slightly imprecise. No action required.


Tests

Shell unit coverage for all three scripts is thorough: empty-input edge case, no-trailing-newline read-loop edge case in test-classify-conflicts.sh, all six mergeStateStatus strings in test-freshen-decide.sh, and pass/fail/boundary in test-commit-title.sh. The verify-then-push double-check (--diff-filter=U then MERGE_HEAD guard) in the workflow is a solid integration safety net.


Commits

All eleven commits conform to commitlint.config.js. Types are valid, headers are within 100 chars, none are ALL-CAPS. The fix(bot): move escalation idempotency marker into an HTML comment landing commit addresses the most recent review finding cleanly.


Security

No new concerns. PR number validated numeric before first use (line 46). PR_TITLE stored in env: before shell consumption. Escalation comment passes $CONFLICT_FILES through printf, not via unquoted interpolation.

Per Reviewer A round-3 follow-up on #2847. The resolve path posted no
conflict-attempted marker, so any outcome that left the PR DIRTY (Claude
leaving unresolved markers, or the command's safety gate aborting the merge)
produced no idempotency token — the freshen sweep would re-dispatch the
resolver every cycle (runaway Opus loop). Rework the verify step to push only
on a clean committed merge, and on any failure post the conflict-attempted
marker + conflict:needs-human label so a human takes over and freshen stops
re-dispatching. Broader than the suggested success-path marker, which the
self-healing DIRTY->clean push already covers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: Fourth review on PR 2847 (head SHA 2474385). This review fires because the previous review body contained the wrong SHA in its idempotency marker (b874d30 instead of the actual head 2474385 - a bot-side marker bug). The previous review did in fact cover the state at 2474385 based on its commit.oid, so there is no new logic to re-examine. The only new content is the last commit, which adds two design-record documents under docs/superpowers/. Three minor nits on the docs; no correctness, cross-engine, security, or test issues.


Correctness

No new correctness issues. All prior findings (author-filter, unused test-env step, long rail lines, escalation marker moved to HTML comment) are confirmed closed.

The resolve-path idempotency gap flagged in review 3 (no conflict-attempted marker posted after a successful push) remains, but continued analysis confirms it is harmless in practice: after a successful auto-resolve push, the PR's mergeStateStatus becomes CLEAN, and the 30-min freshen sweep maps CLEAN to skip (via freshen-decide.sh). The re-dispatch loop cannot actually close unless GitHub's merge-state cache lags the push by the full 30-minute schedule window - an extremely narrow race, and when it hits, the re-run does a clean merge and exits with a notice. Not blocking.


Docs

NIT - PR body contradicts itself about the new commit

The PR body under "Implementation notes" says:

The design spec and step-by-step plan live at docs/superpowers/specs/... and docs/superpowers/plans/..., left untracked locally and intentionally not part of this PR.

But the last commit (2474385) adds exactly those two files. The contradiction is harmless (the docs are useful to have) but the PR description should be updated: either remove the "not part of this PR" clause or note that they were added in the final commit per a maintainer request.

NIT - Plan document has all task steps unchecked

docs/superpowers/plans/2026-06-03-bot-pipeline-unblock.md marks every implementation step - [ ] (unchecked). The implementation is complete. A future reader reaching this via git blame or a docs search would see a pile of un-done work. If the intent is to preserve the plan as a historical design record, a brief header note ("Implementation complete as of this PR; boxes left as-is for historical fidelity") or a bulk-check of the boxes would remove the ambiguity.

NIT - Plan doc's low-risk set description drifted from the implementation

Under "Decisions carried from spec open-questions," the plan says the low-risk allowlist includes web/sites/*/src/content/**. The actual classify-conflicts.sh does not have that pattern - it uses .md|.mdx (any path), which is actually cleaner and covers the same files. The behavior is equivalent for every real conflict path, but a maintainer consulting the plan to extend the low-risk set could be confused by the discrepancy.


Commits

All twelve commits conform to commitlint.config.js: valid types, headers under 100 chars, no ALL-CAPS subjects. DCO Signed-off-by is present on every commit. The new commit header (docs: add bot-pipeline-unblock design spec and implementation plan, 66 chars) is clean.


Security

No new concerns. No credentials, no raw user input in shell contexts, no CFML code touched.

…SC1083)

The loop-safe verify step used `git log @{u}..HEAD` to detect a new commit;
shellcheck (SC1083) misreads the braces in git's @{u} upstream syntax. Capture
the PR head SHA as a step output before the merge and compare HEAD against it
instead — no behavior change, no @{u}, actionlint clean, and it no longer
depends on upstream tracking being configured in the checkout.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri

bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the resolve-path idempotency gap in 315b3c0a2 (lint cleanup in e476c511f).

The gap is real — and broader than the happy-path race the suggestion targeted. If /resolve-conflicts leaves any marker unresolved, or its safety gate aborts the merge, the PR stays DIRTY with no conflict-attempted marker → the freshen sweep re-dispatches the resolver every cycle (runaway). A success-only marker wouldn't close that loop, since the success path already self-heals (the push clears DIRTY, so freshen won't re-dispatch).

Fix: the verify step now pushes only on a clean, committed merge (HEAD ≠ the pre-merge SHA), and on any failure posts the conflict-attempted marker + conflict:needs-human label so a human takes over and freshen stops re-dispatching. e476c511f swaps @{u} for the explicit pre-merge SHA to clear shellcheck SC1083.

The fast-test body-edit note: no change — skipping the suite on a body-only edit is correct (no code change); only the PR-description wording was imprecise.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's review (4421721166) is technically sound on the one correctness finding it raises, but it was submitted against a stale SHA — the head moved from b874d3040 to 2474385d while A was running. I've verified the 12th commit A missed and it is clean. Converging on approve.

Sycophancy

None detected. A's COMMENTED state is backed by concrete line citations, not a rubber-stamp.

False positives

None detected.

  • Missing idempotency marker on resolve path — A's structural analysis is accurate. The escalate path posts the conflict-attempted HTML comment marker (in the escalate step). The resolve path (Claude step + verify-and-push step) posts nothing. The skip-check gate therefore always returns skip=false on a second dispatch against a cleanly-resolved branch. A's edge-case description (classifier/gate invariant break → abort → no-op push → branch stays DIRTY → freshen re-dispatches indefinitely) is correct. A's suggested step would close the gap. Confirmed by reading the full workflow.
  • fast-test fires on body edits too — A's claim that github.event.action != 'edited' suppresses on any edited event (title or body) is accurate. The PR description says "title-only edits" — A correctly notes the imprecision.

Missed issues

Stale SHA in A's marker (procedural). A says "Third review on PR #2847 (head SHA b874d3040)" and embeds that SHA in its idempotency marker. The actual head when A submitted was 2474385d — a 12th commit ("docs: add bot-pipeline-unblock design spec and implementation plan") had already landed. A's review does not cover that commit.

Commit count wrong, but substance correct. A says "all eleven commits conform to commitlint." There are twelve. I checked the 12th (2474385d): type docs, no scope, 66-char header, Signed-off-by present from the human author. It adds two design-record files to docs/superpowers/ — no code, no workflow changes. Conformant.

PR body inconsistency (cosmetic). The PR description says the design spec and plan were "intentionally not part of this PR," but the 12th commit adds exactly those files. A missed this. It is cosmetic — the PR body was not updated when the maintainer added the docs commit — but worth noting for the record.

Verdict alignment

A's COMMENTED verdict (not APPROVED, not CHANGES_REQUESTED) is appropriate. The resolve-path marker gap is a real safety-net omission, but A correctly judges it non-blocking given the classifier/command mirror invariant.

Convergence

Aligned on approve. A's correctness finding is verified and sound; A's COMMENTED verdict is the right call. The 12th commit A missed is benign documentation — I've reviewed it and it is conformant. The PR body inconsistency is cosmetic. No changes are needed to proceed.

@bpamiri

bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

NIT 1 (PR body): fixed — the "not part of this PR" clause was stale once the docs were committed per maintainer request; the description now reflects that they're included as design records.

This review fired on 2474385d3 (a stale marker SHA, as you noted). The resolve-path idempotency gap it carries forward from review 3 is already fixed at the current head e476c511f (commit 315b3c0a2): the verify step now pushes only on a clean committed merge and, on any failure, posts the conflict-attempted marker + conflict:needs-human label — so freshen can't re-dispatch into a loop, even in the failure modes (not just the self-healing happy path).

NIT 2 (unchecked plan checkboxes) and NIT 3 (plan's allowlist wording vs the shipped *.md|*.mdx) are accuracy drift in a point-in-time design record. Leaving them as historical fidelity rather than resetting PR convergence over cosmetic doc nits; happy to fold a one-line clarifying note as a follow-up if preferred.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's review is structurally compromised by a SHA confusion: A analyzed the wrong commit and missed reviewing the actual code change in the current head. The three NITs A raises are accurate but irrelevant to what this review should cover. Not aligned.

Sycophancy

None detected. A's tone is substantive and its NITs are genuinely observed.

False positives

None detected. A's three NITs (PR body contradiction about docs being "not part of this PR", unchecked plan-doc task boxes, low-risk set description drift) are accurate observations about the docs commit (2474385d3). They are not false positives — they are simply about the wrong commit.

Missed issues

Critical — A reviewed the wrong commit. A's review states:

"TL;DR: Fourth review on PR 2847 (head SHA 2474385) ... The only new content is the last commit, which adds two design-record documents under docs/superpowers/."

The actual current head is 315b3c0a2766354fbfa52283168762329971b63b ("fix(bot): make resolver loop-safe — escalate + mark on resolve failure"). The docs commit (2474385d3) was the previous head; 315b3c0a2 is the subsequent code fix on top of it. A has the order of commits transposed.

The change in 315b3c0a2 — 25 insertions / 9 deletions in .github/workflows/bot-resolve-conflicts.yml — rewrites the "Verify resolution, push, or escalate" step: captures BASE_SHA before the merge, then branches on (1) conflict markers still present → abort, (2) resolved but uncommitted → commit, (3) HEAD != BASE_SHA → push + exit 0, (4) otherwise → post conflict:needs-human label + conflict-attempted marker + exit 1. This is the specific fix for the runaway-loop gap A itself raised in a prior review round. It was not examined.

A's correctness section says "All prior findings are confirmed closed" and "the resolve-path idempotency gap remains but is harmless in practice." That assessment predates 315b3c0a2, which closes that exact gap — but A did not review whether the closure is correct.

Verdict alignment

A's COMMENTED verdict with "no concerns" is inconsistent with the actual scope of the review. A described an "only docs" review and raised doc-quality NITs, then concluded no correctness, security, or test issues — without having looked at the code change that is the substantive new content of the current head.

Convergence

Not aligned. A needs to:

  1. Acknowledge the SHA transposition (current head is 315b3c0a2, not 2474385d3).
  2. Review the .github/workflows/bot-resolve-conflicts.yml changes in 315b3c0a2 — specifically: the BASE_SHA capture, the three-branch resolve logic, and the failure-path escalation + marker posting.
  3. Issue an explicit verdict on those code changes.

The three docs NITs remain valid follow-up items for the human, but they are not a substitute for reviewing the code commit. Once A covers 315b3c0a2, the convergence decision can be re-evaluated.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wheels Bot — Reviewer A

TL;DR: Fifth review on PR #2847 (head SHA 315b3c0a). This fires because review 4's idempotency marker contained the wrong SHA (2474385d3 instead of 315b3c0a). The 315b3c0a commit correctly addresses the loop-safety gap flagged in review 3: the verify step now escalates and posts the conflict-attempted marker on any resolution failure, so the freshen sweep cannot re-dispatch the resolver indefinitely. All prior correctness findings are confirmed closed. Three docs nits from review 4 remain open (all minor, none blocking). Commenting.


Correctness

Loop-safe fix (bot-resolve-conflicts.yml lines 149-168) confirmed correct

The reworked verify step handles all cases cleanly:

State when verify runs Branch taken Outcome
Unresolved markers remain git merge --abort then escalation + marker No loop
MERGE_HEAD present, no unresolved files git commit --no-edit then push Merge completes
Claude committed already Neither branch; git log @{u}..HEAD finds commit; push Merge pushed
No new commit for any other reason Neither branch; empty log; escalation + marker No loop

One theoretical edge: if git commit --no-edit in the elif branch exits non-zero (e.g. a pre-commit hook), set -euo pipefail aborts the step before the escalation block runs, so no conflict-attempted marker is posted and freshen can re-dispatch. Extremely unlikely on a clean bot branch with no hooks, and timeout-minutes: 60 is a backstop. Not blocking.

All other previously flagged correctness items (author-filter form, unused test-env step) are confirmed closed.


Docs

NIT: PR body still contradicts itself about the design/plan files

The PR description says:

The design spec and step-by-step plan live at docs/superpowers/specs/... and docs/superpowers/plans/..., left untracked locally and intentionally not part of this PR.

Both files are in fact included (commit 2474385d3). The body was not updated after review 4 flagged this. Harmless, but a future reader skimming the PR summary will be confused. Suggest removing the "intentionally not part of this PR" clause.

NIT: plan document has all task steps unchecked

docs/superpowers/plans/2026-06-03-bot-pipeline-unblock.md marks every implementation step - [ ]. The implementation is complete. A brief note at the top (e.g. "> Implementation complete - boxes left unchecked for historical fidelity") would remove the ambiguity. Not blocking.

NIT: plan doc's low-risk allowlist description drifted from the implementation

Under "Decisions carried from spec open-questions," the plan lists web/sites/*/src/content/** as part of the allowlist. The shipped classify-conflicts.sh does not include that pattern - it uses *.md|*.mdx (any path), which covers MDX content files in practice but would escalate a hypothetical non-markdown file under a content tree. Behavior is equivalent for every real conflict path, but a maintainer consulting the plan to extend the allowlist would encounter the discrepancy.


Correctness recap (all prior findings)

Finding Review Status
Author filter "app/wheels-bot" never matched R1 Closed: both forms accepted (R2)
Unused Set up Wheels test environment step R2/R3 Closed: removed (cc23f01)
Long lines in _shared-rails.md R1/R2 Closed: wrapped (cc23f01)
Escalation marker moved to HTML comment R3 Closed: moved (b874d30)
Resolve path: no conflict-attempted marker posted R3 Closed: reworked verify step (315b3c0)

Commits

All thirteen commits conform to commitlint.config.js. Types are valid (ci, chore, feat, docs, fix), headers are within 100 chars, subjects are not ALL-CAPS. DCO Signed-off-by is present on the latest commit.


Security

No new concerns. PR-number input is validated numeric before first use (line 46). CONFLICT_FILES passes through printf (not unquoted interpolation) in escalation comments. PR_TITLE is stored in env: before shell consumption in pr.yml. Clean.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot -- Reviewer B (round 1)

A review (4421812710) is substantively correct on the loop-safety analysis, with three minor observations -- none blocking. Converging on approve.

Sycophancy

None detected. The COMMENTED verdict is backed by concrete line-level analysis and a full correctness-history table. No rubber-stamping.

False positives

None detected. The four-state verify-step table is logically accurate: the escalation + marker path fires on any failure, closing the runaway-loop gap. Security and commit hygiene are correctly assessed.

Anachronism (not a false positive, a stale reference). Row "Claude committed already" cites git log @{u}..HEAD as the detection mechanism. The current head (e476c511f) replaced that with explicit BASE_SHA comparison, captured before the merge in steps.merge.outputs.base. A reviewed 315b3c0a and has not seen the e476c511f change, so the reference is anachronistic. The described outcome (new commit detected, then push) is correct for the current code.

Missed issues

A did not review e476c511f (refactor: use explicit pre-merge SHA in resolver verify, avoids SC1083). This is the actual current head. The commit swaps @{u} for an explicit pre-merge SHA to clear ShellCheck SC1083 -- a lint fix with no behavioral change. The verify-step logic A analyzed is semantically identical before and after. Low risk, not blocking.

Commit count off by one. A says "all thirteen commits conform." There are fourteen: the count reflects 315b3c0a as the head (twelve prior plus 315b3c0a = thirteen), missing e476c511f. The 14th commit: type refactor, no scope, 68-char header, DCO sign-off present. Conformant.

Idempotency marker has the wrong SHA. The marker in the review body says 315b3c0a but the review was submitted against head e476c511f (confirmed via the GitHub commit_id field). Same SHA confusion B flagged in the prior round. The converged-approve marker B posts on e476c511f should prevent further A runs regardless.

Verdict alignment

COMMENTED is consistent with the findings. The three docs NITs (PR body contradiction, unchecked plan boxes, plan allowlist drift) are accurate and correctly judged non-blocking.

Convergence

Aligned, approve. The substantive analysis of the 315b3c0a loop-safety fix, author-filter hardening, and escalation marker placement is correct. The missed e476c511f is a shellcheck lint fix with no behavior change -- the logic analysis holds for the current code. No code changes needed; the human can mark ready and merge.

@bpamiri
bpamiri merged commit 70291fa into develop Jun 3, 2026
7 checks passed
@bpamiri
bpamiri deleted the claude/compassionate-cohen-4b3192 branch June 3, 2026 19:23
bpamiri pushed a commit that referenced this pull request Jun 5, 2026
…on (#2851)

Add implementation-complete note so unchecked boxes read as historical
record, not pending work. Update allowlist note to reflect that the
shipped classify-conflicts.sh dropped web/sites/*/src/content/** — the
*.md/*.mdx arm already covers MDX content files and non-markdown files
in content trees now correctly escalate.

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant