jared/cc-os

Watch

Fork 0

Fix MutableCollectionLeakViaAttrReader: skip private readers, report every attr, decide by reader return #1010

New issue

Closed

opened 2026-10-15 16:36:56 +00:00 by jared · 3 comments

jared commented

2026-10-15 16:36:56 +00:00

Owner

Problem

The cop Sdlc/Structural/MutableCollectionLeakViaAttrReader has four defects. Jared ruled on all four on 2026-10-15 (ruling record: docs/research/lint-lessons/sdlc-structural-mutable-collection-leak-via-attr-reader.md, section 10). Probes ran on HEAD (aa6e5c3a) with bundle exec rubocop --cache false --only Sdlc/Structural/MutableCollectionLeakViaAttrReader --config .rubocop.yml; probe files are in the scratchpad of session 270dfee2.

False positive on a private reader. Probe p1_private_section.rb (private section, then attr_reader :items; @items = [] and @items << x) gives 1 offense. Probe p1b_private_inline.rb (private attr_reader :items) gives 0 offenses, because AttrDeclarationScan#attr_reader_calls only selects top-level attr_reader sends and the private(...) wrapper hides the inner send. So the code sees the section form only.
Only the first leaking attr is reported. Probe p2_two_attrs.rb (attr_reader :calls, :seen, with @calls << x and @seen << x) gives 1 offense, for calls. check calls add_offense(node, ...) on the class node for every attr. RuboCop's Base#add_offense returns early when current_offense_locations.add?(range) is false (rubocop/cop/base.rb:206), so the second offense at the same location is dropped.
The message gives a wrong fix. "or freeze it" on the ivar makes the next << raise FrozenError.
The cop decides by the initializer, not by what the reader returns. IvarMutationScan#mutable_literal_assigned? requires @x = [], {}, Array.new, or Hash.new. Probe p4_callog.rb: attr_reader :log with @log = CallLog.new and @log.add(x) gives 0 offenses (must fire). The same file with @log << x gives 0 offenses (must fire). So CallLog.new evades the cop by shape.

On HEAD the cop fires on two test classes in plugins/os-sdlc/eval/ticket-takedown/test/run_support.rb: ContentTests (line 80, seen) and FakeGrader (line 241, requests). Jared ruled these are true positives. Tests stay in scope, with no Exclude.

Design

/home/jared/dev/cc-os/.claude/worktrees/prism-architect/docs/takedown/1010-20261016-074221/final-design.json

Approach

Decide the leak from what the public reader returns: walk the class body tracking visibility, report one offense per public attr symbol, treat any collection-like initializer (literal or Const.new) as mutable, and resolve file-local classes' mutating methods; the message prescribes a copy.

Required work:

Write plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader.rb
Write plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/body_statements.rb
Write plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/collection_initializer.rb
Write plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/class_mutators.rb
Write plugins/os-sdlc/eval/ticket-takedown/test/run_support.rb
Write plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/ivar_mutation_scan.rb
Delete plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/mutable_literal.rb
Write test plugins/os-sdlc/tests/cops/mutable_collection_leak_reader_visibility_and_shape_test.rb

Acceptance criteria

private section, then attr_reader, @items << x: none
private attr_reader/attr_accessor :items: none
public attr_reader :calls, :seen, both shoveled: 2 offenses, one on each symbol
@log = CallLog.new, CallLog#add calls push or @a << x, @log.add(x): offense at log
@log = CallLog.new, @log << x: offense at log
def calls = @log.to_a, or def items = @items.dup: none (no attr declaration)
@flags = 0, @flags << 1: none
@log = Remote.new, undefined, @log.add(x): none

Owner sketch

Responsibility	Owner
Which attrs are public, and the location to report	BodyStatements#public_attrs (visibility walk)
Is the initializer a collection, and what class	CollectionInitializer (replaces MutableLiteral)
Does a same-file class method mutate in place	ClassMutators (new)
Find the in-place mutation of the ivar	IvarMutationScan
Report one offense per attr symbol, with the message	the cop
Shared AttrDeclarationScan	unchanged; only the lifecycle cop uses it now

plugins/os-sdlc/tests/cops/mutable_collection_leak_reader_visibility_and_shape_test.rb

# frozen_string_literal: true

require_relative "../test_helper"
require_relative "../../lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader"

