feat(implementation): let the orchestrator keep commit authority over dispatched workers

[kyle-sexton]

Closes #4508

Summary

A plan whose worker fence forbids staging, committing, and pushing (because the orchestrator owns a
commit-subject gate or a push-once rule) could not use the implementation lane:
implementation:implementer was described as always committing and pushing early, and
/implementation:implement-dispatch made commit-and-push-early a clause of every brief while
forbidding a generic subagent for source edits. This adds an explicitly declared commit
authority brief field (worker by default, orchestrator as the alternative) so the orchestrator
can keep commit authority while still dispatching the tier-bound implementer.

Fix

• agents/implementer.md: description made conditional; new ## Commit authority section. Under
  orchestrator the worker edits only (no index or ref writes, no provisioning), works in the
  assigned worktree, and returns git status --porcelain --untracked-files=all paths instead of
  a sha. A commit-forbidding fence with no declared mode, a commit-authority value other than
  worker or orchestrator, a missing worktree path, or a request for worker-side provisioning
  under orchestrator is a STOP.
• skills/implement-dispatch/SKILL.md: new ### Commit authority subsection (when to declare it,
  worktree, brief, verification of the uncommitted tree including untracked files, the
  orchestrator's combined phase-boundary commit, push per the plan's push rule, concurrency);
  Prerequisites, cadence steps 1, 3, 4, Phase boundaries, and Gotchas made conditional where they
  assumed a worker commit. The rule against generic subagents stands.
• skills/implement-dispatch/evals/evals.json: case 8 covers a no-commit plan fence, with a
  negative expectation for a narrow fence.
• README.md implementer row; plugin.json 0.17.0 -> 0.18.0; CHANGELOG [0.18.0].

Absent the field, behavior is unchanged, so /work-items:work and /implementation:implement
callers keep worker commit authority.

Verification

Measured at head 36544fe1d, base 47555b587. A later commit answers the Codex review thread
(untracked files inside a new directory): a scratch-repo probe showed plain status --porcelain
prints ?? newdir/ while --untracked-files=all lists each file; check-skill PASS and zero
U+2014 on the added lines were rerun on it.

• bash scripts/check-changed-skills.sh 47555b587: 1 skill(s) checked, 0 failed.
• bash scripts/check-changelog-parity.sh --check-bump 47555b587: every changed version has its
  CHANGELOG entry.
• Scoped em-dash gate over the implementation plugin's purged paths: 3 files scanned, no em dashes; U+2014 on added diff lines: 0.
• CHECK_SKILL_SKILLS_ROOT=plugins/implementation/skills bash plugins/skill-quality/scripts/check-skill.sh implement-dispatch:
  PASS, 0 errors, 0 warning(s); evals.json parses.
• Suites run by hand on this host: scripts/validate-plugin-contracts.test.sh PASS=42 FAIL=0,
  scripts/check-changed-skills.test.sh PASS=21 FAIL=0,
  plugins/skill-quality/scripts/check-evals-quality.test.sh 0 failures. The remaining
  --explain selection is left to CI; every touched file is a recorded no-suite path.
• A fresh-context phase verifier (opus), rationale withheld, returned PASS on all 11 acceptance
  criteria and named three weak points (shebang gotcha still unconditional, verifier diff not
  covering untracked files, push wording in the agent); all three are fixed in 36544fe1d and the
  gates above were rerun on it.
• skill-evidence block absent: the installed source-control and claude-ops predate feat(source-control): seat the mandatory reviews on the operator's session and retire the OAuth review lanes #4210, so no ledger sha is stamped; the owed reviews ran as nested agents: plan reviewer, code reviewer (correctness, simplification, and AI-slop/markdown-noise lenses on added lines), phase verifier.

Related

• Source: a handoff-queue item from a session whose plan fenced workers from committing; it fell
  back to a generic agent and lost the implementer's model-tier binding.
• Unattended interview: the Brief was locked by the lane lead with no human in the cycle. Design
  questions, each challenged by a fresh-context validator with the rationale withheld:
  • Q1 brief-level mode vs documenting a fenced general-purpose agent. Answer: the mode.
    Validator: confirmed (a generic agent breaks the skill's tier-binding rule).
  • Q2 should a commit-forbidding fence alone trigger the mode? Recommended: yes. Validator:
    challenged (a narrow fence could be misread and silently drop push-early crash insurance).
    Adopted the verdict: explicit declaration only; a commit-forbidding fence without it is a STOP.
• Deferred: a behavioral run of eval case 8 (claude plugin eval) is a paid model run and was not
  executed; the case is statically validated only.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-26T05:57:57.275034Z	36544fe	Draft marked ready

ℹ️ About Codex in GitHub

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

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

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

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

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final findings

No security issues found.

This PR changes only Markdown-authored agent/skill instructions and metadata for the implementation plugin:

• agents/implementer.md — adds a "Commit authority" section describing when the dispatched worker refrains from git add/commit/push/stash and instead returns git status --porcelain output for the orchestrator to commit.
• skills/implement-dispatch/SKILL.md — documents the same "Commit authority" contract for the orchestrating skill (brief field, worktree handling, verification of the uncommitted tree, phase-boundary commit/push).
• evals/evals.json, README.md, CHANGELOG.md, plugin.json — a new eval case and routine doc/version bump.

There's no code here that parses untrusted input, handles secrets/tokens, exposes a network surface, or changes tool grants/permissions (implementer's tools: frontmatter is unchanged). The change is a natural-language workflow contract governing which party in an already-trusted orchestrator/worker pair issues git writes — it doesn't introduce a new trust boundary or injection surface, and it doesn't touch the existing tool cage that scopes what either agent can execute. Applying the skip gate: this change has no security-relevant surface (skip-gate criterion 2), so no CRITICAL/IMPORTANT/SUGGESTION findings are reported.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36544fe1d0

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

Code review complete ✅

• Gather context (PR body, changed files, custom instructions)
• Invoke /review:code-review per custom instructions
• Post inline findings (if any) — none met the bar
• Final summary update

Scope: reviewed the full diff (git diff origin/main...HEAD, 6 files) at head 36544fe1d, focused on agents/implementer.md and skills/implement-dispatch/SKILL.md where the new "commit authority" mode lives, per the code-review lane's correctness/maintainability criteria (security is out of scope for this lane).

What I checked:

• Cross-file consistency of the new worker/orchestrator commit-authority contract between agents/implementer.md and skills/implement-dispatch/SKILL.md (STOP conditions, worktree requirement, return contract, phase-boundary commit shape, push timing).
• Whether the three weak points the PR body says a prior fresh-context verifier flagged (unconditional shebang gotcha, verifier diff missing untracked files, push wording) are actually addressed in this diff — they are: the shebang/exec-bit clause is now conditioned on commit authority (implementer.md:44-46, SKILL.md:74), untracked-file coverage is explicit (SKILL.md:75, git ls-files --others --exclude-standard), and the push-early gotcha is scoped to worker-authority only (SKILL.md:136).
• evals.json case 8 (lines 93–106): structurally valid JSON (brace/bracket balance verified by inspection), and its expectations match the SKILL.md text for the no-commit-plan scenario it targets, including the negative case (a fence forbidding only a narrow action like force-push does not flip the mode).
• Version/changelog/README bookkeeping (plugin.json 0.17.0 → 0.18.0, CHANGELOG.md [0.18.0], README implementer row) is consistent with the change.

