From 533d14d7e030103ba49dcce290f73d429877de42 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Tue, 1 Sep 2026 15:20:36 -0500 Subject: [PATCH 1/2] Have reviews catch renames of names persisted outside the codebase --- prompts/_shared/rename-compatibility-rules.md | 29 +++++++++++++++++++ prompts/claude-synthesize-thesis-first.md | 7 +++-- prompts/claude-synthesize.md | 7 +++-- prompts/codex-first-pass.md | 5 +++- 4 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 prompts/_shared/rename-compatibility-rules.md diff --git a/prompts/_shared/rename-compatibility-rules.md b/prompts/_shared/rename-compatibility-rules.md new file mode 100644 index 0000000..9a631da --- /dev/null +++ b/prompts/_shared/rename-compatibility-rules.md @@ -0,0 +1,29 @@ +A rename or deletion is only safe when every name it changes lives in the codebase. Some +names are written down elsewhere — in Redis, in a database column, in a third party's +records — by code that is already running, and are read back by the new code after the +deploy. Renaming one of those without leaving the old name resolvable strands the data +that still uses it, and the failure lands after merge, on production, with nothing in the +diff pointing at it. + +When the diff renames, moves, or deletes a class, module, constant, or symbol, work out +whether its old name is persisted anywhere and read back. Flag it when it is, and say what +holds the stale name. The usual sources: + +- A Sidekiq or ActiveJob class. The queue and the retry/scheduled sets store the class as a + string; a job enqueued before the deploy raises NameError on constantize and keeps + retrying for weeks. Watch for a job registered on a cron schedule, whose queue is rarely + empty. A subclass of the renamed job left behind under the old name, or an alias + constant, resolves it; either way the diff should say when it can be removed. +- A maintenance task (`app/tasks/maintenance/`). `maintenance_tasks_runs` stores the task + name, so a paused or errored run cannot be resumed or even rendered after the rename. +- An STI `type` column, a serialized or polymorphic `*_type` column, a GlobalID, or an + `ActiveHash` id: every existing row keeps the old string. +- A Flipper feature key, a cache key, a Redis key, or an experiment/variant name. Renaming + the key silently resets the value — the flag reads false for everyone, the cache misses, + the experiment reassigns its buckets — rather than raising. +- A key or value the app has already sent to a third party and later reads back, such as + Stripe subscription metadata or a webhook event name. + +Renaming the Ruby constant while deliberately leaving the persisted name alone is the +correct move, not an inconsistency: a table name, a column name, a metadata key, or a +route path that stays put while the class around it is renamed needs no finding. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/claude-synthesize-thesis-first.md index ed4557a..240c35b 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. 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 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..8f69f76 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. 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 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..51ab15a 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. 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 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 From 9b5fc8b61b079cbe2ef5efa9385662a7b6c3e6da Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Tue, 1 Sep 2026 15:23:25 -0500 Subject: [PATCH 2/2] Tighten the rename-compatibility rule --- prompts/_shared/rename-compatibility-rules.md | 40 ++++++------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/prompts/_shared/rename-compatibility-rules.md b/prompts/_shared/rename-compatibility-rules.md index 9a631da..92df197 100644 --- a/prompts/_shared/rename-compatibility-rules.md +++ b/prompts/_shared/rename-compatibility-rules.md @@ -1,29 +1,15 @@ -A rename or deletion is only safe when every name it changes lives in the codebase. Some -names are written down elsewhere — in Redis, in a database column, in a third party's -records — by code that is already running, and are read back by the new code after the -deploy. Renaming one of those without leaving the old name resolvable strands the data -that still uses it, and the failure lands after merge, on production, with nothing in the -diff pointing at it. +When the diff renames, moves, or deletes a class, module, constant, or symbol, check +whether its old name is persisted somewhere and read back after deploy — code already +running wrote that name down, and the new code cannot resolve it. Flag it, say what holds +the stale name, and ask for a temporary alias or subclass with a note on when to remove +it. Where the old name survives: -When the diff renames, moves, or deletes a class, module, constant, or symbol, work out -whether its old name is persisted anywhere and read back. Flag it when it is, and say what -holds the stale name. The usual sources: +- Sidekiq/ActiveJob queues, retry and scheduled sets, and cron registrations. +- `maintenance_tasks_runs`, for a task under `app/tasks/maintenance/`. +- STI `type`, serialized or polymorphic `*_type`, GlobalIDs, and ActiveHash ids. +- Flipper feature keys, cache keys, Redis keys, and experiment/variant names. +- Keys or values sent to a third party and later read back, such as Stripe metadata or a + webhook event name. -- A Sidekiq or ActiveJob class. The queue and the retry/scheduled sets store the class as a - string; a job enqueued before the deploy raises NameError on constantize and keeps - retrying for weeks. Watch for a job registered on a cron schedule, whose queue is rarely - empty. A subclass of the renamed job left behind under the old name, or an alias - constant, resolves it; either way the diff should say when it can be removed. -- A maintenance task (`app/tasks/maintenance/`). `maintenance_tasks_runs` stores the task - name, so a paused or errored run cannot be resumed or even rendered after the rename. -- An STI `type` column, a serialized or polymorphic `*_type` column, a GlobalID, or an - `ActiveHash` id: every existing row keeps the old string. -- A Flipper feature key, a cache key, a Redis key, or an experiment/variant name. Renaming - the key silently resets the value — the flag reads false for everyone, the cache misses, - the experiment reassigns its buckets — rather than raising. -- A key or value the app has already sent to a third party and later reads back, such as - Stripe subscription metadata or a webhook event name. - -Renaming the Ruby constant while deliberately leaving the persisted name alone is the -correct move, not an inconsistency: a table name, a column name, a metadata key, or a -route path that stays put while the class around it is renamed needs no finding. +Renaming only the Ruby constant while deliberately leaving the persisted name alone — a +table, a column, a metadata key, a route path — is correct, and is not a finding.