class MutableCollectionLeakReaderVisibilityAndShapeTest < Minitest::Test
  include CopTestHelper

  COP = RuboCop::Cop::Sdlc::Structural::MutableCollectionLeakViaAttrReader

  PRIVATE_SECTION_SOURCE = <<~RUBY
    class Bag
      def initialize; @items = []; end
      def add(x); @items << x; end

      private

      attr_reader :items
    end
  RUBY

  TWO_ATTRS_SOURCE = <<~RUBY
    class Spy
      attr_reader :calls, :seen
      def initialize; @calls = []; @seen = []; end
      def record(x); @calls << x; @seen << x; end
    end
  RUBY

  PUSH_HELPER_SOURCE = <<~RUBY
    class CallLog
      def add(x) = push(x)
    end

    class Spy
      attr_reader :log
      def initialize; @log = CallLog.new; end
      def record(x); @log.add(x); end
    end
  RUBY

  IVAR_SHOVEL_HELPER_SOURCE = <<~RUBY
    class CallLog
      def initialize; @a = []; end
      def add(x); @a << x; end
    end

    class Spy
      attr_reader :log
      def initialize; @log = CallLog.new; end
      def record(x); @log.add(x); end
    end
  RUBY

  CONSTRUCTED_SHOVEL_SOURCE = <<~RUBY
    class Spy
      attr_reader :log
      def initialize; @log = CallLog.new; end
      def record(x); @log << x; end
    end
  RUBY

  def test_private_section_reader_is_not_flagged
    assert_empty offenses_for(COP, PRIVATE_SECTION_SOURCE)
  end

  def test_two_public_attrs_each_reported
    offenses = offenses_for(COP, TWO_ATTRS_SOURCE)

    assert_equal 2, offenses.size
    assert(offenses.any? { |m| m.start_with?("calls ") })
    assert(offenses.any? { |m| m.start_with?("seen ") })
  end

  def test_message_prescribes_copy_not_freeze
    message = offenses_for(COP, TWO_ATTRS_SOURCE).first

    refute_includes message, "freeze"
    assert_includes message, "copy"
    assert_includes message, "@calls.dup"
  end

  def test_flags_file_local_class_mutated_through_push_helper
    offenses = offenses_for(COP, PUSH_HELPER_SOURCE)

    assert_equal 1, offenses.size
    assert_includes offenses.first, "log"
  end

  def test_flags_file_local_class_mutated_through_ivar_shovel
    offenses = offenses_for(COP, IVAR_SHOVEL_HELPER_SOURCE)

    assert_equal 1, offenses.size
    assert_includes offenses.first, "log"
  end

  def test_flags_constructed_object_mutated_via_shovel
    offenses = offenses_for(COP, CONSTRUCTED_SHOVEL_SOURCE)

    assert_equal 1, offenses.size
    assert_includes offenses.first, "log"
  end
end

