docs(skills): have babysit-pr fix CI and resolve merge conflicts

[zhangfand]

What this PR does

babysit-pr watched only review feedback. It stopped on an approval while CI was red or the branch conflicted with the base branch, so a "babysat" PR could still be unmergeable.

The skill now names CI failures and merge conflicts as part of the job, and its done condition is reviewed clean, CI green, and merges cleanly. How to fix a failing check or resolve a conflict is left to the agent. Merging stays with the user.

Test plan

• scripts/check-pr-title.sh passes on the title
• Babysat this PR with the updated skill: CI green, MERGEABLE, review rounds answered

🤖 Generated with Claude Code

[zoolsher]

🔁 This review has been superseded. See the latest review.

[Jessie-QingYu]

🔁 This review has been superseded. See the latest review.

[zhangfand]

Addressed the review in a follow-up commit:

• Mergeability: only MERGEABLE counts now. UNKNOWN means read it again next tick. Dropped the unused mergeStateStatus.
• PR pinning: a setup step resolves headRefName/baseRefName, runs gh pr checkout, and passes the PR number to every gh pr command. This also sources <base>, and the description now says "base branch".
• Stale state: a tick that pushed always schedules another. Done counts only checks and reviews on the head commit, and "no checks reported" counts as pending.
• Bounds: the skill hands back to the user after five pushes.
• Consistency: a conflict merge is pushed on its own, and review and CI fixes are batched. The run id is sourced from gh pr checks --json link. Verification cites pnpm typecheck + pnpm test:unit inside nix develop.

Declined: fork-PR handling. Rome PRs come from branches in this repo, and gh pr checkout covers the checkout if that changes.

[zoolsher]

🔁 This review has been superseded. See the latest review.

[zhangfand]

Second round: <workflow> now comes from the failing check (gh pr checks --json workflow).

Declined adding pnpm lint to the pre-push list, because Biome's npm binary does not run on the NixOS host (see CLAUDE.md), so lint stays a CI check. The five-push bound counts pushes, so a lint miss costs one push like any other CI failure.

[Jessie-QingYu]

🔁 This review has been superseded. See the latest review.

[zoolsher]

🔁 This review has been superseded. See the latest review.

[Jessie-QingYu]

Code Review: 🛑 REQUEST_CHANGES

This PR broadens the babysit-pr skill from "watch review feedback" to "keep the PR mergeable": each tick now checks merge conflicts first, then review rounds, then failing CI, and the skill stops only when review is settled, CI is green (or equally red on the base branch), and mergeable is MERGEABLE. It is a single-file, prose-only change to .claude/skills/babysit-pr/SKILL.md. I read the whole post-change file, the sibling respond-to-review, loop-northstar and loop-reconcile skills, AGENTS.md, docs/authoring/prs.md, and .github/workflows/ci.yml to check the procedure against how this repo actually builds and gates PRs.\n\nThe direction is right and the invariants are well chosen: merge instead of rebase (the repo does squash-merge, confirmed in docs/authoring/prs.md), never merge the PR, one push per tick, a bounded push budget, and escalation to the user on incompatible conflicts or repeated failed fixes. The commands are largely accurate — --json mergeable, --json name,bucket,link,workflow, gh run view --log-failed and gh run rerun --failed all exist as written. What needs work is the termination logic: making the done conditions head-commit-scoped means the skill's own CI/conflict pushes invalidate the review condition, and nothing bounds ticks that never push, so a babysat PR can poll forever. Two smaller procedural gaps (a dropped review round on conflicting ticks, and a pre-push gate that misses CI's first job) also cost the loop the very pushes its budget rations.

Verdict: REQUEST_CHANGES — The new head-commit-scoped done conditions make the review condition unsatisfiable after the skill's own pushes, with no tick bound to stop the loop, and the conflict-first skip can silently drop a review round.

7 finding(s) — 3 posted as inline comments below; 4 listed in this table only (no matching diff line).

