From a368eaf720f2a51dc02d330dad1912f9f57e4a4d Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Tue, 8 Sep 2026 08:35:54 -0500 Subject: [PATCH 1/2] Add a review rule for specs that cannot fail usefully --- README.md | 2 +- prompts/_shared/spec-value-rules.md | 36 +++++++++++++++++++++++ prompts/claude-synthesize-thesis-first.md | 12 +++++--- prompts/claude-synthesize.md | 12 +++++--- prompts/codex-first-pass.md | 11 ++++--- 5 files changed, 60 insertions(+), 13 deletions(-) create mode 100644 prompts/_shared/spec-value-rules.md diff --git a/README.md b/README.md index 061537d..6b50ddc 100644 --- a/README.md +++ b/README.md @@ -185,7 +185,7 @@ prompts/ codex-first-pass.md # Stage 1 prompt (template) claude-synthesize.md # Stage 2 control arm (template) claude-synthesize-thesis-first.md # Stage 2 thesis-first arm (template) - _shared/{completeness,privacy,migration-data,perf,parsing,navigation}-rules.md # shared rule blocks + _shared/{completeness,privacy,migration-data,perf,parsing,navigation,rename-compatibility,spec-value}-rules.md # shared rule blocks ``` - **Templates + shared blocks.** Each prompt references the shared rule blocks via diff --git a/prompts/_shared/spec-value-rules.md b/prompts/_shared/spec-value-rules.md new file mode 100644 index 0000000..9285496 --- /dev/null +++ b/prompts/_shared/spec-value-rules.md @@ -0,0 +1,36 @@ +A spec earns its place by failing. Judge every spec the diff adds or changes by one +question: if a plausible bug were introduced in the code under test, would this spec go +red, and would that failure reveal a regression worth fixing and point at what to fix? A +spec that answers no is useless — it spends suite time and review attention and buys no +protection — and saying so is a real finding, not a nitpick. + +Report a spec as useless only when you can name the reason it cannot fail usefully: +- It asserts the implementation back to itself: it mocks or stubs the very object under + test and then asserts the stub was called, asserts a literal equals itself, checks only + `respond_to?`/`be_a`, or expects a method to call the collaborator it just stubbed. + Change the behavior and it still passes. +- It only fails when something louder fails first: asserting a factory is valid, that a + route exists which dozens of other specs already traverse, or re-testing a framework + guarantee (that `validates :x, presence: true` rejects nil, that `belongs_to` returns + the associated record). +- Its failure names nothing actionable: a broad end-to-end example with no assertion tied + to the behavior it is named for, so it goes red on any unrelated change and leaves the + author bisecting to learn why. Ask for a narrower assertion here rather than deletion. +- It pins a private method's internals instead of an observable outcome, so it goes red on + a safe refactor and stays green on a behavior regression. BiggerPockets does not test + private methods. + +Do NOT flag: +- A spec whose failure you can attribute to a bug someone could plausibly introduce, even + if the spec looks small or the behavior looks obvious. +- A regression spec for a fixed bug. Pinning behavior that was once wrong is valuable + precisely because the code now looks like it could not fail. +- Edge-case, boundary, nil, and error-path examples, or the flag-enabled and flag-disabled + pair this repo requires. Near-duplicate setup is not redundancy. +- A spec that is missing. Absent coverage is a completeness finding, not this one. +- Specs the diff only moves, renames, or reindents. + +State the finding as the bug the spec fails to catch — "this passes whether or not +`#deactivate!` persists the score, because the service is stubbed" — and suggest the +assertion that would catch it. A useless spec is not blocking on its own: report it and +still approve unless the review blocks on something else. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/claude-synthesize-thesis-first.md index c4ce31b..eb7e6f0 100644 --- a/prompts/claude-synthesize-thesis-first.md +++ b/prompts/claude-synthesize-thesis-first.md @@ -46,14 +46,18 @@ Synthesize a single review decision for pull request #{{PR}}. 12. Check whether the diff renames, moves, or deletes a name that is persisted outside the codebase and read back after deploy, per these rules: {{@prompts/_shared/rename-compatibility-rules.md}} -13. Validate which of Codex's findings are real (discard false positives), add any genuine +13. Judge the value of the specs the diff adds or changes, per these rules, and report a + useless spec with a file/line reference and the assertion that would make it fail: + {{@prompts/_shared/spec-value-rules.md}} +14. Validate which of Codex's findings are real (discard false positives), add any genuine issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal acceptance criteria in a sound way rather than flagging it as non-compliant. -14. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +15. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, - placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without - breaking the ticket's intent is a reason to approve, not to block. + placeholder, or deferred work); otherwise "approve". A useless spec is reported, never + blocking. A change that exceeds the AC without breaking the ticket's intent is a reason + to approve, not to block. Write a file named verdict.json in the current working directory with EXACTLY this shape: {"verdict": "approve" | "request_changes", "summary": ""} diff --git a/prompts/claude-synthesize.md b/prompts/claude-synthesize.md index 0ab0416..93fe9f0 100644 --- a/prompts/claude-synthesize.md +++ b/prompts/claude-synthesize.md @@ -46,14 +46,18 @@ Synthesize a single review decision for pull request #{{PR}}. 12. Check whether the diff renames, moves, or deletes a name that is persisted outside the codebase and read back after deploy, per these rules: {{@prompts/_shared/rename-compatibility-rules.md}} -13. Validate which of Codex's findings are real (discard false positives), add any genuine +13. Judge the value of the specs the diff adds or changes, per these rules, and report a + useless spec with a file/line reference and the assertion that would make it fail: + {{@prompts/_shared/spec-value-rules.md}} +14. Validate which of Codex's findings are real (discard false positives), add any genuine issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal acceptance criteria in a sound way rather than flagging it as non-compliant. -14. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +15. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, - placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without - breaking the ticket's intent is a reason to approve, not to block. + placeholder, or deferred work); otherwise "approve". A useless spec is reported, never + blocking. A change that exceeds the AC without breaking the ticket's intent is a reason + to approve, not to block. Write a file named verdict.json in the current working directory with EXACTLY this shape: {"verdict": "approve" | "request_changes", "summary": ""} diff --git a/prompts/codex-first-pass.md b/prompts/codex-first-pass.md index 3004c09..11b8921 100644 --- a/prompts/codex-first-pass.md +++ b/prompts/codex-first-pass.md @@ -40,8 +40,11 @@ verbatim and handed to a second reviewer, so do not add conversational preamble. 11. Check whether the diff renames, moves, or deletes a name that is persisted outside the codebase and read back after deploy, per these rules: {{@prompts/_shared/rename-compatibility-rules.md}} -12. Report concrete issues — bugs, regressions, security problems, member-privacy - violations, incomplete tasks/half-measures/placeholders/deferred work, and genuine misses - of the ticket's intent or clear scope creep — each with a file/line reference and a brief - rationale. Do not list "doesn't match acceptance criteria" as an issue by itself; only +12. Judge the value of the specs the diff adds or changes, per these rules, and report a + useless spec with a file/line reference and the assertion that would make it fail: + {{@prompts/_shared/spec-value-rules.md}} +13. Report concrete issues — bugs, regressions, security problems, member-privacy + violations, incomplete tasks/half-measures/placeholders/deferred work, specs that cannot + fail usefully, and genuine misses of the ticket's intent or clear scope creep — each with + a file/line reference and a brief rationale. Do not list "doesn't match acceptance criteria" as an issue by itself; only raise it when the deviation harms the intent. If nothing is blocking, say so briefly. \ No newline at end of file From 637a1d2f4463ba25a17bd5b4ca8955f4606b5b25 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Tue, 8 Sep 2026 08:42:14 -0500 Subject: [PATCH 2/2] Tighten the useless-spec rule block --- prompts/_shared/spec-value-rules.md | 54 +++++++++++------------------ 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/prompts/_shared/spec-value-rules.md b/prompts/_shared/spec-value-rules.md index 9285496..278ac3c 100644 --- a/prompts/_shared/spec-value-rules.md +++ b/prompts/_shared/spec-value-rules.md @@ -1,36 +1,24 @@ -A spec earns its place by failing. Judge every spec the diff adds or changes by one -question: if a plausible bug were introduced in the code under test, would this spec go -red, and would that failure reveal a regression worth fixing and point at what to fix? A -spec that answers no is useless — it spends suite time and review attention and buys no -protection — and saying so is a real finding, not a nitpick. +A spec is useless if its failure would neither reveal a regression worth fixing nor +point at what to fix. -Report a spec as useless only when you can name the reason it cannot fail usefully: -- It asserts the implementation back to itself: it mocks or stubs the very object under - test and then asserts the stub was called, asserts a literal equals itself, checks only - `respond_to?`/`be_a`, or expects a method to call the collaborator it just stubbed. - Change the behavior and it still passes. -- It only fails when something louder fails first: asserting a factory is valid, that a - route exists which dozens of other specs already traverse, or re-testing a framework - guarantee (that `validates :x, presence: true` rejects nil, that `belongs_to` returns - the associated record). -- Its failure names nothing actionable: a broad end-to-end example with no assertion tied - to the behavior it is named for, so it goes red on any unrelated change and leaves the - author bisecting to learn why. Ask for a narrower assertion here rather than deletion. -- It pins a private method's internals instead of an observable outcome, so it goes red on - a safe refactor and stays green on a behavior regression. BiggerPockets does not test - private methods. +Report it only for one of these reasons: +- It asserts the implementation back to itself, such as by stubbing the object under + test and checking the stub, comparing a literal with itself, checking only + `respond_to?` or `be_a`, or checking that a method calls its stubbed collaborator. +- It tests something whose failure would already break louder coverage, such as factory + validity, a widely traversed route, or a framework guarantee. +- It has no assertion tied to its named behavior, so unrelated changes can fail it + without identifying the cause. Request a narrower assertion, not deletion. +- It pins private internals instead of an observable outcome, so safe refactors fail it + while behavior regressions can pass. BiggerPockets does not test private methods. -Do NOT flag: -- A spec whose failure you can attribute to a bug someone could plausibly introduce, even - if the spec looks small or the behavior looks obvious. -- A regression spec for a fixed bug. Pinning behavior that was once wrong is valuable - precisely because the code now looks like it could not fail. -- Edge-case, boundary, nil, and error-path examples, or the flag-enabled and flag-disabled - pair this repo requires. Near-duplicate setup is not redundancy. -- A spec that is missing. Absent coverage is a completeness finding, not this one. -- Specs the diff only moves, renames, or reindents. +Do not flag: +- A spec that catches a plausible bug, however small or obvious the behavior looks. +- A regression spec for a fixed bug. +- Edge, boundary, nil, or error-path coverage, or required flag-enabled and + flag-disabled pairs. Repeated setup is not redundancy. +- Missing coverage. That is a completeness finding. +- A spec the diff only moves, renames, or reindents. -State the finding as the bug the spec fails to catch — "this passes whether or not -`#deactivate!` persists the score, because the service is stubbed" — and suggest the -assertion that would catch it. A useless spec is not blocking on its own: report it and -still approve unless the review blocks on something else. +State the bug the spec fails to catch and the assertion that would catch it. This finding +never blocks a PR by itself. Report it and approve unless another finding blocks.