Skip to content

fix(source-control): hold the babysit merge on a body line starting Hold: - #7349

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/babysit-hold-line
Oct 13, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
fix/babysit-hold-line

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner-approved follow-up to the auto-ready plan; aligns the babysit merge gate with the merge-only Hold: line #7345 documents.

Summary

The babysit merge gate's BODY_MERGE_HOLD_RE held a PR only when its body said "do not merge" or "don't merge". Lane-power PRs in this repo hold with a body line starting Hold: (for example Hold: a human merges this), which the gate did not see. It now does. A Hold: line holds the merge only: #7345's check-ready-holds.sh shares just the do-not-merge part of the pattern and lets the ready flip go ahead past a Hold: line.

Fix

  • babysit_merge.py: BODY_MERGE_HOLD_RE also matches (?:^|\r)[^\S\r\n]*hold: per line (case-insensitive, multiline): any non-newline whitespace before it, NBSP included, and a bare CR counts as a line break. The merge gate and the merge-entry hold in babysit_merge_policy.py both read this one pattern. The blocker message now says the body holds the merge and names both forms.
  • babysit-prs/SKILL.md and babysit-loop/SKILL.md: the do-not-merge hold wording lists the Hold: line. docs/conventions/loop-lane/README.md does not list body forms, so it is unchanged.
  • Changelog fragment for source-control (patch).

Verification

  • test_babysit_merge.py (PullRequestBodyHold): a Hold: line (CRLF, indented lowercase, NBSP-prefixed, bare-CR cases) blocks with and without the tier; hold: inside a line and Threshold: do not block. test_babysit_merge_policy.py: the merge-entry hold returns body-hold for a Hold: line. The NBSP and bare-CR cases fail on the earlier ^[ \t]*hold: pattern (4 failures) and pass after.
  • python3 -m unittest over babysit-prs/scripts/tests: 1405 tests OK on 0aa3466.
  • scripts/check-affected.sh --tests on 0aa3466 (merged with main, after npm ci): 8 passed, 1 skipped (no shell changed).
  • Ready-flip reviews: code review and security review in fresh-context subagents; findings fixed in 0aa3466.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj

…old:

BODY_MERGE_HOLD_RE now also matches a PR body line that starts with
`Hold:`, the form lane-power PRs use and the rule the pull-request ready
hold check applies, so the merge gate and the merge-entry hold both
refuse such a PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj
kyle-sexton and others added 2 commits October 12, 2026 20:38
…e CR

The Hold: alternative now accepts any non-newline whitespace before the
word (an NBSP pasted from a rich-text editor no longer slips past) and
treats a bare CR as a line break. The comment and changelog fragment no
longer claim parity with check-ready-holds.sh, which leaves a Hold: line
to the merge. The merge-entry hold gains a Hold: line test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 13, 2026 00:42
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review babysit_merge.py regex/hold-logic change
  • Review test files and SKILL.md wording changes
  • Check for instruction-surface deletions against protected classes
  • Post findings

Scope: changes in this PR only (babysit_merge.py, babysit_merge_policy.py import site, two SKILL.md files, tests, changelog fragment), diffed against origin/main.