Severity	Category	File	Title
P1	architecture	.claude/skills/babysit-pr/SKILL.md	Review done-condition is unsatisfiable after the skill's own pushes, and nothing bounds the ticks
P2	design	.claude/skills/babysit-pr/SKILL.md	Conflict-first skip can silently drop a review round
P2	error-handling	.claude/skills/babysit-pr/SKILL.md	Pre-push gate misses the CI job that fails first
P2	code-quality	.claude/skills/babysit-pr/SKILL.md	gh run list cannot answer "does the same job fail on the base branch?"
P3	security	.claude/skills/babysit-pr/SKILL.md	Conflict resolutions are committed and pushed with no record for the user
P3	code-quality	.claude/skills/babysit-pr/SKILL.md	<head> and number are resolved but never used
P3	design	.claude/skills/babysit-pr/SKILL.md	git pull at tick start is underspecified after a local commit

---

Automated review by RomeOS Code Review · commit ed63ede

[Jessie-QingYu]

[P2] design — Conflict-first skip can silently drop a review round

Step 1 skips steps 2–3 on CONFLICTING, but step 2 reads "reviews and comments posted since the last tick". A review posted just before a conflicting tick is skipped that tick and then falls outside the next tick's window, so it is never answered — the loop can end "done" with an unanswered round. Make the watermark advance only over rounds actually handled: state that the skipped tick does not move the last-handled marker, or track it as the last review id answered rather than the last tick time.

[Jessie-QingYu]

[P3] code-quality — <head> and number are resolved but never used

Setup queries number,headRefName,baseRefName and defines <head>, but nothing below the Setup section uses <head> or the resolved number — the loop passes <pr> through and the conflict flow only ever names <base>. Drop the unused pieces: gh pr view <pr> --json baseRefName, and "<base> below is that branch name".

[Jessie-QingYu]

[P3] design — git pull at tick start is underspecified after a local commit

When the local branch has a commit the remote does not (a fix that failed its pre-push gate, or a user force-push on the other side), a bare git pull either errors with "need to specify how to reconcile divergent branches" or, depending on the user's pull.rebase, silently rebases — which contradicts this skill's deliberate no-rebase stance. Specify git pull --ff-only and say what to do when it is not a fast-forward.

[zoolsher]

🔁 This review has been superseded. See the latest review.

[Jessie-QingYu]

🔁 This review has been superseded. See the latest review.

[zhangfand]

Third round, on top of the rewrite that dropped the step-by-step commands:

• A check the base branch fails the same way now satisfies the CI done condition, so that state terminates.
• A conflict-merge tick ends after its push; review and CI fixes share the next tick's single push.
• A flake rerun is not a fix attempt.
• Quiet ticks are bounded: an hour with nothing to do hands back to the user.
• Counters live on the PR (babysitting commits, replies that cover a finding), so a watcher-woken session can recover them.
• The merge commit message records each conflict resolution.
• Review feedback is every finding not yet answered, not a time window, so a conflict tick cannot drop a round.

Declined naming the gh fields and bucket values: the skill states responsibilities rather than commands by design, and the check condition is phrased as "no check failed" so skipped checks do not block it.

[zoolsher]

🔁 This review has been superseded. See the latest review.

[Jessie-QingYu]

🔁 This review has been superseded. See the latest review.

[zhangfand]

Cut the skill down to its responsibilities: review feedback, CI failures the diff caused, and merge conflicts, with the done condition. The procedural detail the earlier rounds were tightening is gone, since the agent already knows how to merge a branch or read a CI log. Merging stays with the user.

[zoolsher]

Code Review: ✅ APPROVE

This PR updates a single skill definition file (.claude/skills/babysit-pr/SKILL.md), a documentation-only change with no executable code. It broadens the babysit-pr skill so that, in addition to answering review feedback via respond-to-review, the agent also fixes CI failures the diff caused and resolves merge conflicts with the base branch, and it redefines "done" as reviewed-clean + CI-green + merges-cleanly. Merging remains a user action, and an explicit "stop and ask" escape hatch is added for decisions that need the user.

The change is consistent with the sibling respond-to-review skill's philosophy (scope fixes to what the diff introduced, keep the diff small) and with the repo's conventional-commit title check. The prose is clear and the done condition is a sensible generalization of the prior one. I found no correctness or design issues; one minor tension between the scoped CI mandate and the absolute done condition is noted below as optional.

