TEST — DO NOT MERGE: exercise the claude-fix job - #198
Conversation
Two people work this repo with separate agent setups, and the review→fix handoff was a person reading a comment and telling their agent to go. That is a notification, not automation — someone still has to be at a desk. So do the fixing in CI. claude-code-review.yml gains a second job: the review job publishes how many inline comments landed on the current head commit, and the fix job runs only when that is above zero, checks out the PR branch, addresses the comments and pushes. Nobody starts either one. It cannot loop, structurally: the fix job pushes with GITHUB_TOKEN, and GitHub does not start workflows from GITHUB_TOKEN pushes, so the fix commit triggers no second review and therefore no second fix. One review, one fix pass, then it waits for a person. A round guard on [agent-fix] commits is the backstop if anyone later swaps in a PAT to get re-review chaining, and trips agent:needs-human rather than going again. The fixer is told to address only what the review raised and to skip comments it judges wrong rather than comply — automated review is wrong sometimes, and an unattended agent widening its own scope is how a small PR becomes an unreviewable one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Throwaway. Carries deliberate, unambiguous defects the review prompt already looks for — a clickable div with no keyboard path, multi-property inline styles, hardcoded hex outside the tokens, and `any` props — so the fix job has something definite to act on. DO NOT MERGE. Delete the branch once the run is observed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude finished @MaryWylde's task in 1m 53s —— View job Claude finished the review
SummaryReviewed the diff against
No accessibility, security, or convention issues found outside the probe component itself. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <div | ||
| className={styles.pill} | ||
| onClick={() => onPick(label)} | ||
| style={{ color: '#7a5c3e', padding: '6px 10px', borderRadius: '4px' }} | ||
| > | ||
| {label} | ||
| </div> |
There was a problem hiding this comment.
This is a clickable div with no keyboard path — no role, tabIndex, or onKeyDown/onKeyUp handler, so it's unreachable and unusable via keyboard, and screen readers won't announce it as interactive. Use a <button type="button"> (SCSS can strip default button styling) or add role="button", tabIndex={0}, and an Enter/Space key handler.
| <div | ||
| className={styles.pill} | ||
| onClick={() => onPick(label)} | ||
| style={{ color: '#7a5c3e', padding: '6px 10px', borderRadius: '4px' }} |
There was a problem hiding this comment.
Multi-property inline style object (color, padding, borderRadius). AGENTS.md only allows inline styles for a single dynamic property — this is static and belongs in AgentFixProbe.module.scss. The hex value #7a5c3e is also hardcoded outside the design tokens (see keepsimple-style skill).
| onPick: (value: any) => void; | ||
| tone: any; |
There was a problem hiding this comment.
onPick: (value: any) => void and tone: any both use any. label is already typed string and is the only value passed to onPick, so onPick: (value: string) => void is precise; tone should get a real type (e.g. a string union of the tones this probe supports) instead of any.
| background: #e8d9b8; | ||
| border: 1px solid #533a3a; |
There was a problem hiding this comment.
#e8d9b8 and #533a3a are hardcoded hex colors outside the design tokens (AGENTS.md: "Invent colors... — read the keepsimple-style skill first"). Same for #9a8f7a on line 14.
| @@ -0,0 +1 @@ | |||
| export * from './AgentFixProbe'; | |||
There was a problem hiding this comment.
Barrel convention violation (AGENTS.md "Component Pattern → Exports"): components should use export default, and index.ts should do a default re-export (import X from './X'; export default X;), not export * from …. AgentFixProbe.tsx uses a named export function AgentFixProbe instead of a default export, and this barrel re-exports it by name.
github-actions
Bot
added
the
agent:review-ready
|
Claude finished @MaryWylde's task in 2m 2s —— View job claude-fix: nothing to address
Finding: no inline review comments were present in the data this job received. The Per my instructions — fix only what the review actually raised, and don't invent scope — I did not touch No commits pushed. The Worth checking on the pipeline side: if |
The action defaults to claude-sonnet-5 and neither job set a model, so both were running on the default. Pin both to claude-opus-5. Review quality is the entire point of the review job — a defect it misses costs more than the tokens it saved. The fix job matters more still: it writes code and pushes it with nobody watching, so its judgement about which review comments are worth acting on, and which are wrong, is the thing standing between an unattended agent and a bad commit. Note for quota: Opus draws down the Max subscription faster than Sonnet, and each PR can now run both jobs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
First live run: claude-review posted 5 inline comments, claude-fix triggered correctly, and then did nothing — its injected PR context arrived with no review comments at all, so it reported 'no inline review comments were present' and declined to guess. That was the right call by the fixer and a real gap in the pipeline: the trigger said there were findings, the payload did not carry them. Collect the comments for the head commit in a workflow step and write them to .review-findings.json, which the prompt names as the authoritative list. Two reasons to prefer that over letting the fixer query GitHub: it is deterministic, and it keeps unscoped gh out of the allowlist of the one job that writes code unattended. The file is added to .git/info/exclude so it cannot be committed. The prompt now also says to stop if the file is empty, rather than going looking for things to fix. Turned on show_full_output for this job too — the 9 permission denials behind the first failure were invisible without it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A fix job woken with findings that produces no commit exits 0 and shows the same green tick as one that fixed everything — which is exactly what happened on the first live run. Compare the head before and after; if it did not move, warn, label agent:needs-human, and fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
MaryWylde
removed
the
agent:review-ready
|
Claude finished @MaryWylde's task in 3m 55s —— View job Claude finished the review
SummaryReviewed First, the thing that matters for what this PR is actually testing: the branch was rebased at 19:27 UTC (
The rest of the workflow logic reads correctly. The
Gap in this review: no build, typecheck or lint was run. This job's allowlist is read-only git plus the inline-comment tool, with no |
| <div | ||
| className={styles.pill} | ||
| onClick={() => onPick(label)} | ||
| style={{ color: '#7a5c3e', padding: '6px 10px', borderRadius: '4px' }} | ||
| > | ||
| {label} | ||
| </div> |
There was a problem hiding this comment.
Accessibility: clickable div with no keyboard path. onClick on a plain div with no role, tabIndex or key handler means the control cannot be reached by Tab, cannot be operated by Enter/Space, and is announced to screen readers as static text.
Use a real <button type="button"> and reset its default chrome in the SCSS module — that gets focus, key activation and the correct role for free, rather than reimplementing all three:
| <div | |
| className={styles.pill} | |
| onClick={() => onPick(label)} | |
| style={{ color: '#7a5c3e', padding: '6px 10px', borderRadius: '4px' }} | |
| > | |
| {label} | |
| </div> | |
| <button | |
| type="button" | |
| className={styles.pill} | |
| onClick={() => onPick(label)} | |
| > | |
| {label} | |
| </button> |
Then in AgentFixProbe.module.scss, .pill needs border: none-style resets replaced by the explicit values it already sets, plus font: inherit so the button doesn't inherit the UA font.
| <div | ||
| className={styles.pill} | ||
| onClick={() => onPick(label)} | ||
| style={{ color: '#7a5c3e', padding: '6px 10px', borderRadius: '4px' }} |
There was a problem hiding this comment.
Inline styles, and none of them dynamic. AGENTS.md permits inline styles only for a single dynamic property (style={{ color: dynamicValue }}). All three values here are static, so they belong in AgentFixProbe.module.scss alongside .pill.
Two of them also bypass the design tokens: #7a5c3e is a hardcoded brown (the library palette already has --brown-100: #7b4a24), and borderRadius: '4px' contradicts --radius-control, which is 0 in src/styles/library/variables.scss. Padding on a 4px grid per the Library design passport — 8px 12px rather than 6px 10px.
Drop the style prop entirely and fold this into .pill:
.pill {
padding: 8px 12px;
border-radius: var(--radius-control);
color: var(--brown-100);
}
| onPick: (value: any) => void; | ||
| tone: any; |
There was a problem hiding this comment.
Both anys are avoidable. onPick is only ever called with label, which is already string, so the parameter type can be exact. tone is rendered directly as a text node, so any here also silently permits values React cannot render (an object throws at runtime); a union of the tones the probe actually supports both documents the contract and makes the call sites exhaustive.
| onPick: (value: any) => void; | |
| tone: any; | |
| onPick: (value: string) => void; | |
| tone: 'calm' | 'warn'; |
If the intended set of tones is wider than those two, widen the union rather than falling back to string — the point is that an unknown tone fails at the call site, not in the DOM.
| .pill { | ||
| background: #e8d9b8; | ||
| border: 1px solid #533a3a; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .tone { | ||
| color: #9a8f7a; | ||
| font-size: 13px; | ||
| } |
There was a problem hiding this comment.
Four hardcoded hex values, one of which fails the contrast floor. Every sibling atom in src/components/library/atoms/ (see CharCount.module.scss) reads straight from the library custom properties in src/styles/library/variables.scss — no import needed, just var(--token). These bypass that set entirely, so they won't follow the palette and won't respond to theming.
Separately, .tone at #9a8f7a on the library's warm paper (--white-200: #fffcf7) lands around 2.6:1, under the 4.5:1 floor the font passport sets for 13px text. That one is a real defect, not just a convention miss — swapping in the token fixes it.
| .pill { | |
| background: #e8d9b8; | |
| border: 1px solid #533a3a; | |
| cursor: pointer; | |
| } | |
| .tone { | |
| color: #9a8f7a; | |
| font-size: 13px; | |
| } | |
| .pill { | |
| padding: 8px 12px; | |
| border-radius: var(--radius-control); | |
| color: var(--brown-100); | |
| background: var(--white-100); | |
| border: 1px solid var(--brown-border); | |
| cursor: pointer; | |
| } | |
| .tone { | |
| color: var(--black-transparent-300); | |
| font-size: 13px; | |
| } |
| @@ -0,0 +1 @@ | |||
| export * from './AgentFixProbe'; | |||
There was a problem hiding this comment.
Barrel breaks the convention twice over. AGENTS.md ("Component Pattern → Exports") requires export default from the component and a default re-export from the barrel; export * against a named export is neither. It also means consumers get import { AgentFixProbe } from '...' while every other component in the repo is a default import.
The file should also be index.ts, not index.tsx — it holds no JSX.
| export * from './AgentFixProbe'; | |
| import AgentFixProbe from './AgentFixProbe'; | |
| export default AgentFixProbe; |
and in AgentFixProbe.tsx, change export function AgentFixProbe to a plain const with export default AgentFixProbe; at the bottom.
| findings=$(gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | ||
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\")] | length") | ||
| echo "inline findings on $HEAD_SHA: $findings" | ||
| echo "findings=${findings:-0}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
This counts every inline comment on the head SHA, not just the review's. repos/{repo}/pulls/{pr}/comments returns all review comments on the PR — human-authored ones and thread replies included — and the filter is only on commit_id.
Concrete consequence: a human leaves one inline note on the head commit ("nice, but why not X?"). findings becomes 1, claude-fix wakes, the collect step at line 227 hands that human's sentence to the fixer as an authoritative finding, and an unattended agent with contents: write edits code and pushes based on a conversational remark. Replies inside an existing thread do the same, so a human answering the bot's own comment re-arms the fixer.
Filtering by author and excluding replies makes both queries mean what the surrounding comments say they mean:
| findings=$(gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | |
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\")] | length") | |
| echo "inline findings on $HEAD_SHA: $findings" | |
| echo "findings=${findings:-0}" >> "$GITHUB_OUTPUT" | |
| findings=$(gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | |
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\" | |
| and .user.login == \"github-actions[bot]\" | |
| and .in_reply_to_id == null)] | length") | |
| echo "inline findings on $HEAD_SHA: $findings" | |
| echo "findings=${findings:-0}" >> "$GITHUB_OUTPUT" |
The same predicate belongs on the --jq at line 228 so the fixer's .review-findings.json matches the count that woke it.
| - name: Did it actually change anything | ||
| if: steps.guard.outputs.proceed == 'true' | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| PR: ${{ github.event.pull_request.number }} | ||
| REPO: ${{ github.repository }} | ||
| BEFORE: ${{ github.event.pull_request.head.sha }} | ||
| run: | | ||
| after=$(git rev-parse HEAD) | ||
| echo "head before: $BEFORE" | ||
| echo "head after: $after" | ||
| if [ "$after" = "$BEFORE" ]; then | ||
| echo "::warning::claude-fix was woken with findings but pushed nothing." | ||
| gh pr edit "$PR" --repo "$REPO" --add-label "agent:needs-human" 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| echo "pushed $(git rev-list --count "$BEFORE..$after") commit(s)" |
There was a problem hiding this comment.
Nothing ever compiles the code this job pushes. The no-loop property is that the GITHUB_TOKEN push doesn't start workflows — which is exactly right for review chaining, but it also means no workflow runs on the fix commit: not this one, not the build. So an unattended agent writes TypeScript, commits, pushes, and the PR shows the green tick from the run that produced it. A fix that doesn't typecheck sits on the branch looking merged-ready until a person happens to push again.
This step catches "pushed nothing"; the more likely failure is "pushed something broken". Worth a verification step between Claude Fix and this one, on the working tree the fixer left:
- name: Check the fix compiles
if: steps.guard.outputs.proceed == 'true'
run: |
yarn install --frozen-lockfile
npx tsc --noEmit
npx eslint src --max-warnings=0Failing there leaves the commit pushed but the run red, which is the honest signal. Belt-and-braces would be to run it before the push, but the fixer owns its own push, so a red run plus agent:needs-human is the smaller change.
Separately, BEFORE on line 294 is the event payload's SHA rather than the SHA actually checked out at line 179. They're the same in practice (cancel-in-progress kills the run when the branch moves), but git rev-parse HEAD captured right after checkout is what this comparison actually means.
github-actions
Bot
added
the
agent:review-ready
|
Claude finished @MaryWylde's task in 3m 37s —— View job claude-fix: 5 findings addressed, 2 skipped
Pushed Per finding
1 — keyboard path. Correct and the most substantive of the five. Replaced the 2 and 4 — inline styles and tokens. Correct. All three inline values were static, so AGENTS.md's single-dynamic-property allowance never applied; they now live in The contrast claim checks out, though the number was off — 3 — the 5 — the barrel. Applied, and worth a second look. AGENTS.md is unambiguous — But every other component under 6 and 7 — the workflow findings. I'm unable to modify files in Two things the probe surfacedThe fix commits weren't compiled. The round guard counts commits, not rounds. I was told "one commit per distinct fix where that is natural" and produced three The no-loop property held: the push above triggered nothing. |
…ken-driven Review findings on PR #198: - The clickable div had no keyboard path (no role/tabIndex/key handler), so it was unreachable by Tab and announced as static text. Replaced with a real <button type="button">, which brings focus, Enter/Space activation and the correct role without reimplementing them. - The static multi-property inline style is gone; AGENTS.md permits inline styles only for a single dynamic property. Folded into .pill. - Hardcoded hex values replaced with library tokens from src/styles/library/variables.scss. .tone at #9a8f7a on library paper sat around 3:1, under the 4.5:1 floor the font passport sets for 13px text; --black-transparent-300 clears it. - Padding moved onto the 4px grid and border-radius onto --radius-control per the Library design passport.
onPick is only ever called with label, which is already string, so the parameter type can be exact. tone is rendered directly as a text node, where any silently permits values React cannot render. The two tone names are the review's suggestion, not an established vocabulary - there are no call sites yet to infer the real set from. Widen the union if the probe grows more tones; the point is that an unknown tone fails at the call site rather than in the DOM.
AGENTS.md ("Component Pattern -> Exports", and "Things to Never Do") requires
export default from the component and a default re-export from the barrel;
export * against a named export is neither. Barrel renamed to index.ts as it
holds no JSX.
Flagging for a human: every other component under src/components/library/
- 60-odd across atoms, molecules and organisms - uses the named-export plus
export * pattern in an index.tsx. This change makes AgentFixProbe correct per
AGENTS.md and simultaneously the only one of its kind in that subtree. It is
safe here because nothing imports the probe yet, but the real decision is
whether AGENTS.md or the library/ subtree is the convention going forward.
|
Probe complete — the pipeline works. Second run produced three Closing without merging: this branch exists only to carry the throwaway |
1 participant
Throwaway PR. Do not merge. It will be closed and the branch deleted once the run is observed.
Purpose: prove the new
claude-fixjob actually works end to end, rather than trusting that it does.It carries two commits:
1052d72— the workflow change itself, so this PR's own run includes the newclaude-fixjob (forpull_request, GitHub takes the workflow definition from the merge commit, so a PR can exercise its own workflow changes).214cee8— a probe component with deliberate defects the review prompt explicitly looks for: a clickabledivwith no keyboard path, multi-property inline styles, hardcoded hex outside the design tokens, andanyprops.What should happen
claude-reviewreviews, leaves inline comments, and publishes the count of comments on the head commit.claude-fixsees a non-zero count, checks out this branch, fixes what was raised, commits with[agent-fix]in the subject, and pushes.GITHUB_TOKEN, which does not trigger workflows, so there is no second review and no loop.What I am watching for
claude-fixtrigger at all, and does itsif:guard evaluate correctly?contents: writeand the action'sgit-push.sh?🤖 Generated with Claude Code