## Problem The cop `Sdlc/Structural/MutableCollectionLeakViaAttrReader` has four defects. Jared ruled on all four on 2026-10-15 (ruling record: docs/research/lint-lessons/sdlc-structural-mutable-collection-leak-via-attr-reader.md, section 10). Probes ran on HEAD (aa6e5c3a) with `bundle exec rubocop --cache false --only Sdlc/Structural/MutableCollectionLeakViaAttrReader --config .rubocop.yml`; probe files are in the scratchpad of session 270dfee2. 1. False positive on a private reader. Probe `p1_private_section.rb` (`private` section, then `attr_reader :items`; `@items = []` and `@items << x`) gives 1 offense. Probe `p1b_private_inline.rb` (`private attr_reader :items`) gives 0 offenses, because `AttrDeclarationScan#attr_reader_calls` only selects top-level `attr_reader` sends and the `private(...)` wrapper hides the inner send. So the code sees the section form only. 2. Only the first leaking attr is reported. Probe `p2_two_attrs.rb` (`attr_reader :calls, :seen`, with `@calls << x` and `@seen << x`) gives 1 offense, for `calls`. `check` calls `add_offense(node, ...)` on the class node for every attr. RuboCop's `Base#add_offense` returns early when `current_offense_locations.add?(range)` is false (rubocop/cop/base.rb:206), so the second offense at the same location is dropped. 3. The message gives a wrong fix. "or freeze it" on the ivar makes the next `<<` raise `FrozenError`. 4. The cop decides by the initializer, not by what the reader returns. `IvarMutationScan#mutable_literal_assigned?` requires `@x = []`, `{}`, `Array.new`, or `Hash.new`. Probe `p4_callog.rb`: `attr_reader :log` with `@log = CallLog.new` and `@log.add(x)` gives 0 offenses (must fire). The same file with `@log << x` gives 0 offenses (must fire). So `CallLog.new` evades the cop by shape. On HEAD the cop fires on two test classes in `plugins/os-sdlc/eval/ticket-takedown/test/run_support.rb`: `ContentTests` (line 80, `seen`) and `FakeGrader` (line 241, `requests`). Jared ruled these are true positives. Tests stay in scope, with no `Exclude`. ## Design /home/jared/dev/cc-os/.claude/worktrees/prism-architect/docs/takedown/1010-20261016-074221/final-design.json ## Approach Decide the leak from what the public reader returns: walk the class body tracking visibility, report one offense per public attr symbol, treat any collection-like initializer (literal or Const.new) as mutable, and resolve file-local classes' mutating methods; the message prescribes a copy. Required work: 1. Write `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader.rb` 2. Write `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/body_statements.rb` 3. Write `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/collection_initializer.rb` 4. Write `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/class_mutators.rb` 5. Write `plugins/os-sdlc/eval/ticket-takedown/test/run_support.rb` 6. Write `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/ivar_mutation_scan.rb` 7. Delete `plugins/os-sdlc/lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader/mutable_literal.rb` 8. Write test `plugins/os-sdlc/tests/cops/mutable_collection_leak_reader_visibility_and_shape_test.rb` ## Acceptance criteria - `private` section, then attr_reader, `@items << x`: none - `private attr_reader/attr_accessor :items`: none - public `attr_reader :calls, :seen`, both shoveled: 2 offenses, one on each symbol - `@log = CallLog.new`, CallLog#add calls push or `@a << x`, `@log.add(x)`: offense at `log` - `@log = CallLog.new`, `@log << x`: offense at `log` - `def calls = @log.to_a`, or `def items = @items.dup`: none (no attr declaration) - `@flags = 0`, `@flags << 1`: none - `@log = Remote.new`, undefined, `@log.add(x)`: none ## Owner sketch | Responsibility | Owner | |---|---| | Which attrs are public, and the location to report | BodyStatements#public_attrs (visibility walk) | | Is the initializer a collection, and what class | CollectionInitializer (replaces MutableLiteral) | | Does a same-file class method mutate in place | ClassMutators (new) | | Find the in-place mutation of the ivar | IvarMutationScan | | Report one offense per attr symbol, with the message | the cop | | Shared AttrDeclarationScan | unchanged; only the lifecycle cop uses it now | `plugins/os-sdlc/tests/cops/mutable_collection_leak_reader_visibility_and_shape_test.rb` ```ruby # frozen_string_literal: true require_relative "../test_helper" require_relative "../../lib/os_sdlc/cops/mutable_collection_leak_via_attr_reader" class MutableCollectionLeakReaderVisibilityAndShapeTest < Minitest::Test include CopTestHelper COP = RuboCop::Cop::Sdlc::Structural::MutableCollectionLeakViaAttrReader PRIVATE_SECTION_SOURCE = <<~RUBY class Bag def initialize; @items = []; end def add(x); @items << x; end private attr_reader :items end RUBY TWO_ATTRS_SOURCE = <<~RUBY class Spy attr_reader :calls, :seen def initialize; @calls = []; @seen = []; end def record(x); @calls << x; @seen << x; end end RUBY PUSH_HELPER_SOURCE = <<~RUBY class CallLog def add(x) = push(x) end class Spy attr_reader :log def initialize; @log = CallLog.new; end def record(x); @log.add(x); end end RUBY IVAR_SHOVEL_HELPER_SOURCE = <<~RUBY class CallLog def initialize; @a = []; end def add(x); @a << x; end end class Spy attr_reader :log def initialize; @log = CallLog.new; end def record(x); @log.add(x); end end RUBY CONSTRUCTED_SHOVEL_SOURCE = <<~RUBY class Spy attr_reader :log def initialize; @log = CallLog.new; end def record(x); @log << x; end end RUBY def test_private_section_reader_is_not_flagged assert_empty offenses_for(COP, PRIVATE_SECTION_SOURCE) end def test_two_public_attrs_each_reported offenses = offenses_for(COP, TWO_ATTRS_SOURCE) assert_equal 2, offenses.size assert(offenses.any? { |m| m.start_with?("calls ") }) assert(offenses.any? { |m| m.start_with?("seen ") }) end def test_message_prescribes_copy_not_freeze message = offenses_for(COP, TWO_ATTRS_SOURCE).first refute_includes message, "freeze" assert_includes message, "copy" assert_includes message, "@calls.dup" end def test_flags_file_local_class_mutated_through_push_helper offenses = offenses_for(COP, PUSH_HELPER_SOURCE) assert_equal 1, offenses.size assert_includes offenses.first, "log" end def test_flags_file_local_class_mutated_through_ivar_shovel offenses = offenses_for(COP, IVAR_SHOVEL_HELPER_SOURCE) assert_equal 1, offenses.size assert_includes offenses.first, "log" end def test_flags_constructed_object_mutated_via_shovel offenses = offenses_for(COP, CONSTRUCTED_SHOVEL_SOURCE) assert_equal 1, offenses.size assert_includes offenses.first, "log" end end ```