Verdict: APPROVE — A well-scoped, internally consistent docs change to a skill file that correctly expands babysit-pr's responsibilities and done condition; no bugs or design problems.

1 finding(s) posted as inline comments below.

Severity	Category	File	Title
P3	design	.claude/skills/babysit-pr/SKILL.md	Scoped CI mandate vs. absolute "CI green" done condition

---

Automated review by RomeOS Code Review · commit e3f7000

[zoolsher]

[P3] design — Scoped CI mandate vs. absolute "CI green" done condition

The job is scoped to "fix CI failures the diff caused" (line 10), but the termination condition requires "CI is green" absolutely (line 12). If a check is red for a reason the diff did not cause (a flaky job, or a pre-existing failure on the base branch), the agent cannot both stay in scope and reach the done condition, so the loop could never terminate on its own. The added "When something needs the user's decision, stop and ask" clause largely covers this, but consider making it explicit that a red check the diff did not cause is one such stop-and-ask case, so the agent doesn't spin indefinitely trying to green a failure outside its remit.

[Jessie-QingYu]

Code Review: 💬 COMMENT

This PR widens the babysit-pr skill from "watch review feedback" to "keep the PR mergeable", adding CI failures and base-branch merge conflicts to the job and making the done condition a conjunction of reviewed-clean, green, and conflict-free. The direction is right: an approval on a red or conflicted PR was a false finish, and keeping the actual merge with the user is the correct boundary. The file is the only change, and no other doc or script references the skill, so there is no ripple to verify beyond respond-to-review, which the skill still delegates to correctly.\n\nMy concerns are all about this being a runbook an agent executes unattended, where the prose is the whole contract. The new stop conditions are read from GitHub but never defined against the states GitHub actually returns — this repo's own release-rome-image skill documents that CI cancels superseded runs, and mergeable is UNKNOWN until computed. Paragraph one scopes CI work to "failures the diff caused" while paragraph two demands green, which leaves the flaky or red-main case with no exit. The loop also lost its "since the last tick" delta boundary and gained two conditions that other people's merges can re-break, with no round cap. Most importantly, "resolve merge conflicts" is handed to the agent with no invariant: a blanket --ours resolution reverts base-branch commits into a branch that then squash-merges onto main. None of these need the skill to grow into a command list — each is one clause on the stop condition or the invariant, which is the part the skill does own.

Verdict: COMMENT — The direction is correct and the change is docs-only, but the new autonomous responsibilities ship without a conflict-resolution invariant, a definition of "green"/"merges cleanly", or a loop bound — gaps worth closing before an agent runs this unattended.

6 finding(s) posted as inline comments below.

Severity	Category	File	Title
P1	architecture	.claude/skills/babysit-pr/SKILL.md	Autonomous conflict resolution has no stated invariant or mechanism
P2	design	.claude/skills/babysit-pr/SKILL.md	"CI failures the diff caused" cannot satisfy a "CI is green" done condition
P2	design	.claude/skills/babysit-pr/SKILL.md	"CI is green" and "merges cleanly" are undefined against states this repo actually produces
P2	code-quality	.claude/skills/babysit-pr/SKILL.md	The tick paragraph lost the definition of "new"
P2	error-handling	.claude/skills/babysit-pr/SKILL.md	The three-part done condition has no bound, and the watcher now wakes on the agent's own pushes
P3	code-quality	.claude/skills/babysit-pr/SKILL.md	Four phrasings for one done condition

---

Automated review by RomeOS Code Review · commit e3f7000

[Jessie-QingYu]

[P1] architecture — Autonomous conflict resolution has no stated invariant or mechanism

The skill now tells an unattended agent to "resolve merge conflicts with the base branch" and then says the how is left to the agent. Conflict resolution is the one step here that can silently destroy work that is not the PR's: a wholesale git checkout --ours or git merge -X ours resolves every conflict and reverts other people's main commits into the branch, which then squash-merges back onto main. The mechanism also matters and is unstated — this repo squash-merges (docs/authoring/prs.md), so merging origin/main into the branch is cheap and safe, while a rebase plus force-push detaches in-flight review comments the same loop is trying to answer.

