ruff_ruby_spo: migration-replay codex fixes — skip def down bodies + parenthesized add_column (#73 follow-up)#74
Conversation
…hesized add_column (codex #73 P2) Two P2 correctness fixes from codex review on the merged PR #73, both in the post-baseline migration-replay (schema.rs): 1. Skip `def down` rollback bodies. `def up`/`def change` describe the schema AFTER the migration; `def down` reverses it. Replaying the down half undid valid columns. A lightweight Ruby block-depth tracker (is_ruby_block_opener + an == "end" close check) now suppresses mutations while inside a `def down` method. Corpus impact, verified: WorkPackage is 33 columns, not 31 — create_work_package_semantic_ids's `def up` adds sequence_number/identifier and its `def down` removes them; Rails applies only up, so they are RETAINED. The old "net to zero" pin of 31 was exactly the bug the reviewer caught. Corpus schema gate + drift fuse updated 31 -> 33 with the corrected provenance. 2. Parse parenthesized add_column(:t, :c, :type). parse_add_column did not strip_call_parens while the rename/remove/change parsers did, so the common parenthesized form was silently dropped. Normalized the argument tail like its siblings. Two regression tests added (replay_add_column_parenthesized_form_applies, replay_skips_def_down_rollback_body). Follow-up to merged #73; branch restarted from main per merged-PR protocol. Tests 151 (ruby) + 133 (triplet), clippy-clean, corpus gates green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_55c14eec-41f4-46bd-80d9-cc20f54ec0f5) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 695bf79dc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// with an `== "end"` close check; single-line `def … end` (absent from | ||
| /// real migrations) is out of scope. | ||
| fn is_ruby_block_opener(line: &str) -> bool { | ||
| if line.ends_with(" do") || line.contains(" do ") || line.contains(" do|") { |
There was a problem hiding this comment.
When a valid migration declares def down before def up and the rollback body contains a string or comment with a phrase like we do nothing, this substring check increments depth even though no Ruby block was opened. The real end of down then does not clear down_from, so the following def up body is still treated as rollback and its add_column mutations are skipped, making baseline+replay silently lose columns.
Useful? React with 👍 / 👎.
| && (line == "def down" | ||
| || line.starts_with("def down(") | ||
| || line.starts_with("def down ")) |
There was a problem hiding this comment.
Post-baseline migrations that use the classic Rails class-method style def self.up / def self.down still replay the rollback body because this detector only recognizes bare def down. Since the replay otherwise accepts mutations inside def self.up, a paired remove_column in def self.down will undo the valid column exactly like the bug this patch is fixing for instance-method migrations.
Useful? React with 👍 / 👎.
2 participants
Addresses the two codex P2 review comments on the merged #73, both in the post-baseline migration-replay (
schema.rs).1. Skip
def downrollback bodies (#73 comment)def up/def changedescribe the schema after the migration;def downreverses it. The replay was scanning both halves, so anadd_columninupfollowed by itsremove_columnindownnetted the column away — dropping valid columns.A lightweight Ruby block-depth tracker (
is_ruby_block_opener+ an== "end"close check) now suppresses mutations while inside adef downmethod.Corpus impact, verified: WorkPackage is 33 columns, not 31.
20260330100000_create_work_package_semantic_ids.rb'sdef upaddssequence_number/identifier, and itsdef downremoves them — Rails applies onlyup, so they are retained. The old "net to zero" pin of 31 was precisely the bug the reviewer caught. The corpus schema gate and drift fuse are updated 31 → 33 with the corrected provenance comment.2. Parse parenthesized
add_column(:t, :c, :type)(#73 comment)parse_add_columndid notstrip_call_parenswhile the rename/remove/change parsers did, so the commonadd_column(...)form was silently ignored — leaving newly-added columns out ofbaseline+replay. The argument tail is now normalized like its siblings.Tests
Two regressions added:
replay_add_column_parenthesized_form_applies,replay_skips_def_down_rollback_body. Full run:ruff_ruby_spo151 +ruff_spo_triplet133 passed; clippy-clean; corpus schema gate + routes fuse green underRAILS_CORPUS_SRC/OPENPROJECT_PATH=/home/user/openproject.Follow-up to merged #73; branch restarted from
mainper the merged-PR protocol. Onlyschema.rschanges.🤖 Generated with Claude Code
https://claude.ai/code/session_01LcsmBBVpETdnTqvDzx6ZiG
Generated by Claude Code