lint rule: mutable collection leak via attr_reader #202

New issue

Closed

opened 2026-08-01 14:16:51 +00:00 by jared · 1 comment

jared commented

2026-08-01 14:16:51 +00:00

Owner

Migrated from jared/os-sdlc#27 (repo retired).

lint rule: mutable collection leak via attr_reader

Problem

General pattern (originally found in the retired eval-sandbox's
decision-dice.rb; the specific file no longer matters, the pattern
generalizes): attr_reader :foo (or attr_accessor) exposes an ivar that
is a mutable Array/Hash literal, while some instance method mutates that
same ivar in place (<<, push, []=, merge!, concat, delete,
etc.). Callers holding the returned reference can observe or corrupt
internal state they were never meant to write to -- a POODR
encapsulation violation.

Not covered by Sdlc/Structural/LifecycleIvarBehindAttrReader (checked): that
cop flags a different shape -- an attr_reader whose ivar is set to a real
value in one method and reset to literal nil in another (temporal/lifecycle
state), unrelated to mutable-collection exposure.

Detection

Inputs: lib/ Ruby source (a single file/class, AST-checkable).
Algorithm:
1. Find attr_reader/attr_accessor declarations naming ivar @foo.
2. Find any assignment of @foo to a mutable literal ([], {},
  Array.new, Hash.new).
3. Find any other instance method that calls a mutating method
  (<<, push, pop, shift, unshift, concat, []=, merge!,
  delete, clear, sort!, reject!, select!, map!) on @foo.
4. Flag if both (2) and (3) are present and no call site returns
  @foo.dup / @foo.freeze instead of the raw reader.
Failure message: name the attr, the mutating method, and suggest
returning @foo.dup (or freezing) instead of the raw ivar.

Correction

Return @foo.dup (or a frozen array) from the reader instead of the raw
ivar, or define an explicit reader method rather than attr_reader.

Pass/fail examples

Must fail: attr_reader :history alongside a method that does
@history << x or similar in-place mutation.
Must pass: explicit def history; @history.dup; end, or the ivar is
never mutated in place after assignment.

Provenance

Fable + Codex dual review, run 18 (ticket #18, decision-dice), 2026-07-21.
Reassessed 2026-08-01 during os-sdlc sandbox retirement triage: confirmed
distinct from LifecycleIvarBehindAttrReader, confirmed AST-checkable within
a single file (fits the plugin's existing per-file cop model), example
generalized away from the retired sandbox file.

Implementation plan

Implement as a new Sdlc/Structural cop following the existing cop pattern in
lib/os_sdlc/cops/ (e.g. Sdlc/Structural/MutableCollectionLeakViaAttrReader),
registered via a require line + config section in .rubocop.yml.

Migrated from jared/os-sdlc#27 (repo retired). # lint rule: mutable collection leak via attr_reader ## Problem General pattern (originally found in the retired eval-sandbox's decision-dice.rb; the specific file no longer matters, the pattern generalizes): `attr_reader :foo` (or `attr_accessor`) exposes an ivar that is a mutable Array/Hash literal, while some instance method mutates that same ivar in place (`<<`, `push`, `[]=`, `merge!`, `concat`, `delete`, etc.). Callers holding the returned reference can observe or corrupt internal state they were never meant to write to -- a POODR encapsulation violation. Not covered by Sdlc/Structural/LifecycleIvarBehindAttrReader (checked): that cop flags a *different* shape -- an attr_reader whose ivar is set to a real value in one method and reset to literal `nil` in another (temporal/lifecycle state), unrelated to mutable-collection exposure. ## Detection * **Inputs:** lib/ Ruby source (a single file/class, AST-checkable). * **Algorithm:** 1. Find `attr_reader`/`attr_accessor` declarations naming ivar `@foo`. 2. Find any assignment of `@foo` to a mutable literal (`[]`, `{}`, `Array.new`, `Hash.new`). 3. Find any other instance method that calls a mutating method (`<<`, `push`, `pop`, `shift`, `unshift`, `concat`, `[]=`, `merge!`, `delete`, `clear`, `sort!`, `reject!`, `select!`, `map!`) on `@foo`. 4. Flag if both (2) and (3) are present and no call site returns `@foo.dup` / `@foo.freeze` instead of the raw reader. * **Failure message:** name the attr, the mutating method, and suggest returning `@foo.dup` (or freezing) instead of the raw ivar. ## Correction Return `@foo.dup` (or a frozen array) from the reader instead of the raw ivar, or define an explicit reader method rather than `attr_reader`. ## Pass/fail examples * **Must fail:** `attr_reader :history` alongside a method that does `@history << x` or similar in-place mutation. * **Must pass:** explicit `def history; @history.dup; end`, or the ivar is never mutated in place after assignment. ## Provenance Fable + Codex dual review, run 18 (ticket #18, decision-dice), 2026-07-21. Reassessed 2026-08-01 during os-sdlc sandbox retirement triage: confirmed distinct from LifecycleIvarBehindAttrReader, confirmed AST-checkable within a single file (fits the plugin's existing per-file cop model), example generalized away from the retired sandbox file. ## Implementation plan Implement as a new Sdlc/Structural cop following the existing cop pattern in lib/os_sdlc/cops/ (e.g. Sdlc/Structural/MutableCollectionLeakViaAttrReader), registered via a require line + config section in .rubocop.yml.

jared added the

ready-for-agent

enhancement

labels

2026-08-01 14:16:51 +00:00

jared commented

2026-08-01 14:33:17 +00:00

Author

Owner

Implemented as Sdlc/Structural/MutableCollectionLeakViaAttrReader (commit aafd6cd, branch worktree-sdlc-lint-cops). 6 tests green, rubocop clean.

Implemented as Sdlc/Structural/MutableCollectionLeakViaAttrReader (commit aafd6cd, branch worktree-sdlc-lint-cops). 6 tests green, rubocop clean.

jared closed this issue