fix(source-control): assert babysit worktree HEAD == PR head before mutate; merge-only, upstream refspec push

[kyle-sexton]

Summary

Fixes #548. A babysit PR worker can be assigned a git worktree in detached HEAD — because the PR branch is already checked out in a sibling/foreign worktree — or on a stale local branch tip behind the PR head. The checkout/freshness mechanics then merged and pushed from that tip, so a stale-tip integration could silently revert the newest branch commit — a near-miss where safety depended on the assigned HEAD happening to match, not on a guard.

The initial reviewer pass confirmed the safety.md contract text was sound but raised a CRITICAL: the actual worker mechanics (loop.md/SKILL.md/orchestration.md) were unreconciled, so an agent following the literal steps still hit the failure. This PR fixes the contract and the mechanics that implement it.

The fix

• reference/safety.md Checkout And Push Invariants — assert the assigned worktree's HEAD equals the true PR head (gh pr view <N> --json headRefOid; equal to origin/<headRefName> for a same-repo PR) before any merge/edit/push; stop on a stale/detached mismatch. Push by explicit refspec to the branch's configured upstream git push "$(git config --get branch.<headRefName>.remote)" HEAD:<headRefName> (fast-forward by construction, never --force/--force-with-lease). The reuse rule is reconciled so it permits the detached-HEAD path under the same assertion (no self-contradiction).
• reference/loop.md §5.1.2 — acquire the head via gh pr checkout (heals a behind-origin local branch; --detach for a sibling-locked branch; resolves fork PRs), assert HEAD == PR_HEAD (the live headRefOid) in every checkout path, degrade to read-only on mismatch. SKILL.md Step 0.2 + the cross-tier invariants and reference/orchestration.md's conflict-worker follow the same assertion + refspec push. worktrees.md gets a one-line pointer (not a duplicate).
• Freshness is now merge-only. The prior loop.md path rebased-and---force-with-leased linear-history branches — which both violated the skill's own never-force-push invariant (safety.md "Never Do Automatically", orchestration.md) and was the silent-revert vector. Behind-default branches now always integrate via git merge + a fast-forward refspec push. Behavior change: linear-history branches now carry an interim merge commit during freshness instead of being rebased (the final squash merge still flattens history).

Scope decisions (called out for review)

• Fork/cross-repo heads (Option B — upstream-aware push). The push targets the branch's configured upstream — git push "$(git config --get branch.<headRefName>.remote)" HEAD:<headRefName> — which resolves to origin for a same-repo head and to the fork's remote for a write-allowed in-owner fork head (a real supported case: branch_write_allowed=true for cross-repo heads under <watched-owners>). An earlier revision hardcoded origin, which the re-review flagged as a regression that would silently write a same-named branch on the base repo for a fork PR; this fixes it while keeping the origin refspec for same-repo heads. An external-fork head outside watched owners remains safety.md's read-only stop-and-ask case. The assertion uses the live headRefOid, correct for any PR type.
• Enforcement stays agent discipline. Whether the head assertion belongs in a deterministic push-safety guard (rather than prose the worker follows) is filed as follow-up source-control:babysit-prs: promote the assigned-worktree HEAD==origin/<pr-branch> assertion from prose to a deterministic push-safety guard #885.

Verification (local gates)

• check-skill-portability.sh origin/main — PASS (5 skill files).
• check-changelog-parity.sh --check-bump origin/main — PASS (0.15.10 entry present).
• markdownlint-cli2 on the changed docs — clean; SKILL.md 499/500 lines; evals.json (id 6 updated to the merge-only + upstream-refspec contract) valid; plugin.json valid.
• Python unittest suite unaffected (no code changed) — 339 tests OK.
• Two independent fresh-context review rounds: round 1 raised a CRITICAL (the mechanics docs were unreconciled with the safety.md contract) — fixed by reconciling loop.md/SKILL.md/orchestration.md; round 2 raised a CRITICAL (hardcoded-origin push broke in-owner forks) — fixed by Option B (upstream-aware push).

Version

Per-plugin bump 0.15.9 → 0.15.10 (patch). marketplace.json pins no version for source-control, so only plugin.json changes.

Ordering note for the merge lane: my open PR #882 (#511) bumps source-control to 0.16.0 and is unmerged. If #882 merges first, re-bump this above 0.16.0 (→ 0.16.1) during recompose — do not keep 0.15.10.

Closes #548

Related

• source-control:babysit-prs: promote the assigned-worktree HEAD==origin/<pr-branch> assertion from prose to a deterministic push-safety guard #885 — follow-up (filed): promote the head assertion from prose to a deterministic push-safety guard.
• source-control:babysit-prs: worker-scope per-PR lease did not prevent a concurrent worker on the same PR (duplicate P1 fixes) #524 — worker-lease cross-session exclusivity (distinct: which session may work a PR; this is the git mechanics of the assigned worktree).

🤖 Generated with Claude Code

[claude]

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

---

PR Review

• Gather context and understand request
• Read changed files and git diff
• Review safety.md — invariant correctness and completeness
• Review loop.md — checkout/assertion/push mechanics
• Review SKILL.md — Step 0.2 + cross-tier invariants
• Review orchestration.md — conflict-worker assertion + refspec push
• Review worktrees.md — pointer accuracy
• Review evals.json — eval id 6 update
• Review plugin.json and CHANGELOG.md
• Synthesize findings and post final review

---

The fix addresses a genuine and well-characterized hazard. The safety.md contract language is sound, the orchestration.md conflict-worker update is consistent, worktrees.md gets the right one-liner, evals.json id 6 is correct, and the CHANGELOG is comprehensive. I found two issues that need resolution before merge and one minor grammar nit.

---

Finding 1 — HIGH: SKILL.md Step 0.3 still says "merge vs rebase per the branch's own history"

The cross-tier invariants (updated by this PR) now say:

never a force-push (freshness is merge-only, refspec-pushed fast-forward — reference/loop.md)

But the per-PR checklist Step 0.3 still reads:

Step 0.3 — Branch freshness: fetch + git merge-base --is-ancestor; integrate (merge vs rebase per the branch's own history), graduated conflict handling (§5.1.2)

These two statements within the same file directly contradict each other. An agent following the checklist literally would still see "merge vs rebase" as valid and could choose to rebase — which is exactly the silent-revert vector this PR is fixing. The cross-tier invariants section is normative, but the checklist is what workers execute step-by-step.

Line: SKILL.md:343

Fix: Change the parenthetical to (merge-only — never rebase, §5.1.2) or (git merge only — rebase is forbidden, §5.1.2).

Fix this →

---

Finding 2 — MEDIUM: loop.md §5.1.4 fix-cycle push not updated for detached HEAD

The freshness push (§5.1.2) was correctly updated to use an explicit refspec:

git push "$PUSH_REMOTE" "HEAD:$BRANCH"

But the fix cycle at §5.1.4 still uses a plain git push:

- [ ] `git push`

In the sibling-locked path (gh pr checkout "$PR_NUMBER" --detach), the worktree is in detached HEAD while CHECKOUT_MODE="full" — so the fix cycle runs, commits succeed, then git push without a refspec fails with "fatal: You are not currently on a branch." The freshness push was updated to handle this, but the fix cycle push was not.

Line: loop.md:304

Fix: Change the fix-cycle push step to use the same refspec form and document the variable from §5.1.2:

git push "$PUSH_REMOTE" "HEAD:$BRANCH"   # PUSH_REMOTE from §5.1.2 — works in detached HEAD

Fix this →

---

Finding 3 — LOW: safety.md grammar — orphaned clause after colon on the hardcoded-origin sentence

Line 53 reads:

Never hardcode origin: for a fork head that silently writes a same-named branch on the base repo instead of updating the fork head.

The colon introduces a subordinate clause with no main verb, making the sentence syntactically incomplete. A reader looking for a warning about what the prohibition prevents has to parse the sentence twice. Suggested rewrite:

Never hardcode origin — for a fork PR, hardcoding origin silently writes to a same-named branch on the base repo instead of updating the fork head.

Line: safety.md:53

---

What looks good

• safety.md Checkout And Push Invariants — the head assertion logic is well-specified and covers all three worktree states (branch current, detached at PR head, stale local tip). The reuse rule is now consistent with the detached-HEAD path. The non-fast-forward rejection clause ("re-fetch and stop, never force past it") closes the gap correctly.
• loop.md §5.1.2 checkout logic — the three-branch decision tree (HEAD == PR_HEAD already; sibling-locked --detach; heal-via-checkout) correctly covers same-repo, fork, and detached-HEAD scenarios. PUSH_REMOTE=$(git config --get "branch.$BRANCH.remote" || echo origin) is a correct fallback pattern.
• orchestration.md conflict-worker — the added head assertion + refspec push is consistent with safety.md and the fetch-first invariant was already present.
• worktrees.md — one-line pointer to safety.md without duplicating rules. Correct.
• evals.json id 6 — now correctly tests for merge-only + upstream-refspec push behavior.
• Version bump and CHANGELOG — 0.15.9 → 0.15.10 is the right patch increment; CHANGELOG accurately describes all three sub-changes.
• Ordering note is correctly surfaced in the PR description for the merge lane.

---

Bottom line: Two issues block clean merge. Finding 1 (SKILL.md Step 0.3) creates an internal self-contradiction on the exact behavior this PR sets out to fix. Finding 2 (fix-cycle git push) leaves the detached-HEAD commit push broken in the scenario the PR explicitly enables. Both are small targeted text changes.

[claude]

Claude finished @kyle-sexton's task in 4m 16s —— View job

---

Security Review — PR #895

• Gather context and understand request
• Read PR diff and changed files
• Analyze git command injection surfaces
• Analyze authorization / access-control gaps
• Analyze token/credential handling
• Analyze push safety mechanics
• Post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Findings are about security properties of the instructions given to AI agents — the agent treats them as a behavioral specification, so logic gaps here are as real as bugs in code.

---

IMPORTANT — || echo origin fallback breaks the fork-push fix for sibling-locked branches

Confidence: HIGH

loop.md §5.1.2, detach path — lines ~192–193

When a PR branch is locked in a sibling worktree, the code calls gh pr checkout "$PR_NUMBER" --detach, landing the worktree in detached HEAD mode. Detached HEAD creates no local branch tracking entry — so the immediately subsequent:

PUSH_REMOTE=$(git config --get "branch.$BRANCH.remote" || echo origin)
git push "$PUSH_REMOTE" "HEAD:$BRANCH"

will always have git config --get "branch.$BRANCH.remote" return exit 1 (key does not exist in a detached-HEAD worktree), and PUSH_REMOTE silently becomes origin.

For a same-repo PR in this path, origin is correct.

For an in-owner fork PR in this path, origin is the base repository — exactly the silent wrong-destination push the PR explicitly set out to fix. safety.md states: "Never hardcode origin: for a fork head that silently writes a same-named branch on the base repo instead of updating the fork head." The || echo origin fallback reintroduces this behavior in the sibling-locked case.

The non-detach path (gh pr checkout "$PR_NUMBER" without --detach) correctly configures local branch tracking, so git config --get "branch.$BRANCH.remote" returns the fork remote there. Only the --detach branch has the gap.

Scenario: PR is from an in-owner fork (branch_write_allowed=true, headRepository is the fork). The fork branch happens to be checked out in a sibling worktree (the grep fires). gh pr checkout --detach checks out the fork's head commit in detached HEAD. HEAD assertion passes. Freshness merge runs, PUSH_REMOTE=origin (fallback), git push origin HEAD:<fork-branch-name> creates or updates a branch of the same name on the base repo rather than the fork — a write to the wrong repository.

Suggested fix: after gh pr checkout --detach for a fork PR, resolve the push remote from the PR's head repository rather than relying on local branch config. One approach:

# After gh pr checkout --detach
FORK_REMOTE=$(gh pr view "$PR_NUMBER" --json headRepository -q .headRepository.url \
  | xargs -I{} git remote -v | grep '{}' | awk '{print $1}' | head -1)
PUSH_REMOTE=${FORK_REMOTE:-origin}

Or more reliably, ask gh for the head repo owner and find the matching remote name via git remote -v. Alternatively, note in the spec that the --detach path is same-repo only and block the mutation on a fork PR in that path (stop-and-ask).

---

SUGGESTION — Unescaped branch name used as basic regex in sibling-worktree detection

Confidence: HIGH

loop.md §5.1.2 — line ~159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is the PR's headRefName. The \[…\] wrapper escapes the square brackets, but the content of $BRANCH is unescaped basic-regex. A branch name containing ., *, \, [, or ] makes the pattern incorrect:

• feat/my.thing → pattern \[feat/my.thing\] — . matches any character; [feat/myXthing] would also match.
• A crafted name could produce a false positive, routing the code into the --detach path when the branch is not actually locked. The HEAD assertion after gh pr checkout --detach provides a safety net and degrades to read-only on mismatch, limiting practical impact, but the detection is still fragile.

Fix: use fixed-string search: grep -qF "[$BRANCH]" (note: -F makes the entire pattern literal, so the surrounding [ and ] are matched literally too — this is the intended match against git worktree list's [branchname] output format).

---

No findings for

• Prompt injection — the worker prompt template correctly places all PR-derived content (title, check names, blocker strings) inside a clearly fenced BEGIN QUOTED PR DATA / END QUOTED PR DATA section, explicitly labeled untrusted.
• Token/credential handling — the GH_TOKEN=<approver-bot-token> pattern in the autopilot merge example is the standard secure CLI approach; no plaintext secrets are stored or emitted.
• Force-push removal — removing the --force-with-lease rebase path is a security improvement; the replacement (merge + fast-forward refspec push) is strictly weaker in privilege and correct by construction given the HEAD assertion.
• GitHub Actions — no workflow files changed; no pull_request_target, workflow_run, or run: injection surfaces.
• Scope boundaries — the external-fork stop-and-ask boundary, the mutation_policy.branch_write_allowed gate, and the head-ref uniqueness guard are all preserved or strengthened.
• Autopilot merge tier — the --autopilot-merge-tier fail-closed semantics and the distinct-identity approval requirement are unchanged and remain correct.

---

The one IMPORTANT finding (fork-in-detach push destination) is a real gap where the fix's own stated invariant ("never hardcode origin") is violated in the narrow sibling-worktree × in-owner-fork case. Fix this →

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f471b63640

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 1d7a992)

• Read the new commit diff against the previous review commit
• Check if Finding 1 (SKILL.md Step 0.3 "merge vs rebase") was addressed
• Check if Finding 2 (loop.md §5.1.4 fix-cycle plain git push) was addressed
• Check if Finding 3 (safety.md grammar nit) was addressed
• Check if Security IMPORTANT (|| echo origin fallback in detach path) was addressed
• Check if Security SUGGESTION (unescaped branch regex) was addressed
• Check if Codex P2 (PUSH_REMOTE uninitialized in conflict path) was addressed
• Synthesize and post final review

---

The new commit (1d7a992 — fail-closed push-remote resolution for babysit push paths) addressed most of the prior findings. Status breakdown:

✅ Addressed

Security IMPORTANT — || echo origin fallback (all reviewers): Fully fixed. The inline || echo origin is gone. PUSH_REMOTE is now resolved once at the top of the full-mode block via an explicit isCrossRepository gate — same-repo heads get origin unconditionally; fork heads read git config --get "branch.$BRANCH.remote" and degrade to read-only on empty, ., or origin instead of falling back to the base repo. The comment in the code correctly explains why.

# loop.md lines 182–193
if [ "$(gh pr view "$PR_NUMBER" --json isCrossRepository -q .isCrossRepository)" = "false" ]; then
  PUSH_REMOTE=origin
else
  PUSH_REMOTE=$(git config --get "branch.$BRANCH.remote" || true)
  case "$PUSH_REMOTE" in
    ""|.|origin)
      echo "Fork head with no validated fork remote (detached or unmodifiable) — read-only"
      CHECKOUT_MODE="read-only" ;;
  esac
fi

Finding 2 (HIGH) — loop.md §5.1.4 fix-cycle git push: Fixed. The bare git push step is now git push "$PUSH_REMOTE" HEAD:$BRANCH with a note that PUSH_REMOTE comes from the pre-resolved block. The cross-reference to §5.1.2 is explicit.

Codex P2 — PUSH_REMOTE uninitialized in conflict path: Fixed as a side-effect of the single pre-resolution block. All three push sites (freshness merge, conflict-continue, fix-cycle) now read the same $PUSH_REMOTE variable, which is guaranteed set before any of them run.

Finding 3 (LOW) — safety.md grammar: Fixed. The orphaned-clause sentence was rewritten into a grammatically complete explanation of the fail-closed rule.

---

❌ Still open — one blocker

Finding 1 (HIGH) — SKILL.md Step 0.3 still says "merge vs rebase per the branch's own history"

SKILL.md line 343 still reads:

- [ ] **Step 0.3 — Branch freshness:** fetch + `git merge-base --is-ancestor`; integrate
  (merge vs rebase per the branch's own history), graduated conflict handling (§5.1.2)

Every other location — the cross-tier invariants in SKILL.md itself (line 118), loop.md, orchestration.md, safety.md — now says merge-only. The per-PR checklist at Step 0.3 still contradicts them. An agent executing the checklist step-by-step would still see "merge vs rebase" as a valid choice, which is the silent-revert vector this PR was written to close.

The new commit added prose and code for the fail-closed PUSH_REMOTE block but did not touch this line. It remains the single remaining blocker.

Fix: Change the parenthetical on line 343 from (merge vs rebase per the branch's own history) to (merge-only — never rebase, §5.1.2).

Fix this →

---

Low — Security SUGGESTION (unescaped branch regex) — unchanged, low impact

loop.md line 159 still uses:

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as an unescaped basic-regex, so . in a branch name matches any character and could produce a false positive (routing into --detach when the branch isn't actually locked). The HEAD assertion after gh pr checkout --detach degrades to read-only on mismatch, so the worst outcome is an unnecessary read-only cycle rather than a wrong write. Acceptable to merge with this open but worth noting; the correct fix is grep -qF "[$BRANCH]" (fixed-string match). Not a blocker.

---

Minor observation — isCrossRepository API call could cause unnecessary read-only on network failure

If gh pr view "$PR_NUMBER" --json isCrossRepository fails (rate-limit, transient network error), the substitution returns "", which doesn't equal "false", so the else branch runs. A same-repo PR in this path gets branch.$BRANCH.remote = origin (from a normal gh pr checkout checkout), which is caught by the case ""|.|origin) block and the worker degrades to read-only. This is strictly fail-closed (safe) but may cause confusing silent read-only cycles on transient API errors for same-repo PRs. Not a blocker; a retry or error-message on a non-zero exit code from the gh pr view call would sharpen the diagnostic.

---

Bottom line

One blocker remains: SKILL.md Step 0.3 line 343. It is a one-word parenthetical change. Everything else from the prior two review rounds is resolved. The fail-closed PUSH_REMOTE block in 1d7a992 is the right design — the logic is correct, the comment is clear, and all three push sites consume it properly.

[claude]

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

---

Security Review — PR #895 (head: 1d7a992)

• Gather context and understand request
• Read PR diff (all commits: f471b63, 576aa0d, 1d7a992)
• Review loop.md — PUSH_REMOTE resolution in all paths
• Review SKILL.md — Step 0.3 contradiction check
• Review safety.md, orchestration.md, worktrees.md
• Synthesize findings

---

All changes are prose/documentation (markdown + JSON). Security findings apply to the behavioral spec these documents encode for AI agents — logic gaps in agent instructions are as real as code bugs.

---

Status of Prior Review Findings

Commit 1d7a992 ("fail-closed push-remote resolution") directly addresses the || echo origin IMPORTANT finding from the previous security review. Here is the full disposition:

Prior finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED — replaced with || true; empty/./origin all hit case → read-only
SUGGESTION — unescaped $BRANCH in sibling-worktree grep regex	❌ Not fixed (still present, see below)
PR review Finding 1 (HIGH) — SKILL.md Step 0.3 contradicts cross-tier invariants	❌ Not fixed (see below)
PR review Finding 2 (MEDIUM) — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED — now git push "$PUSH_REMOTE" HEAD:$BRANCH
PR review Finding 3 (LOW) — grammar/orphaned clause in safety.md line 53	✅ FIXED — sentence rewritten correctly

---

IMPORTANT — SKILL.md Step 0.3 still says "merge vs rebase per the branch's own history"

Confidence: HIGH

SKILL.md lines 342–343

The cross-tier invariants block (updated by this PR, line 117–118) now reads:

never a force-push (freshness is merge-only, refspec-pushed fast-forward — reference/loop.md)

But the per-PR checklist Step 0.3 that workers execute step-by-step still reads:

Step 0.3 — Branch freshness: fetch + git merge-base --is-ancestor; integrate (merge vs rebase per the branch's own history), graduated conflict handling (§5.1.2)

These two statements in the same file directly contradict each other. An agent following the checklist literally still sees "merge vs rebase" as valid and may choose to rebase, which is the silent-revert vector the PR is designed to close. Commit 1d7a992 updated every other mention but left Step 0.3 untouched.

Fix: Change the Step 0.3 parenthetical to (merge-only — never rebase, §5.1.2).

Fix this →

---

SUGGESTION — Unescaped $BRANCH used as unanchored basic-regex in sibling-worktree detection

Confidence: HIGH

loop.md line 159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is the PR's headRefName. The surrounding \[…\] escapes the square brackets, but the branch content is unescaped basic-regex: a branch like feat/my.thing makes . match any character, and a crafted name could trigger a false positive that routes into the --detach path when the branch isn't actually locked.

Practical blast radius is contained by the HEAD assertion immediately after ([ "$(git rev-parse HEAD)" = "$PR_HEAD" ]), which degrades to read-only on a false-positive path — so no mutation occurs. The new fail-closed PUSH_REMOTE block also catches the fork-in-detach case even if the grep misfires. Not blocking, but the pattern is fragile.

Fix: use -F (fixed-string) for the second grep:

elif git worktree list | grep -vF "$CUR_WT " | grep -qF "[$BRANCH]"; then

---

Codex findings — disposition

• Codex P1 (loop.md:165 — refspec push in detached-HEAD fix cycle): Fixed in this PR — §5.1.4 now uses git push "$PUSH_REMOTE" HEAD:$BRANCH.
• Codex P1 (fail-closed instead of origin fallback): Fixed by commit 1d7a992.
• Codex P2 (PUSH_REMOTE uninitialized for conflict push): False positive. $PUSH_REMOTE is resolved in the first if [ "$CHECKOUT_MODE" = "full" ] block (lines 182–193), before the freshness block (lines 200+) that uses it. The conflict-continuation comment at line 232–234 correctly references $PUSH_REMOTE from that pre-resolved value.

---

What looks good in 1d7a992

The fail-closed PUSH_REMOTE resolution is well-structured:

• Same-repo (isCrossRepository=false): hardcodes origin — correct, no branch config needed.
• Fork (isCrossRepository=true): uses git config --get "branch.$BRANCH.remote" || true — empty string on failure (detached HEAD or unconfigured), then case "$PUSH_REMOTE" in ""|.|origin) → CHECKOUT_MODE=read-only. This correctly blocks the cross-repo write regression for the sibling-locked fork path, replacing the prior || echo origin that silently pushed to the base repo. An isCrossRepository query returning anything other than "false" also takes the fork/conservative path — fail-closed.
• $PUSH_REMOTE is resolved once and shared across freshness push, conflict-continue push, and fix-cycle push — consistent throughout.

---

Bottom line: The IMPORTANT || echo origin regression from the previous round is fixed. One IMPORTANT gap remains: SKILL.md Step 0.3 still contradicts the merge-only contract at the one place workers follow step-by-step — this is a small targeted text change that should land before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d7a992dc6

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 180793c)

• Read new commit diff against prior head (1d7a992)
• Check if SKILL.md Step 0.3 "merge vs rebase" contradiction was fixed
• Check Codex P2 — orchestration.md conflict-worker push remote resolution
• Check Codex P2 — loop.md read-only per-finding processing contradiction
• Scan for any new issues introduced
• Synthesize and post final review

---

Commit 180793c ("reconcile conflict-worker + safety contract to fail-closed push remote") made targeted fixes to orchestration.md and safety.md. Here is the full disposition after this round.

---

✅ Addressed by 180793c

Codex P2 — orchestration.md conflict-worker push remote

The inline shell form git push "$(git config --get branch.<headRefName>.remote)" HEAD:<headRefName> was replaced with git push "$PUSH_REMOTE" HEAD:<headRefName>, with an explicit cross-reference to the fail-closed resolution contract in reference/safety.md. The prose now correctly describes PUSH_REMOTE as resolving fail-closed: origin for same-repo, fork remote for cross-repo, and read-only stop when the fork remote is unresolved. Consistent with loop.md and safety.md.

safety.md PUSH_REMOTE authority clarification

The change adds that same-repo vs fork is determined by gh pr view --json isCrossRepository — not by whether git config happens to resolve a value — and guards the fork-remote value against empty, ., or literally origin. This correctly closes the gap where a detached-HEAD fork worktree with no branch config would silently fall back to origin.

---

❌ Still open — one blocker

SKILL.md Step 0.3 still says "merge vs rebase per the branch's own history"

SKILL.md line 343 — unchanged through all four commits:

- [ ] **Step 0.3 — Branch freshness:** fetch + `git merge-base --is-ancestor`; integrate
  (merge vs rebase per the branch's own history), graduated conflict handling (§5.1.2)

Every other location now says merge-only:

• SKILL.md cross-tier invariants (line 118): "freshness is merge-only, refspec-pushed fast-forward"
• loop.md §5.1.2: merge block, no rebase path
• orchestration.md: "Resolve with git merge origin/<base-branch> into the PR branch. This is deliberate: a rebase rewrites..."
• safety.md: never-force-push, merge-only construct

Step 0.3 is the per-PR checklist item a worker executes step-by-step. An agent following it still sees "merge vs rebase per the branch's own history" as a valid choice and may rebase — the exact silent-revert vector this PR closes. The cross-tier invariants section is normative, but the checklist is what the worker runs.

This is a one-word parenthetical change. It has been flagged as HIGH/IMPORTANT in every review round (three times across two reviewers) and remains unaddressed.

Fix: change line 343 from (merge vs rebase per the branch's own history) to (merge-only — never rebase, §5.1.2).

Fix this →

---

LOW — §5.1.3 preamble ambiguity (introduced by this PR, non-blocking)

The origin/main baseline read:

"Must be on the PR branch (§5.1.2) before starting."

This PR changed it to:

"Must hold a full-mode checkout — HEAD asserted equal to the true PR head... — before starting."

The word "full-mode" is ambiguous here. The read-only contract just above (lines 268–276) explicitly says D1–D5 still run in read-only mode — the worker investigates, classifies, and replies even without write access. But the §5.1.3 preamble now says the checklist requires "full-mode" before it can start, which an agent following it literally could interpret as "skip §5.1.3 entirely in read-only mode" — forfeiting all D1–D5 investigation in exactly the safety-fallback cases where investigation still has value.

This is not a push-safety regression (a read-only agent can't write), but it undercuts the intent of the read-only contract. A cleaner rewrite: "Must hold a verified checkout — HEAD asserted equal to the true PR head (§5.1.2); full or read-only — before starting. D6–D7 are additionally gated on full mode."

Not blocking, but worth fixing in a follow-up before the next version bump to avoid silent read-only pass-throughs.

---

LOW — Unescaped branch name in sibling-worktree grep (unchanged, guarded)

loop.md line 159 still uses grep -q "\[$BRANCH\]" with $BRANCH as unescaped basic-regex. A . in a branch name could match the wrong worktree. The HEAD assertion immediately after degrades to read-only on mismatch, so the worst outcome is an unnecessary read-only cycle, not a wrong write. Not blocking.

Fix when convenient: grep -qF "[$BRANCH]".

---

Bottom line

180793c correctly reconciles orchestration.md and safety.md with the fail-closed PUSH_REMOTE contract. One blocker remains: SKILL.md line 343 — a single parenthetical that still says "merge vs rebase" while every other file in this PR says merge-only. It is the last unfixed item from the first review round.

[claude]

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

---

Security Review — PR #895 (head: 180793c)

• Gather context and understand request
• Read PR diff (git diff origin/main...HEAD)
• Review loop.md — PUSH_REMOTE resolution, sibling-worktree grep, conflict-worker push
• Review SKILL.md — Step 0.3 contradiction, §5.1.3 prerequisite
• Review orchestration.md — conflict-worker push remote resolution
• Review safety.md — invariant completeness
• Check for new attack surfaces in 180793c
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

Status of Prior Findings

Finding	Status
IMPORTANT — || echo origin sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep regex	❌ Not fixed
PR Review Finding 1 (HIGH) — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	❌ Not fixed
PR Review Finding 2 (MEDIUM) — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
PR Review Finding 3 (LOW) — grammar/orphaned clause in safety.md	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)

---

IMPORTANT — SKILL.md Step 0.3 still contradicts the merge-only invariant

Confidence: HIGH

SKILL.md line 343

The cross-tier invariants block (line 117–118, updated by this PR) now reads:

never a force-push (freshness is merge-only, refspec-pushed fast-forward — reference/loop.md)

Every other location in the PR — loop.md, orchestration.md, safety.md, and the evals.json eval — now asserts merge-only. But the per-PR checklist Step 0.3 that workers execute step-by-step still reads:

- [ ] **Step 0.3 — Branch freshness:** fetch + `git merge-base --is-ancestor`; integrate
  (merge vs rebase per the branch's own history), graduated conflict handling (§5.1.2)

This is a direct in-file self-contradiction. The cross-tier invariants are normative, but Step 0.3 is what a worker executes literally. An agent following the checklist sees "merge vs rebase" as a valid choice and can rebase — rewriting the branch's commit history, requiring a --force-with-lease or --force push, and opening the exact silent-revert vector this PR was written to close. 180793c updated orchestration.md but did not touch this line.

Fix: Change the parenthetical on line 343 from (merge vs rebase per the branch's own history) to (merge-only — never rebase, §5.1.2).

Fix this →

---

SUGGESTION — Unescaped $BRANCH in sibling-worktree grep (unchanged from prior pass)

Confidence: HIGH

loop.md line 159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is the PR's headRefName, interpolated unescaped into a basic-regex pattern. A branch name containing ., *, or [ causes metacharacter mismatches; feat/my.thing makes . match any character, creating a potential false-positive that routes into the --detach path when the branch is not actually locked. Practical blast radius is contained by the HEAD assertion immediately after ([ "$(git rev-parse HEAD)" = "$PR_HEAD" ]), which degrades to read-only on mismatch — so no unauthorized mutation occurs.

Fix: grep -qF "[$BRANCH]" (fixed-string match, brackets matched literally).

---

Findings confirmed fixed by this PR

• || echo origin fork-push regression (1d7a992) — the isCrossRepository gate with || true fallback and case ""|.|origin) → read-only correctly blocks the cross-repo write in the sibling-locked fork path. The fail-closed logic covers the detached-HEAD case where no branch config exists.
• Fix-cycle git push fails in detached HEAD (1d7a992) — §5.1.4 now uses git push "$PUSH_REMOTE" HEAD:$BRANCH.
• PUSH_REMOTE uninitialized for conflict push (1d7a992) — pre-resolved once before all three push sites.
• No new attack surfaces in 180793c — the final commit only updates orchestration.md prose: replacing an inline git config expansion with $PUSH_REMOTE and adding the fail-closed description. No new logic paths, no new shell invocations, no new API calls.

---

What looks good in the full PR

• The isCrossRepository gate design: same-repo heads unconditionally get origin; fork heads must resolve a remote from git config --get "branch.$BRANCH.remote", with empty/./origin all routing to read-only. Fail-closed on API failure (empty result ≠ "false" → takes the fork path → read-only if no branch config).
• PR_HEAD=$(gh pr view "$PR_NUMBER" --json headRefOid -q .headRefOid) failure (empty $PR_HEAD) correctly causes all post-checkout HEAD comparisons to return false → read-only. Fail-closed.
• safety.md Checkout And Push Invariants — the prose is now internally consistent and correctly covers detached HEAD, stale local tip, and fork remote resolution. The explicit "stop (read-only) instead" clause closes the earlier gap.
• orchestration.md conflict-worker — now correctly references $PUSH_REMOTE and delegates the resolution logic to safety.md rather than duplicating it inline with a potentially stale copy.

---

Bottom line: One IMPORTANT issue blocks clean merge — SKILL.md Step 0.3 line 343 still says "merge vs rebase per the branch's own history," directly contradicting the merge-only invariant now enforced everywhere else. It is a one-word parenthetical change. All other findings from prior review rounds are resolved. 180793c introduced no new security issues.

[claude]

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

---

PR Review — Follow-up Pass (commit d2b62d6)

• Gather context and understand request
• Read new commit diff against prior head (180793c)
• Check if SKILL.md Step 0.3 "merge vs rebase" contradiction was fixed
• Check if §5.1.3 read-only prerequisite ambiguity was addressed
• Check for any new issues introduced
• Synthesize and post final review

---

Commit d2b62d6 ("reconcile babysit checklists to merge-only + read-only D1-D5") addresses both remaining open findings from the prior review round. Changes are limited to two files.

✅ Both remaining open findings addressed

Finding 1 (HIGH) — SKILL.md Step 0.3 still said "merge vs rebase per the branch's own history"

SKILL.md line 343 now reads:

- [ ] **Step 0.3 — Branch freshness:** fetch + `git merge-base --is-ancestor`; integrate
  merge-only (never rebase — rebasing a PR branch needs a forbidden force-push), graduated conflict handling (§5.1.2)

This is better than the suggested minimal fix — it includes the why (force-push is forbidden), which makes the prohibition self-explanatory to a worker executing the checklist without needing to cross-reference. The contradiction with the cross-tier invariants, loop.md, orchestration.md, and safety.md is closed. ✅

LOW — §5.1.3 preamble ambiguity ("full-mode" could skip D1-D5 in read-only fallbacks)

loop.md §5.1.3 lines 282–285 now reads:

Must hold a HEAD-asserted checkout — HEAD equal to the true PR head (gh pr view --json headRefOid), on the branch or in detached HEAD (§5.1.2) — before starting. Read-only mode still runs D1-D5 (investigate/classify/reply); only the D6-D7 fix cycle requires full mode.

The prior "full-mode checkout" wording could be read as skipping D1-D5 in read-only safety-fallback cases. The rewrite correctly preserves investigation/reply in read-only mode and gates only the fix cycle on full mode. ✅

---

Complete finding disposition across all review rounds

Finding	Round	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	Security r1	✅ Fixed 1d7a992
Finding 2 (HIGH) — §5.1.4 fix-cycle git push fails in detached HEAD	PR r1	✅ Fixed 1d7a992
Finding 3 (LOW) — safety.md grammar/orphaned clause	PR r1	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	Codex	✅ Fixed 1d7a992
orchestration.md conflict-worker uses inline git config instead of $PUSH_REMOTE	Codex P2 r2	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	PR r3	✅ Fixed 180793c
Finding 1 (HIGH/IMPORTANT) — SKILL.md Step 0.3 says "merge vs rebase"	PR r1 / Security r1–r3	✅ Fixed d2b62d6
LOW — §5.1.3 preamble ambiguity ("full-mode" excludes D1-D5 in read-only)	PR r3	✅ Fixed d2b62d6
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	Security r1–r3	❌ Still present (non-blocking)
Minor — isCrossRepository API failure causes silent read-only for same-repo PRs	PR r3	❌ Still present (non-blocking)

---

Remaining non-blocking items

SUGGESTION (unchanged) — unescaped $BRANCH in sibling-worktree grep

loop.md line 159:

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH content is unescaped basic-regex; a . in a branch name could match the wrong worktree. The HEAD assertion immediately after degrades to read-only on mismatch — no write occurs on a false positive. Not a blocker; grep -qF "[$BRANCH]" would fix it.

---

Bottom line

No blockers remain. All HIGH and IMPORTANT findings from all prior review rounds are resolved. The SKILL.md Step 0.3 fix — the last remaining blocker flagged three times across two reviewers — is closed by d2b62d6. The only open items are a low-severity regex suggestion and a minor diagnostic observation, both confirmed non-blocking in the prior rounds.

The PR is ready to merge, subject to the ordering note in the PR description: if #882 merges first, re-bump above 0.16.0 (→ 0.16.1) during recompose.

[claude]

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

---

Security Review — PR #895 (head: d2b62d6)

• Read PR diff and changed files
• Review SKILL.md — Step 0.3 and §5.1.3 checklist fixes
• Review loop.md — push mechanics, PUSH_REMOTE resolution, branch regex
• Review orchestration.md, safety.md, worktrees.md
• Check for new attack surfaces introduced by latest commit
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
Codex P2 — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Not fixed (see below)

---

What d2b62d6 fixes

SKILL.md Step 0.3 — SKILL.md:343

The parenthetical now reads merge-only (never rebase — rebasing a PR branch needs a forbidden force-push). The self-contradiction that existed through every prior review round is closed. The per-PR checklist step that workers execute literally now matches the cross-tier invariants, loop.md, orchestration.md, and safety.md.

loop.md §5.1.3 preamble — loop.md:279-282

"Must hold a full-mode checkout" → "Must hold a HEAD-asserted checkout" with explicit "Read-only mode still runs D1-D5 (investigate/classify/reply); only the D6-D7 fix cycle requires full mode." The ambiguity that would have caused agents to skip D1-D5 investigation in the exact safety-fallback cases where investigation still has value is resolved. No new logic paths; no new shell invocations.

---

Remaining — SUGGESTION (unchanged from prior passes)

Unescaped $BRANCH in sibling-worktree grep

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH (the PR's headRefName) is interpolated as unescaped basic-regex. A branch name containing . makes . match any character; feat/my.thing would match [feat/myXthing] — a potential false positive routing into the --detach path when the branch is not actually locked. The HEAD assertion immediately after provides a safety net: [ "$(git rev-parse HEAD)" = "$PR_HEAD" ] degrades to read-only on any mismatch, so no unauthorized mutation occurs from a false-positive sibling-detection. The fail-closed PUSH_REMOTE block also catches the fork-in-detach case if the grep misfires. Not blocking.

Fix when convenient: grep -qF "[$BRANCH]" (fixed-string, brackets matched literally).

---

What looks correct across the full PR

• Checkout ordering — dirty-tree guard fires first (read-only before any checkout attempt), then HEAD == PR_HEAD (already current), then sibling-locked (--detach), then heal-via-checkout. Fail-closed at each branch.
• PUSH_REMOTE resolution — isCrossRepository=false → hardcoded origin; fork head → git config --get "branch.$BRANCH.remote" || true with case ""|.|origin) → read-only; isCrossRepository query failure (empty result) takes the fork/conservative path → read-only when no branch config. Fail-closed at every exit.
• All three push sites (freshness merge, conflict-continue, fix-cycle D6) consume the same pre-resolved $PUSH_REMOTE. No site reintroduces the inline git config expansion.
• PR_HEAD failure path — if gh pr view --json headRefOid returns empty, all subsequent HEAD comparisons return false → read-only. Fail-closed.
• No GitHub Actions changes — no pull_request_target, workflow_run, run: injection surfaces, or permission-widening changes.
• evals.json id 6 — correctly updated to test merge-only + upstream-refspec push behavior; expectations no longer reference the old merge-commit detection heuristic.

---

Bottom line: d2b62d6 closes the last IMPORTANT gap (SKILL.md Step 0.3 self-contradiction and §5.1.3 read-only D1-D5 ambiguity). All IMPORTANT and CRITICAL findings across four review rounds are now resolved. The one SUGGESTION (unescaped branch name in the sibling-worktree grep) is low blast-radius and acceptable to merge with open. No new security issues were introduced by any commit in this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2b62d6b05

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 33f1fbe)

• Gather context and read new commit diff
• Check Codex P2 findings from d2b62d6 round — detached conflict-worker rejection + fix-cycle gate
• Verify all prior findings remain addressed
• Check for any new issues introduced by 33f1fbe
• Synthesize and post final review

---

Commit 33f1fbe ("reconcile branch-attached phrasing with detached-HEAD support") makes four targeted changes. Here is the full disposition.

---

✅ Both remaining Codex P2 findings addressed

Codex P2 — "Don't reject asserted detached conflict worktrees" (orchestration.md)

orchestration.md:451

The prior text said "refuse to resolve onto a stale or detached tip" — this blocked the sibling-locked path where the worktree is intentionally detached at the true PR head (HEAD == headRefOid). The new text reads:

refuse to resolve onto a stale or head-mismatched tip (a detached HEAD that equals the head is fine — the sibling-locked case)

The prohibition is now correctly scoped to HEAD ≠ PR_HEAD, not to detached HEAD as a state. A conflict worker in the sibling-locked --detach path that passes the HEAD assertion can now proceed. ✅

Codex P2 — "Don't require a branch checkout for fix cycles" (loop.md §5.1.4)

loop.md:318

The prior gate read "When on the PR branch AND..." — this excluded the sibling-locked --detach path from the fix cycle even when CHECKOUT_MODE=full. The new gate reads:

When in full mode (HEAD asserted at the true PR head — attached or detached per §5.1.2) AND...

Now correctly gates on CHECKOUT_MODE=full + head assertion, not on branch attachment. The --detach full-mode path can run D6-D7. ✅

---

Two ancillary phrasing updates (correct, no new issues)

SKILL.md Step 5 (SKILL.md:353) — "on the PR branch" → "to the PR branch (refspec; works from a detached HEAD)". Consistent with §5.1.4's explicit push step.

loop.md NEVER-do list (loop.md:442) — "must be on the PR branch" → "HEAD must be asserted at the true PR head (attached or detached, §5.1.2)". Matches the checkout contract.

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Not fixed (non-blocking)

---

Remaining — SUGGESTION (unchanged, non-blocking)

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex; a . in a branch name could match a wrong worktree. The HEAD assertion on line 165 degrades to read-only on a false positive — no unauthorized write occurs. The fail-closed PUSH_REMOTE block guards the fork-in-detach case even if the grep misfires. Not a blocker; fix when convenient with grep -qF "[$BRANCH]".

---

No blockers remain. Every HIGH and IMPORTANT finding from all five review rounds is resolved. 33f1fbe correctly closes the last two Codex P2 findings — the conflict-worker detached-HEAD rejection and the branch-attachment gate on the fix cycle — with minimal, targeted changes and no new issues introduced.

The PR is ready to merge, subject to the ordering note in the PR description: if #882 merges first, re-bump above 0.16.0 (→ 0.16.1) during recompose.

[claude]

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

---

Security Review — PR #895 (head: 33f1fbe)

• Gather context and understand request
• Read PR diff (git diff origin/main...HEAD)
• Review new commit (33f1fbe) vs prior head (d2b62d6)
• Check for new security issues introduced
• Verify prior findings still resolved
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — "Don't reject asserted detached conflict worktrees"	✅ FIXED (33f1fbe)
Codex P2 — "Don't require a branch checkout for fix cycles"	✅ FIXED (33f1fbe)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Not fixed (see below)

---

What 33f1fbe fixes (security-relevant)

orchestration.md conflict-worker assertion — The prior text "refuse to resolve onto a stale or detached tip" would have blocked the legitimate sibling-locked path where gh pr checkout --detach places the worktree in detached HEAD at exactly the PR head. The new text:

refuse to resolve onto a stale or head-mismatched tip (a detached HEAD that equals the head is fine — the sibling-locked case)

correctly distinguishes the two cases. The safety property is unchanged: the assertion HEAD == gh pr view --json headRefOid must still pass before any merge or push. Only the description of what triggers a refusal is tightened. No new attack surface.

loop.md §5.1.4 fix-cycle gate — The prior "When on the PR branch AND a comment is classified VALID" excluded detached-HEAD checkouts from the fix cycle. The replacement "When in full mode (HEAD asserted at the true PR head — attached or detached per §5.1.2) AND" correctly enables fixes in the sibling-locked path. The security chain is intact:

• CHECKOUT_MODE=full is only set when git rev-parse HEAD equals the live headRefOid (lines 155, 165, 171 of loop.md)
• For fork PRs in detached HEAD, CHECKOUT_MODE is overridden to read-only by the isCrossRepository gate (lines 186–191) because no branch config exists to validate the fork remote — so the fix cycle cannot run there
• Same-repo PRs in detached HEAD have PUSH_REMOTE=origin set unconditionally and reach full mode correctly; the push git push origin HEAD:<headRefName> is correct

No new attack surface; this completes the sibling-locked path.

---

Remaining — SUGGESTION (unchanged from prior passes)

Unescaped $BRANCH in sibling-worktree grep

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH (the PR's headRefName) is interpolated as unescaped basic-regex. A branch name containing . makes . match any character; feat/my.thing would match [feat/myXthing] — a potential false positive routing into the --detach path when the branch is not actually locked. Practical blast radius is contained by the HEAD assertion on line 165 ([ "$(git rev-parse HEAD)" = "$PR_HEAD" ]), which degrades to read-only on any mismatch — no unauthorized mutation occurs from a false-positive sibling-detection. The fail-closed PUSH_REMOTE block also catches the fork-in-detach case if the grep misfires. Not blocking.

Fix when convenient: grep -qF "[$BRANCH]" (fixed-string, brackets matched literally).

---

What looks correct across the full PR

• Checkout ordering — dirty-tree guard → HEAD == PR_HEAD already → sibling-locked --detach → heal-via-checkout. Fail-closed at each branch.
• PUSH_REMOTE resolution — isCrossRepository=false → hardcoded origin; fork head → git config --get "branch.$BRANCH.remote" || true with case ""|.|origin) → read-only; isCrossRepository query failure (empty ≠ "false") takes the fork/conservative path → read-only when no branch config. Fail-closed at every exit.
• Fix cycle in detached HEAD for same-repo PRs — freshness merge creates a new commit on top of PR_HEAD; the push HEAD:$BRANCH is fast-forward against the remote tip (which was at PR_HEAD). Fix-cycle commits stack on top — also fast-forward. No force-push at any point.
• All three push sites — freshness merge (line 210), conflict-continue prose (line 233), fix-cycle D6 (line 324) — all consume the same pre-resolved $PUSH_REMOTE. No site reintroduces the inline git config expansion.
• PR_HEAD failure path — if gh pr view --json headRefOid returns empty, all HEAD comparisons return false → read-only. Fail-closed.
• No GitHub Actions changes — no pull_request_target, workflow_run, run: injection surfaces, or permission-widening changes.

---

Bottom line: 33f1fbe introduces no new security issues. It closes the two remaining Codex P2 findings from the d2b62d6 review: the conflict-worker now correctly accepts detached HEAD == PR head, and the fix cycle now correctly gates on full mode + HEAD assertion rather than branch attachment. All IMPORTANT and CRITICAL findings across all review rounds are resolved. The one SUGGESTION (unescaped branch name in the sibling-worktree grep) is low blast-radius, guarded by the HEAD assertion, and acceptable to merge with open.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 33f1fbea8e

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eaaa57362d

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 98c35e3 — merge from origin/main)

• Gather context and understand request
• Read PR diff since last reviewed commit (eaaa573)
• Check Codex P2 finding (detached fix worktree cleanup) from eaaa57362d round
• Verify merge commit introduces no regressions
• Synthesize and post final review

---

The new commit (98c35e3 — Merge origin/main into fix/548-babysit-worktree-head-safety) brings in main's 0.16.1 release (the guarded-wrapper bin/-path fix, #484) and re-numbers this PR's entry to 0.16.2. Here is the full disposition.

---

✅ Merge is correct — no regressions

The three babysit-prs files modified by the merge (SKILL.md, orchestration.md, safety.md) correctly absorb the 0.16.1 bin-path changes without touching the head-safety mechanics from this PR. Spot-checked:

• SKILL.md Step 0.3 — still reads merge-only (never rebase — rebasing a PR branch needs a forbidden force-push). ✅
• SKILL.md Guarded mutations — updated to the bash "${CLAUDE_PLUGIN_ROOT}/bin/..." form from 0.16.1; unrelated to the head-safety contract. ✅
• orchestration.md conflict-worker assertion — still reads "refuse to resolve onto a stale or head-mismatched tip (a detached HEAD that equals the head is fine — the sibling-locked case)". ✅
• safety.md Checkout And Push Invariants — still intact with the URL-based fork-remote validation and fail-closed PUSH_REMOTE. ✅
• Version bump — plugin.json correctly incremented to 0.16.2; CHANGELOG.md correctly stacks the head-safety entry above 0.16.1. ✅

---

❌ Still open — one non-blocking finding (unchanged from Codex P2 / eaaa573 round)

loop.md §5.1.6 line 370 — cleanup condition still says "on a PR branch"

loop.md:370

1. If on a PR branch with uncommitted changes from a failed fix: `git reset --hard HEAD` then
   `git clean -fd` (unstage + revert tracked + remove untracked)

This PR now enables the sibling-locked --detach path as a full-mode checkout (CHECKOUT_MODE=full, §5.1.2). The D6 fix cycle runs normally from detached HEAD — edits, commits, and pushes all work. But if a fix attempt fails mid-edit (the D6 cycle is aborted before commit), the transition cleanup at §5.1.6 only fires when "on a PR branch" (branch-attached). A worker in detached-HEAD full mode with uncommitted leftover edits bypasses this cleanup and carries a dirty working tree into parking or the next PR.

The HEAD-assertion and read-only upgrade in subsequent PRs prevent a wrong write, but the dirty state itself is an unexpected hand-off. The Codex P2 reviewer who raised this on eaaa57362d correctly identified the blast radius as contaminating the next assignment rather than causing a bad push.

Fix: Change the condition to cover any full-mode checkout, attached or detached:

-1. If on a PR branch with uncommitted changes from a failed fix: `git reset --hard HEAD` then
+1. If in full mode (attached or detached, §5.1.2) with uncommitted changes from a failed fix: `git reset --hard HEAD` then

Fix this →

Severity: Low — no wrong write occurs (the next assignment's HEAD assertion would degrade to read-only on a dirty tree), but the dirty state is an unclean hand-off.

---

LOW — Unescaped $BRANCH in sibling-worktree grep (unchanged across all rounds)

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex; a . in a branch name (e.g. feat/1.2-my-fix) matches any character. A false-positive routes into the --detach path when the branch is not actually sibling-locked; the HEAD assertion on the next line degrades to read-only on mismatch, so no wrong write occurs. Not blocking; fix when convenient: grep -qF "[$BRANCH]".

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
Codex P1 — fork-remote guard only rejects literal origin name	✅ Fixed eaaa573
Merge introduces no regressions in head-safety mechanics	✅ Verified 98c35e3
LOW — §5.1.6 cleanup condition excludes detached-HEAD full mode	❌ Still open
LOW — unescaped $BRANCH in sibling-worktree grep	❌ Still open (all rounds)

---

Bottom line: The merge commit is clean — the 0.16.1 bin-path changes integrated without touching the head-safety contract. All IMPORTANT/HIGH findings across all rounds are resolved. Two LOW findings remain open, both non-blocking: the §5.1.6 cleanup condition and the unescaped branch-name grep. The merge also correctly re-numbers this PR's version to 0.16.2, consistent with the ordering note in the PR description.

[claude]

Claude finished @kyle-sexton's task in 4m 12s —— View job

---

Security Review — PR #895 (head: 98c35e3)

• Gather context and understand request
• Read PR diff at current HEAD
• Review changed files for security issues
• Check prior open findings (unescaped grep, dirty-worktree cleanup)
• Check for new issues from merge with origin/main
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents. The current HEAD (98c35e3) is a merge of origin/main into the branch; the changes from main don't touch the security-sensitive files and introduce no new attack surface.

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ FIXED (33f1fbe)
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ FIXED (33f1fbe)
IMPORTANT — fork-remote guard accepted base-repo remote by non-origin name	✅ FIXED (eaaa573)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open (see below)

---

SUGGESTION — Unescaped $BRANCH in sibling-worktree grep (unchanged from all prior passes)

Confidence: HIGH

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH (the PR's headRefName) is interpolated as unescaped basic-regex. A branch name containing . makes . match any character; feat/my.thing would match [feat/myXthing] — a false positive routing into the --detach path when the branch is not actually locked. The HEAD assertion on line 165 degrades to read-only on a mismatch, so no unauthorized mutation occurs from a false-positive sibling-detection. The fail-closed URL-validated PUSH_REMOTE block also guards the fork-in-detach case if the grep misfires. Not blocking.

Fix when convenient: grep -qF "[$BRANCH]" (fixed-string match; brackets are also matched literally under -F).

---

LOW — §5.1.6 cleanup gate excludes detached-HEAD full-mode path (Codex P2, previously unresolved)

Confidence: HIGH

loop.md:370

The PR-transition cleanup reads:

1. If on a PR branch with uncommitted changes from a failed fix: git reset --hard HEAD then git clean -fd

The fix cycle (§5.1.4) was updated by 33f1fbe to gate on CHECKOUT_MODE=full regardless of branch attachment — so a sibling-locked --detach checkout can now run D6-D7. But §5.1.6 still conditions cleanup on "on a PR branch." An agent following it literally skips cleanup when in detached HEAD (the sibling-locked path), even if a D6 edit was left uncommitted due to a failed fix. The dirty state then bleeds into the next PR's §5.1.2 dirty-tree guard (git status --porcelain), which correctly forces that next PR to read-only — but the cause ("leftover edits from previous PR's failed fix") is opaque, and the dirty worktree persists until the agent eventually returns to the parking branch.

git reset --hard HEAD and git clean -fd both work correctly in detached HEAD, so the fix is only in the condition wording.

Scenario: Sibling-locked PR → gh pr checkout --detach → CHECKOUT_MODE=full → D6 edit begins → commit fails → §5.1.6 transition fires → "if on a PR branch" is false (detached HEAD) → cleanup skipped → next PR's dirty-tree check forces read-only with no explanation.

Fix: change the condition to apply to any full-mode checkout:

1. If in full mode with uncommitted changes from a failed fix (attached or detached — git status --porcelain is non-empty): git reset --hard HEAD then git clean -fd

Fix this →

---

LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode

Confidence: HIGH

loop.md:381

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)
...
git checkout "$PARKING_BRANCH"

git rev-parse --abbrev-ref HEAD outputs the literal string HEAD in detached mode, so git checkout HEAD re-checks-out the current commit — a no-op. After processing a sibling-locked PR, the worktree stays detached at the PR's last merged head commit rather than returning to the parking branch. The next babysit iteration starts in detached HEAD; the dirty-tree guard fires clean, the HEAD == PR_HEAD check may match a now-stale commit, and the sibling-worktree detection may or may not fire depending on the branch state — unpredictable rather than systematically wrong, but the parking invariant is broken.

Scenario: parking branch is main → sibling-locked PR processed in detached HEAD → all PRs done → PARKING_BRANCH="HEAD" → git checkout HEAD no-op → next session starts detached at old PR head → first HEAD assertion compares against a different PR → falls to read-only.

Fix: capture the parking ref as a SHA when in detached HEAD, or record the home branch before the first PR is processed rather than assuming the initial HEAD is named:

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)
[ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD)  # detached: use SHA

Not a blocker; the failure mode is conservative (next session's head assertion fires read-only), not write-permissive.

---

What looks correct across the full PR (unchanged from prior passes)

• isCrossRepository gate: same-repo heads unconditionally get origin; fork heads must resolve via branch.<b>.pushRemote / branch.<b>.remote, and the URL is extracted and compared against the PR head repo's nameWithOwner. Empty/unresolvable → read-only. isCrossRepository query failure (empty ≠ "false") → takes fork/conservative path → read-only when no branch config. Fail-closed.
• URL slug extraction (sed -E 's#^[^/]+://[^/]+/##; s#^[^@]+@[^:]+:##; s#\.git$##'): handles HTTPS, SSH-protocol, and SCP-style URLs; unrecognized URL formats fail to extract a slug that matches HEAD_REPO and fall to read-only. Fail-closed.
• PR_HEAD empty guard: if gh pr view --json headRefOid returns empty, all HEAD comparisons return false → every path degrades to read-only. Fail-closed.
• Force-push removal: the --force-with-lease rebase path is gone; the replacement (merge + fast-forward refspec push) is strictly weaker in privilege and correct by construction.
• All three push sites (freshness merge, conflict-continue, fix-cycle D6) consume the same pre-resolved $PUSH_REMOTE. No site reintroduces the inline git config expansion or the || echo origin fallback.
• No GitHub Actions changes — no pull_request_target, workflow_run, run: injection surfaces, or permission-widening changes.

---

Bottom line: No new CRITICAL or IMPORTANT findings. The two remaining open items from prior rounds — the unescaped sibling-worktree grep (SUGGESTION) and the §5.1.6 cleanup gate that excludes detached-HEAD full-mode (LOW/Codex P2) — persist unchanged. Both are conservative-failure: a false positive on the grep degrades to read-only; a missed cleanup forces the next PR to read-only. Neither enables unauthorized writes. A third LOW (§5.2 parking in detached mode) is a functional regression introduced by enabling detached-HEAD full mode without updating the parking capture. All are small, targeted text fixes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98c35e3e44

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 635d5d4)

• Gather context and read new commit diff against prior head (98c35e3)
• Check if §5.1.6 cleanup condition for detached-HEAD was addressed
• Check if Codex P1 (push-URL vs fetch-URL validation) from 98c35e3 round was addressed
• Check if §5.2 parking in detached mode was addressed
• Check for any new issues introduced
• Synthesize and post final review

---

Commit 635d5d4 ("extend babysit failed-fix cleanup to detached-HEAD checkouts") makes two targeted changes to loop.md. Here is the full disposition.

---

✅ §5.1.6 and §5.4 — detached-HEAD full-mode cleanup correctly addressed

§5.1.6 PR done — transition to next (loop.md:370)

The cleanup condition changed from "If on a PR branch with uncommitted changes from a failed fix" to "If a full-mode PR checkout (attached or detached per §5.1.2) has uncommitted changes from a failed fix". This correctly extends the git reset --hard HEAD / git clean -fd cleanup to the sibling-locked --detach path enabled by §5.1.4.

§5.4 NEVER-do list (loop.md:464)

Updated from "Never leave uncommitted changes on a PR branch" to "Never leave uncommitted changes on a full-mode PR checkout (attached or detached)". Consistent with the §5.1.6 fix. ✅

Both git reset --hard HEAD and git clean -fd work correctly in detached HEAD mode, so the fix is mechanically sound.

---

LOW — Codex P1 (push-URL vs fetch-URL validation) still open

Confidence: HIGH

loop.md:194

REMOTE_URL=$(git remote get-url "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url without --push returns the fetch URL (remote.<name>.url). If a remote has a different push URL configured (remote.<name>.pushurl), git push uses pushurl while this line validates url — a mismatch that lets a misrouted push through validation. This was the Codex P1 raised at the 98c35e3 round; 635d5d4 did not address it.

Practical blast radius: gh pr checkout does not normally configure pushurl separately from url, so this scenario requires a manually crafted remote configuration. The HEAD assertion and nameWithOwner slug check remain effective against the common case. Not a blocker for this PR, but the spec calls for fail-closed fork-push validation, and validating the wrong URL is a gap in that contract.

Fix: change line 194 to use --push:

REMOTE_URL=$(git remote get-url --push "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url --push returns pushurl if configured, falling back to url — exactly the URL git push will use.

Fix this →

---

LOW — §5.2 parking in detached mode (unchanged, non-blocking)

loop.md:382

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)

git rev-parse --abbrev-ref HEAD outputs the literal string HEAD in detached mode. After a sibling-locked PR, git checkout HEAD is a no-op — the next iteration starts in detached HEAD at the old PR head. The HEAD assertion in the next §5.1.2 correctly degrades to read-only on mismatch, so no wrong write occurs, but the parking invariant is broken. Not a blocker; a guard like [ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD) captures a SHA fallback for detached-HEAD starts.

---

SUGGESTION — Unescaped $BRANCH in sibling-worktree grep (unchanged, non-blocking)

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is unescaped basic-regex; a . in a branch name matches any character. False positives route into --detach; the HEAD assertion on line 165 degrades to read-only, so no write occurs. Fix when convenient: grep -qF "[$BRANCH]".

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
Codex P1 — fork-remote guard accepts base-repo remote by non-origin name	✅ Fixed eaaa573
Merge introduces no regressions	✅ Verified 98c35e3
LOW/Codex P2 — §5.1.6 cleanup condition excludes detached-HEAD full mode	✅ Fixed 635d5d4
LOW/Codex P2 — §5.4 NEVER-do list: "PR branch" excludes detached mode	✅ Fixed 635d5d4
LOW — Codex P1: git remote get-url validates fetch URL, not push URL	❌ Still open
LOW — §5.2 parking in detached mode	❌ Still open (non-blocking)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open (non-blocking)

---

No blocking issues remain. 635d5d4 cleanly fixes the last blocking Codex P2 (§5.1.6 cleanup gate + NEVER-do list) with two focused rewrites. The fix is mechanically correct — git reset --hard HEAD and git clean -fd work in detached HEAD mode.

Three non-blocking items persist: the fetch-vs-push-URL gap (LOW, practical impact limited to manually-configured pushurl), the parking detached-mode issue (LOW, next iteration degrades to read-only), and the unescaped branch grep (SUGGESTION, guarded by HEAD assertion). None of these enable unauthorized writes.

The PR is ready to merge, subject to the ordering note: if #882 merges first, re-bump above 0.16.0 (→ 0.16.1) during recompose; if the merge at 98c35e3 already rebased above that, keep the current version.

[claude]

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

---

Security Review — PR #895 (head: 635d5d4)

• Gather context and read PR diff
• Review loop.md — push mechanics, PUSH_REMOTE resolution, sibling-worktree grep
• Review SKILL.md — checklist consistency
• Review safety.md, orchestration.md, worktrees.md
• Review evals.json and plugin.json
• Synthesize findings and post

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

What 635d5d4 fixes

Commit 635d5d4 ("extend babysit failed-fix cleanup to detached-HEAD checkouts") makes two targeted changes to loop.md:

• §5.1.6 cleanup gate — "If on a PR branch with uncommitted changes" → "If a full-mode PR checkout (attached or detached per §5.1.2) has uncommitted changes." A detached-HEAD worker leaving uncommitted edits from a failed D6 fix now correctly runs git reset --hard HEAD and git clean -fd before advancing, closing the dirty-worktree bleed-over the prior round's Codex P2 identified. ✅
• §5.4 NEVER-do list — "Never leave uncommitted changes on a PR branch when transitioning" → "on a full-mode PR checkout (attached or detached)." Consistent with the §5.1.6 change. ✅

No new attack surface introduced by 635d5d4.

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ FIXED (33f1fbe)
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ FIXED (33f1fbe)
IMPORTANT — fork-remote guard accepted base-repo remote by non-origin name	✅ FIXED (eaaa573)
Codex P2 — §5.1.6 cleanup gate excludes detached-HEAD full-mode	✅ FIXED (635d5d4)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open
Codex P1 (98c35e3) — git remote get-url without --push validates fetch URL, not push URL	❌ Still open
LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open

---

IMPORTANT — git remote get-url validates the fetch URL, not the URL git push will use

Confidence: HIGH

loop.md:194

REMOTE_URL=$(git remote get-url "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url <name> returns remote.<name>.url — the fetch URL. When remote.<name>.pushurl is configured, git push uses that value instead; git remote get-url without --push never reads it. The validation can therefore pass (fetch URL matches the PR head repo) while the actual push goes to a different destination (whatever pushurl resolves to).

git remote get-url --push <name> returns the push URL — remote.<name>.pushurl if set, otherwise falls back to remote.<name>.url if unset. The fix is a one-word flag addition.

Scenario: An in-owner fork PR is checked out via gh pr checkout. For some reason (manual config, a prior git remote set-url --push, or an edge case in gh pr checkout's own remote wiring), the remote ends up with:

• remote.<name>.url = https://github.com/fork-owner/fork-repo.git → slug fork-owner/fork-repo matches HEAD_REPO → validation passes, CHECKOUT_MODE stays full
• remote.<name>.pushurl = https://github.com/base-owner/base-repo.git → git push "$PUSH_REMOTE" HEAD:$BRANCH writes to the base repo

The entire URL-validation fix introduced in eaaa573 — and described in both loop.md's inline comment and safety.md's Checkout And Push Invariants — explicitly claims to validate "the URL git push will actually use." The implementation uses the wrong command to get that URL, meaning the guarantee the spec claims to provide is not actually delivered.

Fix: Change line 194 to use --push:

REMOTE_URL=$(git remote get-url --push "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url --push still falls back to the fetch URL when no pushurl is configured, so this change is backwards-compatible for the common case while correctly validating the push destination when pushurl diverges.

Fix this →

---

SUGGESTION — Unescaped $BRANCH in sibling-worktree grep (unchanged from all prior passes)

Confidence: HIGH

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex; a branch name containing . (common in semver branch names like release/1.2) makes . match any character, creating a false-positive that routes into the --detach path when the branch is not actually sibling-locked. The HEAD assertion on line 165 degrades to read-only on mismatch — no unauthorized mutation occurs. Not blocking.

Fix: grep -qF "[$BRANCH]" (fixed-string; square brackets are also literal under -F).

---

LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode (unchanged from prior pass)

Confidence: HIGH

loop.md:382

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)

If a babysit session starts from a detached-HEAD worktree, PARKING_BRANCH captures the literal string "HEAD". After all PRs are processed, git checkout "HEAD" is a no-op — the worktree stays detached at whatever commit the final PR left it on. The next session's dirty-tree guard fires clean, but the HEAD assertion compares against a different PR's head and likely degrades to read-only. The failure mode is conservative (no unauthorized write), but the parking invariant is broken and the cause is opaque to the operator. Not blocking.

Fix: capture the parking ref as a SHA in detached mode:

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)
[ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD)

---

What looks correct in the full PR

• 635d5d4 §5.1.6 cleanup — git reset --hard HEAD and git clean -fd work correctly in detached HEAD; extending the condition to "full mode (attached or detached)" is the right fix. The §5.4 NEVER-do alignment is consistent.
• isCrossRepository gate — same-repo heads unconditionally get origin; the detached fork path degrades to read-only (no branch config → empty PUSH_REMOTE → case gate → read-only). Fail-closed on API failure (empty ≠ "false" → fork path → read-only).
• PR_HEAD empty guard — if gh pr view --json headRefOid returns empty, all HEAD comparisons return false → read-only throughout. Fail-closed.
• Force-push removal — the --force-with-lease rebase path is gone; the replacement (merge + fast-forward refspec push) is strictly weaker in privilege.
• All three push sites (freshness merge, conflict-continue, fix-cycle D6) consume the same pre-resolved $PUSH_REMOTE. No site reintroduces the || echo origin fallback.
• No GitHub Actions changes — no pull_request_target, workflow_run, run:, or permission-widening changes.

---

Bottom line: 635d5d4 correctly closes the §5.1.6 dirty-worktree gap (the Codex P2 from the 98c35e3 round). One IMPORTANT gap remains open: git remote get-url without --push validates the fetch URL rather than the URL git push actually uses — a one-word flag addition (--push) closes it. The SUGGESTION (unescaped branch grep) and LOW (parking in detached mode) are unchanged from prior rounds and remain non-blocking.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 635d5d4784

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit 1acce0c)

• Gather context and read new commit diff against prior head (635d5d4)
• Check if IMPORTANT finding (fetch URL vs push URL validation) was addressed
• Check if Codex P1 (watched-owner policy before fork pushes) was addressed
• Check for any new issues introduced
• Synthesize and post final review

---

Commit 1acce0c ("validate babysit fork push by host, and by push URL") makes targeted changes to loop.md and safety.md. Here is the full disposition.

---

✅ IMPORTANT finding addressed — push URL validation + host comparison

loop.md lines 194–212 / safety.md lines 54–62

The prior round's IMPORTANT finding (the git remote get-url call validated the fetch URL, while git push honors pushurl if set) is fully fixed. Changes in 1acce0c:

1. --push flag added — git remote get-url --push "$PUSH_REMOTE" now reads remote.<name>.pushurl when set (falling back to remote.<name>.url when not), which is exactly the URL git push will write to. Matches the spec's stated intent.

2. repo_id() canonicalization — a new shell function normalizes any git URL (HTTPS, SSH scheme, SCP-style) to host/owner/repo:

   repo_id() { printf '%s\n' "$1" | sed -E 's#\.git$##; s#/+$##; s#^[a-zA-Z][a-zA-Z0-9+.-]*://##; s#^[^/@]*@##; s#:[0-9]+/#/#; s#:#/#'; }

   Verified against common forms:

   • https://github.com/owner/repo.git → github.com/owner/repo ✓
   • git@github.com:owner/repo.git → github.com/owner/repo ✓
   • git@evil.example.com:owner/repo.git → evil.example.com/owner/repo (≠ github.com/owner/repo → read-only) ✓
   • https://github.com:443/owner/repo.git → port stripped → github.com/owner/repo ✓

3. gh api for the authoritative head-repo URL — gh pr view --json headRepository exposes only nameWithOwner (no host). The fix uses gh api "repos/$HEAD_REPO" --jq .html_url to get the canonical URL, then applies repo_id() to produce HEAD_ID = host/owner/repo. Fail-closed: if gh api fails (rate-limit, transient error), HEAD_ID is empty, and [ -z "$HEAD_ID" ] routes to read-only.

4. Comparison now includes the host — "$(repo_id "$REMOTE_URL")" != "$HEAD_ID" rejects a same-owner/repo path on a different host (the attack surface identified in the eaaa573 round's Codex review), which the prior REMOTE_SLUG != HEAD_REPO check missed.

5. safety.md updated in lockstep — the canonical contract prose now describes all three guarded vectors: a remote named upstream (or any name), a same-owner/repo path on a different host, and a fork fetch URL masking a base-repo pushurl.

Fail-closed verification at every exit:

• PUSH_REMOTE empty → read-only via [ -z "$PUSH_REMOTE" ] ✓
• HEAD_ID empty (API failure) → read-only via [ -z "$HEAD_ID" ] ✓
• PUSH_REMOTE non-existent remote name → git remote get-url --push fails → printf '%s' "$PUSH_REMOTE" returns the name → repo_id("name") → non-matching string → read-only ✓
• Detached HEAD fork path (no branch config → empty PUSH_REMOTE) → read-only ✓

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
Codex P1 — fork-remote guard accepts base-repo remote by non-origin name	✅ Fixed eaaa573
Merge introduces no regressions	✅ Verified 98c35e3
LOW/Codex P2 — §5.1.6 cleanup condition excludes detached-HEAD full mode	✅ Fixed 635d5d4
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ Fixed 1acce0c
HOST not included in URL comparison (same-path different-host attack)	✅ Fixed 1acce0c
LOW — §5.2 parking: abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open (non-blocking)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open (non-blocking)
Codex P1 (635d5d4) — watched-owner policy not re-checked in shell block	❌ Still open (see below)

---

Remaining non-blocking items

Codex P1 (635d5d4) — watched-owner policy gate not present in the shell block

loop.md §5.1.2

The shell block validates the fork push destination by URL identity but does not explicitly check mutation_policy.branch_write_allowed or whether the head repo's owner is within <watched-owners>. safety.md documents this gate ("Honor mutation_policy.branch_write_allowed: never push… when it is false" and the external-fork stop-and-ask rule), but the shell block doesn't enforce it inline — a cross-repo PR outside <watched-owners> whose URL check passes would stay in full mode.

This is not a regression introduced by this PR — the watched-owner gate is an orchestrator-layer constraint documented in safety.md and presumably enforced before a worker is ever assigned. The PR's scope is git mechanics (HEAD assertion + push routing), not policy enforcement. Not a blocker, but the gap between the policy's stated scope in safety.md and its absence from the inline shell spec is worth a follow-up.

LOW — §5.2 parking in detached mode (unchanged)

loop.md:395

git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode; git checkout "HEAD" is a no-op. After a sibling-locked PR, the next session's HEAD assertion degrades to read-only (conservative failure). Not blocking.

SUGGESTION — unescaped $BRANCH in sibling-worktree grep (unchanged)

loop.md:159

grep -q "\[$BRANCH\]" — $BRANCH unescaped basic-regex; . in a branch name matches any character. HEAD assertion degrades false positives to read-only. Not blocking; fix with grep -qF "[$BRANCH]".

---

No blockers remain. All IMPORTANT and HIGH findings across all review rounds are resolved. 1acce0c closes the last security-relevant gap: the URL validation now reads the actual push URL (via --push), includes the host in the comparison (via repo_id()), and derives the authoritative head-repo identity from gh api rather than the incomplete headRepository.nameWithOwner. The fail-closed logic holds at every edge.

The PR is ready to merge, subject to the ordering note in the PR description: if #882 merges first, re-bump above 0.16.0 (→ 0.16.1) during recompose — but the merge at 98c35e3 already absorbed origin/main at 0.16.1, so the current version is 0.16.2, which is correct.

[claude]

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

---

Security Review — PR #895 (head: 1acce0c)

• Gather context and understand request
• Read PR diff at current HEAD (1acce0c)
• Review loop.md — push URL validation, PUSH_REMOTE resolution, sibling-worktree grep
• Review safety.md, orchestration.md, SKILL.md
• Check prior open findings disposition
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

What 1acce0c fixes

Commit 1acce0c ("validate babysit fork push by host, and by push URL") addresses the two security gaps identified at the 635d5d4 round:

Fetch-URL-vs-push-URL mismatch — The prior REMOTE_URL=$(git remote get-url "$PUSH_REMOTE" 2>/dev/null || ...) read the fetch URL (remote.<name>.url). When a remote has remote.<name>.pushurl configured differently, git push uses pushurl while the validation checked url — a fork fetch URL could mask a base-repo push URL and pass. The fix:

REMOTE_URL=$(git remote get-url --push "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url --push returns pushurl when set, falling back to url otherwise — exactly the address git push will write to. ✅

Host not included in URL comparison — The prior sed extraction reduced both sides to owner/repo, stripping the host. A remote on git@evil.example.com:owner/repo.git produced the same slug as github.com/owner/repo and matched. The fix introduces repo_id():

repo_id() { printf '%s\n' "$1" | sed -E 's#\.git$##; s#/+$##; s#^[a-zA-Z][a-zA-Z0-9+.-]*://##; s#^[^/@]*@##; s#:[0-9]+/#/#; s#:#/#'; }

This canonicalizes any git URL (HTTPS, SSH-scheme, SCP-style) to host/owner/repo. The HEAD_ID is then derived from gh api "repos/$HEAD_REPO" --jq .html_url — GitHub's own authoritative canonical URL — so both sides of the comparison include the host. A remote on a different host with the same path now correctly fails to match. ✅

Verification of the fail-closed chain:

• gh api ... failure (empty HEAD_ID) → [ -z "$HEAD_ID" ] → read-only ✅
• Empty PUSH_REMOTE → [ -z "$PUSH_REMOTE" ] → read-only ✅
• URL mismatch → [ "$(repo_id "$REMOTE_URL")" != "$HEAD_ID" ] → read-only ✅
• isCrossRepository API failure (empty ≠ "false") → fork path → likely read-only ✅

repo_id() URL coverage (traced):

• https://github.com/owner/repo.git → github.com/owner/repo ✅
• git@github.com:owner/repo.git → github.com/owner/repo ✅
• ssh://git@github.com:22/owner/repo.git → github.com/owner/repo ✅
• https://user:pass@github.com/owner/repo → github.com/owner/repo ✅
• git@evil.example.com:owner/repo.git → evil.example.com/owner/repo ≠ github.com/owner/repo → read-only ✅

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ FIXED (33f1fbe)
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ FIXED (33f1fbe)
IMPORTANT — fork-remote guard accepted base-repo remote by non-origin name	✅ FIXED (eaaa573)
Codex P2 — §5.1.6 cleanup gate excludes detached-HEAD full mode	✅ FIXED (635d5d4)
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ FIXED (1acce0c)
IMPORTANT — host stripped from URL comparison (different-host same-path bypass)	✅ FIXED (1acce0c)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open
LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open
Codex P1 (635d5d4) — watched-owner policy not enforced in bash fork-push gate	❌ Still open (deferred)

---

Remaining — SUGGESTION (unchanged from all prior passes)

Unescaped $BRANCH in sibling-worktree grep

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex. A branch name containing . (e.g. release/1.2) makes . match any character — a false positive routes into the --detach path when the branch is not actually sibling-locked. The HEAD assertion on line 165 degrades to read-only on mismatch; no unauthorized mutation occurs. Not blocking.

Fix: grep -qF "[$BRANCH]" (fixed-string; brackets are also matched literally under -F).

---

Remaining — LOW (unchanged from prior passes)

§5.2 parking in detached mode

loop.md:395

git rev-parse --abbrev-ref HEAD returns the literal string "HEAD" in detached mode. After processing a sibling-locked PR, git checkout "HEAD" is a no-op — the next session starts detached at the old PR head. The HEAD assertion in the next §5.1.2 correctly degrades to read-only on mismatch; the failure is conservative (no wrong write) but the parking invariant is broken silently. Not blocking.

Fix: [ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD).

---

Remaining — Codex P1 (deferred, still open)

Watched-owner policy not enforced in bash fork-push gate

loop.md:190–213

The URL-identity gate (now correct) validates that the push destination matches the PR head repository by host and path. It does not additionally check whether the head repo's owner is within <watched-owners> or whether mutation_policy.branch_write_allowed is true. safety.md's Stop-And-Ask section specifies "A refresh, edit, commit, or push would write to an external-fork head outside <watched-owners>" as a stop condition, but this is prose the agent follows, not an enforcement gate in the bash script. For an external fork with maintainer_edits_allowed=true whose URL validates, the bash gate permits full mode without consulting the watched-owner boundary.

This was the Codex P1 raised at the 635d5d4 round and explicitly deferred by the PR author to a follow-up issue. It is noted here as still open; it is not introduced or worsened by 1acce0c.

---

What looks correct in 1acce0c

• repo_id() sed pipeline — handles all common git URL formats (HTTPS, SSH-scheme with port, SCP-style, with or without user@, with or without .git). The host is preserved by stripping only the scheme and user prefix, then normalizing the remaining separator from : to /. Traced correctly above.
• gh api "repos/$HEAD_REPO" --jq .html_url — uses GitHub's own authoritative canonical URL (always https://github.com/<owner>/<repo>); no injection risk since nameWithOwner is GitHub-controlled and restricted to alphanumeric/hyphen/underscore. API failure returns empty → HEAD_ID empty → read-only. Fail-closed. ✓
• git remote get-url --push — the --push flag makes it query remote.<name>.pushurl first (falling back to url), matching what git push actually writes to. The 2>/dev/null || printf '%s' "$PUSH_REMOTE" fallback handles the bare-URL config value case (where PUSH_REMOTE is itself a URL, not a remote name). ✓
• Single pre-resolved $PUSH_REMOTE — all three push sites (freshness merge, conflict-continue, fix-cycle D6) consume the same value. No site reintroduces the inline config expansion. ✓
• No new GitHub Actions surface — no workflow files changed. ✓

---

Bottom line: 1acce0c closes the two IMPORTANT findings from the 635d5d4 round — the push-URL/fetch-URL mismatch and the host-stripping bypass. The repo_id() + --push + html_url design is correct and fail-closed. The one SUGGESTION (unescaped branch grep), one LOW (parking in detached mode), and one deferred Codex P1 (watched-owner enforcement) persist unchanged and none enable unauthorized writes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1acce0c1d4

ℹ️ 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".

[claude]

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

---

Claude finished @kyle-sexton's task ——

---

PR Review — Follow-up Pass (commit 74a4198)

• Read new commit diff against prior head (1acce0c)
• Check if 74a4198 touches any safety-critical files or only CHANGELOG
• Check new Codex P1/P2 findings from 1acce0c round (multiple pushurl, branch-identity gate)
• Verify all prior resolved findings remain intact
• Synthesize and post final review

---

Commit 74a4198 ("docs(source-control): align 0.16.2 changelog with host+push-URL fork validation") changes only CHANGELOG.md. Here is the full disposition.

---

✅ CHANGELOG text correctly aligned

The [0.16.2] push-bullet previously described name-based push resolution (git config branch.<b>.remote, "the fork's remote for a cross-repo head") — which described what was implemented before 1acce0c. The new text correctly documents what ships:

• git push "$PUSH_REMOTE" HEAD:<headRefName> via an explicit refspec
• Fail-closed destination — origin for same-repo; for cross-repo, the destination validated by host + owner/repo identity, not by remote name
• git remote get-url --push (honors pushurl that can differ from the fetch URL)
• gh api repos/<nameWithOwner> --jq .html_url as the authoritative head-repo URL

No changes to loop.md, safety.md, orchestration.md, or SKILL.md. No regressions.

---

Pending Codex P1/P2 from 1acce0c round — both non-blocking, both unchanged

Codex P1 — Multiple pushurl values not fully validated

loop.md:208

REMOTE_URL=$(git remote get-url --push "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url --push (without --all) returns the first configured push URL. git push <remote> writes to all configured pushurl entries for that remote. If a second pushurl points to a different destination, the URL validation passes on the first entry while the push also writes to the second.

Practical blast radius is very low: gh pr checkout never configures multiple pushurl entries; reproducing this requires deliberate manual git remote set-url --push --add. The HEAD assertion (HEAD == PR_HEAD) still validates the source is correct; only the destination count is unvalidated. Not blocking.

Fix when convenient: use git remote get-url --push --all "$PUSH_REMOTE" (returns all push URLs, one per line) and validate that every output line canonicalizes to $HEAD_ID.

Codex P2 — Branch identity not asserted in the HEAD-already-matches path

loop.md:155

elif [ "$(git rev-parse HEAD)" = "$PR_HEAD" ]; then
  CHECKOUT_MODE="full"

If the current worktree is on an unrelated local branch other-branch whose tip commit happens to equal $PR_HEAD, this enters full mode without running gh pr checkout. A D6 fix commits on other-branch, then pushes by refspec to $BRANCH on the correct remote — the push destination is right, but other-branch is left with this PR's fix commit locally. The next assignment inherits a local branch that silently diverged.

The failure mode is local branch hygiene, not a wrong-destination push. Not blocking.

Fix when convenient: tighten the condition to also require being on $BRANCH or in detached HEAD:

elif [ "$(git rev-parse HEAD)" = "$PR_HEAD" ] && \
     { [ "$(git branch --show-current)" = "$BRANCH" ] || [ -z "$(git branch --show-current)" ]; }; then

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
Codex P1 — fork-remote guard accepts base-repo remote by non-origin name	✅ Fixed eaaa573
Merge introduces no regressions	✅ Verified 98c35e3
LOW/Codex P2 — §5.1.6 cleanup condition excludes detached-HEAD full mode	✅ Fixed 635d5d4
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ Fixed 1acce0c
IMPORTANT — host stripped from URL comparison (different-host same-path bypass)	✅ Fixed 1acce0c
CHANGELOG text still described name-based push resolution	✅ Fixed 74a4198
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open (non-blocking)
LOW — §5.2 parking: abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open (non-blocking)
Codex P1 (635d5d4) — watched-owner policy not enforced in bash fork-push gate	❌ Deferred (non-blocking)
Codex P1 (1acce0c) — multiple pushurl values not all validated	❌ Still open (non-blocking)
Codex P2 (1acce0c) — branch identity not asserted in HEAD-match path	❌ Still open (non-blocking)

---

No blockers remain. 74a4198 correctly aligns the CHANGELOG to the URL-identity implementation that landed in 1acce0c. All IMPORTANT and HIGH findings across all review rounds are resolved. The five remaining open items are all non-blocking: guarded by the HEAD assertion, conservatively fail-closed, or explicitly deferred to a follow-up issue. The PR is ready to merge, subject to the ordering note: if #882 has merged (it was unmerged at PR open), re-verify the version bump is still above 0.16.0; the 98c35e3 merge already absorbed origin/main at 0.16.1, so 0.16.2 is correct unless another bump landed since.

[claude]

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

---

Security Review — PR #895 (head: 74a4198)

• Gather context and read PR diff
• Review loop.md — all push paths, PUSH_REMOTE resolution, multi-pushurl, sibling-worktree grep
• Review SKILL.md — checklist consistency
• Review safety.md, orchestration.md, worktrees.md
• Check 74a4198 changelog-only commit for regressions
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

74a4198 is docs-only — no new attack surface

The latest commit only updates CHANGELOG.md to align the 0.16.2 bullet with the --push / host+path validation mechanics that landed in 1acce0c. No loop.md, safety.md, SKILL.md, or orchestration.md changes. No regression from this commit.

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ FIXED (33f1fbe)
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ FIXED (33f1fbe)
IMPORTANT — fork-remote guard accepted base-repo remote by non-origin name	✅ FIXED (eaaa573)
Codex P2 — §5.1.6 cleanup gate excludes detached-HEAD full mode	✅ FIXED (635d5d4)
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ FIXED (1acce0c)
IMPORTANT — host stripped from URL comparison (different-host same-path bypass)	✅ FIXED (1acce0c)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Open (unchanged, see below)
LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode	❌ Open (unchanged, see below)

---

SUGGESTION — Multi-pushurl gap: only the first pushurl is validated, but git push writes to all (new finding, Codex P1 from 1acce0c round)

Confidence: HIGH

loop.md:208

REMOTE_URL=$(git remote get-url --push "$PUSH_REMOTE" 2>/dev/null || printf '%s' "$PUSH_REMOTE")

git remote get-url --push <name> returns the first configured remote.<name>.pushurl (falling back to the fetch URL when none is set). When a remote has multiple pushurl entries, git push writes to all of them. If a second remote.<name>.pushurl pointing at the base repo is present, the validation passes on the first (fork) URL, but the subsequent git push "$PUSH_REMOTE" HEAD:$BRANCH silently also writes to the base repo via the extra pushurl — the exact regression this PR was written to close.

git remote get-url --push --all <name> returns all configured pushurls. The fix is to iterate over all of them and degrade to read-only if any resolves to a non-head-repo destination:

# Validate EVERY configured push URL — git push writes to all pushurls, not just the first.
while IFS= read -r url; do
  if [ "$(repo_id "$url")" != "$HEAD_ID" ]; then
    echo "Fork remote has a pushurl not matching the PR head repo — read-only"
    CHECKOUT_MODE="read-only"
    break
  fi
done < <(git remote get-url --push --all "$PUSH_REMOTE" 2>/dev/null || printf '%s\n' "$PUSH_REMOTE")

Practical blast radius: this requires a second pushurl to have been deliberately added to the local git config. gh pr checkout does not configure multiple pushurls; the scenario requires the local git repository to have been pre-tampered with. An operator with that level of local access can already push directly. Not blocking, but the spec's stated guarantee ("the address git push will actually use") is not fully delivered when multiple pushurls are in play.

---

SUGGESTION — Unescaped $BRANCH in sibling-worktree grep (unchanged from all prior passes)

Confidence: HIGH

loop.md:159

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex. A branch name containing . (e.g. release/1.2) makes . match any character — false positive routes into the --detach path when the branch is not actually sibling-locked. The HEAD assertion on line 165 degrades to read-only on any mismatch; no unauthorized mutation occurs from a false-positive detection. Not blocking.

Fix when convenient: grep -qF "[$BRANCH]" (fixed-string; brackets are also matched literally under -F).

---

LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode (unchanged from prior passes)

Confidence: HIGH

loop.md:395

git rev-parse --abbrev-ref HEAD outputs the literal string "HEAD" in detached mode. After processing a sibling-locked PR in detached full mode, git checkout "HEAD" is a no-op — the next session starts detached at the old PR commit. The HEAD assertion in the next §5.1.2 correctly degrades to read-only on mismatch (conservative failure, no wrong write), but the parking invariant is broken silently. Not blocking.

Fix: [ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD) to capture a SHA fallback.

---

What looks correct across the full PR

• repo_id() coverage — traces correctly across HTTPS (https://github.com/o/r.git), SSH-scheme with port (ssh://git@github.com:22/o/r.git), SCP-style (git@github.com:o/r.git), and URL-encoded forms. A same-path remote on a different host (git@evil.example.com:o/r.git) correctly canonicalizes to evil.example.com/o/r ≠ github.com/o/r → read-only.
• HEAD_ID from gh api — gh api "repos/$HEAD_REPO" uses GitHub's own authoritative html_url (https://github.com/owner/repo), guaranteeing the host is always github.com for GitHub-hosted PRs. API failure (empty HEAD_ID) is caught by [ -z "$HEAD_ID" ] → read-only. Fail-closed.
• isCrossRepository fail-closed — an API error (empty result ≠ "false") routes to the fork/conservative path → read-only when PUSH_REMOTE is empty or unresolved. Fail-closed.
• PR_HEAD empty guard — if gh pr view --json headRefOid returns empty, all HEAD comparisons return false → read-only throughout. Fail-closed.
• All three push sites (freshness merge line 231, conflict-continue prose line 254, fix-cycle D6 line 345) consume the same pre-resolved $PUSH_REMOTE. No site reintroduces the || echo origin fallback or the inline git config expansion.
• Force-push removal — the --force-with-lease rebase path is gone; the replacement (merge + fast-forward refspec push) is strictly weaker in privilege and correct by construction.
• No GitHub Actions changes — no pull_request_target, workflow_run, run: injection surfaces, or permission-widening changes.
• SKILL.md Step 0.3 — correctly reads merge-only (never rebase — rebasing a PR branch needs a forbidden force-push). All cross-tier locations are consistent.
• safety.md Checkout And Push Invariants — prose is internally consistent and covers detached HEAD, stale local tip, fork pushurl masking, same-path different-host attack, and the non-origin remote-name bypass. All three threats are explicitly named and the response is unambiguously stop (read-only).

---

Bottom line: 74a4198 is a CHANGELOG-only doc update introducing no new attack surface. The one new finding from this round — the multi-pushurl gap (Codex P1 from the 1acce0c review that was not previously surfaced) — requires deliberate local git config manipulation and is not a blocker. Both remaining open items (unescaped grep, parking in detached mode) are conservative-failure non-blockers confirmed across every prior round. All IMPORTANT and CRITICAL findings from all rounds are resolved.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74a419818f

ℹ️ 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".

[claude]

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

---

PR Review — Follow-up Pass (commit a407f08)

• Gather context and understand request
• Read new commit diff against prior head (74a4198)
• Check Codex P1 — multiple pushurl values not all validated
• Check Codex P2 — branch identity not asserted in HEAD-match path
• Check Codex P1 (635d5d4) — watched-owner policy not enforced in bash fork-push gate
• Check open LOW — §5.2 parking in detached mode
• Check open SUGGESTION — unescaped $BRANCH in sibling-worktree grep
• Check for any new issues introduced
• Synthesize and post final review

---

Commit a407f08 ("complete babysit fork-push guard — owner + all push URLs + branch identity") makes targeted changes to loop.md (58 additions, 29 deletions) and safety.md (29 additions, 4 deletions). Here is the full disposition.

---

✅ All three prior Codex P1/P2 findings addressed

Codex P1 — Multiple pushurl values not all validated

loop.md:229–234

The prior git remote get-url --push "$PUSH_REMOTE" returned only the first push URL; git push writes to ALL configured pushurl entries. The fix:

PUSH_URLS=$(git remote get-url --push --all "$PUSH_REMOTE" 2>/dev/null || printf '%s\n' "$PUSH_REMOTE")
BAD=$(printf '%s\n' "$PUSH_URLS" | canon | grep -vxF "$HEAD_ID" || true)
if [ -z "$PUSH_REMOTE" ] || [ -z "$HEAD_ID" ] || [ -z "$PUSH_URLS" ] || [ -n "$BAD" ]; then

--push --all returns every configured pushurl (one per line). canon canonicalizes each to host/owner/repo via the shared sed pipeline. grep -vxF "$HEAD_ID" identifies any line that is NOT an exact match for the head repo — if any non-matching URL exists, BAD is non-empty and the guard degrades to read-only.

Traced through the two-pushurl failure scenario: PUSH_URLS = "url1\nurl2", canon produces two lines, grep -vxF finds the non-matching base-repo URL, BAD non-empty → read-only. ✅

Codex P2 — Branch identity not asserted in HEAD-match path

loop.md:148 and 156

The prior elif [ "$(git rev-parse HEAD)" = "$PR_HEAD" ] entered full mode on any local branch whose tip matched headRefOid — a coincidental same-tip on other-branch would advance that unrelated branch with the fix commit. The fix captures the current branch name first:

CUR_BRANCH=$(git symbolic-ref --short -q HEAD || true)  # empty when detached

Then tightens the condition:

elif [ "$(git rev-parse HEAD)" = "$PR_HEAD" ] && { [ -z "$CUR_BRANCH" ] || [ "$CUR_BRANCH" = "$BRANCH" ]; }; then

A coincidental same-tip match on another branch now falls through to the gh pr checkout paths, which place the worktree correctly. A correctly-on-$BRANCH or detached checkout continues to enter full mode as before. ✅

Codex P1 (635d5d4) — Watched-owner policy not enforced in bash fork-push gate

loop.md:212–216

A cross-repo head whose URL matched the head repo stayed in full mode even when its owner was outside <watched-owners> — an external fork with maintainer_edits_allowed=true could receive a push. The fix inserts a trust-boundary check before the URL validation:

HEAD_OWNER=${HEAD_REPO%%/*}
case ",<watched-owners>," in
  *",$HEAD_OWNER,"*) : ;;
  *) echo "Cross-repo head owner $HEAD_OWNER outside <watched-owners> — read-only"; CHECKOUT_MODE="read-only" ;;
esac

An external-fork head that passes the watched-owner check continues to URL validation; one that fails degrades to read-only before any push resolution. ✅

The safety.md Checkout And Push Invariants are updated in lockstep to describe all three guards and document the explicit limitation around url.<base>.pushInsteadOf rewrites, which are outside the static guard's threat model (they don't arise from the documented gh pr checkout flow). ✅

---

Complete finding disposition across all review rounds

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ Fixed 1d7a992
HIGH — §5.1.4 fix-cycle git push fails in detached HEAD	✅ Fixed 1d7a992
LOW — safety.md grammar / orphaned clause	✅ Fixed 1d7a992
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ Fixed 1d7a992
orchestration.md conflict-worker inline push remote	✅ Fixed 180793c
safety.md PUSH_REMOTE authority + fail-closed description	✅ Fixed 180793c
HIGH/IMPORTANT — SKILL.md Step 0.3 says "merge vs rebase"	✅ Fixed d2b62d6
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ Fixed d2b62d6
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ Fixed 33f1fbe
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ Fixed 33f1fbe
Codex P1 — fork-remote guard accepts base-repo remote by non-origin name	✅ Fixed eaaa573
Merge introduces no regressions	✅ Verified 98c35e3
LOW/Codex P2 — §5.1.6 cleanup condition excludes detached-HEAD full mode	✅ Fixed 635d5d4
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ Fixed 1acce0c
IMPORTANT — host stripped from URL comparison (different-host same-path bypass)	✅ Fixed 1acce0c
CHANGELOG text still described name-based push resolution	✅ Fixed 74a4198
Codex P1 — multiple pushurl values not all validated	✅ Fixed a407f08
Codex P2 — branch identity not asserted in HEAD-match path	✅ Fixed a407f08
Codex P1 (635d5d4) — watched-owner policy not enforced in bash fork-push gate	✅ Fixed a407f08
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open (non-blocking)
LOW — §5.2 parking: abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open (non-blocking)

---

Remaining — SUGGESTION (unchanged from all prior passes)

loop.md:163

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex; . in a branch name (e.g. release/1.2) matches any character and could cause a false-positive sibling-lock detection. The HEAD assertion on line 169 degrades to read-only on any mismatch — no unauthorized mutation occurs. Not blocking. Fix when convenient: grep -qF "[$BRANCH]".

---

Remaining — LOW (unchanged from prior passes)

loop.md:417

PARKING_BRANCH=$(git rev-parse --abbrev-ref HEAD)

Returns "HEAD" in detached mode. After a sibling-locked PR, git checkout "HEAD" is a no-op — the next session starts detached at the old PR commit. The next §5.1.2 HEAD assertion correctly degrades to read-only on mismatch (conservative, no wrong write), but the parking invariant is broken silently. Not blocking. Fix: [ "$PARKING_BRANCH" = "HEAD" ] && PARKING_BRANCH=$(git rev-parse HEAD).

---

What looks correct in a407f08

• BAD computation — printf '%s\n' "$PUSH_URLS" | canon | grep -vxF "$HEAD_ID": printf correctly handles single and multi-URL cases (no spurious empty lines); grep -vxF requires exact full-line matches; || true makes the pipeline exit-0 when all URLs match. All three sub-cases traced correctly:
  • All URLs match HEAD_ID → BAD="" → gate passes ✓
  • Any URL doesn't match → BAD non-empty → read-only ✓
  • PUSH_URLS empty (no remote config) → [ -z "$PUSH_URLS" ] fires → read-only ✓
• CUR_BRANCH on detached HEAD — git symbolic-ref --short -q HEAD || true outputs empty string on detached HEAD (not "HEAD"). The condition [ -z "$CUR_BRANCH" ] correctly handles detached mode as equivalent to "not on the wrong branch". ✓
• Watched-owner gate ordering — runs before HEAD_ID and PUSH_REMOTE resolution; a blocked owner still has those variables computed but CHECKOUT_MODE is already "read-only" so no push path uses them. Fail-closed. ✓
• canon() renamed from repo_id() — processes stdin line-by-line (sed reads a stream), making it composable with printf ... | canon | grep .... The sed pipeline was traced against HTTPS, SSH-scheme with port, SCP-style, user:pass@host, and GHES forms — all produce correct host/owner/repo. ✓
• Known limitation documented — url.<base>.pushInsteadOf rewrites are correctly called out in safety.md as outside the static guard's threat model. ✓

---

No blockers remain. a407f08 closes the last three Codex P1/P2 findings from the 74a4198 round — multi-pushurl validation, branch-identity in the HEAD-match path, and the watched-owner trust boundary — with minimal, targeted changes and no new issues introduced. The two remaining open items (unescaped branch grep and parking in detached mode) are both conservative-failure non-blockers confirmed across multiple prior rounds.

The PR is ready to merge. Version is 0.16.2, which is correct given the 98c35e3 merge from origin/main at 0.16.1.

[claude]

Claude finished @kyle-sexton's task in 5m 12s —— View job

---

Security Review — PR #895 (head: a407f08)

• Gather context and understand request
• Read PR diff at current HEAD (a407f08)
• Review loop.md — multi-pushurl validation, branch-identity gate, all push paths
• Review safety.md — invariant completeness
• Check 74a4198 and a407f08 for new issues
• Synthesize and post findings

---

All changes in this PR are prose/documentation (markdown + JSON). No GitHub Actions workflows, shell scripts, or executable code are modified. Security findings apply to the behavioral spec these documents encode for AI agents.

---

What a407f08 fixes

Commit a407f08 ("complete babysit fork-push guard (owner + all push urls + branch identity)") addresses the three Codex findings carried over from the 74a4198 round:

Multi-pushurl validation — The prior git remote get-url --push (without --all) returned only the first configured pushurl; git push writes to every one. The fix:

PUSH_URLS=$(git remote get-url --push --all "$PUSH_REMOTE" 2>/dev/null || printf '%s\n' "$PUSH_REMOTE")
BAD=$(printf '%s\n' "$PUSH_URLS" | canon | grep -vxF "$HEAD_ID" || true)
if [ -z "$PUSH_REMOTE" ] || [ -z "$HEAD_ID" ] || [ -z "$PUSH_URLS" ] || [ -n "$BAD" ]; then
  CHECKOUT_MODE="read-only"
fi

git remote get-url --push --all returns every configured pushurl (falling back to the fetch URL when none are set). grep -vxF "$HEAD_ID" collects any URL that does NOT exactly match the head repo's canonical host/owner/repo. Any non-empty BAD → read-only. Empty PUSH_URLS (resolved to nothing) → read-only via [ -z "$PUSH_URLS" ]. Fail-closed at every edge. ✅

Watched-owner trust boundary — A cross-repo head whose push URLs matched the PR head repo but whose owner was outside <watched-owners> (external fork with maintainer edits) previously stayed full-mode. The fix adds:

HEAD_OWNER=${HEAD_REPO%%/*}
case ",<watched-owners>," in
  *",$HEAD_OWNER,"*) : ;;
  *) echo "Cross-repo head owner $HEAD_OWNER outside <watched-owners> — read-only"; CHECKOUT_MODE="read-only" ;;
esac

This gates the fork path on the owner being in the configured trust list before touching the push URL. safety.md updated in lockstep. ✅

Branch-identity check in the HEAD-match path — A clean worktree on an unrelated local branch whose tip happened to equal headRefOid previously entered full mode without a checkout. The fix adds:

CUR_BRANCH=$(git symbolic-ref --short -q HEAD || true)  # empty when detached
# ...
elif [ "$(git rev-parse HEAD)" = "$PR_HEAD" ] && { [ -z "$CUR_BRANCH" ] || [ "$CUR_BRANCH" = "$BRANCH" ]; }; then

A coincidental same-tip match on another branch falls through to gh pr checkout, which puts the worktree on the actual PR branch or detaches correctly. ✅

---

Status of All Prior Findings

Finding	Status
IMPORTANT — || echo origin fallback sends fork-in-detach push to base repo	✅ FIXED (1d7a992)
IMPORTANT — SKILL.md Step 0.3 "merge vs rebase" contradicts cross-tier invariants	✅ FIXED (d2b62d6)
MEDIUM — §5.1.4 fix-cycle git push fails in detached HEAD	✅ FIXED (1d7a992)
LOW — safety.md grammar / orphaned clause	✅ FIXED (1d7a992)
Codex P2 — PUSH_REMOTE uninitialized for conflict push	✅ FIXED (1d7a992)
Codex P2 — orchestration.md conflict-worker inline push remote	✅ FIXED (180793c)
LOW — §5.1.3 "full-mode" prerequisite blocks D1-D5 in read-only	✅ FIXED (d2b62d6)
Codex P2 — conflict-worker rejects asserted detached HEAD	✅ FIXED (33f1fbe)
Codex P2 — fix cycle gated on branch attachment instead of full mode	✅ FIXED (33f1fbe)
IMPORTANT — fork-remote guard accepted base-repo remote by non-origin name	✅ FIXED (eaaa573)
Codex P2 — §5.1.6 cleanup gate excludes detached-HEAD full mode	✅ FIXED (635d5d4)
IMPORTANT — git remote get-url validates fetch URL, not push URL	✅ FIXED (1acce0c)
IMPORTANT — host stripped from URL comparison (different-host same-path bypass)	✅ FIXED (1acce0c)
Codex P1 (1acce0c) — multiple pushurl values not all validated	✅ FIXED (a407f08)
Codex P1 (635d5d4) — watched-owner policy not enforced in bash fork-push gate	✅ FIXED (a407f08)
Codex P2 (1acce0c) — branch identity not asserted in HEAD-match path	✅ FIXED (a407f08)
SUGGESTION — unescaped $BRANCH in sibling-worktree grep	❌ Still open
LOW — §5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode	❌ Still open

---

Fail-closed chain verification for a407f08

Watched-owner gate interaction with URL validation: When case fails (owner not in <watched-owners>), CHECKOUT_MODE="read-only" is set. The URL validation block continues to execute (no early return), resolving PUSH_REMOTE, PUSH_URLS, and BAD. The URL validation can only SET CHECKOUT_MODE="read-only" — it has no path back to "full". So the watched-owner failure is correctly sticky regardless of whether URL validation also fails. All three push sites (freshness merge, conflict-continue, fix-cycle D6) gate on if [ "$CHECKOUT_MODE" = "full" ] — none of them run. ✓

Empty-owner edge case: If gh pr view --json headRepository fails and HEAD_REPO is empty, HEAD_OWNER is also empty. The case pattern *",,"* matches only when <watched-owners> contains a double comma (misconfigured). In a correct config this falls to read-only. Additionally, HEAD_ID=$(gh api "repos/" ...) will fail → HEAD_ID empty → [ -z "$HEAD_ID" ] → read-only. Doubly fail-closed. ✓

BAD computation with empty HEAD_ID: If HEAD_ID is empty, grep -vxF "" matches every non-empty line (empty string with -x only matches empty lines), so all push URLs appear in BAD → read-only. The explicit [ -z "$HEAD_ID" ] guard is redundant but harmless. ✓

isCrossRepository failure: A non-"false" result (including empty) takes the fork/conservative path, where an unresolved push remote or a URL mismatch sends the worker to read-only. ✓

Branch-identity path: git symbolic-ref --short -q HEAD || true produces an empty string in detached HEAD; in that case [ -z "$CUR_BRANCH" ] is true and full mode is granted when HEAD == PR_HEAD. On a named branch, [ "$CUR_BRANCH" = "$BRANCH" ] must also hold. ✓

---

Remaining — SUGGESTION (unchanged, non-blocking)

Unescaped $BRANCH in sibling-worktree grep

loop.md:163

elif git worktree list | grep -vF "$CUR_WT " | grep -q "\[$BRANCH\]"; then

$BRANCH is interpolated as unescaped basic-regex. A . in a branch name (e.g. release/1.2) matches any character and could produce a false positive routing into the --detach path when the branch is not actually locked. The HEAD assertion on line 169 degrades to read-only on mismatch — no unauthorized mutation. Not blocking.

Fix: grep -qF "[$BRANCH]" (fixed-string; brackets are also literal under -F).

---

Remaining — LOW (unchanged, non-blocking)

§5.2 parking: git rev-parse --abbrev-ref HEAD returns "HEAD" in detached mode

loop.md §5.2

After processing a sibling-locked PR in detached full mode, git rev-parse --abbrev-ref HEAD returns the literal "HEAD" and git checkout "HEAD" is a no-op. The next session's HEAD assertion correctly degrades to read-only on mismatch (no wrong write), but the parking invariant is silently broken. Not blocking.

---

Codex P2 from 74a4198 round — false positive

The prior Codex P2 comment on safety.md:35 ("the new invariant says to assert HEAD equals the true PR head before any push — after a freshness merge HEAD is intentionally ahead of headRefOid") is a misreading. The "Before any merge, edit, or push" clause describes the worktree assignment assertion at the start of the session (before the first operation). The second bullet's "Re-check the PR head SHA … before pushing" is re-running gh pr view --json headRefOid to detect concurrent remote pushes — not comparing the local HEAD to headRefOid. After a freshness merge your local HEAD is ahead of headRefOid by design; that is expected and correct. No security issue.

---

No new attack surface in a407f08

• canon() is a renamed equivalent of the prior repo_id() — the sed pipeline is unchanged and traces correctly for all URL forms: HTTPS, SSH-scheme-with-port, SCP-style, with or without user@, .git, or trailing slashes. evil.example.com/owner/repo ≠ github.com/owner/repo → read-only. ✓
• gh api "repos/$HEAD_REPO" --jq .html_url uses GitHub's authoritative html_url; nameWithOwner is GitHub-controlled and restricted to alphanumeric/hyphen/underscore — no injection risk. API failure → HEAD_ID empty → read-only. ✓
• No GitHub Actions changes — no pull_request_target, workflow_run, run: injection surfaces, or permission-widening changes. ✓

---

Bottom line: a407f08 closes the last three open Codex findings: every configured pushurl is validated (not just the first), external-fork owners outside <watched-owners> are rejected before URL validation runs, and the HEAD-match path now requires the correct branch identity or detached HEAD. No new security issues were introduced. All IMPORTANT and CRITICAL findings across all review rounds are resolved. The two remaining items (unescaped branch grep, parking in detached mode) are conservative-failure non-blockers confirmed across all prior rounds and are unchanged by this commit.