refactor(source-control): move the conflict resolver's push to the orchestrator (D10) - #1355
Conversation
…chestrator babysit-prs dispatched merge-conflict resolution to a dedicated subagent that also pushed the result. A dispatched subagent starts with a fresh, isolated context window and never sees the parent conversation (https://code.claude.com/docs/en/sub-agents), so a host runtime that grants mutation authority only from the operator's own turn cannot observe that grant from inside one. The resolver's push could therefore only ever be refused by such a gate or route around it. Split the operation at the authority boundary. The resolver keeps every local step — base fetch, head assertion, git merge (never rebase), marker resolution, local merge commit, affected-file verification — and returns one of resolved / escalate / verification-impossible / no-conflict without touching GitHub. The orchestrator, whose context does hold the operator's turn, performs the push fail-closed: only on resolved, only after re-asserting the live PR head against the merge commit's first parent, re-running the affected-file verification in the worktree itself, and confirming the worktree clean; by refspec, never force. The orchestrator already holds the PR's worker lease across dispatch and cleanup, so the push introduces no unleased window. The pre-push head assertion moves one commit back (HEAD is the merge commit now, its first parent is the vetted head) and the verification is re-run by the pushing agent rather than trusted from a second agent's report. Docs are the contract: orchestration.md gains the Resolver and Orchestrator contracts plus a Conflict-Resolver Prompt Delta — the regular worker template forbids only force-pushing, so a resolver needs an affirmative never-push instruction rather than an exemption note. freshness.md drops its drifting restatement for a pointer. SKILL.md, safety.md, and babysit-loop's Subagents section state the new boundary. test_skill_contract.py pins it. Refs melodic-software/dotfiles#309, melodic-software/dotfiles#315 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6gcbtrPYdtMMwx2M8PfEu
… split The push moved to the orchestrator in the preceding commit; the skill docs, the loop contract, and the contract tests still described the resolver as the pusher. Restate the boundary in each: the conflict worker does the base fetch, head assertion, merge, marker resolution, local merge commit, and affected-file verification, then returns a verdict; the orchestrator owns the push. Tests cover the split rather than assert it in prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bc14fc633
ℹ️ 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".
…olver-push-to-parent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc86231b8
ℹ️ 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".
…olver-push-to-parent
|
Claude finished @kyle-sexton's task in 3m 40s —— View job Code ReviewThe design is well-reasoned: splitting conflict resolution at the authority boundary (not the difficulty boundary) correctly closes the self-authorization hole, the rationale section is clear, and the two-contract structure is clean. The findings below are all on the implementation of that design. Finding 1 — Escalation sequence is logically impossible (correctness)
The prescribed escalation sequence cannot execute as written:
These two steps are mutually exclusive. If conflicted paths are staged with markers still in them, A valid sequence: abort the merge first ( This confirms the Codex P2 at line 526. Fix this → Finding 2 — Fork-head recovery fetches from the wrong remote (correctness)The recovery path for a moved head reads:
For in-owner fork PRs, The recovery step should use the same Finding 3 — Clean-tree check runs before verification, not after (correctness)
The Orchestrator Contract bullet order is:
If the verification step (e.g. a formatter, snapshot updater, or code-generator invoked by The fix is a second Finding 4 — Merge commit's second parent is not verified (defense-in-depth)
The orchestrator asserts:
It does not verify Finding 5 — "Amend... so HEAD stays the reported merge commit" is self-contradicting (clarity)
An amend changes the SHA. Codex findings — dispositionAll four Codex P2 findings are confirmed:
What's working wellThe architectural split itself is sound: the rationale section (why the push stays with the orchestrator) is concrete and cites the subagent context-isolation behavior. The fail-closed principle is consistently applied — |
|
Claude finished @kyle-sexton's task in 3m 53s —— View job Security Review — PR #1355
This PR is documentation/contract only — the changed files are markdown skill contracts and a Python contract-test suite. No executable production code changes. The security surface is therefore the instruction model itself: authorization gaps in the AI agent contracts that could lead to unauthorized GitHub mutations or to wrong content being pushed. IMPORTANT — Fork PR head recovery fetches from
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e6f572589
ℹ️ 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".
The escalation path now preserves partial work with a sequence Git accepts (stage-as-is, concluding commit, WIP branch, reset --keep back — no mid-merge switch, no post-commit merge --abort, never --hard). The worker reports the fetched base SHA and the orchestrator requires it to equal HEAD^2 — two parents alone proves a merge, not the intended one. Superseded-tip recovery re-acquires cross-repo heads through the fork-aware path instead of a bare origin fetch. The verification re-run re-validates HEAD and tree cleanliness afterward, so a formatter-mutated tree cannot lend its green to the pushed commit. The live-head check is repeated immediately before the push, closing the verification-length gap safety.md's check-before-every-push rule already names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 41s —— View job Code Review
|
| Finding | Fix in d1928af |
|---|---|
Escalation sequence (git commit + --abort are mutually exclusive) |
✅ stage-as-is → concluding commit → git branch wip → git reset --keep HEAD^ — --abort never appears |
Fork-head recovery hardcoded origin |
✅ now uses the same fork-aware path as the checkout contract |
| Dirty-tree check ran before verification, not after | ✅ post-run re-validates rev-parse HEAD and status --porcelain |
| Merge commit's second parent unverified | ✅ worker reports fetched base SHA; orchestrator requires HEAD^2 == that SHA |
| Pre-push head check had a verification-length gap | ✅ live-head re-check added immediately before the push command |
Remaining open issues
1 — "Amend... so HEAD stays the reported merge commit" is still self-contradicting (clarity, carry-forward from Finding 5)
orchestration.md lines 510–511
The sentence was not changed in d1928af:
"Amend any post-verification fix into that merge commit rather than stacking a commit on top, so
HEADstays the reported merge commit."
An amend produces a new SHA. After git commit --amend, HEAD is a different commit — not the one originally called "the reported merge commit". The orchestrator's head re-assertion (git rev-parse HEAD == reported SHA, line 575) would fail if the worker reports the pre-amend SHA and then amends. The intent is correct (keep a single commit, not a stack); the prose is wrong. The worker must report the final SHA — the post-amend one — and the orchestrator asserts against that.
2 — Orchestrator re-runs verification commands sourced from an agent that processed untrusted content (security, carry-forward)
orchestration.md lines 599–601
The Orchestrator Contract reads:
"Re-run the affected-file tests/lint/build the conflict worker reported, from that worktree"
The conflict worker reads and interprets conflict markers from untrusted PR content. Nothing in the Orchestrator Contract requires it to derive the verification commands from the target repo's own conventions (e.g., AGENTS.md) independently of what the worker reported. An adversarial PR author crafting conflict markers that influence the worker's sense of "what the repo's verification commands are" has a path to commands the orchestrator will run in the worktree.
The Worker Prompt Template already fences PR-derived fields into an untrusted-data section, but that governs the dispatch prompt — not what the worker writes into its return payload. The fix is to require the orchestrator to derive commands from the repo's conventions independently, treating the worker's reported command list as a cross-check rather than an executable instruction.
New edge case
3 — Escalation: git branch conflict-wip/<pr-number> fails silently if that branch already exists
orchestration.md lines 530–531
The new escalation sequence is:
stage markers →
git commit→git branch conflict-wip/<pr-number>→git reset --keep HEAD^
If the same PR was previously escalated, the conflict-wip/<pr-number> branch already exists and git branch exits non-zero with fatal: A branch named '...' already exists. The contract doesn't say what to do: force-reset the branch to the new WIP commit (git branch -f), pick a versioned name (conflict-wip/<pr-number>-2), or stop. Any of these is reasonable; the contract should specify which, because the failure leaves the worktree at the mid-merge-commit state with the reset not run. Low severity since a human reviews escalations, but the sequence should be mechanically complete.
What's working well
The five correctness/security gaps from the prior round were all addressed with technically precise fixes — the escalation sequence is now a sequence Git will actually execute (no mid-merge branch-switch, no post-commit abort), the second-parent verification closes the wrong-ref-merge hole, the fork-aware recovery fetch correctly mirrors the push path's remote resolution, and both the dirty-tree-after-verification and verification-gap-before-push issues have clean solutions. The commit message accurately describes all five changes.
|
Claude finished @kyle-sexton's task in 6m 3s —— View job Security Review — PR #1355
This PR is documentation/contract only — all changed files are markdown skill contracts and a Python contract-test suite. The security surface is the AI agent instruction model itself: authorization gaps or injection surfaces in the contracts that could lead to unauthorized GitHub mutations or to wrong content being pushed. The latest commit ( IMPORTANT — Orchestrator executes verification commands derived from the conflict worker's report of untrusted contentConfidence: MEDIUM The conflict-worker contract at L514–517 requires the worker to "report the exact commands and their results, named precisely enough for the orchestrator to repeat them." The orchestrator then repeats those commands. The conflict worker reads actual source files containing the conflict markers — untrusted content that is NOT covered by the Worker Prompt Template's untrusted-data fencing (that fencing applies to PR metadata interpolated into the dispatch prompt, not to the files the worker reads on disk). Adversarial text embedded in conflict markers (code comments, string literals, surrounding code) can influence what the conflict worker determines to be the repo's verification commands, and thereby what commands the orchestrator executes. The Conflict-Worker Prompt Delta correctly adds the Fix: the orchestrator should derive verification commands from the target repository's own conventions ( SUGGESTION — "Stays the reported merge commit" is still contradictory after amendConfidence: HIGH "The reported merge commit" implies the SHA already reported, but an amend produces a new SHA. The phrase is self-contradicting: following this instruction (amend) violates the stated outcome (SHA stays the same). This was flagged as code-review Finding 5 and remains in the current wording. The system is safe regardless of ordering: if the worker amends before reporting, it reports the post-amend SHA (correct); if it amends after reporting, the orchestrator's re-assertion of HEAD against the reported SHA fails (no-push). But a worker following the prose literally may amend and then report the pre-amend SHA, believing "HEAD stays the reported commit." The sentence should instead say: amend rather than stacking, then report the final SHA (post-amend), and the orchestrator asserts against that final SHA. Fix this → What the latest commit correctly fixedAll five contract gaps the code review confirmed — and both overlapping findings from the prior security review — are addressed in
No findings in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1928af63e
ℹ️ 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".
…olver-push-to-parent
…olver-push-to-parent
|
Claude finished @kyle-sexton's task in 2m 47s —— View job Code Review
|
| Finding | Status |
|---|---|
Escalation sequence (commit + abort mutually exclusive) |
✅ Fixed (d1928af6) |
Fork-head recovery hardcoded origin instead of fork remote |
✅ Fixed (d1928af6) |
| Clean-tree check ran before verification, not after | ✅ Fixed (d1928af6) |
HEAD^2 not verified against fetched base SHA |
✅ Fixed (d1928af6) |
| Live-head re-check gap before push | ✅ Fixed (d1928af6) |
The three findings above are the only remaining gaps before the contract is tight enough to merge.
|
Claude finished @kyle-sexton's task in 4m 7s —— View job Security Review — PR #1355
This PR is documentation/contract only — all changed files are markdown skill contracts and a Python contract-test suite. The security surface is the AI agent instruction model itself: authorization gaps or injection surfaces in the contracts that could lead to unauthorized GitHub mutations or wrong content being pushed. The IMPORTANT — Orchestrator executes verification commands sourced from conflict worker report of untrusted PR contentConfidence: MEDIUM
The Conflict-Worker Contract requires the worker to:
The Orchestrator Contract then acts on that:
The conflict worker reads actual source files containing conflict markers — untrusted content from the PR. The Worker Prompt Template's A PR author who can craft conflict markers that influence the conflict worker's sense of "what the repo's verification commands are" has a path to commands the orchestrator will execute in the worktree. The injection surface is the translation from "read conflict-marked source" to "choose and name verification commands" — and the worker's return payload carries the result of that translation with no fencing applied. Fix: The orchestrator should derive verification commands from the target repository's own conventions ( This is a carry-forward from the prior two security reviews; it was not addressed in SUGGESTION — "Stays the reported merge commit" is self-contradicting after amendConfidence: HIGH An amend produces a new SHA. After
The orchestrator's re-assertion ( The outcome fields listed in the This is a carry-forward from both prior review rounds and was not addressed in What
|
| Prior finding | Status |
|---|---|
Escalation sequence: commit while unmerged paths exist → --abort (mutually exclusive) |
Fixed — sequence is now: stage, git commit (concludes merge), git branch conflict-wip/<N>, git reset --keep HEAD^ |
Fork-head recovery hardcoded git fetch origin <headRefName> for fork PRs |
Fixed — recovery now explicitly uses the fork-aware path; bare git fetch origin on a fork PR is called out as wrong |
| Dirty-tree check ran before verification (formatter/snapshot-updater could dirty the tree between check and push) | Fixed — second git status --porcelain check now required after verification re-run |
HEAD^2 never verified (wrong-ref merge passes all prior checks) |
Fixed — worker reports fetched base SHA; orchestrator requires HEAD^2 to equal it |
| Live-head re-check happened before the verification re-run, not before the push | Fixed — gh pr view headRefOid == HEAD^1 is now repeated immediately before the push command |
No new security findings in safety.md, test_skill_contract.py, SKILL.md variants, freshness.md, plugin.json, or CHANGELOG.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b90830fe
ℹ️ 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".
…olver-push-to-parent
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
…-free The escalation sequence concluded the merge with a porcelain git commit — which runs the repository's pre-commit and commit-msg hooks, and a hook that rejects conflict markers or the WIP message aborts it with --no-verify forbidden, stranding the worktree mid-merge — and parked the work on a fixed conflict-wip/<pr-number> ref, which collides the moment the same PR escalates twice. The preservation commit is now created with plumbing (write-tree + commit-tree with both parents), which runs no hooks by design rather than by bypass; the branch is qualified by the commit's own short SHA so every attempt is preserved; and the worktree exits the merge with git merge --abort taken only after preservation, when MERGE_HEAD still exists and nothing resolved is lost — the abort-as-status-report objection the resolve-conflicts skill states does not reach a sequence that commits the work first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ha8DkNT4nSnVjKDNWVpj3w
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f86644b15a
ℹ️ 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".
…olver-push-to-parent
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17ba49fe19
ℹ️ 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".
Three ways the conflict-push contract could strand state or contradict itself. A no-push outcome left the worktree on the unpushed merge commit, which the next cycle's head assertion refuses — it now preserves the commit on the SHA-qualified conflict-wip scheme and returns the worktree to the asserted head with reset --keep before the lease releases. Verification byproducts were allowed at push time but classify as keep_dirty in the prune helper, leaving an integrated worktree neither prunable nor reusable — the orchestrator now snapshots status around the re-run and deletes exactly the paths it added, never git clean, never pre-existing entries. And safety.md's unconditional HEAD == headRefOid pre-push assertion contradicted the first-parent push this contract performs — the assertion now codifies that single exception (two parents, first parent equals the live head re-checked immediately pre-push, second parent the reported base) and binds everywhere else unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ha8DkNT4nSnVjKDNWVpj3w
…olver-push-to-parent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90156c60f6
ℹ️ 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".
The no-push cleanup applied preserve-and-reset to every outcome, but escalate and no-conflict leave the worktree already at the asserted head — resetting there rewinds the real PR head — and an interrupted worker may still be mid-merge. The unwind is now keyed to the worktree's actual state: preserve+reset only behind a verified two-parent merge commit at the asserted head's child, the escalation sequence's own plumbing preservation + abort when MERGE_HEAD exists, nothing when already clean at the head, and report-don't-guess otherwise. The byproduct cleanup likewise spans both verification runs: the worker's leavings predate the orchestrator's snapshot, so the deletion set is the union of the re-run delta and the untracked paths the worker's report names — which the Worker Contract now requires listing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ha8DkNT4nSnVjKDNWVpj3w
…olver-push-to-parent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed368c506
ℹ️ 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 encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
The verification-byproduct cleanup ran only after a successful push, so a failed or superseded re-run left its own leavings to fail the next assignment's clean-checkout requirement — the deletion of the two named byproduct sets now runs on every exit, push or no-push, with genuinely pre-existing entries still retained. And the pre-push check proved only that the merge integrated the base SHA the worker fetched, never that this SHA was still the live base tip — the orchestrator now re-fetches the base ref immediately before the push and requires its fresh tip to equal HEAD^2, treating a moved base as a stale resolution: unwind per the state-keyed rules and dispatch a fresh conflict worker against the new base, rather than landing a merge that re-conflicts the PR at the cost of a pointless merge commit and CI round. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ha8DkNT4nSnVjKDNWVpj3w
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 213531c230
ℹ️ 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".
Version only: #1355 took 0.29.0 while this branch was open, so this work moves to 0.30.0 and its changelog section is renamed. #1355's own 0.29.0 section is kept in place beneath it. No content conflict; 388 python tests and 123 gate tests pass. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
No linked issue ## Summary Four stranded P2 review findings, each filed by `chatgpt-codex-connector` on a merged `source-control` PR and left unresolved, verdicted REAL by an independent auditor at `main`. All four are discharged here. A fifth thread on the same sweep (`babysit-prs/reference/safety.md:457`, from #1264) was verdicted ALREADY-FIXED by `72ee66e0` and carries **no change** in this PR. Filed line anchors are stale on several of these threads; every fix was made against the complaint text, not the anchor. ## Fix ### `exec-bit-check.sh` keys its candidate set on a new index *entry* (#1590) `git diff --cached --name-status` reports the same staged file as `A <path>` with rename/copy detection off and as `R<score> <old> <new>` / `C<score> <src> <dst>` with it on. The script read and discarded both pair forms, so whether a newly added shebang file staged `100644` got caught was a function of the consumer's `diff.renames` setting rather than of the staged content. A pair destination is now a candidate when its **source was `100755`** — the mode pairing that means the bit was *dropped*. The scan reads `git diff --cached --raw` rather than `--name-status` for exactly this reason: only the raw record (`:<srcmode> <dstmode> <srcsha> <dstsha> <status>`) carries the source mode. The existing `100644`-plus-shebang filter still does the rest. ### `prune_babysit_worktrees.py` restores the gitfile on every surviving path (#1331) Restoration was keyed on `rmdir` raising. Two other paths leave the directory standing: the rescan after the unlink can itself raise, and a file appearing between the unlink and the rmdir skips the removal *without raising at all*. Either way the directory outlived the only record of its owning repository, turning a retryable failure into a permanent `unresolved`. Restoration is now keyed on whether the removal actually happened (a `removed` flag, not a second `exists()` probe — a probe that transiently failed would skip the restore precisely when the directory survives), and the `Path.exists()` probe runs inside the guarded write. ### Two defects this PR's own first pass introduced, caught in review and fixed here Both were filed by `chatgpt-codex-connector` on this PR, both reproduced before fixing, both real. - **A source-mode-blind candidate set** (`exec-bit-check.sh`). Widening to every `R*`/`C*` destination reported a shebang file that is *deliberately* non-executable — a sourced library, a template — merely for being renamed, and `--fix` flipped it to `100755`. Nothing dropped a bit; the file is already tracked, outside the newly-added-only scope. Reproduced: a committed `100644` shebang plus `git mv` gives `:100644 100644 … R100`, and the pre-fix script listed the destination. Hence the source-mode gate above. - **An unguarded existence probe** (`prune_babysit_worktrees.py`). `pointer.exists()` sat in the `finally` *outside* the try guarding the write. `Path.exists()` re-raises an `OSError` whose errno is outside the ignored not-found family, so a permission denial on the very directory the block exists to rescue escaped the `finally` — replacing the original exception and leaving the pointer deleted, the exact loss the block prevents. On `main` this was contained because the probe sat inside an `except OSError` handler; moving it to `finally` uncontained it. The probe is now inside the guard. ### The conflict orchestrator runs base → head → push, in that order (#1355, two threads) Both threads edit the same push-contract bullet, so they land together. - `safety.md` requires the head check immediately before every push, but the base re-fetch — a network round trip — sat between that check and the push, re-opening the exact window the check closes. The contract is now a three-step numbered list with nothing between step 2 (head) and step 3 (push). - Both orchestrator head checks now spell `GH_REPO=<owner>/<repo>`. The bare `gh pr view <N>` had no target: the orchestrator's cwd is whatever the fleet run started from, never reliably the target repository. Deliberately **not** changed: the bare `gh pr view --json headRefOid` at `orchestration.md:513`. That one is in the **Conflict-Worker Contract**, whose cwd *is* the assigned worktree, and the worker contract's own rule offers `cd`-into-the-worktree and `GH_REPO` as alternatives. The finding scopes itself to "both orchestrator head checks". ### The `VALID (defer)` grounding rule states its no-tracker branch (#1633) **Narrower than filed.** The finding claims the missing branch "permanently blocks `full` mode"; it does not — `pull-request/SKILL.md` §Adapting to your environment and a `VALID (fix now)` reclassification both already escape it. The real defect is the *unstated branch*: the rule mandated filing a tracker item before the D5 reply and said nothing about the consumer with no tracker, even though the same skill documents a tracker as optional. That branch is now stated. The CHANGELOG entry was rewritten to the narrower framing rather than restating the overstated claim. **Surface scope, stated explicitly.** The branch is added to the three surfaces that state the *filing mandate*: the canonical `reference/review-discipline.md` §3 clause and its `pull-request/SKILL.md` and `pull-request/reference/monitor.md` restatements. `babysit-prs/reference/independent-resolution.md` also carries the `D4.6-deferral-grounding` tag and is deliberately left alone — it states what an already-chosen `VALID (defer)` must *show* (eligibility), not an instruction to file, so it has no dead end to branch out of. One rule, both directions: mandate ⇒ branch, eligibility criterion ⇒ no branch. `monitor.md` is **not** forced by `check-contract-clause-coverage.py` — verified by reverting only that file's hunk and re-running the gate, which still passes. It is included on the merits above. ## Verification Every fix was confirmed to stop the complaint reproducing, each with a pre-fix control proving the fixture discriminates. **#1590** — `git version 2.54.0.windows.1`. Same fixture, three configurations: | fixture | raw record | `origin/main` `--list` | this branch `--list` | | --- | --- | --- | --- | | copy, **control** (`diff.renames` unset) | `A dest.sh` | reported | reported | | copy, `diff.renames=copies` | `:100755 100644 … C095 src.sh dest.sh` | *(nothing)* | `dest.sh` | | rename, **default** config | `:100755 100644 … R100 src.sh moved.sh` | *(nothing)* | `moved.sh` | | rename off a **non-exec** source | `:100644 100644 … R100 lib.sh lib-moved.sh` | *(nothing)* | *(nothing)* | The control row is the discriminator: the identical `cp` produces `A` with copy detection off and `C095` with it on, and the destination stages `100644` in both. The last row is the regression guard — nothing dropped a bit there, so nothing is reported on either tree. Running this branch's `exec-bit-check.test.sh` against `origin/main`'s script fails exactly cases 50 and 52; against the intermediate source-mode-blind version it fails exactly case 59; against this branch, **59 cases, 0 failures**. Fixture-assertion cases 57/58 pass on every tree, which is what makes 59 a real discriminator rather than a broken fixture. Note for reviewers: the sibling `--fix -- <dest>` cases pass on *both* trees and are not discriminating — a pathspec naming only the destination breaks the rename pairing back to `A`. The defect is in unscoped detection (`--list` / `--probe` / `--fix --all`), which is what cases 50/52 cover. Case 56 is the other negative half: an ordinary rename that *kept* `100755`. **#1331** — both new tests run against `origin/main`'s module (branch tests, old code) **FAIL** on `assertTrue(pointer.is_file())`. The third test (the raising probe) **ERRORs** with an escaped `PermissionError` against the intermediate version, while its sibling passes there — the control that shows the new fixture targets the new defect. Against this branch the full suite is **45 tests, OK**. **#1355** — prose. Control: `origin/main`'s bullet textually places the base re-fetch after the head check and before the push ("Revalidate the base side in the same breath"). Current: numbered 1-Base / 2-Head / 3-Push with nothing between 2 and 3, and `GH_REPO=` on both orchestrator head checks (`:615`, `:681`). **#1633** — prose. Control: `origin/main` states the filing mandate on all three surfaces with no no-tracker branch. The cited escape hatch (`SKILL.md` §Adapting to your environment, line 36) was read and does say a work-item tracker is optional and that its absence must never block a phase. **Gates run locally from the worktree root, all green:** - `python scripts/check-contract-clause-coverage.py` — passed (4 canonical, 14 tagged restatements, 16 pointing surfaces) - `scripts/check-changelog-parity.sh` `--check` / `--check-order` / `--check-bump origin/main` - `scripts/check-contract-slice-prune.sh` `--check` / `--check-diff origin/main` - `scripts/check-shell-portability.sh origin/main`, `scripts/check-skill-portability.sh origin/main` - `scripts/check-changed-skills.sh origin/main` - `scripts/validate-plugins.sh` - `markdownlint-cli2` over all six changed markdown files — 0 errors - `shellcheck -x` over both changed shell files — clean - `scripts/run-ruff.sh check plugins/source-control` — all checks passed - all nine affected `source-control` suites from `scripts/affected-tests.sh origin/main`, plus the two suites the changed scripts own (`exec-bit-check.test.sh` 59/59, `test_prune_babysit_worktrees.py` 45/45) The prune test file's diff is **purely additive** vs `origin/main` — an editor format-on-write pass had rewrapped three untouched regions, and that drift was stripped so every hunk maps to a finding. `plugins/source-control/skills/babysit-loop/SKILL.md` is untouched and stays at 499 lines. Version `0.48.0` → `0.49.3`, renumbered above `main`'s current `0.49.2` after the merge. ## Related Refs #1590, #1331, #1355, #1633 — the merged PRs carrying the four review threads. Refs #1264 — the fifth thread on this sweep, verdicted ALREADY-FIXED (`72ee66e0`); no change here. Refs #1939 — owns the defects in `babysit_resolve_thread.py`, deliberately untouched by this PR. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
D10 of the auto-mode migration: split conflict resolution along the tier-evidence line. A typed conflict-resolution subagent receives its delegation message and no conversation history, so it cannot see the operator message that establishes the worker/autopilot tier — and a delegation message counting as tier evidence would reopen the self-authorization hole the tier condition exists to close.
refactor(source-control): move the conflict resolver's push to the orchestrator— the dispatched resolver now produces resolved text and re-runs lint only; the tier-gated acts (merging the base branch into the PR branch, pushing without force) stay in the parent context that holds the operator's tier-naming message.docs(source-control): align the conflict-resolution contract with the split— skill docs, loop contract, freshness/safety/orchestration references aligned; contract tests added.Contract tests pass: 18 passed, 40 subtests.
Related
No linked issue. Implements decision D10 of the auto-mode migration (melodic-software/dotfiles#315 is the host-side counterpart; the conflict-resolution allow entry there states the same split).
🤖 Generated with Claude Code