From 4983e09b5aaa5cbdff0510e9f109924d693f237f Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Sat, 5 Sep 2026 07:29:19 -0500 Subject: [PATCH] Add nullability-boundary review rule Codex/Claude approved biggerpockets PRs #30877/#30879/#30880/#30881/#30883 (a React port of /notifications) that crashed on first load in a review app: TypeError: Cannot read properties of null (reading 'charAt'). The colleague_requests.status and referenceships.status columns are nullable in db/schema.rb, the serializer passed the column straight through, and the TypeScript layer declared status: string and laundered the null through an unchecked `as raw.status as string` assertion, which the component then called .charAt(0) on. The throw hit React Router's error boundary and blanked the whole page. Test factories always set a status, so no spec caught it. Adds prompts/_shared/nullability-boundary-rules.md, wired into both review stages, to check nullability at the schema/serializer source rather than trusting a declared TypeScript type, and to weigh blast radius when a new throw sits inside a subtree covered by an error boundary. --- README.md | 2 +- prompts/_shared/nullability-boundary-rules.md | 31 +++++++++++++++++++ prompts/claude-synthesize-thesis-first.md | 7 +++-- prompts/claude-synthesize.md | 7 +++-- prompts/codex-first-pass.md | 5 ++- 5 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 prompts/_shared/nullability-boundary-rules.md diff --git a/README.md b/README.md index 07c51b3..82fa543 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,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,nullability-boundary}-rules.md # shared rule blocks ``` - **Templates + shared blocks.** Each prompt references the shared rule blocks via diff --git a/prompts/_shared/nullability-boundary-rules.md b/prompts/_shared/nullability-boundary-rules.md new file mode 100644 index 0000000..06ec61d --- /dev/null +++ b/prompts/_shared/nullability-boundary-rules.md @@ -0,0 +1,31 @@ +When a value crosses the boundary from a DB column into a serializer, into a TypeScript +API type, and into a component, verify its actual nullability at the source rather than +trusting a type further down the chain: +- Check `db/schema.rb` for the column backing any field a serializer passes through + (`status: parent.status`, `foo: record.foo`, etc.). A column without `null: false` can + hold NULL in production even if every factory in the test suite sets a value, and the + serializer must handle that case (a default, an explicit null branch, or a documented + reason the column can never actually be null despite the schema). +- Treat an unchecked TypeScript `as` assertion narrowing a nullable value to a non-null + type (e.g. `status: raw.status as string` where the API can return `null`) as a finding. + The assertion must be justified against the actual schema/serializer, not just against + the shape the author expects the API to return. Flag it and ask for either a real type + (`string | null`) with the component handling `null`, or a comment citing why the source + column is guaranteed non-null. +- This applies most sharply to a new frontend page/component reading a field for the first + time — a nullable column that has been silently null in production for years only + becomes a crash once something finally calls a method on it (`.charAt`, `.toUpperCase`, + etc.) without a null check. + +Separately, weigh blast radius when a diff adds a call that can throw inside a React +component subtree sitting under an error boundary (React Router's route error boundary, +or any explicit `ErrorBoundary`): an uncaught throw there takes down everything the +boundary covers, not just the one card or row that failed. Flag a new, unguarded method +call on a value that can be null/undefined (per the point above) more strongly when it +sits in a shared list/page component rather than in an isolated leaf that already has its +own boundary or fallback. + +Don't flag routine null-safe code (optional chaining, a default, a type that already +includes `null`/`undefined` and is handled), and don't demand `string | null` for a value +backed by a `null: false` column — the point is to check the real nullability, not to add +a null case to everything reflexively. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/claude-synthesize-thesis-first.md index 240c35b..f5fb7cb 100644 --- a/prompts/claude-synthesize-thesis-first.md +++ b/prompts/claude-synthesize-thesis-first.md @@ -40,11 +40,14 @@ Synthesize a single review decision for pull request #{{PR}}. 10. 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}} -11. Validate which of Codex's findings are real (discard false positives), add any genuine +11. Check how the diff carries a value across a DB column / serializer / TypeScript API + type / component boundary, per these rules: + {{@prompts/_shared/nullability-boundary-rules.md}} +12. 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. -12. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +13. 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 8f69f76..1c0374b 100644 --- a/prompts/claude-synthesize.md +++ b/prompts/claude-synthesize.md @@ -40,11 +40,14 @@ Synthesize a single review decision for pull request #{{PR}}. 10. 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}} -11. Validate which of Codex's findings are real (discard false positives), add any genuine +11. Check how the diff carries a value across a DB column / serializer / TypeScript API + type / component boundary, per these rules: + {{@prompts/_shared/nullability-boundary-rules.md}} +12. 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. -12. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +13. 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 51ab15a..033d7d3 100644 --- a/prompts/codex-first-pass.md +++ b/prompts/codex-first-pass.md @@ -34,7 +34,10 @@ verbatim and handed to a second reviewer, so do not add conversational preamble. 9. 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}} -10. Report concrete issues — bugs, regressions, security problems, member-privacy +10. Check how the diff carries a value across a DB column / serializer / TypeScript API + type / component boundary, per these rules: + {{@prompts/_shared/nullability-boundary-rules.md}} +11. 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