jared added the

labels

2026-10-15 16:36:56 +00:00

jared commented

2026-10-15 16:41:35 +00:00

Author

Owner

Ruling (Jared, 2026-10-15) on the open design question: option A. When the receiver's class is defined in another file, the cop stays silent on a non-listed method such as add. The 4a rule forbids false positives, not missed offenses. When #1008 lands a cross-file owner index that is safe with the RuboCop result cache, a follow-up reuses that index for this cop. Do not build a separate index here.

Ruling (Jared, 2026-10-15) on the open design question: option A. When the receiver's class is defined in another file, the cop stays silent on a non-listed method such as `add`. The 4a rule forbids false positives, not missed offenses. When #1008 lands a cross-file owner index that is safe with the RuboCop result cache, a follow-up reuses that index for this cop. Do not build a separate index here.

jared commented

2026-10-16 11:59:15 +00:00

Author

Owner

Implementation started: takedown-implement map on branch prism-architect (persistent worktree), design from docs/takedown/1010-20261016-074221/final-design.json.

~~jared referenced this issue 2026-10-16 12:04:48 +00:00~~

takedown-implement Runner commit omits design-placer's placed files #1018

jared commented

2026-10-16 12:05:01 +00:00

Author

Owner

Resolution

Done: Takedown run 1010-20261016-074221 produced the final design; takedown-implement implementation 47 placed it and passed behavior-verifier, suite-check, contract-auditor (one refactor round). MutableCollectionLeakViaAttrReader now handles reader visibility and collection shape per the design.

Evidence: Commits 56686026 (Runner, refactor paths), b51ef274 (placed design files the Runner commit omitted), 913661b7 (P5 lens signal) on prism-architect; suite gate green twice (dispatches 421, 424).

Follow-ups: #1018 Runner commit omits design-placer paths (filed, skeptic CREATE). #1017 rejected polish round cost logged as $0 (filed). Grader marked design-1 cause_remains while the critic accepted it; reported to Jared in the session summary, no ticket.

## Resolution **Done:** Takedown run 1010-20261016-074221 produced the final design; takedown-implement implementation 47 placed it and passed behavior-verifier, suite-check, contract-auditor (one refactor round). MutableCollectionLeakViaAttrReader now handles reader visibility and collection shape per the design. **Evidence:** Commits 56686026 (Runner, refactor paths), b51ef274 (placed design files the Runner commit omitted), 913661b7 (P5 lens signal) on prism-architect; suite gate green twice (dispatches 421, 424). **Follow-ups:** #1018 Runner commit omits design-placer paths (filed, skeptic CREATE). #1017 rejected polish round cost logged as $0 (filed). Grader marked design-1 cause_remains while the critic accepted it; reported to Jared in the session summary, no ticket.

jared closed this issue

2026-10-16 12:05:01 +00:00

jared referenced this issue from a commit

2026-10-16 16:51:23 +00:00

docs(lint-lessons): add MutableCollectionLeakViaAttrReader lesson (#1010)

jared referenced this issue from a commit

2026-10-16 16:51:23 +00:00

docs(takedown): plan: #1005 and #1009 closed, tracer #1010 failed at final-red (session 5)

jared referenced this issue from a commit

2026-10-16 16:51:23 +00:00

docs(takedown): session-5 handoff: correct #1010 ruling claim; one failure cause

jared referenced this issue from a commit