From 191764e98190198ee753a46e13377666e0805b28 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Mon, 31 Aug 2026 10:00:48 -0500 Subject: [PATCH] Review the interface a change publishes, not just its implementation Three defects shipped through review on BiggerPockets/biggerpockets#30718 and were found only by driving the server by hand afterwards. All three were invisible in the diff for the same reason: the implementation was self-consistent, and what was wrong was the contract it offered a caller who cannot see it. An MCP tool required a criterion no tool enumerated, because the browser funnel supplies it from a landing-page widget rather than a question. One tool told callers to pass values "exactly as" another enumerates them, and that other tool enumerated none of them. And a payload written with symbol keys was read with string keys, so every answer a client sent read as no answer at all, with the unit specs green because they built the hash themselves. The new shared block asks four things a reviewer can check from a diff: that every required input is obtainable through the interface, that a missing one is named rather than rescued into a generic failure, that a value crossing a serialization boundary is read the way it arrives and is tested through that boundary, and that a whitelist filter is judged by what it drops today rather than by the comment beside it. Included by the Codex first pass and both synthesize arms, so the two cannot drift. --- prompts/_shared/interface-contract-rules.md | 43 +++++++++++++++++++++ prompts/claude-synthesize-thesis-first.md | 7 +++- prompts/claude-synthesize.md | 7 +++- prompts/codex-first-pass.md | 5 ++- 4 files changed, 57 insertions(+), 5 deletions(-) create mode 100644 prompts/_shared/interface-contract-rules.md diff --git a/prompts/_shared/interface-contract-rules.md b/prompts/_shared/interface-contract-rules.md new file mode 100644 index 0000000..44d84c4 --- /dev/null +++ b/prompts/_shared/interface-contract-rules.md @@ -0,0 +1,43 @@ +A change that publishes an interface — an MCP tool's schema and description, an +endpoint's params, a serializer's payload, a webhook contract — is read by a caller who +cannot see the implementation and can only do what the interface tells them. Check the +interface against the implementation, not only the implementation against itself. + +1. Every required input must be obtainable through the interface itself: + - Grep for the values the implementation reads out of the incoming payload, and check + that the interface's own schema, description, or documented requirements name each + one. An input the implementation requires but the interface never names makes the + documented path fail every time it is followed, and the caller has no way to + discover what is missing. + - Pay particular attention to values the existing UI supplies out of band: a hidden + field, a landing-page widget, a session value, an id resolved by an earlier page. + Those are exactly the ones a new non-browser caller cannot produce, and the ones an + author working from the browser flow is least likely to notice are missing. + - When one operation tells callers to take an input from another ("exactly as X + enumerates them", "pass the id returned by Y"), open that other operation and + confirm it actually produces it. A cross-reference to something that does not exist + reads as complete and is not. + +2. Failing on a missing or unusable input must name the input: + - An interface whose only answer to a missing required value is a generic error, a + 500, or a rescued "something went wrong" leaves the caller nothing to correct. Ask + that the error name the value, and where one exists, how to obtain it. + +3. Values that cross a serialization boundary must be read the way they arrive: + - Where a payload is parsed from JSON, read back from a jsonb column, pulled off a + queue, or handed over by an SDK, check the key type on both sides. A hash written + with symbol keys and read with string keys (or the reverse) does not raise — the + read returns nil, the value silently reads as absent, and the code takes its + "nothing was supplied" branch on a fully supplied payload. + - Tests that build the payload themselves cannot catch this, because the test picks + the key type the implementation already expects. When a diff adds or changes a + handler for an externally supplied payload, look for a test that drives it through + the real boundary — a request spec posting real JSON, not a direct call to the + handler with a hand-built hash — and flag its absence as a genuine gap rather than a + style preference. + +4. A filter that silently drops unrecognized entries hides both kinds of mistake: + - `select` / `reject` / `filter_map` against a list of known names drops a newly added + name as quietly as a bad one, so an omission looks identical to a deliberate + exclusion. When a diff adds one or depends on one, check what it drops today rather + than what the comment beside it says it drops. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/claude-synthesize-thesis-first.md index ed4557a..57ea983 100644 --- a/prompts/claude-synthesize-thesis-first.md +++ b/prompts/claude-synthesize-thesis-first.md @@ -37,11 +37,14 @@ Synthesize a single review decision for pull request #{{PR}}. 9. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -10. Validate which of Codex's findings are real (discard false positives), add any genuine +10. When the diff publishes or changes an interface an outside caller drives, check it + against these rules: + {{@prompts/_shared/interface-contract-rules.md}} +11. 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. -11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +12. 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. diff --git a/prompts/claude-synthesize.md b/prompts/claude-synthesize.md index 14ea29e..ddde264 100644 --- a/prompts/claude-synthesize.md +++ b/prompts/claude-synthesize.md @@ -37,11 +37,14 @@ Synthesize a single review decision for pull request #{{PR}}. 9. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -10. Validate which of Codex's findings are real (discard false positives), add any genuine +10. When the diff publishes or changes an interface an outside caller drives, check it + against these rules: + {{@prompts/_shared/interface-contract-rules.md}} +11. 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. -11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +12. 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. diff --git a/prompts/codex-first-pass.md b/prompts/codex-first-pass.md index a490f53..2f72fd9 100644 --- a/prompts/codex-first-pass.md +++ b/prompts/codex-first-pass.md @@ -31,7 +31,10 @@ verbatim and handed to a second reviewer, so do not add conversational preamble. 8. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -9. Report concrete issues — bugs, regressions, security problems, member-privacy +9. When the diff publishes or changes an interface an outside caller drives, check it + against these rules: + {{@prompts/_shared/interface-contract-rules.md}} +10. 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