docs(source-control): merge-forward for stale PR branches + statusCheckRollup running-check pitfall - #2513
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 35s —— View job Security review — skipped (skip gate)
Per
No code, scripts, hooks, or GitHub Actions workflows are touched, and no injection, authorization, credential-handling, or supply-chain surface is introduced. No security findings to report. |
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b05bcc6c
ℹ️ 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".
…s; statusCheckRollup running-check pitfall Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
05b05bc to
ac3231b
Compare
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Security review — skipped (skip gate)
Per
No code, scripts, hooks, or GitHub Actions workflows are touched, and no injection, authorization, credential-handling, or supply-chain surface is introduced. Note: the two Codex review comments on this PR (incomplete No security findings to report. |
|
Claude finished @kyle-sexton's task in 44s —— View job Security review — skipped (skip gate)
Per
No code, scripts, hooks, or GitHub Actions workflows are touched, and no injection, authorization, credential-handling, or supply-chain surface is introduced. Note: the two open Codex review comments on this PR (incomplete No security findings to report. |
|
Claude finished @kyle-sexton's task in 1m 51s —— View job Code review
This is a docs-only change (skill reference prose + version bump + changelog) to Two findings posted inline (both confirm and add repo-specific detail to the existing Codex review comments on this PR):
No other correctness issues found in the diff (version bump and changelog entries are consistent with the prose changes). No security-relevant surface, consistent with the earlier security-lane pass on this PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # failures only | ||
| --jq '[.statusCheckRollup[] | select(.conclusion=="FAILURE" or .conclusion=="TIMED_OUT" or .conclusion=="CANCELLED")]' | ||
| # still running | ||
| --jq '[.statusCheckRollup[] | select(.status!="COMPLETED")]' |
There was a problem hiding this comment.
The two new jq selectors only classify CheckRun nodes correctly; statusCheckRollup also contains StatusContext nodes (legacy commit statuses), which carry state instead of status/conclusion.
- "still running" (
select(.status!="COMPLETED")): aStatusContexthas nostatusfield, so this is always true for it — a long-finished legacy status (state: "SUCCESS") is reported as still running forever. - "failures only": omits
ACTION_REQUIRED,STALE, andSTARTUP_FAILURE, whichplugins/source-control/skills/babysit-prs/scripts/babysit_checks.py:21-29already classifies as failing states alongsideFAILURE/TIMED_OUT/CANCELLED. Those conclusions read as neither failing nor running here and get silently dropped.
The repo already has the correct classification for this exact union type in babysit_checks.py's check_category() (plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py:44-51) — worth mirroring its node-type/state handling here rather than introducing a second, narrower selector.
| **Stale branch recovery** — if CI fails because the branch is out of date with the default branch (merge conflicts, "branch is not up to date" errors, or tests failing due to default-branch-only changes): integrate (merge or rebase per the project's convention and the branch's own history), resolve conflicts conservatively, force-push with lease, restart the monitor loop from 3.1. Distinct from code failures — no research gate for the integration itself, only for conflicts requiring intent judgment. | ||
| **Stale branch recovery** — if CI fails because the branch is out of date with the default branch (merge conflicts, "branch is not up to date" errors, or tests failing due to default-branch-only changes): integrate, resolve conflicts conservatively, push, restart the monitor loop from 3.1. Distinct from code failures — no research gate for the integration itself, only for conflicts requiring intent judgment. | ||
|
|
||
| **Merge-forward is the default integration; rebase is the exception.** `git merge <remote>/<default-branch>` *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push, no history rewrite. A rebase rewrites the branch and demands `git push --force-with-lease`, which permission classifiers commonly deny in autonomous/auto-mode sessions; the observed failure shape is a lane concluding every rebase costs a fresh branch + fresh PR + closing the old one (re-opening every review thread from scratch), when the stale branch never needed a rebase at all. Under a squash-only default branch, the merge commits inside the PR branch collapse to one commit on merge and linear-history requirements stay satisfied — repeated merge-forwards as the default branch moves cost nothing. Rebase only when the project's convention requires a linear PR branch *and* force-push is actually available. |
There was a problem hiding this comment.
This new default-integration guidance runs git merge <remote>/<default-branch> without first fetching, so it merges whatever the local remote-tracking ref happened to be at the last fetch — which can be stale by the time "stale branch recovery" (§3.2) or the mergeable pre-check (§3.1 step 1) actually fires, since CI/mergeable status can lag the last local fetch by minutes to hours. That reproduces the exact staleness this section is meant to resolve: merging an old tip either leaves the PR still CONFLICTING/behind, or falsely reports "already up to date" when it isn't.
create.md:84-93 already gets this right in the same skill — git fetch "$REMOTE" "$DEFAULT_BRANCH" immediately before computing the merge-base / merging. Worth adding the same fetch step here (or an explicit pointer to that snippet) before the git merge <remote>/<default-branch> instruction.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ckRollup running-check pitfall (#2513) ## Summary Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path: - **Stale-branch recovery now defaults to merge-forward.** `monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the #1315 → #1377 → #1393 churn, with every review thread re-opened on the successor). Merging the default branch *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: #1393 landed that way and #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted. - **`statusCheckRollup` reports a running check as `conclusion: ""` (empty string), not `null`.** The complement-shaped filter (`conclusion != null and != "SUCCESS"`) therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill reads `statusCheckRollup`) now documents the pitfall with value-positive jq selectors for "failed" and "still running". Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern). ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0 issues. - Docs-only change to skill reference text; no scripts or hooks touched. The jq forms added are the ones from the issue, verified against `gh pr view --json statusCheckRollup` semantics. ## Related Fixes #1436 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
2 participants
Summary
Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path:
monitor.md's mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315 → fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1377 → fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1393 churn, with every review thread re-opened on the successor). Merging the default branch into the PR branch resolves staleness and pushes fast-forward — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1393 landed that way and feat(claude-config): add audit-pass, one coordinated resumable instruction-surface pass #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted.statusCheckRollupreports a running check asconclusion: ""(empty string), notnull. The complement-shaped filter (conclusion != null and != "SUCCESS") therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill readsstatusCheckRollup) now documents the pitfall with value-positive jq selectors for "failed" and "still running".Version
0.53.11→0.53.14(patch;0.53.12/0.53.13are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern).Test plan
npx markdownlint-cli2@0.23.2on both edited markdown files — 0 issues.gh pr view --json statusCheckRollupsemantics.Related
Fixes #1436