Skip to content

Use T.unsafe only where Sorbet can't type the code, and remove mailer metadata that never collected anything - #23

Merged
ndbroadbent merged 2 commits into
mainfrom
nathan/t-unsafe-policy
Oct 18, 2026
Merged

ndbroadbent merged 2 commits into
mainfrom
nathan/t-unsafe-policy

Conversation

@ndbroadbent

@ndbroadbent ndbroadbent commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Eighth of twelve stacked PRs for 0.2.0.

What this changes

  • T.unsafe: from 32 uses in lib to 3. Most uses wrapped values that are already T.untyped
    (const_get, instance_variable_get, an untyped parameter), so they did nothing. The others have typed
    alternatives now: an override saying ActiveSupport.on_load's block runs with an untyped self, a shim for
    Shrine's subscribe (added by its instrumentation plugin), requires_ancestor in the Rails server command patch and
    the test line filter, and typed Arrays in two tests. The tagged logging formatter calls current_tags directly:
    Rails 7.1, the oldest version LogStruct supports, always has it.
  • T.unsafe stays for a splat Sorbet can't size, a duck-typed call after respond_to?, Minitest::Mock, and a test
    that passes a wrong type on purpose. AGENTS.md lists what to try first and these cases, instead of a ban that pushed
    code towards equivalent workarounds.
  • Removed the mailer's add_current_tags_to_log_data. It asked ActiveSupport::TaggedLogging and
    ActiveJob::Logging for current_tags and job_id, which neither has, and its fallback read a thread-local key
    Rails doesn't use, so it never added anything to a mailer's lines. Its tests passed only because they defined those
    methods first.

Testing

Sorbet and the unit suite (308 tests) pass at this branch's tip.


Stack for 0.2.0, merged in order: #16 → #17 → #18 → #19 → #20 → #21 → #22 → #23 → #24 → #25 → #26 → #27

add_current_tags_to_log_data asked ActiveSupport::TaggedLogging and
ActiveJob::Logging for current_tags and job_id, which neither has (the tags
belong to each tagged logger's formatter, and the job ID to each job), and its
fallback read a thread-local key Rails doesn't use. So it never added tags or a
job ID to a mailer's log lines; its tests passed only because they defined
those methods first.
Most uses wrapped values that are already T.untyped (const_get,
instance_variable_get, an untyped parameter), so they did nothing. The others now
have typed alternatives: an override saying ActiveSupport.on_load's block runs
with an untyped self, a shim for Shrine's subscribe (added by its
instrumentation plugin), requires_ancestor in the Rails server command patch and
the test line filter, and typed Arrays in two tests. The tagged logging
formatter calls current_tags directly: Rails 7.1, the oldest version LogStruct
supports, always has it.

T.unsafe stays for a splat Sorbet can't size, a duck-typed call after
respond_to?, Minitest::Mock and a test that passes a wrong type on purpose.
AGENTS.md now lists what to try first and these cases, in place of a ban that
pushed code towards equivalent workarounds.
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 23 billable files and costs up to $5.75.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 40 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 54 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 5b9b331e-a16c-49bc-a898-b97681fa646f
📥 Commits

Reviewing files that changed from the base of the PR and between 117f514 and 973b666.

⛔ Files ignored due to path filters: 2
  • sorbet/rbi/overrides/active_support.rbi is excluded by !sorbet/**
  • sorbet/rbi/shims/shrine.rbi is excluded by !sorbet/**
📒 Files selected for processing: 23
  • .cursor/rules/sorbet.mdc
  • AGENTS.md
  • lib/log_struct/integrations/action_mailer.rb
  • lib/log_struct/integrations/action_mailer/error_handling.rb
  • lib/log_struct/integrations/action_mailer/metadata_collection.rb
  • lib/log_struct/integrations/ahoy.rb
  • lib/log_struct/integrations/dotenv.rb
  • lib/log_struct/integrations/puma.rb
  • lib/log_struct/integrations/shrine.rb
  • lib/log_struct/monkey_patches/active_support/tagged_logging/formatter.rb
  • lib/log_struct/rails_boot_banner_silencer.rb
  • lib/log_struct/semantic_logger/formatter.rb
  • lib/log_struct/semantic_logger/logger.rb
  • lib/log_struct/semantic_logger/setup.rb
  • rails_test_app/templates/app/controllers/logging_controller.rb
  • test/log_struct/configuration_test.rb
  • test/log_struct/error_handling_test.rb
  • test/log_struct/formatter_test.rb
  • test/log_struct/integrations/action_mailer/metadata_collection_test.rb
  • test/log_struct/log/sql_test.rb
  • test/log_struct/multi_error_reporter_test.rb
  • test/log_struct/rails_boot_banner_silencer_test.rb
  • test/support/filter_by_line.rb
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant