Make @claude prompt demand a commit before finishing

[d-morrison]

Summary

• Sharpens step 4 of the @claude action prompt in .github/workflows/claude.yml to make explicit that uncommitted working-tree edits are lost when the runner exits, and that "I edited files" is not the same as "I committed".
• Adds a mandatory step 5 self-check: before stopping, run git status --porcelain (must be empty) and git log --oneline -5 (must show the new commits). If nothing was committed, either commit or explicitly say "no changes needed" — describing edits that weren't committed is a task failure.
• Makes the git push exclusion louder so a denied push doesn't lead Claude to wrap up before committing.

Why

Run 27045987736 on PR #896 finished with subtype: success, permission_denials_count: 1, and SHA_BEFORE == SHA_AFTER. Claude used Edit/Write to modify files and posted a "Done. Here's a summary of what I implemented" comment, but never invoked git add + git commit — so the post-step correctly fell through to the prose-post path and the edits were discarded with the runner. The fix is in the prompt, not the permission model: keeping git push out of Claude's hands remains the right design.

Test plan

• Trigger an @claude mention on a PR that requires file edits and confirm the new commits land on the PR head.
• Trigger an @claude mention where no edits are needed and confirm Claude says so explicitly rather than describing edits.

---

Generated by Claude Code

[claude]

Claude finished @d-morrison's task in 2m 4s —— View job

---

Code Review: PR #897 — Strengthen @claude commit-before-finish instructions

• Gather context
• Read changed file and diff
• Review changes
• Post findings

---

Overall verdict: LGTM with one nit. The changes are well-targeted at the documented failure mode. The core fix — git status --porcelain MUST be empty — directly catches the bug: if Claude runs Edit/Write but forgets git add + git commit, the porcelain output won't be empty, and step 5 forces the issue. The step 4 expansion and the louder git push framing are clear improvements.

---

Findings

[Strength] git status --porcelain check directly catches the documented bug
If Claude uses Edit/Write without committing, the working tree is dirty → porcelain output is non-empty → step 5 halts and demands a commit. Staged-but-not-committed changes (git add without git commit) are also caught. This is the right tool for the job.

[Strength] git push denial clarification
The old text just said "git push is NOT [allowed]." The new text adds "any attempt to call it will fail with a permission denial" and bolds "Do not attempt git push." This prevents the failure mode where a push denial signals "wrap up now" rather than "you still need to commit."

[Strength] "Task failure" framing

Treat this as a task failure: commit the work, or if you've decided not to change anything, say so explicitly.

Strong, unambiguous instruction. Explicitly naming the "no changes needed" path prevents Claude from going silent when it genuinely decided not to edit anything.

---

[Nit] git log --oneline -5 has no baseline

Step 5 says:

git log --oneline -5     # confirm your commits are present

If you intended to make code changes but git log shows no new commits on top of the branch you started on, you have NOT done the task.

The phrase "on top of the branch you started on" assumes Claude knows the session-start SHA. It doesn't — the prompt doesn't surface it. On a PR branch with prior commits, Claude is eyeballing whether the top 5 entries include something it authored, which is a heuristic rather than a hard check.

This is a secondary concern (the porcelain check is the primary guard), but a more precise instruction would be something like:

git log --oneline $(git merge-base HEAD origin/${{ github.base_ref }})..HEAD

or, for the issue-trigger case where a fresh branch was created from main:

git log --oneline main..HEAD

A simpler alternative: capture the starting SHA as a GitHub Actions expression and embed it in the prompt context block (the PR trigger already captures steps.head_before.outputs.sha — that value could be passed through as a context line alongside the other pre-resolved variables on lines 337–342). Then step 5 can say git log --oneline <STARTING_SHA>..HEAD — zero commits means nothing was committed, no eyeballing required.

This nit doesn't block the fix (the git status --porcelain check is sufficient for the documented failure), but it would make step 5 self-contained without ambiguity.

---

