fastapi-address-review · diff
git:20260918.4ac7924 to git:20260922.549c04f
29 added, 101 removed. Audit A to A.
---
name: fastapi-address-review
- description: Use when a PR has review feedback and the user wants it addressed — pull the review comments, triage each one, apply the changes it warrants, and draft replies. Closes the loop between a review and the follow-up commit; it does not re-review the code from scratch. Also known as `klaussy-address-review`.
+ description: Use when a PR has review feedback and the user wants it addressed — pull the review comments, triage each one, apply the changes it warrants, then commit, push, and post a humanized reply per comment. Closes the loop between a review and the follow-up commit; it does not re-review the code from scratch. Also known as `klaussy-address-review`.
---
Address the review feedback on the current PR. Every comment gets a response: a change, or a reasoned reply explaining why not. Don't silently skip any.
## Phase 1: Gather the feedback
- 1. **Get the review comments** for the current branch's request, using the adapter below. Pull the inline (line-level) comments as well as the summary review bodies; the inline ones are where the substance usually is. If there's no request yet, no CLI, or no credentials, ask the user to paste the feedback and carry on from there.
+ 1. **Get the review comments** for the current branch's request, using the adapter below. Pull the inline (line-level) comments, the summary review bodies, and the general conversation comments; the inline ones are where the substance usually is, but a request made in the conversation is just as binding. Fetch every page: a partial read drops comments without any error. If there's no request yet, no CLI, or no credentials, ask the user to paste the feedback and carry on from there.
2. **Read CLAUDE.md** and any `.claude/rules/*.md` covering the touched files — a fix must still satisfy the repo's conventions.
3. **Build the change list.** For each comment, capture: the file/line, what's asked, and the reviewer's intent (not just the literal words). Group comments that touch the same code so you fix each spot once.
### Forge commands (GitHub)
`origin` points at GitHub, so the `gh` CLI is the adapter. Confirm a flag with `gh <command> --help` before running one you haven't used in this repo; CLI interfaces drift between versions.
| Need | Command |
| :--- | :--- |
| Read a ticket | `gh issue view <n> --comments` |
| Open a request | `gh pr create --base <branch> --title <title> --body-file <file>` |
| Request status | `gh pr view <n> --json state,mergeable,reviewDecision,baseRefName` |
| CI status | `gh pr checks <n>`, then `gh run view <run-id> --log-failed` on a failure |
- | Read review comments | `gh api repos/{owner}/{repo}/pulls/<n>/comments` |
- | Reply in a thread | `gh api --method POST repos/{owner}/{repo}/pulls/<n>/comments/<comment-id>/replies -f body=<text>` |
- | Resolve a thread | two steps, see below — REST can't do it |
| Retarget a request | `gh pr edit <n> --base <branch>` |
`{owner}/{repo}` are placeholders `gh` fills from the current repo, leave them literal.
+ #### Review feedback (GitHub)
+
+ | Need | Command |
+ | :--- | :--- |
+ | Read inline review comments | `gh api --paginate repos/{owner}/{repo}/pulls/<n>/comments` |
+ | Read review summaries | `gh api --paginate repos/{owner}/{repo}/pulls/<n>/reviews` |
+ | Read conversation comments | `gh api --paginate repos/{owner}/{repo}/issues/<n>/comments` |
+ | Reply in a thread | `gh api --method POST repos/{owner}/{repo}/pulls/<n>/comments/<comment-id>/replies -f body=<text>` |
+ | Answer a conversation comment | `gh pr comment <n> --body-file <file>` (these aren't threaded, so quote or link the comment you're answering) |
+ | Resolve a thread | two steps, see below — REST can't do it |
+
+ **Feedback lives in three places, and each list is paged.** Inline comments, review summary bodies, and comments on the conversation tab are separate endpoints; read all three, since a reviewer who writes "please also rename X" in the conversation expects it handled like a line comment. Without `--paginate` each call returns only the first 30, so a busy request silently loses the rest. Pages print as separate JSON arrays; add `--slurp` when piping to `jq` and you want one.
+
**Resolving needs GraphQL, and the id it wants is not the comment id.** The REST comment objects don't carry it, so read the thread ids first, then resolve one:
```
- gh api graphql -f query='{ repository(owner: "<owner>", name: "<repo>") {
- pullRequest(number: <n>) { reviewThreads(first: 50) { nodes {
- id isResolved comments(first: 1) { nodes { databaseId body } } } } } } }'
+ gh api graphql --paginate -f query='query($endCursor: String) {
+ repository(owner: "<owner>", name: "<repo>") { pullRequest(number: <n>) {
+ reviewThreads(first: 100, after: $endCursor) {
+ pageInfo { hasNextPage endCursor }
+ nodes { id isResolved comments(first: 1) { nodes { databaseId body } } } } } } }'
gh api graphql -f query='mutation($id: ID!) {
resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }' -F id=<thread-node-id>
```
Match a thread to the comment you replied to through `comments.nodes[].databaseId`, which is the REST comment id. `threadId` is the only required input.
A reply must name the thread it answers. The `replies` endpoint above takes only `body`; the alternative is `POST .../pulls/<n>/comments` with `-F in_reply_to=<comment-id>` (an integer, hence `-F`). Posting to `comments` without `in_reply_to` opens a new top-level review comment rather than replying.
- **GitHub has native stacks**, driven by the `gh-stack` extension. `gh extension list` says whether it's installed. If it isn't, **offer to install it** — `gh extension install github/gh-stack`, one command, no repo changes — and say what it buys before asking: a stack map and layer navigation on every request page, plus cascading rebase when the base moves. Ask rather than installing unprompted, since it touches the user's `gh` setup and not this repo, but do ask; silently settling for bare chained bases hands back a worse result than the one command would have. Declining is a fine answer and the fallback below still works.
-
- The extension is in public preview, so check `gh stack <command> --help` before relying on a flag.
-
- | Need | Command |
- | :--- | :--- |
- | Link requests that already exist into a stack | `gh stack link --base <branch> <branch-or-pr> <branch-or-pr> ...` |
- | Track a carved chain locally | `gh stack init --base <branch> <branch> ...` |
- | Push the tracked chain and open or update its requests | `gh stack submit` |
- | See the stack | `gh stack view` |
- | Cascading rebase after the base moved | `gh stack rebase` |
- | Fetch, rebase, push, and sync in one pass | `gh stack sync` |
-
- Arguments run bottom-up, nearest the base first. Two constraints decide whether a stack is available at all: **every branch must live in this repo** (cross-fork stacks aren't supported), and the extension has to be installed.
-
- `link` and `init` are two different entry points and the difference shows up later. `link` stacks requests that already exist and leaves nothing behind locally, so a later `gh stack rebase` needs `gh stack checkout <stack-number>` first to pick the stack back up. `init` registers the branches locally up front and `submit` then opens the requests itself, which means the bodies are its own — write them with `gh pr edit <n> --body-file` afterwards if they have to say something specific.
-
- Without it, chained `--base` targets still give reviewers a per-layer diff, and GitHub often offers to convert an eligible chain into a stack — a banner on the request, or "Add to stack" behind the stack icon. Say which route you took.
-
-
## Phase 2: Triage each comment
Sort every comment into one of:
- **Accept & fix** — a real issue or a clear improvement. Most comments.
- **Accept with a different fix** — the concern is valid but the reviewer's suggested change isn't the best one; do the better fix and say why in the reply.
- **Discuss / decline** — you believe the current code is correct, or the change is out of scope. This is legitimate, but it requires a specific, respectful reason, not a dismissal. When unsure whether to decline, ask the user rather than deciding unilaterally.
State the triage before editing, so the user can redirect if they disagree.
## Phase 3: Apply the changes
1. **Make each accepted change** as a minimal, targeted edit — fix what the comment raised, don't refactor around it.
2. **Check for siblings.** If a comment points at a pattern (not just one line), grep for the same pattern elsewhere in the diff and fix those too, unless the reviewer scoped it to the one spot.
3. **Re-verify after editing** — run the tests/lint from CLAUDE.md. A fix that breaks the suite isn't done.
4. **Keep the changes reviewable.** One logical follow-up; don't fold in unrelated work that a re-review would have to untangle.
- ## Phase 4: Reply and hand off
+ ## Phase 4: Reply, post, and hand off
1. **Draft a reply per comment** (or per group): what you changed and where, or — for a decline — the specific reason. One sentence is the target; two is the ceiling. "Fixed, moved the check into `validate_session`." is a complete reply.
- 2. **Summarize** the follow-up for the user: which comments led to changes, which were declined and why, and the suggested commit message (`fix: address review feedback` or per the repo convention). A few lines, not a report; they can read the diff.
- 3. **Do not push, resolve threads, or re-request review** unless the user asks — leave those actions to them.
-
- ### Write like a person, not a chatbot
-
- Whatever you output for a human (review comments, PR text, explanations, replies) must read like a colleague wrote it in a hurry, not like a model composed it. Two failure modes, and you have to beat both: sounding like AI, and saying more than the reader needs. These rules mirror klaussy's deterministic humanizer (klaussy-desktop `humanize-comment.js`):
-
- Before anything else: **no em-dashes or en-dashes** (`—` / `–`) in prose. Use a comma or rewrite the sentence. That one tell gives the game away faster than everything below it combined.
-
- **Voice: say it out loud.** The target is a competent engineer typing this once, in a hurry, who isn't going to read it back. Not a careful writer, not a summary of the facts: a person with an opinion who wants to get on with their day.
-
- - **Write what you'd say standing at their desk.** If you wouldn't say the sentence to a colleague, don't write it. That one test catches most of what follows.
- - **Use contractions.** it's, doesn't, won't, that's, here's. Prose without them reads like a manual.
- - **Verbs, not noun phrases.** "This validates the token", not "this performs validation of the token". "We cache it", not "caching is applied". Turning verbs into nouns is the loudest tell after em-dashes.
- - **Name the thing doing the work.** "The retry loop eats the 429", not "error handling may result in suppression of the status".
- - **Short common words.** *before* not *prior to*, *if* not *in the event that*, *can* not *is able to*, *about* not *regarding*, *but* not *however*, *so* not *thus*, *use* not *utilize*.
- - **Fragments are fine.** "Same bug two lines down." is a complete thought; don't pad it into a sentence.
- - **One idea per sentence.** If a sentence has two clauses joined by a comma and a *which*, it's two sentences. Short sentences are easier to read than clever ones.
- - **One modifier, not three.** Cut the triads ("clear, concise, and maintainable"). Pick the word that carries the point.
- - **Don't announce structure.** No "There are three issues here:", no "Let me walk through this". Say the thing.
- - **Type it once and don't polish it.** The last tell isn't a wrong word, it's evenness: every sentence complete, every paragraph the same shape, every point covered in order. Let it be uneven. A long sentence next to a three-word one. Two points where a tidy version would make four.
- - **Have a stance.** "I'd drop this", "no idea why this is here", "this'll fall over under load". First person and an opinion read as a person; an even, neutral summary reads as generated, however short it is.
- - **Skip the obvious.** A lazy writer leaves out what the reader can already see and doesn't round the thought off. "Tests cover the happy path and the concurrent case" is "tests for both". What it never drops is the thing being talked about: keep the nouns that carry the meaning ("we invalidated the cache on every write", not "we invalidated on every write"). Being lazy costs the reader nothing they needed.
- - **Don't mirror the source.** Same facts, your own shape: merge its paragraphs, reorder them, drop a section that isn't worth its space. Keep every noun that carries meaning while you do it.
-
- **Shape: the smallest thing that carries the point.**
-
- - **Budgets.** A thread reply is one sentence. A single review comment is one to three. An explanation leads with two or three sentences that answer the question, then adds detail only where the reader can't infer it. Over budget means you're saying more than the reader needs, not that you write long.
- - **Unrelated problems are separate comments.** Two findings that happen to sit near each other read better apart. One finding that spans a few files because the fix touches them all is still one comment, don't fracture it. The test is whether the reader would act on them separately.
- - **Lead with the change, not the discovery.** Your first sentence names what to do ("set `soft_time_limit=3600` here"), not what you noticed ("this task inherits the app-wide limits"). The reader stops as soon as they have what they need, so someone who reads one sentence should already be able to act. Why it matters comes second, the mechanism last if it earns a place at all.
- - **Prose by default.** No headings, tables, or bold field labels. Bullets only for a real list of three or more parallel items, never as a wrapper around one paragraph.
- - **Three sentences to a paragraph.** A fourth one means a second paragraph or a second comment. Put a blank line between them, a wall of text is hard to get back into after looking away.
- - **No bookends.** Don't open by restating the request and don't close by summarizing what you just said. Start at the point, stop when it's made.
- - **Don't quote what they're already looking at.** In an inline comment the code is on screen. Point at it, don't paste it back.
- - **No status theater.** Severity labels, confidence scores, checkbox lists, and "Method:" footers only when the output format requires them.
- - **Cut detail, not just words.** The verbose tell isn't long words, it's over-explaining. Drop what the reader can reconstruct from the code, the diff, or the commit: explanatory parentheticals, restated identifiers, and "I did X to do Y" narration of changes the diff already shows. Keep the load-bearing fact, drop what merely supports it. This is the one place humanizing may drop content, never reverse or invent meaning.
- - **Keep the concrete parts.** A suggested diff or code block, a command to run, a `file:line`, a version number, a config key: none of that is reconstructable prose, and cutting it costs the reader a trip back to the code. Trim the sentences around them, keep them.
-
- **Answer what was asked, then stop.** Padding is the tell that survives every style fix, and it takes three shapes. All three are cuts, not rewrites:
-
- - **No closing principle.** Don't end by restating your decision as a general rule ("I'd still reach for an iframe when you want a separate document context for third-party code"). It answers nothing about this change and only validates the view you already gave. Stop at the last concrete point.
- - **No mechanism they didn't ask for.** Explaining how the thing works, in terms only you are holding in your head, reads as padding even to the person who wrote the code. If a paragraph doesn't change what the reader does next, cut it. When they need it, they'll ask.
- - **Grant a point in four words, or not at all.** Where the other person is right about something, say so and move on: "Yes, Shadow DOM wouldn't need the ResizeObserver" beats "the ResizeObserver cost is real and Shadow DOM wouldn't pay it". Dressing agreement up in a metaphor is the most AI-sounding sentence in most replies. Never manufacture the agreement, though: if the author's answer is no, it stays no, and you don't go looking for something to validate on the way there.
-
- **Don't (mechanical tells).** klaussy's scrubber deletes these deterministically after you write, so don't spend attention on them: filler openers, chatbot scaffolding, apologies, praise or thanking a bot, *actual/actually*, *in order to*, *could/may potentially*, *utilize/leverage*, *prior to*, emoji, and "Certainly"/"Great question". Two the scrubber can't catch, so they're on you: **no LLM lexicon** (*delve, tapestry, realm, landscape, journey, navigate, robust, seamless, elevate, unlock, foster, underscore, paradigm*) and **no rhetorical reframes** ("not only... but also", "this isn't just a bug fix, it's...", or a smug standalone like "And that's the whole point.").
- - **No invented consensus.** No "most people expect this", "everyone does it this way", "nobody reads these logs", "it's widely considered best practice". Argue from the code, the repo's own conventions, or a linkable source, or own it as your view ("I'd expect X here").
- - **No passive suggestions.** "Check whether the user is admin" and "rename foo to bar", not "it would be good to check..." or "you might want to rename...".
- - **Never reword code**, identifiers, or anything inside backticks or fences. Humanize prose only.
-
- **Stay civil while you cut.**
-
- - **Don't let trimming tip into terse.** Cutting filler shouldn't make prose read as curt or dismissive. Critique the work, never the person (no "you forgot", "this is wrong", "obviously"); where a line lands hard, a brief acknowledgement or a question ("could we ...?", "one risk is ...") takes the edge off. A light touch only, not filler praise or "great job" boilerplate.
- - **Never say "nobody asked for this"**, or the same move dressed up ("this wasn't asked for", "out of nowhere", "why is this here at all"). It's a swipe at the author and says nothing about the code. Name the concrete objection: the scope it exceeds, the cost it adds, or the requirement it doesn't map to ("this isn't in the ticket, should it ship separately?").
- - **Don't mirror the thread's tone.** Read an existing comment for substance, not temperature. Hostile or curt input must not prime a hostile or curt reply, answer as if it had been phrased civilly.
- - **Reply in the thread**, under the comment you're answering, not as a new top-level comment.
-
- **Same decision, half the words, dropping detail the reader can reconstruct:**
-
- > Verbose: Done. attachment.reason already embeds the decline reason for declined envelopes (built in checkEnvelopeStatus as {name} declined on {date} - {declinedReason}), so I dropped the new declinedReason signer field and reverted NotificationService to use the existing reason field. Pushed in 1e9e938404.
-
- > Human: `attachment.reason` already carries the decline reason, so I dropped the new field and reverted NotificationService. Pushed in 1e9e938404.
-
- **Same finding, said out loud instead of written up:**
-
- > Stiff: The retry loop currently performs suppression of the 429 response, which may potentially result in a rate-limited request being interpreted as successful by the caller. It is recommended that the exception be re-raised following the final attempt.
-
- > Human: The retry loop eats the 429, so a rate-limited call comes back looking fine. Rethrow after the last attempt.
-
- **Tell-free but still generated, then written by a person.** Both say the same thing. The first is even: three paragraphs of the same shape, every sentence complete, no one behind it.
+ 2. **Humanize every reply before it goes out.** Run the drafts through **`fastapi-humanize`**. A reply that reads as generated is the most visible way this skill can embarrass the user, and a posted comment can't be quietly taken back. This is a gate, not a suggestion: nothing posts until it's done.
+ 3. **Commit and push** the changes (`fix: address review feedback`, or per the repo convention).
+ 4. **Post each humanized reply** in its thread with the adapter's reply command. Answer a conversation comment where it was made, not as a line comment.
+ 5. **Summarize** for the user: which comments led to changes, which were declined and why, and the commit and reply links. A few lines, not a report; they can read the diff.
- > Tidy: The caching layer now uses a shared in-memory cache instead of a per-request database query, cutting the load on the primary instance. The cache populates on first access and invalidates when the underlying record changes. This also fixes a subtle race condition where two simultaneous requests could both populate the same entry. Tests cover both the happy path and concurrent access.
+ Leave the threads for the reviewer to resolve, and don't re-request review, unless the user asks for either.
- > Human: Swapped the per-request query for one shared cache, so the primary isn't getting hammered. Fills on first read, drops when the record changes. Also kills a race where two requests could populate the same key, there's a per-key lock now. Tests for both.
+ **Humanize anything a human will read.** Before prose ships — a PR body, a review comment or reply, a commit message, a changelog entry, docs — run it through the `fastapi-humanize` skill and use what comes back. That skill holds the rules; don't keep a second copy of them here.
- **The scrubber is not the humanize pass.** `klaussy humanize` deletes a fixed list of mechanical tells (dashes, filler openers, a few hedges) and changes nothing else. It can't cut a paragraph that shouldn't exist, turn a noun phrase back into a verb, drop the closing principle, or make three sentences one, and that's most of what makes prose read as generated. Anything a human will read gets the `fastapi-humanize` skill: cut, voice, check, then scrub. Running the CLI, or `klaussy humanize --check`, is not that pass and doesn't stand in for it.
+ **The scrubber is not that pass.** `klaussy humanize` deletes a fixed list of mechanical tells (dashes, filler openers, a few hedges) and changes nothing else. It can't cut a paragraph that shouldn't exist, turn a noun phrase back into a verb, drop the closing principle, or make three sentences one, and that's most of what makes prose read as generated. Anything a human will read gets the `fastapi-humanize` skill: cut, voice, check, then scrub. Running the CLI, or `klaussy humanize --check`, is not that pass and doesn't stand in for it.
## Rules
+ - Humanize before posting. Every reply is public and permanent; that pass is what keeps it reading like a person wrote it.
- Respond to every comment — a change or a reason. Silence on a comment reads as ignoring the reviewer.
- Critique the code, never the reviewer; when you decline, give evidence, not a brush-off. Read a blunt comment for its substance, not its tone, and reply civilly regardless.
- Don't over-correct: fix what was raised, not the whole file. Scope creep in a review-response commit makes the re-review harder.
- If two comments conflict, or a comment contradicts CLAUDE.md, surface the conflict to the user instead of silently picking one.
## When NOT to use
- There's no review yet — the user wants their branch reviewed; use the review skill.
- The feedback is a full redesign, not line comments — that's a re-plan; use the plan skill.
- The user only wants the reply text drafted, not the code changed — draft the replies and skip the edit phase.