chore(ai): L3-0000 add per-response deslop stop hook and fix PR template workflow - #911
scottdickerson wants to merge 3 commits into
Conversation
✅ Deploy Preview for phillips-seldon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Tip All tests passed and all changes approved!🟢 UI Tests: 476 tests unchanged |
|
🚀 Storybook preview is ready. • Preview: https://68b9f094608b90f3cfec5a06-rzuypnedca.chromatic.com/ |
There was a problem hiding this comment.
Pull request overview
Adds Claude Code hook tooling to nudge a per-response deslop step only when the current response modifies src/ code, and fixes the PR-title/template validation workflow so it actually runs on this repo’s main branch.
Changes:
- Added paired
.claude/hooks/scripts to snapshot response start and blockStoponce per response whensrc/TS/TSX/SCSS changes are detected. - Registered the new
UserPromptSubmit+Stophooks in.claude/settings.json. - Updated the PR validation workflow to trigger on
mainand removed the repo-inappropriate title-prefix enforcement.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .github/workflows/pr-template-and-prefix.yaml | Retargets workflow to main, removes title-prefix rule, and updates naming/messages accordingly. |
| .claude/settings.json | Registers new UserPromptSubmit and Stop hooks for per-response deslop nudging. |
| .claude/hooks/deslop-if-changes.sh | New Stop hook that diffs the response-start snapshot vs current tree and blocks stop (exit 2) on src/ TS/TSX/SCSS changes. |
| .claude/hooks/deslop-capture-response-base.sh | New UserPromptSubmit hook that captures a response-start git snapshot and resets the loop-guard marker. |
| @@ -0,0 +1,53 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Good catch, fixed in cb5a70b — both hooks now use #!/usr/bin/env bash, matching check-pr-template.sh.
| SESSION_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin).get("session_id",""))' 2>/dev/null || echo "") | ||
| [ -z "$SESSION_ID" ] && exit 0 | ||
|
|
||
| MARKER="/tmp/claude-deslop-ran-${SESSION_ID}" | ||
| BASE_FILE="/tmp/claude-response-base-${SESSION_ID}" | ||
|
|
There was a problem hiding this comment.
Not taking this one, though happy to be argued round.
session_id comes from the Claude Code hook payload — a harness-generated UUID (5f45fcfa-e9c1-4cf9-a067-a4faa75cd1a5), not user or network input. I tested the hostile cases rather than assuming:
session_id=../../etc/pwned -> exit 1, "…/tmp/claude-response-base-../../etc/pwned: No such file or directory"
session_id="a b/c" -> exit 1, same
session_id="" -> exit 0 (already guarded by the [ -z ] check)
ls /etc/pwned -> does not exist
Traversal cannot land: claude-response-base-.. is read as a missing directory component, so the redirect fails. Worst realistic case is a loud exit-1 hook error, not a silent write outside /tmp — and empty is already handled.
The specific reason I am declining rather than adding a one-line tr -cd: this PR ships the deslop skill, whose first two criteria are "extra comments that are unnecessary" and "defensive checks abnormal for trusted code paths". A sanitiser on a harness-generated UUID is exactly the second one. It would be odd to add the slop in the same commit that adds the tool for removing it.
If session IDs ever stop being UUIDs, the failure is immediate and loud, which is the behaviour I would want anyway.
| CHANGED=$(git diff --name-only "$BASE" -- src 2>/dev/null \ | ||
| | grep -E '\.(ts|tsx|scss)$' | head -1 || true) |
There was a problem hiding this comment.
This was a real bug and the most valuable comment here — fixed in cb5a70b.
It was worse than "until it's git add'd": untracked files are invisible at both ends, since git stash create also snapshots only tracked content. So scaffolding a new component exited 0 with no nudge at all. For this repo that is the worst possible blind spot — the documented workflow in CLAUDE.md is literally "create a new component", i.e. all-new files, which is exactly when fresh AI-written code most needs deslopping.
Reproduced before the fix:
new untracked src/components/FakeNew/FakeNew.tsx
git diff --name-only $BASE -- src -> (empty)
git ls-files --others --exclude-standard -> src/components/FakeNew/FakeNew.tsx
Stop hook exit=0 <- missed
after git add -N:
Stop hook exit=2 <- fired
Now unions the two, so .tsx/.scss in a brand-new folder exit 2, an untracked non-src/ file still exits 0, and all seven prior scenarios are unchanged. --exclude-standard keeps .gitignored paths out.
| @@ -0,0 +1,32 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Fixed in cb5a70b (same change as the sibling hook).
| SESSION_ID=$(python3 -c 'import json,sys; print(json.load(sys.stdin).get("session_id",""))' 2>/dev/null || echo "") | ||
| [ -z "$SESSION_ID" ] && exit 0 |
There was a problem hiding this comment.
Not taking this one, though happy to be argued round.
session_id comes from the Claude Code hook payload — a harness-generated UUID (5f45fcfa-e9c1-4cf9-a067-a4faa75cd1a5), not user or network input. I tested the hostile cases rather than assuming:
session_id=../../etc/pwned -> exit 1, "…/tmp/claude-response-base-../../etc/pwned: No such file or directory"
session_id="a b/c" -> exit 1, same
session_id="" -> exit 0 (already guarded by the [ -z ] check)
ls /etc/pwned -> does not exist
Traversal cannot land: claude-response-base-.. is read as a missing directory component, so the redirect fails. Worst realistic case is a loud exit-1 hook error, not a silent write outside /tmp — and empty is already handled.
The specific reason I am declining rather than adding a one-line tr -cd: this PR ships the deslop skill, whose first two criteria are "extra comments that are unnecessary" and "defensive checks abnormal for trusted code paths". A sanitiser on a harness-generated UUID is exactly the second one. It would be odd to add the slop in the same commit that adds the tool for removing it.
If session IDs ever stop being UUIDs, the failure is immediate and loud, which is the behaviour I would want anyway.
| @@ -22,7 +21,7 @@ jobs: | |||
| - name: Checkout base branch (for PR template) | |||
| uses: actions/checkout@v6 | |||
There was a problem hiding this comment.
Checked this one and the premise does not hold, so leaving @v6.
v6 is a real, current release — v6.1.0 and v7.0.1 were both published 2026-07-20 — so there is no "not available" risk.
There is also no repo standard to align to. Actual usage:
auto-merge.yml actions/checkout@v2
build.yml actions/checkout@v3
lint.yml actions/checkout@v3
publish.yml actions/checkout@v3
test.yml actions/checkout@v3
chromatic.yml actions/checkout@v4 <- the one cited
this workflow actions/checkout@v6
v4 is a minority (1 of 6); the plurality is v3. So the change would be a two-major downgrade to a non-standard version, on a pull_request_target workflow where checkout is the security-sensitive step. That is the wrong direction.
Also worth noting this line is pre-existing — it came with the dormant workflow and is not part of this change. Bringing the other six up to a consistent modern major is a reasonable follow-up, but it is its own PR, not this one.
Nudge Claude to run the `deslop` skill before stopping, but only when the current response actually changed src/ code, so read-only sessions don't pay for it. A UserPromptSubmit hook snapshots the tree with `git stash create`; the Stop hook diffs that snapshot against the working tree rather than the whole branch delta vs main. A per-session marker breaks the Stop -> block -> deslop -> Stop loop and is reset on the next prompt. Ported from phillips-public-remix#2438, with the pathspec widened: git treats `src/**/*.ts` as needing an intermediate directory, so it silently missed top-level entrypoints like src/index.ts. Filtering extensions with grep catches those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pr-template-and-prefix.yaml has never run in this repo. It was copied from phillips-public-remix with its trigger filtered on `next` and `hotfix/**`, but main is this repo's default and only release branch (see .releaserc), so the PR template body check has been silently dormant. Pointing the trigger at main exposed two problems with its title rules: - The prefix rule was inverted here. It rejected `fix(scope): ` on the release branch to force a minor bump, which is right for phillips-public-remix but wrong for a library that ships patches: CHANGELOG.md has 165 patch versions and 175 Bug Fixes sections. Its other half guarded `hotfix/**` branches, which have never existed here. - The Jira-ticket rule duplicated validate-pr.yaml, which already enforces the conventional-commit type, a required scope, and a leading ticket via amannn/action-semantic-pull-request — and does actually run. So this workflow now checks the PR body only, and says so, leaving titles to validate-pr.yaml. Note the trigger fix cannot take effect until this merges: pull_request_target always evaluates the workflow from the base branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1822094 to
2206f5f
Compare
Addresses PR review on #911. The Stop hook missed brand-new components entirely. `git stash create` snapshots only tracked content and `git diff <base>` compares only tracked files, so an untracked .tsx/.scss was invisible at both ends: scaffolding a component exited 0 with no nudge, and only started firing once the files were git add'd. That is the case where fresh AI-written code most needs deslopping, and this repo's documented workflow is precisely "create a new component". Union the diff with `git ls-files --others --exclude-standard` so untracked files count. Verified: new untracked .tsx and .scss now exit 2, an untracked non-src file still exits 0, and all seven prior scenarios are unchanged. Also switch both hooks to `#!/usr/bin/env bash`, matching check-pr-template.sh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jira ticket
L3-0000 (Claude Code hooks / CI tooling — no Jira ticket)
Screenshots
N/A — touches only
.claude/hooks/,.claude/settings.json, and a GitHub Actions workflow. No UI change, no shipped code.Figma link
N/A — no design.
Summary
Two related tooling changes, the first ported from phillips-public-remix#2438:
Per-response deslop nudge. Adds a paired
UserPromptSubmit+Stophook so Claude is nudged to run thedeslopskill before stopping — but only when the current response actually changedsrc/code. Read-only or docs-only sessions pay nothing.Revive the dormant PR body check. While porting, I found
.github/workflows/pr-template-and-prefix.yamlhas never run in this repo: it was copied from phillips-public-remix with a trigger filtered onnext/hotfix/**, but Seldon's default and only release branch ismain. Pointing it atmainthen exposed that both of its title rules are wrong here, so it now checks the PR body only.Change List (describe the changes made to the files)
.claude/hooks/deslop-capture-response-base.sh(UserPromptSubmit) — snapshots the tree withgit stash create, which records HEAD + index + working tree as a commit object without touching the working tree. Falls back toHEADon a clean tree. Also clears the Stop-hook loop guard so each response is eligible for one nudge..claude/hooks/deslop-if-changes.sh(Stop) — diffs that snapshot against the current tree unioned with untracked files, scoped tosrc/.ts/.tsx/.scss. On a hit, exits 2 with a message asking Claude to invoke thedeslopskill; otherwise exits 0. A per-session marker breaks the Stop → block → deslop → Stop loop..claude/settings.json— registers both hooks, using the"$CLAUDE_PROJECT_DIR"convention already used by thecheck-pr-template.shhook..github/workflows/pr-template-and-prefix.yaml— triggernext/hotfix/**→main; removed both title rules (see below); renamed workflow, job, and step to reflect that it checks the body only. The body-template logic itself is unchanged.Why both title rules were removed rather than fixed:
fix(scope):on the release branch to force a minor bump — correct for phillips-public-remix, wrong for a library that ships patches.CHANGELOG.mdhas 165 patch versions and 175 "Bug Fixes" sections, and plenty of mergedfix(...)PRs targetmain. Its other half requiredfix(onhotfix/**branches, which have never existed here. Keeping it would have blocked normal fix PRs the moment the trigger started working.validate-pr.yaml, which already enforces the conventional-commit type,requireScope: true, and a leading Jira ticket viaamannn/action-semantic-pull-request— and unlike this workflow, actually runs. Two title checks with different regexes will drift, so titles now have exactly one owner.Two deliberate deviations from the upstream hook PR:
app/→src/, and the diff-baseline comments referencemainrather thannext.grep, not a git pathspec. Git treatssrc/**/*.tsas requiring an intermediate directory, so it silently misses top-level entrypoints likesrc/index.ts. Verified directly:Acceptance Test (how to verify the PR)
Hooks — all ten cases run against the real scripts. Exit code is the whole contract:
0= allow stop,2= block and feed stderr back to Claude.-pmode / session resumed from before install)src/changes this responsesrc/**/*.tsxchangedsrc/index.tschangedsrc/change only (README.md)src/**/_*.scsschanged.tsx(scaffolded component).scsssrc/Workflow — the
run:block was extracted and executed against real bodies:**Regression Test**pull_request_targetalways evaluates the workflow file from the base branch, somain's copy — still filtered tonext— is what runs. The fix only takes effect on PRs opened after this merges. To verify post-merge: open any PR tomainwith a section deleted from the body and confirm "Validate PR Body Template" fails.Regression Test
-p/ noUserPromptSubmit: Stop hook exits 0 gracefully (no base file) — verified.src/paths (README.md,.claude/) still stop without the nudge — verified.fix(...)PRs intomainare not blocked — this is the regression the removed prefix rule would have introduced once the trigger started working.validate-pr.yamlstill owns title validation and is untouched; it passed on this PR, confirmingchore(ai): L3-0000 ...satisfies type + scope + leading ticket.grepfor bold headers,mapfile, per-section error) is byte-for-byte unchanged, so behavior for a compliant body is identical.Evidence of testing
bash -nclean on both hook scripts;.claude/settings.jsonparses as JSON.prettier --checkclean on.claude/settings.jsonand the workflow YAML. The two.shfiles have no prettier parser (expected).set -o pipefail,grepreturning 1 on no-match aborted the script with exit 1 — a hook error on every response that didn't touchsrc/. Fixed with|| true, with a comment noting it's load-bearing.mapfileshim (macOS ships bash 3.2;mapfileis bash 4+, CI runners have bash 5) took the array name from$3instead of$2, making every case report failure underset -u. The workflow itself was fine.check-pr-template.sh), which is exactly the "inconsistent with local style" criterion these hooks exist to catch. Trimmed to 32–35% (7 and 10 comment lines), keeping only non-obvious why —git stash createsemantics, thepipefailfootgun, the pathspec gap. All scenarios re-run green afterwards to confirm behavior was unchanged.Review feedback addressed
Copilot raised 6 comments (4 distinct issues). Two were valid and are fixed in
cb5a70b; two did not survive checking and are declined with evidence on their threads.git stash createsnapshots only tracked content andgit diff <base>compares only tracked files, so an untracked.tsx/.scsswas invisible at both ends: scaffolding a new component exited 0 with no nudge. Worst possible blind spot here, since this repo's documented workflow is creating new components. Now unionsgit ls-files --others --exclude-standard.#!/usr/bin/env bash, matchingcheck-pr-template.sh.session_id— declined. It is a harness-generated UUID on a trusted path. Traversal is impossible in practice (claude-response-base-..is a missing directory component, so the write fails with exit 1); empty is already guarded. Adding a sanitiser would be the "defensive checks abnormal for trusted code paths" slop that this PR's own skill exists to remove.actions/checkout@v6→v4— declined.v6.1.0andv7.0.1both exist, and the repo has no standard to align to (v2 ×1, v3 ×4, v4 ×1, v6 ×1), sov4would be a two-major downgrade to a minority version on the one workflow where checkout is security-sensitive. The line is also pre-existing. Modernising the other six is a sensible follow-up PR.Things to look for during review
chore, so semantic-release cuts no version — intended, since nothing indist/changes.validate-pr.yamlalready covers titles properly and the prefix rule is actively harmful here, but if you'd rather keep a prefix rule onmain, say so and I'll reinstate a corrected version.-and-prefix, which is now inaccurate. Left as-is to keep the diff readable as a fix rather than a delete + add; happy to rename topr-body-template.yamlif preferred./tmp, keyed by session ID (as upstream). Fine on dev machines; flagging in case anyone objects.--no-verify: huskypre-commitrunsnpm run lint && npm run formatandpre-pushrunsnpm run coverage && npm run test-storybook, and this worktree has nonode_modules. No.ts/.tsx/.scss/.mdfile is touched, so those gates cover nothing in this diff — and CI'slint/test/buildjobs run on the PR regardless.phillipsclass prefix variable,data-testid, jsdoc props, translatable strings, unit-test coverage, accessibility,Playgroundstory,index.tsexport,componentStyles.scssimport — no components or shipped source touched.🤖 Generated with Claude Code