os-sdlc lint rule: module owning instance state (disguised inheritance) #86

New issue

Closed

opened 2026-07-22 18:29:38 +00:00 by jared · 2 comments

jared commented

2026-07-22 18:29:38 +00:00

Owner

Context

Sandi Metz review of plugins/os_sdlc/lib/os_sdlc/issue_source.rb found a behavioral mixin
that owns instance state — a module doing the job inheritance is for, without saying so.

Problem

plugins/os-sdlc/lib/os_sdlc/issue_source.rb:100-104

module CommandFetch
  def initialize(slug, runner: Shell.new)
    @slug = slug
    @runner = runner
  end

CommandFetch is included into Forgejo and Github (lines 127, 141). It defines
initialize and both fetch (line 106-115) and parse_json (119-123) read @runner,
@slug. A module that owns initialize plus ivars isn't sharing stateless behavior — it's
disguised inheritance. Forgejo/Github only differ in command/description, exactly the
template-method shape a base class expresses directly.

Detection

Inputs: the AST of every module ... end node in a file (RuboCop on_module).
Algorithm:
1. Walk the module body's method definitions (def/def_node).
2. Flag if any method named initialize is defined directly in the module body.
3. Flag if any method body in the module references an instance variable node (ivar/
  ivasgn).
4. Report once per module if either check trips (don't double-report per ivar).
Failure message shown to the pipeline model (verbatim):
"module CommandFetch defines initialize and reads @slug/@runner -- a module that owns instance state is disguised inheritance. Convert it to a base class (Forgejo/Github inherit from it) or extract the shared behavior into a composed collaborator object, instead of a module mixed into classes that need that state."

Correction

-    module CommandFetch
+    class CommandFetch
       def initialize(slug, runner: Shell.new)
         @slug = slug
         @runner = runner
       end
@@
     class Forgejo
-      include CommandFetch
+      class Forgejo < CommandFetch
       private

(mirrored for Github; each subclass keeps only its own command/description.)

Pass/fail examples

Must fail: module CommandFetch as shown — defines initialize, reads @slug/
@runner in fetch/parse_json.
Must pass: a module with only stateless instance methods operating purely on their own
arguments (e.g. a Comparable-style mixin defining between? in terms of <=> results
passed in, no initialize, no ivar reference anywhere in the module body).

Provenance

Sandi Metz review of issue_source.rb, cc-os session 2026-07-22 (organic finding — not a
council/run finding).

Implementation plan

Custom RuboCop cop (this repo's .rubocop.yml scaffolding reserves a "batch 2" slot for
custom, non-stock cops — this rule needs one, since no stock cop flags stateful modules).
Implemented directly by a Fable agent, with a Codex review/audit before merge.

## Context Sandi Metz review of `plugins/os_sdlc/lib/os_sdlc/issue_source.rb` found a behavioral mixin that owns instance state — a module doing the job inheritance is for, without saying so. ### Problem `plugins/os-sdlc/lib/os_sdlc/issue_source.rb:100-104` ```ruby module CommandFetch def initialize(slug, runner: Shell.new) @slug = slug @runner = runner end ``` `CommandFetch` is `include`d into `Forgejo` and `Github` (lines 127, 141). It defines `initialize` and both `fetch` (line 106-115) and `parse_json` (119-123) read `@runner`, `@slug`. A module that owns `initialize` plus ivars isn't sharing stateless behavior — it's disguised inheritance. `Forgejo`/`Github` only differ in `command`/`description`, exactly the template-method shape a base class expresses directly. ### Detection - **Inputs:** the AST of every `module ... end` node in a file (RuboCop `on_module`). - **Algorithm:** 1. Walk the module body's method definitions (`def`/`def_node`). 2. Flag if any method named `initialize` is defined directly in the module body. 3. Flag if any method body in the module references an instance variable node (`ivar`/ `ivasgn`). 4. Report once per module if either check trips (don't double-report per ivar). - **Failure message shown to the pipeline model** (verbatim): `"module CommandFetch defines initialize and reads @slug/@runner -- a module that owns instance state is disguised inheritance. Convert it to a base class (Forgejo/Github inherit from it) or extract the shared behavior into a composed collaborator object, instead of a module mixed into classes that need that state."` ### Correction ```diff - module CommandFetch + class CommandFetch def initialize(slug, runner: Shell.new) @slug = slug @runner = runner end @@ class Forgejo - include CommandFetch + class Forgejo < CommandFetch private ``` (mirrored for `Github`; each subclass keeps only its own `command`/`description`.) ### Pass/fail examples - **Must fail:** `module CommandFetch` as shown — defines `initialize`, reads `@slug`/ `@runner` in `fetch`/`parse_json`. - **Must pass:** a module with only stateless instance methods operating purely on their own arguments (e.g. a `Comparable`-style mixin defining `between?` in terms of `<=>` results passed in, no `initialize`, no ivar reference anywhere in the module body). ### Provenance Sandi Metz review of `issue_source.rb`, cc-os session 2026-07-22 (organic finding — not a council/run finding). ### Implementation plan Custom RuboCop cop (this repo's `.rubocop.yml` scaffolding reserves a "batch 2" slot for custom, non-stock cops — this rule needs one, since no stock cop flags stateful modules). Implemented directly by a Fable agent, with a Codex review/audit before merge.

jared commented

2026-07-22 19:06:51 +00:00

Author

Owner

Implemented in cc-os commit 82d6d87 as Sdlc/ModuleOwnsInstanceState. Verified firing on issue_source.rb (module CommandFetch). Pending validation in a live run per the lint-rule loop.

Implemented in cc-os commit 82d6d87 as Sdlc/ModuleOwnsInstanceState. Verified firing on issue_source.rb (module CommandFetch). Pending validation in a live run per the lint-rule loop.

jared closed this issue

2026-07-23 19:32:47 +00:00

jared commented

2026-07-23 19:33:09 +00:00

Author

Owner

Covered: Sdlc/Structural/ModuleOwnsInstanceState shipped (lib/os_sdlc/cops/module_owns_instance_state.rb), enabled in .rubocop.yml with tests. ADR-0061 finalizes the os-sdlc lint program; closing per commit 8e88bbe.

jared referenced this issue

2026-10-05 17:02:55 +00:00

ticket-takedown: start and finish its worktree via /os:worktree #957