Analysis:

  • The core change widens BODY_MERGE_HOLD_RE in babysit_merge.py to also match a Hold: line at line-start (or after a bare \r). This is a hold/guardrail getting stricter (more inputs now block a merge), not narrowed or removed — so the instruction-surface-deletion lens doesn't apply here.
  • babysit_merge_policy.py only imports the shared BODY_MERGE_HOLD_RE/MERGE_HOLD_LABEL constants (no diff to that file itself) — confirmed both the merge gate and merge-entry hold consume the one pattern, as the PR description claims.
  • The regex itself (\bdo(?:n['’]?t| not)\s+merge\b|(?:^|\r)[^\S\r\n]*hold:) has no nested/overlapping quantifiers and poses no ReDoS risk; it's a plain .search() over PR body text, with no eval, shell interpolation, or templating of that text.
  • SKILL.md wording changes only document the widened behavior; no logic there.
  • Test additions in test_babysit_merge.py / test_babysit_merge_policy.py are inert (assertions only).

No security issues found (no CRITICAL / IMPORTANT / SUGGESTION findings).
· Branch

claude-lane-summary: lane=claude-security-review sha=fbbe1d17cbf946d3aa653626f3dbdc6b2822832e run=38328602902

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Reviewing PR #7349

  • Gather context (diff, SKILL.md changes, tests)
  • Review BODY_MERGE_HOLD_RE regex change in babysit_merge.py
  • Check babysit_merge_policy.py consumer of the same regex
  • Check test coverage for edge cases
  • Check SKILL.md wording changes
  • Post findings

View job run

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: fix/babysit-hold-line (#7349)

  • Gather context (diff, CLAUDE.md/REVIEW.md, standards)
  • Review changed files for correctness/maintainability
  • Adversarial validation of candidate findings (none to verify)
  • Post findings / summary

Scope: 6 changed files — babysit_merge.py's BODY_MERGE_HOLD_RE, the two SKILL.md wording updates, the two test files, and the changelog fragment.

Findings: none. I traced BODY_MERGE_HOLD_RE (plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py#L196-L198) by hand against every case in the new tests plus several adversarial ones I added mentally (mid-line Hold: after other text, Holding:/Threshold:/Holders: substrings, NBSP after a real \n, double bare-CR line breaks, position-0 Hold:) — the (?:^|\r)[^\S\r\n]*hold: branch only fires on a genuinely line-initial (optionally indented) Hold:, never mid-line, matching the intent in the surrounding comment and in plugins/source-control/skills/babysit-prs/SKILL.md#L149-L150.

  • Single source of truth confirmed: babysit_merge_policy.py imports BODY_MERGE_HOLD_RE from babysit_merge.py rather than duplicating it, so the merge gate and the merge-entry hold can't drift (plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py#L1463).
  • docs/conventions/loop-lane/README.md has no body-hold wording to update, so leaving it alone (as the PR body states) is correct.
  • source-control is listed in scripts/fragment-plugins.txt, so the changelog fragment (vs. a manual plugin version bump) is the right mechanism here.
  • Test coverage (PullRequestBodyHold in test_babysit_merge.py, EntryHold.test_a_body_hold_line in test_babysit_merge_policy.py) exercises CRLF, bare-CR, indented-lowercase, mid-line hold:, and Threshold: — covers the edge cases I'd otherwise flag.

I could not execute Python in this sandbox (every python3 invocation needed interactive approval the lane can't grant), so I verified the regex by manual trace rather than running the test suite myself; the author's own test-run numbers in the PR body are author-claimed and unverified by me.

No paved-path citations applied (none of the standards index rows matched this change).

claude-lane-summary: lane=claude-review sha=0aa346668f0b4bcff5bdb4597f6a2f829e3f3e21 run=38328607961

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through 1aa9a32; a later push is reviewed from there.

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 44s —— View job


Security review complete

  • Gather context (incremental diff, custom instructions)
  • Invoke /review:security-review
  • Review changed files for security issues
  • Post findings / summary

Scope (incremental, since fbbe1d1): the BODY_MERGE_HOLD_RE regex change in
babysit_merge.py,
plus the matching test additions and the changelog-fragment wording tweak. Everything else in this PR was reviewed at fbbe1d1.

What changed: the merge-hold pattern widened from ^[ \t]*hold: to
(?:^|\r)[^\S\r\n]*hold:, so a Hold: line is also recognized when it's preceded only by a
bare \r (old Mac-style line break) or by non-tab/space whitespace such as NBSP, and both call
sites (babysit_merge.py and babysit_merge_policy.py) read this one shared pattern.

Security read: BODY_MERGE_HOLD_RE gates an automated-merge control, so I checked whether
the change could let a merge proceed when a human hold was intended (an authorization-bypass
direction), rather than the reverse (an extra, safe-side block).

  • The ^ alternative is unchanged in its position requirement and the whitespace class only
    widened ([^\S\r\n]* is a strict superset of [ \t]*), so every input the old pattern caught
    is still caught — no new false negative on that branch.
  • The \r alternative is purely additive (a bare-CR line start that the old pattern, lacking a
    DOTALL/line-ending workaround for ^, could not see at all), so it only catches more holds,
    never fewer.
  • Word-boundary / negative cases (Threshold:, hold: embedded mid-line) still correctly fail
    to match, since the whitespace-only prefix still can't skip over non-whitespace characters to
    reach hold: — confirmed by reading the regex semantics against the new test_hold_word_inside_a_line_does_not_block and the added CRLF/CR/NBSP cases in test_babysit_merge.py.
  • No catastrophic-backtracking surface: both alternatives use a single bounded character-class
    star with no nested/overlapping quantifiers, so matching stays linear in input length.
  • The blocker message strings are static; the matched PR body text is never echoed back into a
    comment, so there's no injection surface from attacker-controlled body content here.
  • The comment text above the regex that changed is prose/documentation inside the .py file, not
    a standing instruction surface (CLAUDE.md/AGENTS.md/rules file/skill or agent body/hook
    allow-or-block list), so the instruction-surface-deletion lens doesn't apply to this diff.

No security issues found in this incremental scope.

claude-lane-summary: lane=claude-security-review sha=0aa346668f0b4bcff5bdb4597f6a2f829e3f3e21 run=38328607848

@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: 0aa346668f

ℹ️ 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/source-control/skills/babysit-prs/SKILL.md Outdated
@chatgpt-codex-connector

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-10-13T00:45:29.070594Z 0aa3466 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.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through 1aa9a32; a later push is reviewed from there.

Reflow the do-not-merge hold paragraphs so babysit-loop (501 lines on
main) and babysit-prs (500 after the Hold: wording) pass the static
skill-quality contract's hard cap. No wording changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review: fix/babysit-hold-line (#7349), incremental

  • Gather context (incremental diff, custom instructions)
  • Review .claude-lane/incremental.diff changes to the two SKILL.md files
  • Check Codex's P1 finding about re-measuring skill output
  • Post findings

Scope: incremental since 0aa3466 — commit 1aa9a32 ("keep the babysit SKILL.md files under the line cap"), touching only
plugins/source-control/skills/babysit-loop/SKILL.md
and
plugins/source-control/skills/babysit-prs/SKILL.md.

Findings: none. This commit only rejoins previously-wrapped lines (and in the babysit-loop file, one comma becomes a colon) — no instructional content changed from what 0aa3466 already shipped and both prior review lanes cleared. I confirmed the stated motivation is real and correct: plugins/skill-quality/scripts/check-skill.sh enforces LINE_HARD_CAP=500 (check 4, counted whole-file via grep -c ''), and both files were at 501 lines before this commit's merges and are exactly 499 after — under the cap, verified by direct count rather than taking the commit message's word for it. Rendering is unaffected since both edits are inside prose paragraphs, not list items.

This lands under the skip gate's "trivial/automated change with no meaningful review surface" criterion — no inline comments posted.

Codex's earlier P1 comment (re-measure skill output on any body change, citing skill-criteria.md:196-198) targets the prior commit's actual wording change (the Hold: addition), not this reflow; it isn't reopened by a pure line-join with no instructional delta, so I'm not duplicating it here.

Branch

claude-lane-summary: lane=claude-review sha=1aa9a325e3447c6a6a8dccc3f6ab4202bef7563b run=38329299400

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (custom instructions, incremental diff)
  • Read .claude-lane/incremental.diff
  • Review changed SKILL.md wording for security issues
  • Post findings

Scope: incremental, since 0aa346668f0b4bcff5bdb4597f6a2f829e3f3e21. Only
babysit-loop/SKILL.md
and
babysit-prs/SKILL.md
changed in this increment (commit 1aa9a32, "keep the babysit SKILL.md files under the line cap"); all other files were reviewed at 0aa3466.

Analysis: diffed via .claude-lane/incremental.diff against gh pr diff for cross-check. The change is a pure line-rewrap of prose that was already reviewed at the prior sha:

  • babysit-loop/SKILL.md — the do-not-merge paragraph is reflowed from 4 lines to 2; the words, their order, and the described behavior (label/body hold respected in every tier, stripped only via explicit --strip-do-not-merge flag, never persisted) are byte-identical in content, just re-wrapped.
  • babysit-prs/SKILL.md — same: the Hold:/do-not-merge hold paragraph is reflowed from 5 lines to 4, no wording added, removed, or reordered.

No instruction-surface deletion, no narrowing of the hold/guardrail language, no new logic, no code change. This is a formatting-only fix for the skill-body line cap and carries no security implications.

No security issues found (no CRITICAL / IMPORTANT / SUGGESTION findings).

Branch ·

claude-lane-summary: lane=claude-security-review sha=1aa9a325e3447c6a6a8dccc3f6ab4202bef7563b run=38329299404

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit 736fb71 Oct 13, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the fix/babysit-hold-line branch October 13, 2026 01:09
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.

1 participant