Name the invariant and the mechanism in one clause, e.g.: "Resolve a conflict by merging the base branch into the PR branch, never by force-pushing a rebase. The resolution keeps both sides' intent — dropping a base-branch change is a defect, not a resolution — and the verification commands in CLAUDE.md run green afterward."

[Jessie-QingYu]

[P2] design — "CI failures the diff caused" cannot satisfy a "CI is green" done condition

Paragraph one scopes the job to "CI failures the diff caused", but paragraph two makes "CI is green" a condition for stopping. A flake, a red main, or an infrastructure failure is out of scope to fix yet blocks the stop condition, so the loop has no defined exit for the most common red-CI case. The frontmatter description drops the qualifier entirely ("fix failing CI"), so the description and the body state two different contracts. Route the non-diff failure explicitly — rerun once for a suspected flake, otherwise stop and report — and align the description with whichever scope wins.

[Jessie-QingYu]

[P2] design — "CI is green" and "merges cleanly" are undefined against states this repo actually produces

Both new stop conditions are read from GitHub, and both have a known non-binary state that this skill leaves the agent to guess at. release-rome-image/SKILL.md already documents that CI cancels superseded runs, so a rapid series of pushes leaves cancelled checks that are neither green nor a real failure — treating them as a failure sends the agent fixing nothing, treating them as green stops the loop on an unverified head. Likewise gh pr view --json mergeable returns UNKNOWN until GitHub computes mergeability, and both readings of UNKNOWN are wrong. One sentence fixes it: green means every check for the current head SHA reports success or skipped, and pending, cancelled, or UNKNOWN mean not yet decided — tick again rather than act.

[Jessie-QingYu]

[P2] code-quality — The tick paragraph lost the definition of "new"

The old tick step said to "read the reviews and comments posted since the last tick and run respond-to-review on the new findings". "Each tick, handle whatever is new" drops both the delta boundary and the hand-off to respond-to-review at the point of use. respond-to-review posts exactly one summary reply per round (its Phase 4), so an agent without the "since the last tick" boundary can re-answer settled findings and post duplicate replies on the same round. Keep the boundary in the tick sentence, now widened to the other two inputs: "Each tick, read the reviews, comments, and check runs posted since the last tick, and handle only those."

[Jessie-QingYu]

[P2] error-handling — The three-part done condition has no bound, and the watcher now wakes on the agent's own pushes

The old loop terminated on an event nothing external could undo (approval). Two of the three new conditions depend on the base branch, which other people keep moving: resolving a conflict, pushing, and having main advance again puts the loop straight back to not-done, with no cap on rounds or wall-clock time. The widened watcher ("a review or check lands") also fires on every check run of the agent's own push, so each fix restarts the cycle. Add a stop rule so the loop hands back instead of running indefinitely, e.g. "After three rounds that end not-done, or when the base branch has moved under an already-resolved conflict, stop and report the state to the user."

[Jessie-QingYu]

[P3] code-quality — Four phrasings for one done condition

The same end state is named four ways across twelve lines: "keep it mergeable", "reviewed clean, green, and conflict-free" (frontmatter), "keep it ready to merge" (line 10), and "approved or a round raises no new points, CI is green, and the branch merges cleanly" (line 12). docs/authoring/WRITING.md asks for one name per thing, and here the variation costs more than style — a reader cannot tell whether "mergeable" is shorthand for the three-part condition or a fourth requirement (GitHub's MERGEABLE). Pick one phrase for the state and one canonical spelling of its three parts, then reuse both. The two new sentences also run about 35 and 48 words against WRITING.md's 25-word flag, and splitting them would make the reuse natural.

[zhangfand]

Declining this round. The findings ask the skill to spell out mechanism — how to merge without dropping base-branch changes, which check states count as green, tick boundaries and round caps — and the skill states responsibilities, not procedure the agent already knows. A red check the diff did not cause falls under "when something needs the user's decision, stop and ask."