Skip to content

Commit 6f18cdd

Browse files
committed
fix: Make review questions conditional, not mandatory
Questions section now explicitly says to omit entirely when the change is straightforward. Forcing 2-5 questions on every PR produces the same generic filler the nemawashi approach was meant to eliminate.
1 parent 4eca118 commit 6f18cdd

1 file changed

Lines changed: 15 additions & 14 deletions

File tree

.github/workflows/claude-code-review.yml

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -257,16 +257,18 @@ jobs:
257257
For each changed function, check whether the test file is in the diff. If it is, review whether the test actually verifies the behavior. If not, check if a `*_test.go` file exists for the package, then note: "No test changes for [function] - verify existing tests cover the new behavior" or "No test file found for [file]." Focus on domain edge cases, not generic coverage.
258258
259259
**Questions for the Author (根回し - Nemawashi):**
260-
End your review with 2-5 probing questions. Each question
261-
MUST reference a specific file and line number from the diff.
262-
The goal is not "have you considered..." but to surface
263-
unstated assumptions, implicit invariants, and the gap between
264-
intent and reality.
260+
Only include questions when you have genuine uncertainty about
261+
the change. A clean config tweak or straightforward bug fix
262+
needs zero questions. Do not manufacture questions to fill a
263+
section.
264+
265+
When questions ARE warranted, each MUST reference a specific
266+
file and line number from the diff. The goal is to surface
267+
unstated assumptions and the gap between intent and reality:
265268
266-
Question types that produce depth:
267269
- **Invariant surfacing**: "Line 47 of `registry.go` assumes
268270
`account.Balance` is non-negative. What enforces that
269-
upstream? If nothing, this is a latent bug."
271+
upstream?"
270272
- **Interest behind position**: "Why synchronous here
271273
(`handler.go:82`) rather than async? The saga pattern
272274
elsewhere suggests eventual consistency was the intent."
@@ -278,11 +280,11 @@ jobs:
278280
than adding a new code path?"
279281
- **Current vs ideal state**: "The test at `_test.go:92`
280282
asserts the happy path. What's the actual failure mode when
281-
the upstream returns partial data - silent corruption or
282-
explicit error?"
283+
the upstream returns partial data?"
283284
284-
Avoid generic questions. If you can't anchor a question to a
285-
specific line, it probably isn't specific enough to be useful.
285+
If you can't anchor a question to a specific line, it
286+
probably isn't specific enough to be useful. Omit the section
287+
entirely rather than ask generic questions.
286288
287289
For PRDs and architecture docs, also ask:
288290
- What edge cases are missing from the spec?
@@ -352,9 +354,8 @@ jobs:
352354
| 🔴 | `file.go:42` | Description | Open |
353355
| 🟡 | `file.go:88` | Description | Open |
354356
355-
### Questions for the Author
356-
1. `file.go:47` - [Probing question about invariant/assumption/intent]
357-
2. `other.go:82` - [Question anchored to specific code]
357+
### Questions for the Author (omit if none)
358+
1. `file.go:47` - [Question anchored to specific code]
358359
359360
### Previously Flagged
360361
| Severity | Location | Description | Status |

0 commit comments

Comments
 (0)