From 67db0b9d06f1ff8065aa4353d01baa388e000f62 Mon Sep 17 00:00:00 2001 From: Gord Pearson Date: Fri, 14 Aug 2026 14:04:26 -0400 Subject: [PATCH] Make the router's routing state authoritative instead of sniffed Disabling the router restored the original write method but left the write_without_cli_ui marker method installed on the stream. Since enabled? checked for that marker, the router reported itself enabled while routing was actually inactive, and a later enable refused to do anything. The process was stuck: output permanently bypassed routing, and Capture#run's assert_enabled! still passed while captures silently collected nothing. with_enabled compounded this by always disabling on exit, so entering that scope in a process that had already enabled the router (the common production state) permanently broke routing. Routing state now lives in a private stream => original-write registry rather than being inferred from a marker method on the stream, so nothing on the stream can make enabled? lie: * enable/disable act per stream instead of all-or-nothing, so a process that ends up half-routed (e.g. $stdout reassigned after enable, or $stderr aliased to $stdout) can recover. with_enabled unroutes exactly the streams it routed, leaving an already-enabled router alone. * Writer holds the stream's pre-routing write method, so it no longer dispatches through write_without_cli_ui. A write already in flight on another thread survives a concurrent disable. * deactivate restores only a write it displaced. Previously every disable left a singleton copy of the class's write behind, shadowing the class method for the life of the stream. * write_without_routing is the supported way to bypass the router. Callers currently reach for the write_without_cli_ui alias directly (dev's cd/edit commands do, unguarded); that alias only exists while the stream is routed, and now really does disappear on disable. * NotEnabled moves out of class << self, where it was only reachable as StdoutRouter.singleton_class::NotEnabled - i.e. nobody could rescue the error Capture#run raises. * ensure_activated is now just enable, which is idempotent. Co-authored-by: River Co-Authored-By: Claude Fable 5 Assisted-By: devx/c0685ec8-2be2-4abc-9131-691e185f92ae --- lib/cli/ui/stdout_router.rb | 163 ++++++++++++++++---- test/cli/ui/stdout_router_test.rb | 244 ++++++++++++++++++++++++++++++ 2 files changed, 381 insertions(+), 26 deletions(-) diff --git a/lib/cli/ui/stdout_router.rb b/lib/cli/ui/stdout_router.rb index 20f8144b..7bc24fc7 100644 --- a/lib/cli/ui/stdout_router.rb +++ b/lib/cli/ui/stdout_router.rb @@ -11,11 +11,36 @@ module CLI module UI module StdoutRouter + WRITE_WITHOUT_CLI_UI = :write_without_cli_ui + + # Defined here rather than on the singleton class, where callers had no + # way to name it. + NotEnabled = Class.new(StandardError) + + class Route + #: Method + attr_reader :original_write + + #: bool + attr_reader :remove_compatibility_write + + #: (Method original_write, bool remove_compatibility_write) -> void + def initialize(original_write, remove_compatibility_write) + @original_write = original_write + @remove_compatibility_write = remove_compatibility_write + end + end + private_constant :Route + class Writer - #: (io_like stream, Symbol name) -> void - def initialize(stream, name) + # `original_write` must be the stream's pre-routing `write`; `activate` + # passes it in. Holding the method itself keeps an in-flight write + # working after `deactivate` removes the WRITE_WITHOUT_CLI_UI alias. + #: (io_like stream, Symbol name, Method original_write) -> void + def initialize(stream, name, original_write) @stream = stream @name = name + @original_write = original_write end #: (*Object args) -> Integer @@ -38,7 +63,7 @@ def write(*args) end stream_args = prepend_id(@stream, strs) #: as untyped - ret = @stream.write_without_cli_ui(*stream_args) #: as Integer + ret = @original_write.call(*stream_args) #: as Integer if (dup = StdoutRouter.duplicate_output_to) begin dup_args = prepend_id(dup, strs) #: as untyped @@ -307,10 +332,6 @@ def synchronize(&block) end class << self - WRITE_WITHOUT_CLI_UI = :write_without_cli_ui - - NotEnabled = Class.new(StandardError) - #: io_like? attr_accessor :duplicate_output_to @@ -339,62 +360,152 @@ def assert_enabled! raise NotEnabled unless enabled? end + # Only unroutes what it routed, so nesting inside an already-enabled + # router leaves that router alone. #: [T] { -> T } -> T def with_enabled(&block) - enable + activated = activate_current_streams yield ensure - disable + activated&.each { |stream| deactivate(stream) if enabled?(stream) } end # TODO: remove this #: -> void def ensure_activated - enable unless enabled? + enable end + # Makes the current $stdout/$stderr the only routed streams; returns + # whether anything changed. Per-stream so a half-routed process can + # recover. #: -> bool def enable - return false if enabled?($stdout) || enabled?($stderr) - - activate($stdout, :stdout) - activate($stderr, :stderr) - true + streams = current_streams + activated = activate_streams(streams) + deactivated = deactivate_streams_except(streams) + deactivated || !activated.empty? end #: (?io_like stream) -> bool def enabled?(stream = $stdout) - stream.respond_to?(WRITE_WITHOUT_CLI_UI) + routed_streams.key?(stream) end #: -> bool def disable - return false unless enabled?($stdout) && enabled?($stderr) + routed = routed_streams.keys + routed.each { |stream| deactivate(stream) } + !routed.empty? + end + + # Writes past the router: no frame inset, capture hooks, or output + # duplication. Prefer this to calling WRITE_WITHOUT_CLI_UI directly, + # which only exists while the stream is routed. + #: (io_like stream, *Object args) -> Integer + def write_without_routing(stream, *args) + original_write = routed_streams[stream]&.original_write + write_args = args #: as untyped + return stream.write(*write_args) unless original_write - deactivate($stdout) - deactivate($stderr) - true + original_write.call(*write_args) #: as Integer end private + # Routed stream => the state needed to restore it. State lives here + # rather than in a method on the stream, so nothing on the stream can + # make `enabled?` lie. `deactivate` drops the entry. + #: -> Hash[io_like, Route] + def routed_streams + @routed_streams ||= {}.compare_by_identity + end + + #: -> Hash[io_like, Symbol] + def current_streams + streams = {}.compare_by_identity #: Hash[io_like, Symbol] + streams[$stdout] = :stdout + streams[$stderr] ||= :stderr + streams + end + + #: (Hash[io_like, Symbol] streams) -> bool + def deactivate_streams_except(streams) + stale = routed_streams.keys.reject { |stream| streams.key?(stream) } + stale.each { |stream| deactivate(stream) } + !stale.empty? + end + + #: (Hash[io_like, Symbol] streams) -> Array[io_like] + def activate_streams(streams) + activated = [] #: Array[io_like] + begin + streams.each do |stream, streamname| + next if enabled?(stream) + + activate(stream, streamname) + activated << stream + end + activated + rescue + activated.reverse_each { |stream| deactivate(stream) if enabled?(stream) } + raise + end + end + + #: -> Array[io_like] + def activate_current_streams + activate_streams(current_streams) + end + #: (io_like stream) -> void def deactivate(stream) + route = routed_streams.delete(stream) + original_write = route&.original_write sc = stream.singleton_class + sc.send(:remove_method, :write) - sc.send(:alias_method, :write, WRITE_WITHOUT_CLI_UI) + # Restore only a `write` we displaced; otherwise leave the class + # method exposed again. Prefer the compatibility alias when it is + # ours; otherwise restore the stored method without disturbing a + # foreign method using that name. + if original_write&.owner == sc + if route&.remove_compatibility_write + sc.send(:alias_method, :write, WRITE_WITHOUT_CLI_UI) + else + sc.send(:define_method, :write, original_write) + end + end + sc.send(:remove_method, WRITE_WITHOUT_CLI_UI) if route&.remove_compatibility_write end #: (io_like stream, Symbol streamname) -> void def activate(stream, streamname) - writer = StdoutRouter::Writer.new(stream, streamname) - - raise if stream.respond_to?(WRITE_WITHOUT_CLI_UI) - - stream.singleton_class.send(:alias_method, WRITE_WITHOUT_CLI_UI, :write) + original_write = stream.method(:write) + writer = StdoutRouter::Writer.new(stream, streamname, original_write) + installed_compatibility_write = false + compatibility_write = if stream.respond_to?(WRITE_WITHOUT_CLI_UI, true) + stream.method(WRITE_WITHOUT_CLI_UI) + end + remove_compatibility_write = compatibility_write&.owner == stream.singleton_class && + compatibility_write == original_write + + # Kept for callers that bypass the router through the alias itself, + # without overwriting an unrelated method using the same name. + unless compatibility_write + stream.singleton_class.send(:alias_method, WRITE_WITHOUT_CLI_UI, :write) + installed_compatibility_write = true + remove_compatibility_write = true + end stream.define_singleton_method(:write) do |*args| writer.write(*args) end + routed_streams[stream] = Route.new(original_write, !!remove_compatibility_write) + rescue + if installed_compatibility_write + stream.singleton_class.send(:remove_method, WRITE_WITHOUT_CLI_UI) + end + raise end end end diff --git a/test/cli/ui/stdout_router_test.rb b/test/cli/ui/stdout_router_test.rb index 23be5aa5..20aad61f 100644 --- a/test/cli/ui/stdout_router_test.rb +++ b/test/cli/ui/stdout_router_test.rb @@ -24,6 +24,250 @@ def test_current_id end end + def test_write_without_cli_ui_constant_is_accessible + assert_equal(:write_without_cli_ui, StdoutRouter::WRITE_WITHOUT_CLI_UI) + end + + def test_writer_requires_the_original_write + assert_raises(ArgumentError) do + StdoutRouter::Writer.new(StringIO.new, :stdout) + end + end + + def test_router_can_reenable_after_disable + # capture_io swaps in fresh StringIO streams, so enable/disable here + # can't interfere with a router installed on the real stdio streams. + capture_io do + assert(StdoutRouter.enable) + assert($stdout.respond_to?(:write_without_cli_ui)) + assert(StdoutRouter.disable) + + refute_predicate(StdoutRouter, :enabled?) + refute($stdout.respond_to?(:write_without_cli_ui)) + + assert(StdoutRouter.enable) + assert_predicate(StdoutRouter, :enabled?) + + cap = StdoutRouter::Capture.new { print('hi') } + cap.run + assert_equal('hi', cap.stdout) + ensure + StdoutRouter.disable + end + end + + def test_capture_raises_when_the_router_is_disabled + capture_io do + assert_raises(StdoutRouter::NotEnabled) do + StdoutRouter::Capture.new {}.run + end + end + end + + def test_disable_leaves_the_streams_as_it_found_them + capture_io do + StdoutRouter.enable + StdoutRouter.disable + + assert_empty($stdout.singleton_methods) + assert_empty($stderr.singleton_methods) + end + end + + def test_disable_restores_a_pre_existing_singleton_write + capture_io do + written = [] + $stdout.define_singleton_method(:write) do |*args| + written << args.join + super(*args) + end + + StdoutRouter.enable + $stdout.write('routed') + StdoutRouter.disable + $stdout.write('plain') + + assert_equal(['routed', 'plain'], written) + assert_equal([:write], $stdout.singleton_methods) + end + end + + def test_enable_recovers_a_half_routed_process + capture_io do + StdoutRouter.enable + original_stderr = $stderr + $stderr = StringIO.new + + assert(StdoutRouter.enable) + assert(StdoutRouter.enabled?($stderr)) + refute(StdoutRouter.enabled?(original_stderr)) + refute(original_stderr.respond_to?(:write_without_cli_ui)) + + assert(StdoutRouter.disable) + refute_predicate(StdoutRouter, :enabled?) + refute(StdoutRouter.enabled?($stderr)) + ensure + StdoutRouter.disable + end + end + + def test_disable_unroutes_streams_replaced_in_globals + capture_io do + StdoutRouter.enable + original_stdout = $stdout + $stdout = StringIO.new + + assert(StdoutRouter.disable) + refute(StdoutRouter.enabled?(original_stdout)) + refute(original_stdout.respond_to?(:write_without_cli_ui)) + refute(StdoutRouter.enabled?($stdout)) + assert_empty(StdoutRouter.send(:routed_streams)) + ensure + StdoutRouter.disable + end + end + + def test_ensure_activated_does_not_retain_previous_capture_streams + previous_streams = nil + capture_io do + StdoutRouter.ensure_activated + previous_streams = [$stdout, $stderr] + end + + capture_io do + StdoutRouter.ensure_activated + + previous_streams.each do |stream| + refute(StdoutRouter.enabled?(stream)) + refute(stream.respond_to?(:write_without_cli_ui)) + end + assert_equal(2, StdoutRouter.send(:routed_streams).size) + ensure + StdoutRouter.disable + end + end + + def test_enable_handles_stderr_aliased_to_stdout + capture_io do + $stderr = $stdout + + assert(StdoutRouter.enable) + assert(StdoutRouter.disable) + assert_empty($stdout.singleton_methods) + end + end + + def test_enable_preserves_a_foreign_write_without_cli_ui + capture_io do + foreign_write = ->(*) { 123 } + $stderr.define_singleton_method(:write_without_cli_ui, foreign_write) + + assert(StdoutRouter.enable) + assert(StdoutRouter.enabled?($stdout)) + assert(StdoutRouter.enabled?($stderr)) + assert_equal(123, $stderr.write_without_cli_ui('ignored')) + + assert(StdoutRouter.disable) + assert_equal(123, $stderr.write_without_cli_ui('ignored')) + assert_equal([:write_without_cli_ui], $stderr.singleton_methods) + ensure + StdoutRouter.disable + end + end + + def test_enable_adopts_and_removes_a_stale_write_without_cli_ui + capture_io do + $stderr.singleton_class.send(:alias_method, :write_without_cli_ui, :write) + + assert(StdoutRouter.enable) + assert(StdoutRouter.enabled?($stderr)) + assert(StdoutRouter.disable) + refute($stderr.respond_to?(:write_without_cli_ui)) + ensure + StdoutRouter.disable + end + end + + def test_enable_is_transactional + capture_io do + failing_stderr = StringIO.new + failing_stderr.define_singleton_method(:define_singleton_method) do |name, *, &| + raise "unexpected method: #{name}" unless name == :write + + raise 'cannot install write' + end + $stderr = failing_stderr + + error = assert_raises(RuntimeError) do + StdoutRouter.enable + end + assert_equal('cannot install write', error.message) + refute(StdoutRouter.enabled?($stdout)) + refute($stdout.respond_to?(:write_without_cli_ui)) + refute($stderr.respond_to?(:write_without_cli_ui)) + assert_empty(StdoutRouter.send(:routed_streams)) + ensure + StdoutRouter.disable + end + end + + def test_in_flight_writes_survive_disable + capture_io do + StdoutRouter.enable + in_flight = $stdout.method(:write) + StdoutRouter.disable + + in_flight.call('late') + assert_includes($stdout.string, 'late') + end + end + + def test_write_without_routing_bypasses_the_router + capture_io do + StdoutRouter.enable + cap = StdoutRouter::Capture.new { StdoutRouter.write_without_routing($stdout, 'bypass') } + cap.run + + assert_equal('', cap.stdout) + assert_includes($stdout.string, 'bypass') + ensure + StdoutRouter.disable + end + end + + def test_write_without_routing_falls_back_to_write_when_disabled + capture_io do + StdoutRouter.write_without_routing($stdout, 'unrouted') + + assert_includes($stdout.string, 'unrouted') + end + end + + def test_with_enabled_preserves_already_enabled_router + capture_io do + StdoutRouter.enable + StdoutRouter.with_enabled {} + + assert_predicate(StdoutRouter, :enabled?) + cap = StdoutRouter::Capture.new { print('still routed') } + cap.run + assert_equal('still routed', cap.stdout) + ensure + StdoutRouter.disable + end + end + + def test_with_enabled_unroutes_only_what_it_routed + capture_io do + StdoutRouter.with_enabled do + assert_predicate(StdoutRouter, :enabled?) + end + + refute_predicate(StdoutRouter, :enabled?) + assert_empty($stdout.singleton_methods) + end + end + def test_frame_can_autoload_after_router_is_enabled script = <<~RUBY require 'stringio'