Skip to content

feat(implementation): front-load CI-hygiene and early-push clauses in dispatch brief#849

Merged
kyle-sexton merged 7 commits into
mainfrom
feat/819-implement-dispatch-brief-clauses
Jul 21, 2026
Merged

feat(implementation): front-load CI-hygiene and early-push clauses in dispatch brief#849
kyle-sexton merged 7 commits into
mainfrom
feat/819-implement-dispatch-brief-clauses

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Fresh dispatched workers repeatedly learn the same PR-contract constraints via red CI instead of the brief, each costing an extra fix-and-CI round trip.

Fix

Added three clauses to implement-dispatch's "Compose the brief" step (plugins/implementation/skills/implement-dispatch/SKILL.md), alongside the existing worktree-cwd clause, and mirrored them in Gotchas:

  • No issue-number back-references in code comments (the comment-hygiene check flags them; TODO(#issue) is the sanctioned exception).
  • Any new shebang file must be marked executable via git update-index --chmod=+x before the commit that introduces it (the exec-bit check flags a tracked shebang file recorded non-executable).
  • Commit, push, and open the PR as early as practical — before the CI-poll tail — so a mid-flight worker session-limit death never orphans unpushed work.

Bumped plugins/implementation/.claude-plugin/plugin.json to 0.7.6 and added a CHANGELOG.md entry, matching the 0.7.4 precedent for the same class of change (the worktree-cwd clause).

Verification

  • wc -l confirms SKILL.md stays well under the 500-line skill-quality-gate cap (85 lines).
  • Checks are named (comment-hygiene, exec-bit), not file-pathed, per the issue's durability requirement.
  • Confirmed no other open PR touches plugins/implementation/ before opening this one.
  • CI pending on this PR — will confirm green before requesting review.

Closes #819

Related

🤖 Generated with Claude Code

… dispatch brief

Fresh dispatched workers repeatedly learned the same PR-contract constraints
via red CI instead of the brief: issue-number back-references in code
comments, unset exec bits on new shebang files, and pushing late enough that
a mid-session death orphans unpushed work.

Add three clauses to implement-dispatch's "Compose the brief" step, alongside
the existing worktree-cwd clause, and mirror them in Gotchas: no issue-number
back-references in code comments (comment-hygiene check), git update-index
--chmod=+x before commit for new shebang files (exec-bit check), and commit/
push/open the PR early — before the CI-poll tail.

Closes #819

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read changed files
  • Review changes for correctness, security, and guideline alignment
  • Post findings

Summary

Clean, well-scoped change. The three new clauses follow the established 0.7.4 / worktree-cwd pattern exactly — brief-level instruction in Step 1 plus a Gotchas reminder — and the versioning and changelog are consistent with precedent. No security issues, no breaking changes. A few minor observations below, none blocking.


Findings

1. Step 1 is now one very long paragraph (cosmetic)

SKILL.md line 33

The entire brief-composition instruction is now a single ~1,500-character paragraph. A freshly dispatched orchestrator scanning Step 1 has to read a long run-on to extract the three new clauses. The existing worktree-cwd block is already long, but this pushes it further. Not blocking, and it matches the document's existing style — but if this paragraph continues to grow, consider a nested sub-list (like the integration-table in the same file) to keep each must-clause scannable.

2. "Before the CI-poll tail" — opaque timing marker

SKILL.md line 33 and line 84

commit, push, and open the PR as early as practical — before the CI-poll tail — an experienced operator will understand this, but a first-time worker reading the brief might not know what "the CI-poll tail" refers to without following the dispatch cadence steps. The Gotcha rephrases it slightly ("rather than deferring until its fix-and-verify loop is done") which is actually clearer. Might be worth mirroring that phrasing back in Step 1 too, or adding a brief parenthetical — again, not blocking.

3. Closes #819 in the CHANGELOG (irony check — confirmed fine)

CHANGELOG.md line 18

The entry closes with Closes #819 — this is markdown changelog prose, not a code comment, so comment-hygiene doesn't apply here. The 0.7.4 entry uses the same pattern (Closes the correctness gap behind #566). Consistent and correct.

4. Gotchas ordering — coherent

The three new bullets land between the worktree-cwd gotcha and the scope-fence drift closing bullet. The grouping makes sense: the first four bullets are all "brief every worker to …" dispatch-time concerns, and scope-fence drift is a return-time concern. No issue.

5. Semver bump — appropriate

0.7.5 → 0.7.6 is a patch bump for a documentation/guidance-only addition to an existing skill. Same class as 0.7.4. Correct.


Verdict

Approve. The changes are correct, consistent with the established pattern, and directly address the problem statement. The two phrasing observations above (paragraph length, "CI-poll tail") are suggestions for future polish, not blockers for this PR.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and read PR diff
  • Review changed files
  • Analyze for security vulnerabilities
  • Post findings

No security issues found.

This PR adds plain-English instruction text to a skill file (SKILL.md), bumps a version in plugin.json, and adds a CHANGELOG.md entry. None of the three changed files introduce a meaningful attack surface.

Analysis of each change:

SKILL.md — three new instruction clauses for dispatched workers:

  • No issue-number back-references in comments — pure style/CI-hygiene guidance text. No security surface.
  • Exec-bit on new shebang files via git update-index --chmod=+x — instructs workers to mark legitimately-created shebang scripts executable before committing. This is scoped to "new files with a shebang" created as part of the task. It doesn't broaden exec-bit grants to arbitrary files, doesn't run any command on behalf of untrusted input, and doesn't introduce a code-execution path not already present in any repo that contains shell scripts. No meaningful escalation vector.
  • Commit, push, and open the PR early — a workflow-optimization clause. Pushing early surfaces work to CI and review sooner; the PR lifecycle retains the human review gate. No injection surface, no credential handling, no permission widening.

plugin.json — version bump 0.7.5 → 0.7.6. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Issue references in a changelog (Closes #819) are standard practice and distinct from the code-comment back-references the new clause guards against.

Items checked and not found:

  • No GitHub Actions workflows touched (.github/workflows/ not modified).
  • No pull_request_target / workflow_run patterns introduced.
  • No github context interpolated into run: blocks.
  • No new permissions: grants.
  • No secrets, tokens, or credentials referenced or stored.
  • No new MCP servers, external network calls, or supply-chain dependencies.
  • No template/command/path injection surfaces opened.
  • No authorization or access-control logic changed.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 21, 2026 10:19
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Post-green review classification (work lane):

Finding Source Classification Disposition
Everything PR review + security review "Approve" / "No security issues found" No action needed
Step 1 paragraph length (cosmetic) PR review Non-blocking style suggestion for future polish Left alone
"CI-poll tail" timing marker phrasing PR review Non-blocking suggestion (Gotchas phrasing already clearer) Left alone

No DEFER-FORBIDDEN class applies (doc/prompt-guidance-only change, no behavioral/security surface).

No inline review threads to resolve (PR-level comments only).

Flipping to ready, handing off to the babysit lane for ongoing watch.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read changed files
  • Review changes for correctness, security, and guideline alignment
  • Post findings

Summary

Clean, targeted change. Three new CI-hygiene clauses follow the 0.7.4 / worktree-cwd precedent exactly — both a Step 1 brief-level instruction and a Gotchas-section reinforcement for each. Versioning, changelog format, and commit scope are all consistent with prior patches in this class.


Findings

1. Brief vs. Gotchas — "CI-poll tail" phrasing asymmetry (cosmetic)

SKILL.md line 33 vs. line 84

The Gotcha rephrases the timing marker as "rather than deferring until its fix-and-verify loop is done" — which is self-contained and concrete. The Step 1 phrase "before the CI-poll tail" requires knowing what the CI-poll tail is. Non-blocking; the Gotcha already provides the clearer explanation. If Step 1 is ever touched again, mirroring the Gotcha's phrasing would improve first-read comprehension for a fresh worker.

2. Step 1 paragraph length — continued growth (cosmetic)

SKILL.md line 33

The dispatch cadence Step 1 is now a single ~1,800-character sentence covering five mandatory-clause families. Each addition is correct and necessary; the document style is consistent. But if more clauses land here, a nested sub-list (matching the integration table below) would make each MUST scannable at a glance. Worth noting for the next similar patch.

3. No trailing period on Step 1 (matches existing document style — fine)

The appended clause ends without a period, consistent with every other step in the "Dispatch cadence" list (items 2–5 all omit terminal periods). No action needed.

4. Closes #819 in CHANGELOG — correct

Prose in a changelog entry, not a code comment. The comment-hygiene check does not apply here. Consistent with the 0.7.4 entry (Closes the correctness gap behind #566). Confirmed fine.

5. Line count — confirmed under cap

SKILL.md is 86 lines, well within the 500-line skill-quality-gate cap.

6. CLAUDE.md / plugin-philosophy compliance — all clear

  • Repo-agnostic: new clauses are generic guidance, no hardcoded paths or project-specific values
  • No new dependencies, permissions, secrets, or external network calls
  • Version bump (0.7.5 → 0.7.6) is correct for a documentation/guidance-only patch

Verdict

Approve. The three clauses are correct, actionable, and aligned with the established worktree-cwd pattern. The two cosmetic observations above are suggestions for future polish, not blockers. Both the security review and prior PR review reached the same conclusion.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and read PR diff
  • Review changed files
  • Analyze for security vulnerabilities
  • Post findings

No security issues found.

Three changed files, all reviewed:

SKILL.md — plain-English instruction text appended to an existing paragraph and three new Gotcha bullets. Analyzed each new clause:

  • No issue-number back-references in comments — style/CI-hygiene prose. No security surface.
  • git update-index --chmod=+x on new shebang files — instructs workers to mark legitimately-created shebang scripts executable before committing. Scoped to "new file with a shebang," which is the conventional definition of a shell script. No arbitrary exec-bit widening, no untrusted-input path, no escalation vector beyond what already exists in any repo that ships shell scripts.
  • Commit, push, and open the PR early — workflow-optimization clause. PR review gate remains intact. No authorization or access-control change.

plugin.json — version bump 0.7.5 → 0.7.6 only. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Closes #819 is markdown prose, not a code comment, so the new comment-hygiene clause does not apply here. No injection surface.

Items checked and not found:

  • No .github/workflows/ changes
  • No pull_request_target / workflow_run patterns introduced
  • No github context interpolated into run: blocks
  • No permissions: grants widened
  • No secrets, tokens, or credentials referenced
  • No new MCP servers, external network calls, or supply-chain dependencies
  • No template, command, path, or prompt-injection surfaces opened
  • No authorization or access-control logic changed

@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: e791c0baeb

ℹ️ 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/implementation/skills/implement-dispatch/SKILL.md Outdated
…rom worker briefs

Address two Codex review findings on the implement-dispatch brief clauses:
`git update-index --chmod=+x` alone only updates the index and a later
`git add` reverts it, so the brief now requires `chmod +x` on the worktree
file first, matching the source-control commit skill's documented failure
modes. The early-push clause told workers to open a PR before the
orchestrator verifies their changes, conflicting with implement Step 5's
review/PR flow under user control; workers now stop at commit+push, and PR
creation is scoped to the orchestrator's post-verification flow.
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read PR diff at HEAD (58aae3c)
  • Review changed files for correctness, security, and CLAUDE.md alignment
  • Post findings

Summary

The two Codex P2 fixes landed correctly in 58aae3c. The exec-bit clause now requires chmod +x <path> before git update-index --chmod=+x <path> in both Step 1 and Gotchas — exactly matching the three-failure-mode analysis in plugins/source-control/skills/commit/SKILL.md (worktree/index mismatch → git status mode diff, later git add reverts index, pathspec commit reads worktree mode). The PR-in-every-worker-brief conflict with /implementation:implement Step 5 is resolved: workers now commit+push only; PR creation is explicitly delegated to the orchestrator's post-verification flow.


Findings

1. exec-bit two-step — correctly fixed

SKILL.md line 33 and line 83

Both Step 1 and Gotchas now state chmod +x <path> before git update-index --chmod=+x <path>. Cross-verified against the source-control commit skill's exec-bit section, which documents the exact three failure modes the ordering fixes. The instruction is scoped correctly to "new file with a shebang" — not a blanket exec-bit grant.

2. PR creation scoped out of worker briefs — correctly fixed

SKILL.md line 33 and line 84

Step 1 now ends with "PR creation stays out of the brief: it belongs to the orchestrator's post-verification flow (/implementation:implement Step 5), invoked only after every worker return is verified and the phase's build/test gate passes." The Gotcha mirrors this. No longer conflicts with /implementation:implement Step 5's "Do not commit-and-push unilaterally" rule.

3. CHANGELOG describes final state — appropriate

CHANGELOG.md lines 5–21

The [0.7.6] entry documents both the exec-bit two-step (chmod +x before git update-index) and the PR-stays-out-of-worker-brief decision. It reflects the shipped behavior, not the intermediate pre-fix state. This is the correct approach for a changelog.

4. Line count — confirmed under cap

SKILL.md is 86 lines; the 500-line skill-quality-gate cap is not a concern.

5. CLAUDE.md compliance — all clear

  • Repo-agnostic: all new clauses are generic guidance, no hardcoded paths or project-specific values ✓
  • Plugin-form-safe: no ${CLAUDE_PLUGIN_ROOT} references, no reach-outs ✓
  • Versioned: 0.7.5 → 0.7.6 patch bump for guidance-only addition ✓
  • No new dependencies, permissions, MCP servers, or external network calls ✓

6. Step 1 paragraph length — cosmetic, noted for future

SKILL.md line 33

Step 1 is now a single ~2,000-character sentence covering five mandatory-clause families. Each addition is correct. Prior reviews flagged this as a future refactor candidate (nested sub-list, matching the integration table). Not blocking — flagging for the record only.


Verdict

Approve. Both Codex P2 findings are correctly resolved at 58aae3c. The exec-bit fix is technically sound (verified against the source-control commit skill's exec-bit spec), the PR-scoping fix resolves the conflict with /implementation:implement Step 5, and all three original CI-hygiene clauses are in place in both Step 1 and Gotchas. No security issues. No CLAUDE.md violations.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and understand request
  • Read PR diff
  • Review changed files for security vulnerabilities
  • Post findings

No security issues found.

Three files changed across two commits (e791c0b, 58aae3c), all reviewed:

SKILL.md — plain-English instruction text added to the "Compose the brief" paragraph and three new Gotcha bullets. Each new clause analyzed:

  • No issue-number back-references in code comments — style/CI-hygiene prose. No execution path, no injection surface.
  • chmod +x <path> before git update-index --chmod=+x <path> on new shebang files — instructs workers to apply exec permissions to files they themselves created. The <path> placeholder is a file the worker is authoring, not untrusted external input; no command-injection vector. The operation is scoped to "any new file with a shebang," which is the conventional definition of a shell script. No arbitrary privilege escalation, no exec-bit widening beyond what already exists in any repo shipping shell scripts.
  • Commit and push early, PR creation stays out of the brief — a workflow-optimization clause that actually tightens the security posture of prior language. The earlier version of this clause told workers to open a PR before the orchestrator verified their output; the fix in 58aae3c removes PR creation from workers entirely and restricts it to the orchestrator's post-verification flow under user control. The human review gate is preserved and the authorization boundary is now more explicit.

plugin.json — version bump 0.7.5 → 0.7.6 only. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Closes #819 is markdown prose, not a code comment; the new comment-hygiene clause does not apply. No injection surface.

Items checked and not found:

  • No .github/workflows/ changes
  • No pull_request_target / workflow_run patterns introduced
  • No github context interpolated into run: blocks
  • No permissions: grants widened
  • No secrets, tokens, or credentials referenced or stored
  • No new MCP servers, external network calls, or supply-chain dependencies
  • No template, command, path, or prompt-injection surfaces opened
  • No authorization or access-control logic changed

@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: 58aae3c96b

ℹ️ 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
…ause

The exec-bit clause told workers to `chmod +x` and `git update-index
--chmod=+x` any new file whose first line is a shebang. Reading that first
line follows a symlink, so a newly added symlink to a `#!` target would be
caught — but git stages symlinks as mode 120000, `git update-index
--chmod=+x` fails outright on one, and chmod could mutate a target outside
the repo. Both the Step 1 clause and the Gotchas reminder now scope to new
regular files (staged 100644/100755), matching the source-control commit
skill's "Skip symlinks first" exec-bit handling.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read changed files at HEAD (94a15c9)
  • Review changes for correctness, security, and guideline alignment
  • Post findings

Summary

Three commits, three issues cleanly resolved. The symlink fix in 94a15c9 is the final piece — it correctly exempts 120000-mode symlinks from the exec-bit instruction in both Step 1 and Gotchas, matching the /source-control:commit skill's own exec-bit guidance. All Codex P2 findings are addressed; no regressions introduced.


Findings

1. Symlink exemption — correctly implemented

SKILL.md line 33 and line 83

Step 1 now reads "any new regular file (staged 100644/100755, never a 120000 symlink — git update-index --chmod=+x fails on one)". The Gotchas bullet at line 83 mirrors it: "on any new regular shebang file (skip symlinks — staged 120000, they fail git update-index --chmod=+x)". Both sides of the document are consistent. The Codex P2 finding was valid: git update-index --chmod=+x exits with fatal: git update-index: cannot chmod +x on a symlink; chmod on a symlink also mutates the target. The fix is sound.

2. Exec-bit two-step and PR-scoping — carried through correctly

SKILL.md line 33 and line 84

The 58aae3c fixes survive the 94a15c9 retouch intact. chmod +x <path> before git update-index --chmod=+x <path> appears in both Step 1 and Gotchas. PR creation is correctly scoped out of every worker brief in both locations, referencing /implementation:implement Step 5 as the owner.

3. CHANGELOG missing symlink refinement (minor)

CHANGELOG.md lines 10–21

The [0.7.6] entry says "any new shebang file" — the word "regular" and the symlink exemption added by 94a15c9 are absent. The code is correct; the changelog understates the precision of the instruction. This is non-blocking (the changelog is internal documentation and the symlink exemption is a narrowing refinement, not a behavior reversal), but if this entry is updated before merge, adding "(regular file only — symlinks are exempt; git update-index --chmod=+x fails on 120000-mode entries)" after "any new shebang file" would make the changelog match the shipped text. Fix this →

4. Step 1 paragraph length — cosmetic, still non-blocking

SKILL.md line 33

The dispatch cadence Step 1 is now a single ~2,100-character sentence covering five mandatory-clause families. Each clause is correct and necessary. Prior reviews flagged this for future refactor (nested sub-list matching the integration table below). Still not blocking; noted for the next pass.

5. Line count — under cap

86 lines. The 500-line skill-quality-gate cap is not a concern.

6. CLAUDE.md compliance — all clear

  • Repo-agnostic: all new clauses are generic guidance; no hardcoded paths ✓
  • Plugin-form-safe: no ${CLAUDE_PLUGIN_ROOT} reach-outs ✓
  • Versioned: 0.7.5 → 0.7.6 patch bump for guidance-only addition ✓
  • No new dependencies, permissions, MCP servers, or external network calls ✓

Verdict

Approve. All Codex P2 findings are correctly resolved across the three commits. The symlink exemption is technically sound, the exec-bit two-step is consistent between Step 1 and Gotchas, and PR creation is cleanly scoped to the orchestrator. Finding #3 (CHANGELOG gap) is the only item worth considering before merge — it's a one-line prose update, not a code change.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and understand request
  • Read PR diff
  • Review changed files for security vulnerabilities
  • Post findings

No security issues found.

Three files changed across three commits (e791c0b, 58aae3c, 94a15c9), all reviewed:

SKILL.md — plain-English instruction text added to Step 1 and three new Gotcha bullets. Each new clause analyzed:

  • No issue-number back-references in code comments — CI-hygiene prose. No execution path, no injection surface.
  • chmod +x <path> before git update-index --chmod=+x <path> on new regular shebang files — instructs workers to mark files they themselves create as executable before committing. The <path> placeholder is a worker-authored path, not untrusted external input; no command-injection vector. Scoped to staged 100644/100755 regular files only — symlinks (120000) are explicitly excluded in 94a15c9, which prevents chmod from following a symlink and mutating a target outside the repository boundary. This is a security improvement over the prior draft.
  • Commit and push early; PR creation stays out of worker briefs — the final commit in this PR (58aae3c) actually tightens the authorization boundary relative to the initial draft. The earlier version instructed workers to open a PR before orchestrator verification; the fix restricts PR creation to the orchestrator's post-verification flow, preserving the human review gate. No authorization or access-control gap introduced.

plugin.json — version bump 0.7.5 → 0.7.6 only. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Closes #819 is markdown prose, not a code comment; the comment-hygiene clause does not apply. No injection surface.

Items checked and not found:

  • No .github/workflows/ changes
  • No pull_request_target / workflow_run patterns introduced
  • No github context interpolated into run: blocks
  • No permissions: grants widened
  • No secrets, tokens, or credentials referenced or stored
  • No new MCP servers, external network calls, or supply-chain dependencies
  • No template, command, path, or prompt-injection surfaces opened
  • No authorization or access-control logic weakened (PR-scoping fix tightens it)

@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: 94a15c9785

ℹ️ 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/implementation/skills/implement-dispatch/SKILL.md Outdated
…econcile early-push with phase-boundary commit

Address two more Codex review findings on the implement-dispatch brief clauses:
`git update-index --chmod=+x` fails outright on a path not yet in the index
("cannot add to the index"), so the exec-bit clause now requires `git add
<path>` between `chmod +x` and the index override for a brand-new shebang
file. The early-push clause told workers to commit+push source before the
orchestrator's phase-boundary mark-then-commit exists, which Step 4 item 4
reads as forbidden (committing source, then marking DONE in a follow-up
commit); the brief and "Phase boundaries" section now state explicitly that
the worker's early commit is a source-only checkpoint and the phase-boundary
commit becomes plan-marks-only for a dispatched phase, a scoped exception to
inline mode's combined commit.

@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: 780042072e

ℹ️ 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
…lauses

Base-branch drift: origin/main picked up unrelated commits (runner-policy
sync, README plugin-listing updates, a shell:bash chore, and more) since this
branch forked. Took origin/main's version verbatim for every file this
branch doesn't touch (.github/standards/runner-policy/policy.json, README.md,
and the rest of the auto-merged tree). The one real conflict was
plugins/implementation/CHANGELOG.md: both branches independently bumped the
implementation plugin to 0.7.6 for unrelated changes, a version collision.
Per this org's collision protocol (serialize + bump past the live claim),
bumped this branch's plugin.json to 0.7.7 and gave its CHANGELOG entry its
own [0.7.7] section above main's already-landed [0.7.6] entry.
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read changed files at HEAD (4bc8270)
  • Review changes for correctness, security, and guideline alignment
  • Assess outstanding Codex P2 findings
  • Post findings

Summary

This review covers the current HEAD (4bc8270 — a merge recompose over origin/main). Three productive fix rounds landed prior to this merge: the exec-bit worktree/index split (58aae3c), the symlink exemption (94a15c9), and the stage-before-chmod + phase-boundary reconciliation (7800420). Two Codex P2 findings from the final automated pass (780042072e) remain open. Assessed below.


Outstanding Codex P2 findings (from 780042072e)

1. Exec-bit override after formatter re-staging

SKILL.md line 33 and line 83

Assessment: PLAUSIBLE, low practical risk — not blocking.

The concern is real: if a worker runs a formatter after the three-step exec-bit sequence (chmod, git add, git update-index) and then does a subsequent git add <path> to stage the formatter's changes, the index mode can be reset to 100644 on filesystems that don't retain the executable bit. The instruction as written doesn't constrain when in the workflow the sequence must occur.

Two mitigations keep the practical risk low:

  • A git add by itself won't reset the index mode for an already-staged file unless the fs mode is 0644 — on most CI hosts (linux) chmod +x persists across the formatter run, so the subsequent git add re-reads 0755 and preserves it. The failure mode is filesystem/OS-specific.
  • The exec-bit CI check (the one this clause is defending against) would still catch the failure before merge, just one round later than ideal.

The fix — adding "run this sequence after all formatting, immediately before staging the commit" — is correct and low-effort. Fix this → — but can be deferred without reopening a CI failure path that the existing check would still catch.

2. Respect the consuming project's commit and push policy

SKILL.md line 33 and line 84

Assessment: PLAUSIBLE but design-level, not correctness — not blocking.

The finding is that an unconditional early-push clause violates the CLAUDE.md design principle of "configurable without editing the plugin." The counter: this plugin's entire stated purpose is autonomous/orchestrated execution, where automated commits and pushes are expected. The consuming project has opted into that mode by invoking this skill. The "as early as practical" qualifier already concedes discretion. Making the clause conditional on a resolved consumer policy would require workers to know that policy, which isn't surfaced in a standard brief.

This is a legitimate design observation for edge cases (a team that wants human-gate on every commit before push), but it doesn't rise to a correctness defect for the plugin's target audience. Suitable for a follow-up if a consumer reports the friction.


Remaining findings

3. CHANGELOG [0.7.7] — complete and accurate

CHANGELOG.md lines 6–26

The entry correctly reflects the final shipped state: three-step exec-bit ordering (chmod, git add, git update-index), symlink exemption, phase-boundary-commit exception, and PR-stays-out-of-worker-brief decision. The prior review's observation (CHANGELOG said "any new shebang file" without the "regular file" qualifier) is resolved. Closes #819 in prose is correct.

4. Version bump 0.7.6 → 0.7.7 — correct

0.7.6 landed on origin/main via the shell: bash declarations (7688ae7). This PR correctly advances to 0.7.7 for the CI-hygiene clauses.

5. Line count — under cap

85 lines. The 500-line skill-quality-gate cap is not a concern.

6. CLAUDE.md compliance — all clear

  • Repo-agnostic: all new clauses are generic guidance, no hardcoded paths or project-specific values ✓
  • Plugin-form-safe: no ${CLAUDE_PLUGIN_ROOT} reach-outs ✓
  • Versioned: 0.7.6 → 0.7.7 patch bump for guidance-only addition ✓
  • No new dependencies, permissions, MCP servers, or external network calls ✓

7. Step 1 paragraph length — cosmetic, previously noted

Step 1 is now a ~2,500-character sentence covering five mandatory-clause families. Each clause is correct. Prior reviews flagged this for a future sub-list refactor; still not blocking.


Verdict

Approve. Both open Codex P2 findings are assessed as low practical risk — neither reopens a failure path that CI wouldn't catch, and neither represents a correctness defect for this plugin's intended use case. All prior Codex findings (e791c0b through 7800420) are correctly resolved. The exec-bit three-step sequencing is sound, the phase-boundary exception is correctly scoped and documented, and the symlink guard is in place in both Step 1 and Gotchas.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and understand request
  • Read PR diff and changed files
  • Analyze for security vulnerabilities
  • Post findings

No security issues found.

Three files changed at HEAD (4bc8270), all reviewed:

SKILL.md — plain-English instruction text appended to Step 1 and three new Gotcha bullets. Each new clause analyzed:

  • No issue-number back-references in code comments — CI-hygiene prose. No execution path, no injection surface.
  • chmod +x <path>, git add <path>, git update-index --chmod=+x <path> on new regular shebang files — instructs workers to mark files they themselves author as executable before committing. The <path> placeholder is a worker-authored path under the worker's scope fence, not untrusted external input fed to a shell. Symlinks are explicitly excluded (120000 mode), which prevents chmod from following a link and mutating a target outside the repository boundary. No command-injection vector; the instruction is natural-language guidance, not an executed template.
  • Commit and push early; PR creation stays out of worker briefs — restricts PR creation to the orchestrator's post-verification flow. The human PR review gate is explicitly preserved. Workers pushing to a branch they are already authorized to write to introduces no new access-control gap.

plugin.json — version bump 0.7.6 → 0.7.7 only. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Closes #819 is markdown prose, not a code comment; the new comment-hygiene clause does not apply here. No injection surface.

Items checked and not found:

  • No .github/workflows/ changes
  • No pull_request_target / workflow_run patterns introduced
  • No github context interpolated into run: blocks
  • No permissions: grants widened
  • No secrets, tokens, or credentials referenced or stored
  • No new MCP servers, external network calls, or supply-chain dependencies
  • No template, command, path, or prompt-injection surfaces opened
  • No authorization or access-control logic weakened (PR-scoping explicitly preserves the human review gate)

@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: 4bc827052f

ℹ️ 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
…spatch-brief-clauses

# Conflicts:
#	plugins/implementation/CHANGELOG.md
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Post-rebase re-review classification (work lane):

Rebased cleanly onto post-#860 main (0.7.6 → 0.7.7). Worker did solid iterative work here beyond a mechanical rebase — Codex's automated pass found real gaps in the new exec-bit clause (worktree/index split, symlink handling, stage-order vs. formatter re-staging) across 3 fix rounds, all resolved except 2 explicitly assessed as non-blocking:

Finding Source Classification Disposition
Exec-bit sequence could be reset by a post-sequence formatter re-stage Codex P2 → reviewer assessment PLAUSIBLE, low practical risk — CI's own exec-bit check would still catch it one round later; not a silent failure Left alone (fix suggested for later polish)
Unconditional early-push clause vs. "configurable without editing the plugin" principle Codex P2 → reviewer assessment PLAUSIBLE but design-level, not a correctness defect for this plugin's autonomous-execution target audience Left alone (follow-up only if a consumer reports friction)
Everything else (CHANGELOG, version bump, line count, CLAUDE.md compliance) PR review All correct/clear No action needed

No DEFER-FORBIDDEN class applies. No inline review threads outstanding — all addressed across the fix rounds already reflected in this history.

Flipping to ready again, handing off to babysit.

…f-clauses' into feat/819-implement-dispatch-brief-clauses
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context and understand request
  • Read PR diff
  • Review changed files for security vulnerabilities
  • Post findings

No security issues found.

Three files changed at HEAD (f160665), all reviewed:

SKILL.md — plain-English instruction text added to the "Compose the brief" paragraph (Step 1) and three new Gotcha bullets. Each new clause analyzed:

  • No issue-number back-references in code comments — CI-hygiene prose. No execution path, no injection surface.

  • chmod +x <path>, git add <path>, git update-index --chmod=+x <path> on new regular shebang files — instructs workers to mark files they themselves create as executable before committing. The <path> placeholder refers to a worker-authored file within the worker's scope fence, not untrusted external input fed to a shell — no command-injection vector. Symlinks (120000 mode) are explicitly excluded, which prevents chmod from following a link and mutating a target outside the repository boundary. This constraint is a narrowing of a footgun, not an expansion of permissions.

  • Commit and push early; PR creation stays out of worker briefs — the final state of this clause restricts PR creation to the orchestrator's post-verification flow. The human PR review gate is explicitly preserved, and the authorization boundary is more explicit than it was before this PR (an earlier draft told workers to open PRs before orchestrator verification; that was corrected in 58aae3c). Net effect: tighter authorization, not looser.

plugin.json — version bump 0.7.6 → 0.7.7 only. No dependencies added or changed, no permissions altered.

CHANGELOG.md — human-readable release notes. Closes #819 is markdown prose, not a code comment; the new comment-hygiene clause does not apply here. No injection surface.

Items checked and not found:

  • No .github/workflows/ changes
  • No pull_request_target / workflow_run patterns introduced
  • No github context interpolated into run: blocks
  • No permissions: grants widened
  • No secrets, tokens, or credentials referenced or stored
  • No new MCP servers, external network calls, or supply-chain dependencies
  • No template, command, path, or prompt-injection surfaces opened
  • No authorization or access-control logic weakened (PR-scoping fix tightens the boundary)

The two open Codex P2 findings from the automated pass (780042072e) — formatter re-staging resetting the exec bit, and the unconditional push clause vs. consumer commit policy — are functional/workflow concerns assessed as low practical risk in prior reviews. Neither introduces an exploitable security vulnerability in the threat model applicable to a guidance-only skill file.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read changed files at HEAD (f160665)
  • Review changes for correctness, security, and guideline alignment
  • Assess outstanding Codex P2 findings
  • Post findings

Summary

This review covers HEAD (f160665 — a merge of the remote branch after the 7800420 fixes). All prior Codex P2 findings from earlier rounds are correctly resolved. One new Codex P2 from the final automated pass (780042072e4bc8270) remains open and is assessed below.


Outstanding Codex P2 (from 4bc8270 automated pass)

Keep source and plan marks in one phase commit

SKILL.md line 45

Assessment: PLAUSIBLE, narrow failure path — not blocking.

The concern is genuine: the early-push clause splits what plugins/implementation/reference/topic-docs.md:31-36 calls a single "one commit, one story" — the worker's source checkpoint commit lands before the orchestrator's marks-only phase-boundary commit. If a worker commits+pushes, then both the worker session and the orchestrator session die before the [DONE] mark is committed, a resumed orchestrator reading the committed plan ([TODO] still) could re-dispatch work that's already in git.

Why it's non-blocking:

  1. topic-docs.md:35-36 already carves space for this: "Dispatched workers return results by value; this session writes both tiers in its own checkout" — the authoritative contract text acknowledges the dispatched model. The orchestrator's session remains the single writer of plan marks, so the two-commit sequence (worker source → orchestrator marks) is consistent with the contract's intent even if not the canonical single-commit form.
  2. The failure chain requiring re-dispatch is narrow: worker commits+pushes, worker dies, orchestrator dies without verifying the return, and a resumed orchestrator session reads the stale [TODO]. Any of these failing to chain prevents the problem.
  3. The prior reviewer (at the 4bc8270 round) explicitly assessed the analogous "early-push clause conflicts with consumer push policy" finding as non-blocking and design-level. The same judgment applies here.

What would fully close it: Adding a note to topic-docs.md's phase-commit rule documenting the dispatched-mode exception (two-commit sequence in orchestrated runs, plan-marks-only phase-boundary commit). This is a topic-docs.md update, not a SKILL.md update, and belongs in a follow-up rather than this PR. Fix this in a follow-up →


Remaining findings

1. Prior Codex P2s — all correctly resolved

Finding (round) Resolution Status
Worktree exec bit not set (e791c0b) chmod +x <path> before git update-index in both Step 1 and Gotchas ✓ Fixed (58aae3c)
PR creation in every worker brief (e791c0b) PR creation removed from worker briefs; scoped to orchestrator post-verification flow ✓ Fixed (58aae3c)
Symlink exemption missing (58aae3c) "never a 120000 symlink" guard added in both Step 1 and Gotchas ✓ Fixed (94a15c9)
Stage before chmod override (94a15c9) git add <path> step inserted between chmod and git update-index ✓ Fixed (7800420)
Early-push conflicts with atomic phase commit (94a15c9) Phase boundaries section documents two-commit exception; early commit scoped as source-only checkpoint ✓ Fixed (7800420)

2. Exec-bit formatter re-staging (previously assessed non-blocking)

SKILL.md line 33

Still assessed as non-blocking: on Linux CI hosts chmod +x persists the mode through a formatter run, and the exec-bit check would still catch the failure if it regressed — just one round later. No change from prior assessment.

3. CHANGELOG [0.7.7] — accurate

CHANGELOG.md lines 6–26

Describes the final shipped state accurately: three-step exec-bit ordering, symlink exemption, phase-boundary exception, PR-stays-out-of-brief. Prior observation about the missing "regular file" qualifier is resolved. Closes #819 in prose is correct.

4. Version bump 0.7.6 → 0.7.7 — correct

0.7.6 corresponds to the shell: bash declarations that merged via main. This PR correctly advances to 0.7.7 for the CI-hygiene clause additions.

5. Line count — under cap

85 lines. The 500-line skill-quality-gate cap is not a concern.

6. CLAUDE.md compliance — all clear

  • Repo-agnostic: all clauses are generic guidance, no hardcoded paths ✓
  • Plugin-form-safe: no ${CLAUDE_PLUGIN_ROOT} reach-outs ✓
  • Versioned: 0.7.6 → 0.7.7 patch bump for guidance-only addition ✓
  • No new dependencies, permissions, MCP servers, or external network calls ✓

7. Step 1 paragraph length — cosmetic, noted for future

SKILL.md line 33

~2,500-character single sentence covering five mandatory-clause families. Every prior review round flagged this for a future sub-list refactor. Still not blocking.


Verdict

Approve. All prior Codex P2 findings are resolved. The one remaining open finding (source/marks split in dispatched commits) is assessed as PLAUSIBLE but non-blocking — it reflects a narrow failure path already documented in Phase boundaries, and topic-docs.md updating is appropriate follow-up work on main rather than a blocker for this PR. No security issues. No CLAUDE.md violations.

@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: f160665671

ℹ️ 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
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

This was generated by AI (decide lane — decision-brief pass; parked PR-thread decision surfaced via periodic scan).

Decision brief — orchestrator-side worker-commit integration when a worker early-pushes

Parking the one unbriefed live finding on thread #discussion_r3624479204 for an author call. This is the only finding on that thread not already covered by a prior disposition (issuecomment-5036980253 / discussion_r3624111040 rule the exec-bit and atomic-commit siblings — not re-briefed here).

Problem. This PR's push-early clause has a dispatched worker commit and push its source early (SKILL.md:33), and the new phase-boundary exception then makes the orchestrator's phase-boundary commit plan-marks-only on the premise that "the worker's earlier commit already carries the source" (SKILL.md:45). But when the worker edits in a dedicated worktree/checkout, that pushed source commit is not automatically present in the orchestrator's checkout — so the plan-marks-only commit either omits the source entirely (worker on a separate branch) or is rejected non-fast-forward on push (same branch, orchestrator behind the worker's remote head).

Today. The skill defines that the worker pushes early and that the phase-boundary commit drops to plan-marks-only, but never defines the orchestrator-side integration step between them. Cadence step 3 verifies the return against direct evidence (SKILL.md, "Verify the return"), and step 4 runs the main-side build/test gate — but neither fetches or integrates the worker's pushed commit into the orchestrator's branch before the plan-marks commit lands. The gap is topology-dependent (dedicated worktree vs. shared checkout) and follows directly from the exception introduced on this branch.

Options (reversibility noted; all are doc-only edits to implement-dispatch, so cheaply reversible):

  • RECOMMENDED — Define an explicit orchestrator integration step (fetch-then-integrate) before the plan-marks-only commit. After the return is verified (cadence step 3) and before the phase-boundary commit, the orchestrator MUST git fetch and land the worker's pushed source onto the orchestrator's branch: fast-forward when worker and orchestrator share the branch, merge/cherry-pick when the worker used a dedicated branch. The plan-marks-only commit then always lands on a ref that already contains the source, and the push is a fast-forward in both topologies. Rationale: fixes the root cause (undefined integration protocol) rather than constraining topology; preserves the early-push orphan-protection benefit; makes the who-edits/when-source-lands contract topology-explicit. Tradeoff: adds one integration action the orchestrator must get right; merge/cherry-pick path needs a conflict-handling note.

  • Defer the worker's push until the orchestrator signals. Worker commits locally, orchestrator pulls/integrates on its own schedule. Tradeoff: directly negates the reason early-push was added on this branch — a mid-flight worker session-limit death would again orphan unpushed work. Dominated unless orphan-protection is dropped. Reversible.

  • Constrain dispatch to a single shared branch/checkout. Require worker and orchestrator to share one branch so the worker's push + orchestrator pull --ff-only is always a fast-forward. Tradeoff: eliminates the failure mode by removing dedicated-worktree isolation — narrows the supported topology and loses parallel-worker separation. Reversible but limiting.

  • Tolerant fetch-or-skip rule. A protocol that, at the phase boundary, probes whether the source is present — fetch; integrate if on a dedicated worker branch; pull --ff-only if same branch; and in all cases keep the phase commit plan-marks-only. Tradeoff: essentially the RECOMMENDED step written as a branch-detecting conditional; more robust to unknown topology but more moving parts to specify and test.

Cross-links. #572 (autonomous orchestrator-dispatch lifecycle underspecified — branch/worktree provisioning, PR-creation ownership; this integration protocol is a concrete instance of that gap). #513 / #715 (role-separated worker mini-SDLC / complexity-dispatcher — the same orchestrator↔worker commit-handoff surface).

Decision needed: which integration protocol the orchestrator owns. Comment-only park — no thread state changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

implementation/implement-dispatch: front-load CI-hygiene + early-push clauses in worker brief

1 participant