No blocking findings. The one thing I weighed and decided not to raise: the new "Concurrency" bullet in SKILL.md (plugins/implementation/skills/implement-dispatch/SKILL.md#L77, permalink) opens with "Run one worker per worktree at a time" immediately followed by instructions for when several workers share one worktree concurrently. Read as general-rule-then-exception (consistent with the doc's style elsewhere, e.g. the worker/orchestrator default-then-override pattern), it's coherent, so I did not file it as a finding, but a future reader skimming only the first clause could reasonably ask when the exception applies — worth a second look if it causes confusion in practice, not worth blocking on now.

I did not independently re-run the author's shell-script verification suites or the paid claude plugin eval behavioral run cited in the PR body; those are author-claimed and unverified by me.

Branch: implementation-implementer-commits-and-p

[claude]

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

---

Code review complete ✅

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:code-review per custom instructions
• Post inline findings (if any) — none met the bar
• Final summary update

Scope: this session's diff since the lane's prior review (posted at 36544fe1d) is exactly one commit, eddb7ad ("list every untracked file under orchestrator commit authority"), which resolves the earlier Codex review comment on SKILL.md about git status --porcelain collapsing a new untracked directory into one ?? newdir/ entry. I re-read the full PR diff against origin/main (6 files) and focused verification on this fix.

What I checked:

• The fix changes the return/verification commands in both files from git status --porcelain to git status --porcelain --untracked-files=all, which reports every new file individually instead of collapsing a directory — agents/implementer.md:50, skills/implement-dispatch/SKILL.md:74-75. This correctly addresses the earlier finding: --untracked-files=all is the documented (git status -h) way to stop untracked-directory collapsing.
• Cross-file consistency: the worker-side return contract (implementer.md) and the orchestrator-side verification/phase-verifier text (SKILL.md) now agree on the same flag everywhere it's mentioned — grepped both files for status --porcelain and ls-files --others, found no stale bare status --porcelain reference left over from before the fix.
• evals.json case 8's expectation text ("status --porcelain including untracked entries") is generic enough not to conflict with the more specific flag now used in the skill body.
• Bookkeeping (plugin.json 0.17.0 → 0.18.0, CHANGELOG.md [0.18.0], README.md implementer row) matches the code change; no discrepancies.
• Confirmed git status --porcelain without --untracked-files=all is unrelated pre-existing text at plugins/implementation/skills/implement/SKILL.md:17 (untouched file, different purpose — checking a clean tree, not enumerating new files) and out of this PR's scope.

No blocking findings. The fix is narrowly scoped, correct, and applied consistently across both files that describe the orchestrator commit-authority contract. I did not independently re-run the author's shell-script verification suites or the paid claude plugin eval behavioral run cited in the PR body; those remain author-claimed and unverified by me.
· Branch: implementation-implementer-commits-and-p

[claude]

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

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final findings

No security issues found.

Reviewed the full diff at head eddb7ad (git diff origin/main...HEAD, 6 files) and, separately, the incremental change since the previously reviewed commit 36544fe1d (the two commits c1bcea7/eddb7ad that followed it).

Every changed file is Markdown-authored agent/skill instructions or plain metadata for the implementation plugin:

• agents/implementer.md — "Commit authority" section; the only change since 36544fe1d is that the return command changes from git -C <path> status --porcelain to git -C <path> status --porcelain --untracked-files=all so a new directory's files are enumerated individually rather than collapsed to one ?? line.
• skills/implement-dispatch/SKILL.md — same --untracked-files=all change applied to the return-verification and phase-verifier-input recipes, addressing the Codex P2 comment about untracked directories.
• evals/evals.json, README.md, CHANGELOG.md, plugin.json — eval case text, doc row, and version bump; no logic.

None of this parses untrusted input, handles secrets/credentials, exposes a network surface, or changes tool grants (implementer's tools: frontmatter is unchanged, still Read, Edit, Write, Grep, Glob, Bash, WebFetch, WebSearch, Skill, Agent). The change remains a natural-language workflow contract for which party in an already-trusted orchestrator/worker pair issues git writes; the untracked-files fix only changes how much of the uncommitted tree gets inspected before acceptance (more thorough, not less), so it does not introduce a new trust boundary or injection surface. Applying the skip gate: no security-relevant surface (criterion 2), consistent with the prior security-review pass on this PR.

[claude]

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

---

Security review complete ✅

• Gather context (PR diff, changed files, custom instructions)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidate findings
• Post final findings

No security issues found.

This session reviewed the full PR diff at head 26f696f, with focused attention on the one commit added since the prior security-review pass (eddb7ad): 26f696f ("cover untracked files for the verifier and stop on an unknown commit authority").

That incremental change is two small wording edits, both in Markdown-authored agent/skill instructions:

• plugins/implementation/agents/implementer.md#L40-L41 — an unrecognized commit authority value (anything other than worker/orchestrator) is now an explicit STOP-and-report conflict instead of being silently unhandled.
• plugins/implementation/skills/implement-dispatch/SKILL.md#L75 — the phase-verifier recipe now explicitly tells the verifier to read untracked files (status --porcelain --untracked-files=all or ls-files --others --exclude-standard) in addition to git diff <base>, closing the same untracked-directory gap the earlier Codex comment raised, now applied to the verifier's own inspection path rather than just the orchestrator's.

Neither edit touches parsing of untrusted input, secrets/credential handling, network surface, or tool grants (implementer's tools: frontmatter is unchanged). Both edits tighten an existing trust boundary (an unrecognized mode now fails closed via STOP; the verifier is told to look at more of the uncommitted tree, not less), consistent with the two prior security-review passes on this PR (at 36544fe1d and eddb7ad), which also found no issues across the rest of the diff (the implementer.md/SKILL.md commit-authority sections, evals.json, README.md, CHANGELOG.md, plugin.json). Applying the skip gate: no security-relevant surface in the new commit (criterion 2), and the prior reviews' findings on the rest of the diff still apply unchanged (criterion 3).
· Branch: implementation-implementer-commits-and-p

[claude]

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

---

Code review complete ✅

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:code-review per custom instructions
• Post inline findings (if any) — none met the bar
• Final summary update

Scope: reviewed the full diff (git diff origin/main...HEAD, 6 files) at head 26f696fc4080182f99650bdacaa1fe215a2de091, with focused attention on the one commit since this lane's last review (eddb7ad): 26f696f ("cover untracked files for the verifier and stop on an unknown commit authority").

What changed since the last review and what I checked:

• plugins/implementation/agents/implementer.md — the commit-authority field's contract now explicitly STOPs on any value other than worker/orchestrator, closing the gap where an absent or unrecognized field could be silently misread. This is consistent with the existing STOP for a commit-forbidding fence with no declared mode (L55-58) and doesn't conflict with it: an absent field still means worker by default, only an actual other string is a conflict.
• plugins/implementation/skills/implement-dispatch/SKILL.md:75 — the phase-verifier dispatch recipe under orchestrator commit authority now spells out that when the verifier is given the worktree path + base ref (rather than an assembled diff), it must itself enumerate untracked files via status --porcelain --untracked-files=all or ls-files --others --exclude-standard, not just git diff <base>. This matches the --untracked-files=all fix already applied to the return-verification recipe two commits earlier, and matches phase-verifier.md's own (unchanged) contract of "a worktree path plus base ref, or the diff itself."
• Cross-file consistency: implementer.md's return contract (status --porcelain --untracked-files=all, no index/ref writes under orchestrator) and SKILL.md's verification/commit recipe still agree everywhere both are stated; no stale references to a bare status --porcelain remain in the touched sections.
• evals.json case 8's expectations still match the SKILL.md text for the no-commit-plan scenario; JSON structure is well-formed (visually verified — a sandbox restriction blocked running python3 -m json.tool in this session, so I couldn't shell out to reconfirm parsing, but the previously-posted Codex/security passes on this PR already confirmed it parses and check-skill passes).
• Version/changelog/README bookkeeping (plugin.json 0.17.0 → 0.18.0, CHANGELOG.md [0.18.0], README implementer row) is unchanged since the last review and still consistent.

No blocking findings. The incremental commit is a narrow, correct closure of two gaps a prior fresh-context verifier and Codex flagged; both fixes are internally consistent with the rest of the contract and with each other.

I did not independently re-run the author's shell-script verification suites or the paid claude plugin eval behavioral run cited in the PR body; those remain author-claimed and unverified by me.
· Branch: implementation-implementer-commits-and-p