ci: refuse lossy RBS comment rewrites - #620
Conversation
jbeckwith-oai
added
codex-maintenance
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 47 mixed files remain; 0 existing customizations changed. Compared 47 existing customizations unchanged
7 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 33903404622 --repo openai/openai-ruby \
--name castiron-custom-code-33903404622-1 --dir /tmp/castiron-custom-code-33903404622-1
git apply --stat /tmp/castiron-custom-code-33903404622-1/custom-code.patch
cat /tmp/castiron-custom-code-33903404622-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 4322e076e57b769d619175006307763e3c630cfb 7faace239fea22cb7574ec99c5880bb97dd8e874
python3 scripts/castiron/custom_code_report.py report \
--base 4322e076e57b769d619175006307763e3c630cfb \
--head 7faace239fea22cb7574ec99c5880bb97dd8e874 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-7faace239fea
cat /tmp/castiron-custom-code-7faace239fea/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Using the RBS lexer rather than textual # matching is the right safety boundary: comments inside strings/annotations are excluded, while the ordered real-comment token sequence must survive formatting. Normalizing only trailing space/tab and CRLF noise keeps the guard narrow enough not to bless substantive rewrites. Failing before format returns also protects both check and write modes from partial loss.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the comment-preservation guard and its check/write paths. The lexer-based comparison runs after alias restoration and rejects lossy output before a file is written. The regression tests cover trailing-comment loss, preserved comments, literal hashes, CRLF, and idempotence. No substantive issues found. Existing native CI passed across Ruby 3.3, 3.4, and 4.0; local tests were not rerun.
3 participants
Problem
The repository RBS formatter can silently drop trailing comments that its underlying formatter does not preserve. A formatting run could therefore rewrite an RBS file and lose human-authored context without reporting an error.
Change
RBS::Parser.lex(...).valueso only real RBS comments participate;#inside strings and annotations remains ordinary syntax.formatreturns, so both check and write modes refuse a lossy result before a file can be written.Reproduction and verification
Before this change, the offline synthetic reproduction wrote away trailing comments on a type declaration, constant, and method without error. With this change, those same cases raise
RBS formatter cannot safely preserve comments; format manually, and the source tempfile remains byte-for-byte unchanged. Standalone comments plus literal/annotation#controls continue to succeed.Tests and checks run with Ruby 4.0.6:
bundle exec ruby -Itest test/scripts/rbs_format_comment_loss_test.rb— 4 runs, 22 assertionsbundle exec ruby -Itest test/scripts/rbs_format_test.rb— 17 runs, 52 assertionsbundle exec ruby -Itest test/scripts/formatting_policy_test.rb— 11 runs, 208 assertionsverify_rbs_comment_loss.rbred/green reproduction — lossy cases now fail before writes; controls passsig/**/*.rbsscan — 1,279 files, 0 proposed changes, 0 guard errorsbundle exec rake lint— RuboCop 2,874 files, typecheck clean, 1,279 RBS files validatedTEST_API_BASE_URL=http://127.0.0.1:49137 bundle exec rake test— 1,634 runs, 14,513 assertions, 0 failures, 0 errors, 1 skipScope and compatibility
This is deliberately a fail-safe output guard, not a new comment-placement implementation or formatter rewrite. It changes only
scripts/rbs_format.rband a focused handwritten test. Existing record formatting, class/module alias restoration, parse errors, deterministic path ordering, ordinary check/write behavior, and idempotence remain intact.Review
Adversarial review used fresh independent correctness and architecture reviewers. After correcting narrow whitespace and CRLF normalization findings, two consecutive rounds were clean. A bounded security review found no credential, network, dependency, deserialization, workflow, or logging surface change; the new error intentionally contains no source or comment text.
Requesting
@openai/sdks-teamreview for this build-tooling behavior.