[Minor wording] "confirm your commits are present" is misleading on no-change sessions

The line immediately preceding the code block reads:

run these and verify the output:
…
git log --oneline -5 # confirm your commits are present

If the correct outcome is "no changes needed," there are no commits to confirm. The instruction later handles this correctly, but the inline comment # confirm your commits are present reads as an unconditional expectation. Consider # review recent commits or # check whether you committed.

---

[claude]

Claude finished @d-morrison's task in 5m 20s —— View job

---

Code Review: PR #897 — Make @claude prompt demand a commit before finishing

• Gather context
• Read changed files and diff
• Review changes
• Post findings

---

Overall verdict: LGTM with two nits. The core fix — step 5's git status --porcelain check — directly catches the documented failure mode for PR triggers, where the action's git-push.sh pushes nothing if Claude forgot to commit. The step 4 expansion and louder git push framing are clear improvements.

One structural finding worth noting before the nits:

---

Key asymmetry between PR and issue triggers

The post-step auto-commit sweep (git status --porcelain → git add -A → auto-commit, lines 876–882) exists only for the "Push branch and open draft PR for issue trigger" step. The PR trigger's "Re-request review" step (line 821) has no equivalent sweep.

This means for issue triggers, uncommitted edits are caught by the post-step even if Claude ignores step 5. For PR triggers, the new step 5 self-check in the prompt is the only guard (short of Claude's commits simply not being pushed). The PR is addressing the right failure mode with the right tool (prompt instruction), but adding the same auto-commit sweep to the PR trigger post-step would give belt-and-suspenders coverage there too. That's a separate PR, not a blocker here.

---

Nit 1 — git log --oneline -5 has no baseline [.github/workflows/claude.yml line 507]

git log --oneline -5     # confirm your commits are present

If you intended to make code changes but git log shows no new commits on top of the branch you started on, you have NOT done the task.

"On top of the branch you started on" requires Claude to know the session-start SHA, which isn't surfaced anywhere in the prompt context. On a PR branch with prior commits, Claude has to eyeball whether any of the five entries were authored this session — a heuristic rather than a hard check.

The git status --porcelain check (the primary guard) is sufficient on its own, but if you want the git log step to be unambiguous, the cleanest fix is to surface $STARTING_SHA in the prompt context block. It's already computed: for issue triggers it's steps.issue_branch.outputs.starting_sha (line 868); for PR triggers it's steps.head_before.outputs.sha (line 825). Passing it in alongside the other pre-resolved context variables (lines 337–342) would let step 5 say:

git log --oneline <STARTING_SHA>..HEAD   # zero output = nothing committed

Zero lines = nothing committed. No eyeballing. This nit was also flagged in the previous review (comment at 2026-06-06T00:06:27Z) and hasn't been addressed yet.

---

Nit 2 — # confirm your commits are present is misleading on no-change sessions [.github/workflows/claude.yml line 507]

git log --oneline -5     # confirm your commits are present

On a session that legitimately made no changes, there are no commits to confirm. The inline comment reads as an unconditional expectation. The fix text right below handles the no-change case correctly, but the comment contradicts it. A neutral label like # review recent commits or # verify work was committed would match both outcomes.

---

CLAUDE.md changes

Both additions in commit 3ff5f45 are clean:

• "Always merge main into the feature branch before pushing" — earlier than "before requesting review" catches stale-base CI failures sooner and avoids the two-push cycle.
• Workflow/CI changes in dedicated PRs — useful rule. This PR (which only touches .github/ and CLAUDE.md) is compliant; it doesn't mix with book content.

[claude]

Claude finished @d-morrison's task in 1m 56s —— View job

---

Code Review: PR #897 — Make @claude prompt demand a commit before finishing

• Gather context
• Read changed files and diff
• Review changes
• Post findings

---

Overall verdict: LGTM. Both nits flagged in prior reviews have been resolved by commit f5be610. One small new nit below; doesn't block merge.

---

What's fixed vs. prior reviews

Nit 1 (baseline SHA for git log) — resolved by f5be610.
The Session-start SHA is now surfaced in the context block (.github/workflows/claude.yml lines 389–394) as ${{ steps.head_before.outputs.sha || steps.issue_branch.outputs.starting_sha }}, and step 5 uses git log --oneline <START>..HEAD — zero output means nothing was committed. No more eyeballing.

Nit 2 (# confirm your commits are present) — resolved.
Step 5 now uses # check whether you committed, which is neutral and correct whether or not changes were intended.

---

Remaining nit

[Nit] .github/workflows/claude.yml lines 525–527 — example phrase split across two lines

               comment ("no changes
               needed because X") rather than describing edits that
               weren't committed.

In the YAML block scalar, this becomes a hard line break in the rendered prompt — Claude sees ("no changes\nneded because X"). The phrase is still interpretable but looks oddly formatted mid-sentence. One line would be cleaner:

               comment ("no changes needed because X") rather than
               describing edits that weren't committed.

Minor; the instruction is clear enough as-is. Fix this →

---

Strengths

• git status --porcelain check — catches both "edited but not staged" and "staged but not committed" in one command. Precisely right for the failure mode.
• git push denial framing — "any attempt to call it will fail with a permission denial" removes the ambiguity where a denied push could be mistaken as a signal to wrap up.
• "Task failure" language — unambiguous and forces one of two explicit outcomes (commit or say so), closing the silent-success gap.
• CLAUDE.md additions — "merge main before pushing" (not just before review-request) and dedicated-PR rule for workflow changes are both clean and this PR itself follows the latter.

[claude]

Claude finished @d-morrison's task in 2m 42s —— View job

---

Code Review: PR #897 — Make @claude prompt demand a commit before finishing

• Gather context
• Read changed files and diff
• Review changes
• Post findings

---

Overall verdict: LGTM. All three nits raised in prior reviews have been addressed. The core fix is correct, the CLAUDE.md additions are clean, and this PR is ready to merge.

---

Status of prior nits

Nit 1 — git log baseline ✅ resolved by f5be610

Step 5 now reads:

git log --oneline <START>..HEAD   # check whether you committed

and <START> is defined in the context block (lines 389–394) as ${{ steps.head_before.outputs.sha || steps.issue_branch.outputs.starting_sha }}. I verified that both step outputs exist:

• head_before (line 156): runs for PR triggers, captures the live PR head SHA via gh api.
• issue_branch (line 187): runs for issue triggers, captures git rev-parse HEAD after branch creation (line 197).

The || fallback handles both trigger types correctly. Zero lines from git log <START>..HEAD → nothing committed; no eyeballing needed.

Nit 2 — # confirm your commits are present misleading on no-change sessions ✅ resolved in same commit

Now reads # check whether you committed — neutral and correct regardless of whether the session made changes.

Nit 3 — split phrase introducing a hard line break ✅ resolved by 2d78f4d

The phrase is now on one line:

               comment ("no changes needed because X") rather than
               describing edits that weren't committed.

No YAML block-scalar newline mid-phrase.

---

Observation — working tree has CLAUDE.md reverted

git status shows M CLAUDE.md (working tree modified), and git diff HEAD -- CLAUDE.md shows the working tree has reverted the two additions from commit 3ff5f45:

-- Always merge `main` into the feature branch before pushing — not just before requesting review
+- Ensure branch is up to date with `main` before requesting review
 ...
-- Workflow / `.github/` / CI / infra changes go in their own dedicated PRs — never mix them with book-content PRs

The HEAD commit has the correct content, so this won't affect the PR or merge. It's likely an artifact from the previous @claude runner. No action needed unless a subsequent commit is pushed from this runner session (which shouldn't happen in a review-only run).

---

Strengths (unchanged from prior reviews, still valid)

• git status --porcelain as primary guard — catches both unstaged edits and staged-but-not-committed changes in one command.
• git push denial framing — "any attempt to call it will fail with a permission denial" closes the ambiguity where a denied push could be mistaken for "wrap up now."
• "Task failure" language — forces one of two explicit outcomes (commit or say so explicitly), eliminating the silent-success gap.
• CLAUDE.md additions — "merge main before pushing" (earlier than "before review-request") catches stale-base CI failures sooner; dedicated-PR rule for workflow changes is well-scoped and this PR itself complies with it.

---

[claude]

Claude finished @d-morrison's task in 3m 9s —— View job

---

Code Review: PR #897 — Make @claude prompt demand a commit before finishing

• Gather context
• Read changed files and diff
• Review changes
• Post findings

---

Overall verdict: LGTM. All three nits raised in prior reviews have been resolved. The core fix is correct and the PR is appropriately scoped.

Note: an inline-comment tool was not available in this session, so all findings are in this summary.

---

What the PR does

1. .github/workflows/claude.yml — three related changes:
   • Surfaces the session-start SHA in the context block (lines 389–394) via ${{ steps.head_before.outputs.sha || steps.issue_branch.outputs.starting_sha }}, so Claude can use it as a baseline without guessing.
   • Expands step 4 to make explicit that Edit/Write is not the same as git commit, and that a push-permission denial is not a signal to wrap up (lines 479–506).
   • Adds mandatory step 5 self-check: git status --porcelain must be empty; git log <START>..HEAD must show at least one commit when code changes were intended (lines 508–527).
2. CLAUDE.md — two additions to the Pull Requests section:
   • "merge main into the feature branch before pushing" (earlier than the previous "before requesting review")
   • Workflow/CI changes go in dedicated PRs — never mixed with book-content PRs

---

Prior nits — all resolved

Nit 1 — git log baseline SHA ✅ resolved

<START> is now defined in the context block (lines 389–394) and step 5 reads:

git log --oneline <START>..HEAD   # check whether you committed

Zero lines = nothing committed; no eyeballing.

Verified: steps.head_before (line 155) fires for all PR-trigger types (github.event.pull_request.number || github.event.issue.pull_request) and captures the live PR head SHA via gh api. steps.issue_branch (line 186) fires for issue triggers and emits git rev-parse HEAD as starting_sha (line 197). The || fallback in the context block correctly resolves to the right SHA for each event type.

Nit 2 — # confirm your commits are present misleading ✅ resolved

Now reads # check whether you committed — neutral for both code-change and no-change sessions.

Nit 3 — split phrase introducing a hard line break ✅ resolved

The phrase ("no changes needed because X") is on one line; no YAML block-scalar newline mid-phrase.

---

New minor observation

.github/workflows/claude.yml line 515 — <START> is a manual-substitution placeholder in a shell code block

git log --oneline <START>..HEAD   # check whether you committed

If Claude runs this verbatim without substituting the SHA, git interprets <START> as a redirect and the command errors. The preceding sentence ("substitute the Session-start SHA from the context block above for <START>") makes the intent clear, but an explicit inline reminder inside the block would make it harder to miss:

git log --oneline <START>..HEAD   # replace <START> with the SHA from context

Not a blocker — the instruction above the block is clear, and a git error would prompt Claude to fix it. Just noting it as the one remaining rough edge.

---

Strengths

• git status --porcelain as primary guard — catches both unstaged edits and staged-but-not-committed changes in one command. Exactly right for the documented failure mode.
• "Task failure" language (lines 523–527) — forces one of two explicit outcomes (commit or say so explicitly), closing the silent-success gap.
• git push denial framing (lines 490–493) — "any attempt to call it will fail with a permission denial" removes the ambiguity where a denied push could be mistaken for a wrap-up signal.
• CLAUDE.md: Both additions are clean. "Merge main before pushing" catches stale-base CI failures earlier than the previous "before requesting review." This PR itself complies with the new workflow-changes-in-dedicated-PRs rule.