Skip to content

feat(work-items): specify autonomous orchestrator-dispatch PR lifecycle (#572)#1244

Merged
kyle-sexton merged 6 commits into
mainfrom
feat/572-orchestrator-dispatch-lifecycle
Jul 24, 2026
Merged

feat(work-items): specify autonomous orchestrator-dispatch PR lifecycle (#572)#1244
kyle-sexton merged 6 commits into
mainfrom
feat/572-orchestrator-dispatch-lifecycle

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #572

Summary

Resolves the previously-deferred autonomous orchestrator-dispatch lifecycle — the seam that spanned /work-items:work, /implementation:implement-dispatch, /source-control:worktree, and /source-control:pull-request, where each sibling skill's preflight assumed the others already solved provisioning and PR-creation timing. Adopts the posted decision brief's Option A (orchestrator-owned PR invocation), reconciled with the interim worker-side-provisioning workaround already landed in work-loop.

The lifecycle is now specified end-to-end across its three facets:

  • Provisioning is worker-side. The dispatched worker materializes its own out-of-tree worktree as its first step (via /source-control:worktree's non-entering seam when installed, or a plain git worktree add) and works it via git -C without entering it — matching implement-dispatch's existing worktree-cwd contract, and avoiding any dependency on EnterWorktree-in-a-subagent. The orchestrator never invokes /source-control:worktree create (its EnterWorktree terminal would transition the orchestrator's own session). The worker commits, pushes, and brings the branch current with the default branch before returning the worktree path + branch; a worker that cannot provision parks and escalates.
  • PR creation is orchestrator-owned. After the worker returns and the pre-PR diff gate passes, the orchestrator opens the PR via a new /source-control:pull-request create --pushed --worktree <path> PR-only entry that re-resolves branch and diff from the target worktree (ignoring the session-cwd pre-computed context), passes the branch explicitly to parse-branch-issue.sh so Closes #N is parsed from the worker's branch, skips commit/push/rebase, and calls gh pr create --head <branch>. The worker scope-fence forbids PR creation; detection of a consuming project's own PR stage lives in the orchestrator (invoke-vs-defer).
  • Fix re-dispatch + worktree persistence. Branch-owned fixes (failing CI, review findings) re-dispatch a fresh scope-fenced subagent into the same persisted worktree — the worktree is the state carrier across dispatches. It persists through the whole PR lifecycle and is cleaned up only by whoever merges, never by the work lane.

work-loop's former interim #572 workaround (SKILL + evals) is reframed as the now-canonical behavior it inherits from work.

Authority

The needs-human + wayfind: design decision was delegated to the session by the operator (session d557362f handoff, "have you do ALL of these"); the delegation is the human decision. A decision-adoption comment is posted on #572. Labels are left in place — repo convention keeps needs-human / wayfind: design on issues through close (e.g. #1065, #1041, #696, #695, #684 all retain them closed).

Test plan

  • changelog-parity --check-bump origin/main: green — all three bumped plugins (work-items 0.22.1→0.23.0, implementation 0.7.8→0.8.0, source-control 0.25.1→0.26.0) carry a matching ## [<version>] CHANGELOG entry.
  • markdownlint-cli2 over all 9 changed markdown files: 0 errors.
  • skill-quality static gate (check-skill.sh) on all 5 changed skills (work, work-loop, implement-dispatch, pull-request, worktree): PASS, 0 errors (only pre-existing line-count / Gotchas / quote-style warnings inherited from base).
  • check-skill-portability, check-skill-leaf-names, check-orphaned-fixtures, check-silent-skips: all green.
  • Both changed evals.json (work, work-loop) validate against plugins/skill-quality/reference/evals.schema.json.
  • Docs-only change (SKILL/reference/CHANGELOG/manifest prose + version bumps); no shell or hook logic touched, so preflight.test.sh and the shared-lib tests are unaffected.

Related

🤖 Generated with Claude Code

…le (#572)

Resolves the previously-deferred autonomous orchestrator-dispatch lifecycle
seam that spanned work-items, implementation, and source-control. Adopts the
posted decision brief's Option A (orchestrator-owned PR invocation), reconciled
with work-loop's landed interim workaround.

- work-items (0.23.0): `work`'s execute step now specifies the full lifecycle —
  worker-side worktree provisioning (materialize via git worktree add / the
  non-entering seam, work via `git -C`, never enter; orchestrator never invokes
  `/source-control:worktree create`), orchestrator-owned PR creation after the
  pre-PR diff gate via the new `create --pushed` entry (worker scope-fence
  forbids PR creation), and fresh-subagent fix re-dispatch into the same
  persisted worktree. `work-loop`'s interim #572 workaround (SKILL + evals) is
  reframed as the now-canonical behavior it inherits; `work`'s evals updated.
- implementation (0.8.0): `implement-dispatch`'s brief-composition step gains an
  additive, autonomous-scoped worker-side provisioning clause; the interactive
  "brief supplies the worktree path / anchor with git -C" default is unchanged.
- source-control (0.26.0): `pull-request` gains a `create --pushed --worktree`
  PR-only entry (create.md §2.7) that re-resolves branch/diff from the target
  worktree, passes the branch explicitly to parse-branch-issue.sh, skips
  commit/push/rebase, and passes explicit `--head`; `worktree`'s create action
  documents that orchestrated provisioning does not use it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review

Tasks:

  • Load tools and initialize
  • Read changed files and gather context
  • Review skill specifications for correctness and completeness
  • Review CHANGELOG and version bumps
  • Review plugin manifests
  • Review eval changes
  • Synthesize findings and post review

Summary

This PR correctly resolves the multi-plugin autonomous dispatch lifecycle gap tracked in #572. The three-facet design (worker-side provisioning, orchestrator-owned PR, same-worktree fix re-dispatch) is internally consistent, and the supporting changes to implement-dispatch, pull-request, and worktree correctly scope each plugin to what it owns. One real gap and one minor fragility are noted below.


Finding 1 — work/SKILL.md invokes create --pushed without --worktree <path> (gap)

work/SKILL.md line 202:

"the orchestrator invokes the PR-only entry /source-control:pull-request create --pushed against the worker's returned worktree/branch"

The pull-request action table (line 54) names the mode create --pushed --worktree <path>, and create.md §2.7 binds the worktree path via that argument (WT="<path>" # from --worktree). Without --worktree <path>, WT has nothing to resolve from and the mode cannot re-resolve the branch or diff from the target worktree.

The worker does return the worktree path — the preceding clause says so — but work's orchestrator step doesn't wire it into the invocation. An agent following work's prose could invoke create --pushed alone and rely on §2.7 to reveal the missing flag only as a second lookup. The mode name in pull-request/SKILL.md's table makes the flag discoverable, but work's own text should be the authoritative call-site spec.

Suggested fix: add --worktree <path> to the invocation prose in the "Open the PR" step — e.g. change the phrase to create --pushed --worktree <returned-path>. Fix this →


Finding 2 — @{u} precondition in create.md §2.7 errors when no upstream is configured (minor fragility)

create.md line 523:

git -C "$WT" log @{u}..

If the worker pushed with a plain git push origin <branch> (no -u), there is no tracking upstream set in the worktree and this command exits with "fatal: no upstream configured for branch '…'" rather than printing nothing. The subsequent check ("the branch resolves on the remote") is stated as a separate prose condition and isn't expressed in the shell snippet, so the failure mode is "command errors" rather than "prints commits". In practice the worker brief in implement-dispatch instructs the worker to push before returning, which for new branches typically sets upstream — but this is a fragility if a worker uses a push variant that doesn't.

The ambiguity is low-impact (both "no upstream" and "unpushed commits" are STOP conditions, just with different error messages), but documenting the edge case or using git -C "$WT" log origin/"$BRANCH"..HEAD 2>/dev/null would make the check more robust.


What's working well

  • Lifecycle split is clean. implement-dispatch gains only one additive clause for the autonomous case; the interactive default (brief supplies the worktree path) is untouched. The PR-only entry in pull-request is additive and correctly skips the steps the worker already did.
  • parse-branch-issue.sh explicit first-positional (parse-branch-issue.sh "$BRANCH" [...]) in §2.7 is the right fix for the session-cwd process-spawn issue. The rationale comment explains exactly why implicit use of git branch --show-current breaks here.
  • worktree/SKILL.md note is correct and well-scoped. "Orchestrated provisioning does not use this action" with the EnterWorktree rationale removes the ambiguity about when create applies without adding any constraint on other callers.
  • Evals are tight. Eval feat: markdown-formatter plugin (pilot migration) #3 for work covers all eight of the lifecycle boundaries that matter (worker-side provisioning, orchestrator-owns-PR, worktree-as-state-carrier, fresh-subagent-for-fixes, ## Related read-modify-write, babysit handoff). The updated work-loop eval fix: correct plugin-system inaccuracies and harden references #2 correctly reframes the interim workaround as canonical behavior with no remaining "workaround" language.
  • Version semantics are correct. All three are MINOR bumps (new backward-compatible surface). No existing callers break.
  • CHANGELOGs match the diff. The three entries accurately describe what each plugin gained, including the work-loop reframing as inherited-not-workaround.

Overall: approve with the --worktree <path> fix in mind. Finding 1 is the only gap that could cause a real behavioral deviation at agent-execution time; Finding 2 is a heads-up for a narrow edge case. Everything else is well-specified and internally consistent.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244

  • Gather context and read PR diff
  • Audit changed skill files for injection surfaces and unsafe constructs
  • Review worktree/branch provisioning logic for path traversal or arbitrary command injection
  • Check PR-creation seam for authorization/access-control gaps
  • Inspect evals.json and manifest changes for supply-chain risk
  • Compile and post findings

Scope

All 14 changed files reviewed:

  • 3 × plugin.json (version bumps only)
  • 3 × CHANGELOG.md (prose only)
  • 5 × SKILL.md / reference/create.md (AI agent instruction updates)
  • 2 × evals.json (evaluation expected-output updates)

No shell scripts, GitHub Actions workflows, package dependencies, or credentials are added or modified.


Findings

No CRITICAL or IMPORTANT security issues found.

SUGGESTION — Confidence: LOW

The new create --pushed --worktree <path> seam introduces a pattern where the orchestrator uses a worker-supplied path in subsequent shell commands (git -C "$WT", gh pr create --head "$BRANCH"). The documented examples correctly show double-quoted variable expansion ("$WT", "$BRANCH"), and git branch names are structurally constrained against most shell metacharacters by git's own validation rules. The path originates from another Claude agent session (the dispatched worker), not from external user input. The practical injection surface here is very narrow, but if the AI fails to faithfully replicate the quoting from the example (e.g., drops the quotes around $WT in an ad-hoc command), a crafted worktree path could be problematic. This is inherent to prose-instruction-driven agent systems and is not a regression introduced by this PR — the pattern is correctly documented, with the existing --pushed preconditions (status --porcelain + log @{u}.. assertions) serving as a consistency gate before the path is used.

No other injection surfaces, path traversal vectors, authorization gaps, supply-chain changes (no action pins or dependency versions loosened), dangerous GitHub Actions patterns, or secret/credential flows are present in this diff.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 300ad22b03

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/source-control/skills/pull-request/reference/create.md Outdated
Comment thread plugins/work-items/skills/work/SKILL.md Outdated
…-attach (#572)

Address two P2 review findings on the #572 lifecycle spec:

- create.md §2.7 verified "nothing unpushed" via `git log @{u}..`, which exits
  128 when the worker pushed without `-u` (no upstream configured) even though
  the branch is fully on the remote. Replace with an upstream-independent
  remote-tip-equals-HEAD comparison.
- work SKILL's provisioning clause always used `git worktree add -b <name>`,
  which fails on an existing branch — including one the user created via the
  Suggest-branch-name step's existing-branch detection. Attach an existing
  branch without `-b` in that case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (updated after fix commit ad7b147)

Tasks:

  • Load tools and initialize
  • Read the new fix commit changes
  • Check if Finding 1 (--worktree <path> missing) is addressed
  • Check if Finding 2 (@{u} fragility) is addressed
  • Check if Codex P2 (git worktree add -b on existing branch) is addressed
  • Synthesize and post final review

What the fix commit addressed

Finding 2 + Codex P2 #1 (the @{u} precondition) — FIXED ✅

create.md line 523–530 now replaces git -C "$WT" log @{u}.. with a direct fetch + ref comparison that does not depend on upstream tracking being configured:

REMOTE=$(git -C "$WT" config "branch.$BRANCH.remote" 2>/dev/null || echo origin)
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
[ "$(git -C "$WT" rev-parse HEAD)" = "$(git -C "$WT" rev-parse "$REMOTE/$BRANCH" 2>/dev/null)" ] \
  || { echo 'worker branch is not fully pushed to the remote' >&2; exit 1; }

This is the correct fix. The REMOTE fallback to origin is sound since that's the canonical push target when no tracking is configured.

Codex P2 #2 (git worktree add -b on an existing branch) — FIXED ✅

work/SKILL.md line 194 now explicitly distinguishes the two worktree-attach forms:

"the worker attaches it to the worktree with git worktree add -b <name> <path> <base> for a new branch, or git worktree add <path> <name> (no -b) when that sub-step already detected the branch as existing-b fails outright on an existing branch, including one the user created by following that sub-step"

The existing-branch detection is already defined in the preceding Suggest branch name sub-step, so the dispatch brief carries the "new vs. existing" classification and the worker can branch correctly without re-detecting.


What remains open

Finding 1 — work/SKILL.md line 202 still omits --worktree <path> (gap, unaddressed)

work/SKILL.md line 202:

"the orchestrator invokes the PR-only entry /source-control:pull-request create --pushed against the worker's returned worktree/branch"

The action table in pull-request/SKILL.md line 54 names the full mode as create --pushed --worktree <path>, and create.md §2.7 (line 516) binds the worktree path exclusively from that flag (WT="<path>" # from --worktree). Without it, $WT is unbound and every subsequent git -C "$WT" in the precondition checks and sub-steps evaluates against a null/empty path.

The fix commit updated the neighboring -b clause in the same paragraph (line 194) but left line 202's invocation prose unchanged. This was flagged by the earlier Claude review and is still the only gap that would cause a real behavioral deviation at agent-execution time.

Suggested fix: change the invocation phrase at line 202 from create --pushed to create --pushed --worktree <returned-path> — the worker already returns the worktree path in the preceding clause, so the value is available. Fix this →


Summary

2 of 3 prior findings fixed in this commit. The @{u} fix is clean and robust; the -b/no--b split is correct and well-explained. Finding 1 (missing --worktree <path> in work/SKILL.md line 202) is the sole remaining gap — one-word prose fix. Everything else in the PR remains well-specified.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244

Scope: Both commits reviewed (300ad22, ad7b147). All 14 changed files are AI agent instruction prose (SKILL.md, reference/create.md, CHANGELOG.md, plugin.json, evals.json). No shell scripts, GitHub Actions workflows, package manifests, or credentials are added or modified.


No CRITICAL or IMPORTANT security issues found.


SUGGESTION — Worker-returned path/branch injection surface (Confidence: LOW)

File: plugins/source-control/skills/pull-request/reference/create.md L515–543

The new §2.7 entry constructs shell commands from two worker-supplied values: the --worktree <path> argument (WT) and the branch name derived from the worktree (BRANCH). All example snippets in the prose correctly double-quote both values throughout:

WT="<path>"
BRANCH=$(git -C "$WT" branch --show-current)
REMOTE=$(git -C "$WT" config "branch.$BRANCH.remote" 2>/dev/null || echo origin)
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
git -C "$WT" rev-parse "$REMOTE/$BRANCH" 2>/dev/null
gh pr create --head "$BRANCH" ...

The quoting discipline is correct in every example. The practical injection surface is narrow for two reasons: git's own branch-naming validation rejects most shell metacharacters in branch names (spaces, ~, ^, :, ?, *, [, \, and ../@{ prefixes), and the path comes from a trusted Claude agent in the same operator-controlled session. This is the same SUGGESTION-level pattern already noted in the prior security review and is inherent to prose-instruction-driven agent systems — not a regression introduced by this PR.

One nuance worth noting: $BRANCH is expanded inside the git config key string "branch.$BRANCH.remote". If the worktree is in detached-HEAD state, git branch --show-current returns empty, making the key "branch..remote"git config returns nothing, and the || echo origin fallback fires correctly. No injection vector here; the failure mode is safe.


What's clean

  • No GitHub Actions changes. No pull_request_target, workflow_run, or run: script-injection risk introduced.
  • No supply-chain changes. The three plugin.json files are version-number-only bumps (0.7.8→0.8.0, 0.25.1→0.26.0, 0.22.1→0.23.0); no action pins or dependency versions are altered.
  • No credential/token flows introduced. gh pr create, git fetch, and git push run with the agent's ambient GitHub credentials already present in the session; no new token handling or storage is specified.
  • Authorization split is sound. The worker scope-fence explicitly forbids PR creation; PR creation is gated behind the orchestrator's own pre-PR diff read. A compromised or hallucinating worker cannot open a PR on its own — the orchestrator's separate verification step is the gate.
  • @{u} fix is correct and closes the false-STOP risk. The replacement (git fetch + SHA comparison) is both upstream-independent and structurally safe: rev-parse "$REMOTE/$BRANCH" is double-quoted, and the 2>/dev/null fallback makes a missing remote ref produce an empty string, which fails the equality check and triggers STOP as intended.
  • No PR body injection surface. The assembled $BODY is passed as a double-quoted argument to gh pr create, so newlines and shell metacharacters in issue content are treated as literal text by the shell and by the GitHub API downstream.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad7b1479d2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/implementation/skills/implement-dispatch/SKILL.md Outdated
Comment thread plugins/work-items/skills/work/SKILL.md Outdated
… worktree path (#572)

Address two more review findings on the #572 lifecycle spec:

- implement-dispatch Prerequisites (P1): `/implementation:implement` Step 1
  stops on the default branch, but under worker-side provisioning the
  orchestrator legitimately stays on the default branch (it never edits source)
  and each worker provisions its own non-default branch first. Add an explicit
  exception so the branch-correct check governs where the worker's edits land,
  not the orchestrator's checkout — a default-branch autonomous start no longer
  parks at the preflight.
- work SKILL (P2): the orchestrator's PR-only invocation now passes
  `--worktree <returned-path>` explicitly, since it stays in its own checkout
  and the mode needs the path to resolve the worker's branch and diff.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (updated after fix commit a692d8f)

Tasks:

  • Load tools and initialize
  • Read the new fix commit changes
  • Check if Finding 1 (--worktree <path> missing in work/SKILL.md line 202) is addressed
  • Check if Codex P1 (preflight ordering in implement-dispatch) is addressed
  • Synthesize and post final review

What the fix commit addressed

Finding 1 — work/SKILL.md line 202 missing --worktree <path> — FIXED ✅

work/SKILL.md line 202 now reads:

"the orchestrator invokes the PR-only entry /source-control:pull-request create --pushed --worktree <the worker's returned worktree path> — passing that path explicitly, since the orchestrator stays in its own (default-branch) checkout and the mode needs the path to resolve the worker's branch and diff rather than the orchestrator's."

This is the correct and complete fix. The --worktree flag is now present in the invocation prose, $WT in create.md §2.7 has a binding, and the rationale explains exactly why the flag is necessary — tying it to the orchestrator's default-branch position that the implement-dispatch fix formalizes.

Codex P1 — preflight ordering in implement-dispatch/SKILL.md — FIXED ✅

implement-dispatch/SKILL.md lines 31–32 adds:

"Exception under worker-side provisioning (the autonomous work-lane): Step 1's branch correct (never the default branch) check governs where the worker's edits land... A default-branch autonomous start is therefore valid and MUST NOT stop this preflight — the invariant is satisfied per-worker at provisioning time, never by the orchestrator's own session sitting on a feature branch. Only the plan-present and no-unrelated-dirty-tree checks apply to the orchestrator's own session."

This is the right design. The exception is correctly scoped to only relax the non-default-branch check; the plan-present and no-unrelated-dirty-tree checks are explicitly stated to still apply. The "MUST NOT stop" instruction is appropriately strong. The invariant is correctly preserved — it's satisfied per-worker at provisioning time rather than by the orchestrator sitting on a feature branch.


All prior findings are now resolved

Finding Commit Status
F2: @{u} upstream dependency ad7b147 ✅ Fixed
Codex P2-#2: -b on existing branch ad7b147 ✅ Fixed
F1: missing --worktree <path> in work/SKILL.md a692d8f ✅ Fixed
Codex P1: preflight blocks orchestrator on default branch a692d8f ✅ Fixed

The three-commit sequence is internally consistent and complete. The two fixes in a692d8f also cross-reinforce each other: the implement-dispatch exception explains why the orchestrator legitimately stays on the default branch, and the work/SKILL.md invocation now correctly names the path flag that create.md §2.7 requires to resolve the worker's branch from that position.

Overall: all identified gaps are closed. Ready to merge.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244 (all 3 commits: 300ad22, ad7b147, a692d8f)

Tasks:

  • Load tools and initialize
  • Fetch PR diff and read changed files
  • Audit implement-dispatch preflight exception for authorization bypass
  • Re-audit create.md §2.7 injection surfaces (new commit)
  • Check work/SKILL.md --worktree fix for new surfaces
  • Verify no new credential/token flows, Actions changes, or supply-chain changes
  • Compile and post findings

Scope

All 14 changed files reviewed across all 3 commits. The diff touches:

  • 3 × plugin.json (version-number-only bumps)
  • 3 × CHANGELOG.md (prose only)
  • 5 × SKILL.md / reference/create.md (AI agent instruction updates)
  • 2 × evals.json (evaluation expected-output updates)

No shell scripts, GitHub Actions workflows, package manifests, or credentials are added or modified in any commit.


Findings

No CRITICAL or IMPORTANT security issues found.


SUGGESTION — Confidence: LOW

Inherited from prior reviews: worker-returned path/branch injection surface in create.md §2.7

plugins/source-control/skills/pull-request/reference/create.md L515–543

The --pushed entry constructs shell commands from two worker-supplied values: the --worktree <path> argument (WT) and the derived branch name (BRANCH). All example snippets correctly double-quote both throughout:

WT="<path>"
BRANCH=$(git -C "$WT" branch --show-current)
REMOTE=$(git -C "$WT" config "branch.$BRANCH.remote" 2>/dev/null || echo origin)
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
gh pr create --head "$BRANCH" ...

Git's own branch-naming rules reject most shell metacharacters; the path originates from a trusted Claude agent in the same operator-controlled session. This pattern is correctly documented and is not a regression — it was already flagged in the previous two security reviews and remains at the same surface-level as before.


Specific analysis of commit a692d8f (the new changes)

implement-dispatch/SKILL.md — preflight exception (lines 31–32)

The new "Exception under worker-side provisioning" paragraph exempts an autonomous orchestrator from the "branch correct (never the default branch)" preflight check when it is staying on the default branch to orchestrate. The security question is whether this opens a path where the default branch is directly modified.

It does not. The exception is structurally sound:

  • The orchestrator "never edits source" is stated explicitly and reinforced throughout the dispatch cadence
  • Workers are scope-fenced; they discharge the non-default-branch invariant by materializing their own worktree+branch before any edit
  • PR creation uses --head "$BRANCH" (the worker's branch), so the PR targets the worker's feature branch, not main
  • The only way this exception could matter for security would be if a worker brief somehow lacked the scope fence — but the scope fence is a mandatory component of every brief, and the orchestrator's own verification step (verify returns against direct evidence) sits between the worker and any downstream action

No authorization bypass is introduced.

work/SKILL.md line 202 — --worktree <the worker's returned worktree path> now explicit

The gh pr create --head "$BRANCH" path in §2.7 will now correctly receive the worktree path. No new injection surface introduced — the path is the same worker-returned value analysed above.

gh pr edit --body-file - read-modify-write pattern (work/SKILL.md line 204)

The pattern for adding ## Related content to an existing PR body reads the body via gh pr view ... --jq '.body', appends content, then writes back via --body-file -. The PR body contents go to the GitHub API as a string — no shell execution of body content occurs at any point. No injection surface.


What's clean

  • No GitHub Actions changes. No pull_request_target, workflow_run, or run: script-injection risk.
  • No supply-chain changes. All three plugin.json files are version-number-only bumps; no action pins, dependency versions, or lockfiles altered.
  • No credential or token flows introduced. gh pr create, git fetch, and git push run with the agent's ambient session credentials; no new token handling or storage is specified.
  • Authorization split is architecturally sound. The worker scope-fence explicitly forbids PR creation; the orchestrator's pre-PR diff gate reads what the worker actually touched before the PR opens; a compromised worker cannot open a PR on its own. The new preflight exception does not weaken this gate.
  • @{u} fix (ad7b147) is correct and tightens the precondition. The replacement (git fetch + SHA comparison) is upstream-independent and robustly quoted; rev-parse "$REMOTE/$BRANCH" 2>/dev/null returning empty on a missing remote ref fails the equality check and stops the flow as intended — no false-pass possible.
  • parse-branch-issue.sh "$BRANCH" [...] (§2.7 line 539). Passing $BRANCH as an explicit positional avoids the script's implicit git branch --show-current in its own process — this is the correct fix, and branch names are validated by git before they can reach this point.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a692d8ffc7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/implementation/skills/implement-dispatch/SKILL.md
Comment thread plugins/implementation/skills/implement-dispatch/SKILL.md
…ed worktree (#572)

Address two more P1 review findings (consequences of the orchestrator staying
on the default branch under worker-side provisioning), plus a proactive sibling
fix:

- Build/test gate and return verification (implement-dispatch cadence steps 3-4)
  must run against the returned worktree, not the orchestrator's default
  checkout, which lacks the worker's changes — else a worker-branch failure
  could pass. `main-side` means the orchestrator runs the gate, not that it runs
  in the orchestrator's checkout.
- The phase-boundary plan-mark commit must land on the worker's branch (committed
  in the worktree via `git -C <path>`), not the orchestrator's default checkout,
  which would diverge local default from the remote and divorce plan progress
  from the PR branch.
- Consolidated the principle in the Prerequisites exception, and anchored work's
  pre-PR diff gate to the worktree for the same reason.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (updated after fix commit 35a7bf9)

Tasks:


What the fix commit addressed

Codex P1 #1 — "Run validation from the returned worktree" — FIXED ✅

implement-dispatch/SKILL.md lines 33–34 adds a consolidating principle paragraph to the Prerequisites exception:

"Because the orchestrator stays on the default branch, every source-touching operation it runs targets the returned worktree, never its own checkout — which does not contain the worker's changes. That covers the return verification (cadence step 3), the build/test gate (cadence step 4 — main-side means the orchestrator runs the gate, not that it runs in the orchestrator's checkout), and the phase-boundary plan-mark commit (Phase boundaries): each runs against the worker's worktree via git -C <path> (or from that directory)."

The fix is complete and covers all three operations. Step 4 (line 40) is also updated at its call site: "Under worker-side provisioning, run it against the returned worktree (git -C <path> or from that directory), not the orchestrator's default checkout — see the Prerequisites exception."

work/SKILL.md's pre-PR diff gate (line 200) is also corrected: "read against the worker's returned worktree (git -C <path>), since the orchestrator's own default-branch checkout does not contain the worker's changes."

Codex P1 #2 — "Keep phase-mark commits off the default branch" — FIXED ✅

implement-dispatch/SKILL.md Phase boundaries (line 49) now says:

"Under worker-side provisioning this plan-mark commit MUST land on the worker's branch, committed in the returned worktree via git -C <path>, never in the orchestrator's default checkout (which would put it on the local default branch, off the PR branch — see the Prerequisites exception)."

The rationale is complete — naming both failure modes (wrong tree inspection and local-default divergence), cross-referencing the prerequisite principle, and giving the concrete mechanism (git -C <path>).

The plan-mark-in-worktree is also feasible: a worker-side-provisioned worktree is checked out from the default branch and therefore already contains the plan artifact; the orchestrator edits and commits the [DONE] mark there via git -C <worktree-path>.


One minor observation (no action required)

The integration table (line 70) and Gotcha at line 82 still say "verify against direct evidence, then /toolchain:check main-side" and "the main window runs /toolchain:check ... itself after every accepted return" without explicitly repeating the worktree-anchoring qualifier. This is not a gap — the Prerequisites exception paragraph is the governing principle and names step 3 and step 4 by number — but a future reader scanning only the table or Gotchas section could miss it. Given the principle is now stated once, clearly, and references the specific steps, this is low-risk as-is.


All findings now resolved

Finding Commit Status
F2: @{u} upstream dependency ad7b147 ✅ Fixed
Codex P2-#2: -b on existing branch ad7b147 ✅ Fixed
F1: missing --worktree <path> in work/SKILL.md a692d8f ✅ Fixed
Codex P1: preflight blocks orchestrator on default branch a692d8f ✅ Fixed
Codex P1 #1: validation runs in orchestrator's checkout 35a7bf9 ✅ Fixed
Codex P1 #2: phase-mark commit lands on default branch 35a7bf9 ✅ Fixed

The four-commit sequence is internally consistent and complete. The principle paragraph in implement-dispatch's Prerequisites exception now consolidates the worktree-anchoring rule for all three orchestrator-run operations (step 3 verify, step 4 build/test, phase-boundary commit), with work's pre-PR diff gate also corrected for the same reason. All identified gaps are closed.

Overall: ready to merge.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244 (all 4 commits: 300ad22, ad7b147, a692d8f, 35a7bf9)

Tasks:

  • Load tools and initialize
  • Fetch PR diff and read changed files
  • Audit skill specifications for injection surfaces and authorization gaps
  • Review the create --pushed --worktree seam for path/branch injection
  • Audit implement-dispatch preflight exception for authorization bypass
  • Check for dangerous GitHub Actions patterns or supply-chain changes
  • Compile and post findings

Scope

All 14 changed files reviewed across all 4 commits. The diff touches:

  • 3 × plugin.json (version-number-only bumps)
  • 3 × CHANGELOG.md (prose only)
  • 5 × SKILL.md / reference/create.md (AI agent instruction updates)
  • 2 × evals.json (evaluation expected-output updates)

No shell scripts, GitHub Actions workflows, package manifests, or credentials are added or modified in any commit.


Findings

No CRITICAL or IMPORTANT security issues found.


SUGGESTION — Confidence: LOW

Inherited through all four commits: worker-returned path/branch injection surface in create.md §2.7

plugins/source-control/skills/pull-request/reference/create.md L515–543

The create --pushed --worktree <path> entry constructs shell commands from two worker-supplied values: the --worktree <path> argument (WT) and the derived branch name (BRANCH). All example snippets correctly double-quote both throughout:

WT="<path>"
BRANCH=$(git -C "$WT" branch --show-current)
REMOTE=$(git -C "$WT" config "branch.$BRANCH.remote" 2>/dev/null || echo origin)
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
gh pr create --head "$BRANCH" ...

Git's own branch-naming validation rejects most shell metacharacters; the path originates from a trusted Claude agent in the same operator-controlled session. This pattern is not a regression — it was flagged at SUGGESTION/LOW-confidence in every prior security review pass and remains at the same surface level. No new instances were introduced in the final commit.


Analysis of the final commit 35a7bf9

The final commit addresses two Codex P1 functional findings that had a secondary security angle: build/test running against the orchestrator's default checkout (a worker-branch failure could pass the gate) and the phase-mark commit landing on the local default branch (would diverge local default from remote, put plan-tracking off the PR branch). Both are now explicitly specified:

Build/test gate — implement-dispatch/SKILL.md line 40:

"Under worker-side provisioning, run it against the returned worktree (git -C <path> or from that directory), not the orchestrator's default checkout"

Running the gate in the wrong checkout would allow a worker branch with a failing build to pass the pre-PR verification — this is now correctly anchored to the worker's worktree.

Phase-boundary plan-mark commit — implement-dispatch/SKILL.md line 49:

"Under worker-side provisioning this plan-mark commit MUST land on the worker's branch, committed in the returned worktree via git -C <path>, never in the orchestrator's default checkout"

A commit landing on the local default branch while the remote remains at its prior tip would create an unreachable-from-PR divergence with no security bypass, but the explicit specification correctly prevents the confusion.

Prerequisites consolidation — implement-dispatch/SKILL.md line 33:

"Because the orchestrator stays on the default branch, every source-touching operation it runs targets the returned worktree, never its own checkout"

This correctly scopes the invariant: the orchestrator is confined to reading from and writing to the returned worktree path, never its own checkout. No authorization bypass is introduced — PR creation still requires the worker branch to have been pushed (verified by the rev-parse HEAD == rev-parse REMOTE/BRANCH assertion), and gh pr create --head "$BRANCH" targets the worker's branch, not the default.


What's clean across all commits

  • No GitHub Actions changes. No pull_request_target, workflow_run, or run: script-injection risk introduced in any commit.
  • No supply-chain changes. The three plugin.json files carry version-number-only bumps; no action pins or dependency versions loosened.
  • No credential or token flows introduced. gh pr create, git fetch, and git push run with the agent's ambient session credentials; no new token handling or storage is specified.
  • Authorization split is architecturally sound across all commits. Worker scope-fence forbids PR creation; orchestrator's pre-PR diff gate reads what the worker actually touched; create --pushed preconditions assert the branch is pushed before gh pr create runs; gh pr create --head "$BRANCH" targets the feature branch. A compromised or hallucinating worker cannot open a PR on its own — the orchestrator's separate verification step is the gate.
  • @{u} fix (ad7b147) is correct and tightens the precondition. The replacement (git fetch + SHA comparison) is upstream-independent and robustly quoted; rev-parse "$REMOTE/$BRANCH" 2>/dev/null returning empty on a missing remote ref fails the equality check and stops the flow as intended — no false-pass possible.
  • parse-branch-issue.sh "$BRANCH" [...] (§2.7 line 539). Explicit first positional avoids the script's implicit git branch --show-current in its own process. Branch names are git-validated before reaching this point.
  • gh pr edit --body-file - read-modify-write pattern (work/SKILL.md line 204). PR body contents go to the GitHub API as a string; no shell execution of body content occurs at any point. No injection surface.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35a7bf91be

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/implementation/skills/implement-dispatch/SKILL.md Outdated
Comment thread plugins/work-items/skills/work/SKILL.md
Comment thread plugins/source-control/skills/pull-request/reference/create.md Outdated
…ays consistent (#572)

Reconcile two earlier fixes that could deadlock: the phase-boundary plan-mark
commit lands in the worker worktree (advancing HEAD past the pushed tip), while
`create --pushed`'s precondition asserts HEAD equals the remote tip. Require the
plan-mark commit to be pushed — it keeps the worktree tip in sync with the
remote (satisfying the precondition) and keeps tracked plan progress on the PR
branch. Also make the interactive Suggest-branch-name step explicit about the
autonomous hand-off (name carried in the brief; the worker creates the branch;
the orchestrator still never creates it).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (updated after fix commit df34a89)

Tasks:

  • Load tools and initialize
  • Read the new fix commit changes
  • Check if Codex P1 (push phase-mark commit before PR-only creation) is addressed
  • Check if Codex P1 (reuse persisted worktree for later plan phases) is addressed
  • Check if Codex P2 (resolve push remote instead of assuming origin) is addressed
  • Synthesize and post final review

What the fix commit addressed

Codex P1 — "Push the phase-mark commit before PR-only creation" — FIXED ✅

implement-dispatch/SKILL.md line 49 now appends **and pushed** to the phase-boundary commit instruction and adds the rationale:

"Under worker-side provisioning this plan-mark commit MUST land on the worker's branch, committed in the returned worktree via git -C <path> and pushed ... Pushing it is not optional: it keeps the worktree tip in sync with the remote, which /source-control:pull-request create --pushed's HEAD-equals-remote precondition requires, and it keeps tracked plan progress on the PR branch."

This directly resolves the deadlock the Codex reviewer identified: the phase-mark-only commit would have advanced HEAD past the pushed tip, causing create --pushed's precondition to fail. Now the phase-mark commit is pushed immediately after it's written, keeping HEAD == remote tip through the whole lifecycle.

work/SKILL.md line 192 also adds the clarification that in autonomous runs the orchestrator carries the branch name into the brief and the worker creates the branch, making explicit that the orchestrator still never creates the branch itself.


What remains open

Codex P1 — "Reuse the persisted worktree for later plan phases" — unaddressed, spec ambiguity remains

The brief composition in implement-dispatch/SKILL.md line 37 has two When... branches:

  1. "When the worker edits in a dedicated worktree" — brief supplies the existing path
  2. "When provisioning is worker-side (the autonomous work-lane)" — worker materializes the worktree as its first step

For a multi-phase autonomous item, phase 1 uses branch (2): the worker provisions the worktree and returns its path. For phase 2, the orchestrator has the returned path, so the correct branch is (1): the brief supplies the path and the worker reuses it. But the spec doesn't make this transition explicit — the parenthetical "(the autonomous work-lane)" on branch (2) could be read as applying to all autonomous dispatches, not only the first. A phase 2 worker told to "materialize the worktree" for a branch that is already checked out in the persisted worktree will fail (git worktree add rejects a branch that is already checked out elsewhere).

The spec implicitly handles this through the two-branch model once the orchestrator holds the path, but an agent following the prose could reasonably apply worker-side provisioning to all phases in the autonomous lane. Adding a single transitional sentence — e.g. "After phase 1 returns the worktree path, subsequent phase briefs use the dedicated-worktree pattern (branch 1 above), passing the returned path directly — the worker reuses the existing worktree rather than provisioning again" — would close this ambiguity.

Codex P2 — "origin fallback when no remote configured" — unaddressed

create.md line 526:

REMOTE=$(git -C "$WT" config "branch.$BRANCH.remote" 2>/dev/null || echo origin)

In a repo cloned with a non-origin remote (e.g. git clone -o vendor), a freshly provisioned branch has no branch.<name>.remote and the fallback fetches origin, which does not exist. The Codex reviewer raised this in the 35a7bf9 pass and it is unchanged. A more robust fallback — e.g. git remote | head -1 in place of echo origin — would handle non-default remote names without requiring explicit configuration. The practical blast radius is narrow (non-standard remote naming is uncommon), but the fix is low-cost.


Summary

Finding Commit Status
F2: @{u} upstream dependency ad7b147 ✅ Fixed
Codex P2-#2: -b on existing branch ad7b147 ✅ Fixed
F1: missing --worktree <path> in work/SKILL.md a692d8f ✅ Fixed
Codex P1: preflight blocks orchestrator on default branch a692d8f ✅ Fixed
Codex P1 #1: validation runs in orchestrator's checkout 35a7bf9 ✅ Fixed
Codex P1 #2: phase-mark commit lands on default branch 35a7bf9 ✅ Fixed
Codex P1: phase-mark commit not pushed before create --pushed df34a89 ✅ Fixed
Codex P1: multi-phase worktree reuse ambiguity ⚠️ Unaddressed (spec ambiguity)
Codex P2: origin fallback in remote resolution ⚠️ Unaddressed (narrow edge case)

Seven of nine findings are resolved. The two open items are:

  • A spec ambiguity for multi-phase autonomous plans (Codex P1) — the two-branch briefing model implicitly handles it but doesn't call out the phase-1→phase-2 transition
  • A narrow edge case for non-origin remote names in create.md §2.7 (Codex P2)

Neither is blocking if the intended consumer projects use standard remote naming and single-phase plans; both are low-cost prose fixes if a follow-up is warranted.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244 (all 5 commits through df34a89)

Tasks:

  • Load tools and initialize
  • Read the final fix commit changes (df34a89)
  • Audit phase-mark push for new injection surfaces or authorization gaps
  • Check for new credential/token flows or supply-chain changes
  • Compile and post findings

Scope

All 14 changed files reviewed across all 5 commits. The final commit (df34a89) touches two files:

  • plugins/implementation/skills/implement-dispatch/SKILL.md — phase-boundary section updated to require pushing the plan-mark commit
  • plugins/work-items/skills/work/SKILL.md — branch-name step clarified for autonomous worker-side provisioning

No shell scripts, GitHub Actions workflows, package manifests, or credentials are added or modified in any commit.


Findings

No CRITICAL or IMPORTANT security issues found.


SUGGESTION — Confidence: LOW

Inherited through all five commits: worker-returned path/branch injection surface in create.md §2.7

plugins/source-control/skills/pull-request/reference/create.md L515–543

The create --pushed --worktree <path> entry constructs shell commands from two worker-supplied values — WT and BRANCH. All example snippets correctly double-quote both throughout. Git's own branch-naming validation rejects most shell metacharacters; the path originates from a trusted Claude agent in the same operator-controlled session. This pattern was flagged in every prior security review pass at SUGGESTION/LOW-confidence and is unchanged by the final commit — not a regression.


Analysis of commit df34a89 — phase-mark push

The final commit's sole behavioral change is adding **and pushed** to the phase-boundary section of implement-dispatch/SKILL.md (line 49), requiring the orchestrator to push the plan-mark commit after landing it in the returned worktree via git -C <path>.

Security question: does adding a push operation after the phase-mark commit introduce new authorization gaps, injection surfaces, or credential flows?

It does not:

  • Target is the worker's feature branch, never default. The push is scoped to the worker's branch in its own worktree. The default branch is not a reachable target — the orchestrator cannot push origin HEAD:main from the worker's worktree when HEAD points at the worker's branch.
  • Ambient credentials, same as all other push operations. gh/git push in this spec always runs with the agent's session credentials; no new token is introduced or stored.
  • No new injection surface. The push command follows the same git -C "$WT" push pattern used throughout the spec and already audited in prior reviews. Double-quoting discipline on $WT is unchanged.
  • Addresses a correctness precondition, not a new capability. The create --pushed mode's rev-parse HEAD == rev-parse REMOTE/BRANCH assertion requires HEAD and remote to be in sync. A phase-mark commit without a push would cause create --pushed to STOP — the fix makes the flow proceed correctly rather than opening any new authorization path.
  • No change to who can open a PR. The scope fence forbidding worker-initiated PR creation is unchanged. The orchestrator still opens the PR after the push; the worker still cannot.

The work/SKILL.md addition (line 192) is a documentation clarification — the orchestrator carries the resolved branch name into the dispatch brief rather than emitting a git checkout -b suggestion for an interactive user. This is a prose fix with no new runtime behavior and no security surface.


What's clean across all 5 commits

  • No GitHub Actions changes. No pull_request_target, workflow_run, or run: script-injection risk introduced in any commit.
  • No supply-chain changes. The three plugin.json files are version-number-only bumps; no action pins, dependency versions, or lockfiles altered.
  • No credential or token flows introduced. All gh/git operations use the agent's ambient session credentials; no new token handling or storage is specified.
  • Authorization split is architecturally sound. Worker scope-fence forbids PR creation; orchestrator's pre-PR diff gate reads what the worker actually touched; create --pushed preconditions assert the branch is pushed before gh pr create runs; gh pr create --head "$BRANCH" targets the feature branch. A compromised worker cannot open a PR on its own.
  • Phase-mark push does not widen the blast radius. The push is to the worker's feature branch, gated by the same git -C "$WT" anchoring as all other worktree operations. The create --pushed HEAD-equals-remote assertion still fires after the push and remains the final consistency gate before gh pr create.
  • @{u} fix (ad7b147) remains correct. The upstream-independent git fetch + SHA comparison is unchanged and makes the push-present assertion robust regardless of whether the worker set tracking.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df34a895e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/work-items/skills/work/SKILL.md Outdated
Convergence sweep addressing four review findings — the remaining operational
surfaces of the "orchestrator on default branch, worker in its own worktree"
model:

- implement-dispatch: provisioning is once-per-item (first phase); later phases
  of the same item reuse the persisted worktree rather than re-provisioning an
  already-checked-out branch (which fails). Consolidated statement now also
  states the plan-mark commit is committed AND pushed.
- create.md §2.7: resolve the push remote via the shared resolve-remote.sh
  --push (run from the worktree) instead of assuming `origin`, so a
  non-origin/`-o vendor` clone or triangular fork flow verifies the ref at the
  right destination; compare against FETCH_HEAD.
- work: monitoring and the review pass run from the persisted worktree (which
  owns the PR branch) with the PR number passed explicitly — the orchestrator's
  default-branch session cannot host pull-request monitor's branch-owning watch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (updated after fix commit 7a81507)

Tasks:

  • Load tools and initialize
  • Read the new fix commit changes (7a81507)
  • Check if Codex P1 (multi-phase worktree reuse ambiguity) is addressed
  • Check if Codex P2 (origin fallback in remote resolution) is addressed
  • Check if Codex monitoring-via-worker-worktree finding is addressed
  • Synthesize and post final review

What the fix commit addressed

Codex P1 — multi-phase worktree reuse ambiguity — FIXED ✅

implement-dispatch/SKILL.md line 37 now contains an explicit statement of the once-per-item provisioning rule, inserted directly into the worker-side-provisioning clause of the brief composition section:

"Provisioning happens once per item, on the first dispatched phase; the worktree persists across the item's phases, so every later phase of the same item is handed that same worktree path and works in it — never re-provisioning the already-checked-out item branch (both git worktree add -b <name> and attaching the branch fail while it is checked out in the persisted worktree)."

This directly closes the ambiguity the Codex reviewer identified. The parenthetical "(the autonomous work-lane)" on the provisioning clause is now unambiguous: it applies only at first-phase dispatch; subsequent phases of the same item use the dedicated-worktree briefing pattern (hand over the existing path). The failure mode (git worktree add rejecting a branch already checked out) is explicitly named, making the rule self-explanatory.

Codex P2 — origin fallback in remote resolution — FIXED ✅

create.md §2.7 line 530 replaces the hardcoded || echo origin fallback with the shared resolver:

REMOTE=$( cd "$WT" && bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push ) || exit 1
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
[ "$(git -C "$WT" rev-parse HEAD)" = "$(git -C "$WT" rev-parse FETCH_HEAD)" ] \
  || { echo 'worker branch is not fully pushed to the remote' >&2; exit 1; }

The script (resolve-remote.sh) already exists and correctly implements Git's documented push-remote precedence chain (branch.<name>.pushRemoteremote.pushDefaultbranch.<name>.remoteorigin → sole-other remote), confirmed by reading the script directly. Running it with cd "$WT" is the right approach: the cd is needed so git config reads the worktree's branch-specific config; $CLAUDE_PLUGIN_ROOT is an environment variable independent of working directory, so the script path resolves correctly. The || exit 1 propagates a STOP when the remote is genuinely ambiguous (multiple remotes, none named origin, no push config) — which is the correct failure mode per the script's own design. The comparison is now against FETCH_HEAD (written by the preceding git fetch) rather than "$REMOTE/$BRANCH" — this avoids the FETCH_HEAD-in-worktree symref ambiguity and is the canonical comparison for "fetch returned this ref." ✅

Codex P1 (from df34a89 review) — monitoring from orchestrator's default checkout — FIXED ✅

work/SKILL.md line 202 now specifies:

"After the PR is open, monitoring (Phase 3) and the post-green review pass run from the persisted worktree, not the orchestrator's default-branch session: /source-control:pull-request monitor resolves the PR by bare gh pr view and requires its watch to run in the session that owns the PR branch, which the orchestrator's own checkout is not — so the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the PR number explicitly."

The rationale is complete and the mechanism is correct. The orchestrator holds both the worktree path and the PR number (from the gh pr create --head "$BRANCH" ... output) at this point in the lifecycle, so dispatching a monitoring worker into the persisted worktree with an explicit PR number is fully supported by the preceding steps.


All findings across the full review cycle are now resolved

Finding Commit Status
F2: @{u} upstream dependency ad7b147 ✅ Fixed
Codex P2-#2: -b on existing branch ad7b147 ✅ Fixed
F1: missing --worktree <path> in work/SKILL.md a692d8f ✅ Fixed
Codex P1: preflight blocks orchestrator on default branch a692d8f ✅ Fixed
Codex P1 #1: validation runs in orchestrator's checkout 35a7bf9 ✅ Fixed
Codex P1 #2: phase-mark commit lands on default branch 35a7bf9 ✅ Fixed
Codex P1: phase-mark commit not pushed before create --pushed df34a89 ✅ Fixed
Codex P1: multi-phase worktree reuse ambiguity 7a81507 ✅ Fixed
Codex P2: origin fallback in remote resolution 7a81507 ✅ Fixed
Codex P1: monitoring dispatched from orchestrator's own checkout 7a81507 ✅ Fixed

The six-commit sequence is internally consistent and complete. The final commit's three fixes cross-reinforce each other: once-per-item provisioning (commit 1 is the first phase) feeds the persisted-worktree path into the monitoring dispatch (commit 2), and the push-remote resolver correctly verifies the push destination the first-phase worker used (commit 3). No remaining gaps.

Overall: all identified gaps are closed. Ready to merge.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review — PR #1244 (commit 7a81507)

Tasks:

  • Load tools and initialize
  • Fetch diff for the new commit (7a81507)
  • Audit changed files for injection surfaces and authorization gaps
  • Audit resolve-remote.sh for shell injection and trust boundaries
  • Check monitoring dispatch change for authorization gaps
  • Compile and post findings

Scope

Three files changed in commit 7a81507:

  • plugins/implementation/skills/implement-dispatch/SKILL.md — multi-phase worktree-reuse clarification; phase-mark push note
  • plugins/source-control/skills/pull-request/reference/create.md — remote resolver replaced; FETCH_HEAD comparison
  • plugins/work-items/skills/work/SKILL.md — monitoring and review dispatched into persisted worktree; PR number passed explicitly

No shell scripts, GitHub Actions workflows, package manifests, or credentials are added or modified. resolve-remote.sh (invoked by the new prose) is a pre-existing script, not added in this commit; its content was read and audited below.


Findings

No CRITICAL or IMPORTANT security issues found.


SUGGESTION — Confidence: LOW (unchanged surface, not a regression)

Inherited: worker-returned path/branch injection surface in create.md §2.7

plugins/source-control/skills/pull-request/reference/create.md L525–533

REMOTE=$( cd "$WT" && bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/resolve-remote.sh" --push ) || exit 1
git -C "$WT" fetch -q "$REMOTE" "$BRANCH"
[ "$(git -C "$WT" rev-parse HEAD)" = "$(git -C "$WT" rev-parse FETCH_HEAD)" ] \
  || { echo 'worker branch is not fully pushed to the remote' >&2; exit 1; }

$WT (from --worktree) and $BRANCH (derived from git -C "$WT" branch --show-current) originate from the worker-supplied path. Quoting discipline is correct throughout — cd "$WT", "${CLAUDE_PLUGIN_ROOT}/...", "$REMOTE", "$BRANCH", "$WT" are all double-quoted, and git's own branch-name validation rejects most shell metacharacters. Path originates from a trusted Claude agent in the same operator-controlled session. This pattern was first flagged in the initial security review and is unchanged in character by this commit.


Analysis of the new resolve-remote.sh invocation

The replacement of the hardcoded || echo origin fallback with resolve-remote.sh --push is both a functional improvement and a security neutral-or-positive change:

Script hygiene: resolve-remote.sh opens with set -uo pipefail, uses mapfile -t for safe array population from git remote, and double-quotes all variable expansions throughout. No eval, no unquoted variable in command position, no subprocess spawning beyond git config lookups.

Branch-name interpolation in config key: resolve-remote.sh evaluates git config "branch.${BRANCH}.remote". If the branch name contains characters that look like a git config key separator (.), the key becomes branch.foo.bar.remotegit config will simply return nothing and the script falls through to the origin fallback or errors loudly. Git's own branch-validation rules forbid \n, NUL bytes, and most shell metacharacters in branch names, so this is not an exploitable surface.

FETCH_HEAD comparison is more robust than prior "$REMOTE/$BRANCH" ref: the previous commit used rev-parse "$REMOTE/$BRANCH" in a double-quoted string, which interpolated both the remote name and branch name into a ref spec. Using FETCH_HEAD (set by the preceding git fetch) removes the branch name from the ref spec entirely — no branch-name injection in the comparison. This is a correctness improvement that also tightens the injection surface.

cd "$WT" subshell: the $( cd "$WT" && bash ... ) subshell changes directory only within the subshell, leaving the outer shell's working directory unchanged. $WT is double-quoted; bash is invoked with the script path as a double-quoted argument. The pattern is sound.


Analysis of the monitoring-dispatch change (work/SKILL.md)

The new text routes monitoring and post-green review into the persisted worktree by dispatching a scope-fenced subagent there, passing the PR number explicitly:

"the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the PR number explicitly"

Authorization analysis: monitor.md §3.0.1 requires the watch to run in the session that owns the PR branch (a branch-lock constraint). The monitoring worker runs in the persisted worktree (on the PR branch), satisfying that constraint. The PR number is passed explicitly — gh pr view <N> uses the number rather than the branch name, avoiding branch-name injection into the gh invocation. The monitoring worker is scope-fenced to read-only CI/review actions. No new authorization bypass is introduced.

No new supply-chain, credential, or token flows. Monitoring uses gh pr view, gh pr checks, and gh api with the ambient session token already present — unchanged from pre-existing monitor behavior.


What's clean

  • No GitHub Actions changes. No pull_request_target, workflow_run, or run: script-injection risk in any commit.
  • No supply-chain changes. No action pins or dependency versions loosened in this commit.
  • resolve-remote.sh is a pre-existing, well-guarded script. set -uo pipefail, correct quoting, loud failure on ambiguity — no silent fallback to an incorrect remote.
  • FETCH_HEAD comparison closes the "$REMOTE/$BRANCH" ref-spec injection vector present in an earlier version of the spec.
  • Monitoring PR-number explicit pass prevents branch-name injection into gh pr view calls within the monitoring worker.
  • Phase-mark push and multi-phase worktree-reuse clarifications are prose-only functional fixes with no new security surface.

@kyle-sexton
kyle-sexton merged commit 58cbd84 into main Jul 24, 2026
24 checks passed
@kyle-sexton
kyle-sexton deleted the feat/572-orchestrator-dispatch-lifecycle branch July 24, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant