Review the interface a change publishes, not just its implementation - #45
Open
bockets wants to merge 1 commit into
Open
Review the interface a change publishes, not just its implementation#45bockets wants to merge 1 commit into
bockets wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
prompts/_shared/interface-contract-rules.md, injected into the Codex first pass and both synthesize arms.Why
Three defects shipped through review on BiggerPockets/biggerpockets#30718 and were found only by driving the server by hand against a review app afterwards.
marketcriterion that no tool enumerated. The browser funnel resolves it from a landing-page widget rather than asking it as a question, so a caller following the tools' own documentation could not produce a runnable call — the documented path failed 100% of the time, and reported"Something went wrong".run_finder_searchtold callers to pass contact criteria "exactly asdescribe_finder_questionsenumerates them". For the lender finder, that tool enumerated none of them.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. Nothing in the existing rule blocks points a reviewer at that. Completeness rules ask whether the work was finished; these ask whether the published interface is usable by someone holding only the interface.
What it asks
Four things checkable from a diff:
Scope
Included from all three prompts so Codex and the synthesize arms can't drift. Editing a
_sharedblock bumps the derived prompt version for every consuming repo, which is the intended Roll.I ran
scripts/resolve-prompts.shlocally: both prompts resolve, no markers remain, and both templates are include-expanded.