jared/cc-os

Watch

Fork 0

os-sdlc lint rule: parameter threading through private methods (wants instance state) #87

New issue

Closed

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

jared commented

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

Owner

Context

Sandi Metz review of plugins/os_sdlc/lib/os_sdlc/issue_source.rb found runner threaded
through a chain of private class methods as a plain parameter — a smell for state that wants
an object of its own.

Problem

plugins/os-sdlc/lib/os_sdlc/issue_source.rb — runner appears in the signature of every
method in this call chain:

26: def self.for(project, runner: Shell.new)
38: def self.resolve_and_persist(project, runner)
57: def self.tracker_from_git_remote(project, runner)
80: def self.forgejo_host?(host, runner)

resolve_and_persist, tracker_from_git_remote, and forgejo_host? are all
private_class_methods (line 97) that exist solely to keep passing runner one level
deeper so forgejo_host? can eventually call runner.capture(...). runner is de facto
instance state being smuggled through parameter lists instead of being held by an object.

Detection

Inputs: the set of private_class_method (or private) method definitions in a
class/module body, and their parameter lists.
Algorithm:
1. Collect every private method definition in the class/module.
2. For each parameter name, count how many distinct private method signatures include a
  parameter of that exact name.
3. Flag the class/module if any single parameter name appears in the signatures of 3 or
  more private methods.
Failure message shown to the pipeline model (verbatim):
"runner is passed through 3+ private methods (resolve_and_persist, tracker_from_git_remote, forgejo_host?) -- extract an object that holds runner as instance state (e.g. a GitRemoteTracker initialized with runner:) instead of threading it through every private method's parameter list."

Correction

-    def self.resolve_and_persist(project, runner)
-      tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner)
+    def self.resolve_and_persist(project, runner)
+      tracker = tracker_from_config(project) ||
+        GitRemoteTracker.new(runner: runner).resolve(project)
       return nil unless tracker
       project.save_tracker(tracker)
       tracker
     end
+
+    class GitRemoteTracker
+      def initialize(runner: Shell.new)
+        @runner = runner
+      end
+
+      def resolve(project)
+        out, _err, ok = @runner.capture("git", "-C", project.root, "remote", "get-url", "origin")
+        return nil unless ok
+
+        host, slug = IssueSource.parse_git_url(out.strip)
+        return nil unless host && slug
+
+        host == "github.com" ? "github:#{slug}" : (forgejo_host?(host) && "forgejo:#{slug}")
+      end
+
+      private
+
+      def forgejo_host?(host)
+        out, _err, ok = @runner.capture("tea", "logins", "list")
+        ok && out.include?(host)
+      end
+    end

Pass/fail examples

Must fail: a class/module where runner (or any other single parameter name) appears
in the signatures of resolve_and_persist, tracker_from_git_remote, and
forgejo_host? (3 private methods) as in the current file.
Must pass: the corrected form above, where only GitRemoteTracker#initialize takes
runner:; every other method in the chain reads it from @runner.

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 (batch-2 custom-cop slot; no stock cop counts repeated parameter names
across a class's private methods). 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 `runner` threaded through a chain of private class methods as a plain parameter — a smell for state that wants an object of its own. ### Problem `plugins/os-sdlc/lib/os_sdlc/issue_source.rb` — `runner` appears in the signature of every method in this call chain: ``` 26: def self.for(project, runner: Shell.new) 38: def self.resolve_and_persist(project, runner) 57: def self.tracker_from_git_remote(project, runner) 80: def self.forgejo_host?(host, runner) ``` `resolve_and_persist`, `tracker_from_git_remote`, and `forgejo_host?` are all `private_class_method`s (line 97) that exist solely to keep passing `runner` one level deeper so `forgejo_host?` can eventually call `runner.capture(...)`. `runner` is de facto instance state being smuggled through parameter lists instead of being held by an object. ### Detection - **Inputs:** the set of `private_class_method` (or `private`) method definitions in a class/module body, and their parameter lists. - **Algorithm:** 1. Collect every private method definition in the class/module. 2. For each parameter name, count how many *distinct* private method signatures include a parameter of that exact name. 3. Flag the class/module if any single parameter name appears in the signatures of 3 or more private methods. - **Failure message shown to the pipeline model** (verbatim): `"runner is passed through 3+ private methods (resolve_and_persist, tracker_from_git_remote, forgejo_host?) -- extract an object that holds runner as instance state (e.g. a GitRemoteTracker initialized with runner:) instead of threading it through every private method's parameter list."` ### Correction ```diff - def self.resolve_and_persist(project, runner) - tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner) + def self.resolve_and_persist(project, runner) + tracker = tracker_from_config(project) || + GitRemoteTracker.new(runner: runner).resolve(project) return nil unless tracker project.save_tracker(tracker) tracker end + + class GitRemoteTracker + def initialize(runner: Shell.new) + @runner = runner + end + + def resolve(project) + out, _err, ok = @runner.capture("git", "-C", project.root, "remote", "get-url", "origin") + return nil unless ok + + host, slug = IssueSource.parse_git_url(out.strip) + return nil unless host && slug + + host == "github.com" ? "github:#{slug}" : (forgejo_host?(host) && "forgejo:#{slug}") + end + + private + + def forgejo_host?(host) + out, _err, ok = @runner.capture("tea", "logins", "list") + ok && out.include?(host) + end + end ``` ### Pass/fail examples - **Must fail:** a class/module where `runner` (or any other single parameter name) appears in the signatures of `resolve_and_persist`, `tracker_from_git_remote`, and `forgejo_host?` (3 private methods) as in the current file. - **Must pass:** the corrected form above, where only `GitRemoteTracker#initialize` takes `runner:`; every other method in the chain reads it from `@runner`. ### 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 (batch-2 custom-cop slot; no stock cop counts repeated parameter names across a class's private methods). Implemented directly by a Fable agent, with a Codex review/audit before merge. </content>

jared commented

2026-07-22 18:55:37 +00:00

Author

Owner

Implementation note (2026-07-22): the detection algorithm as specified correctly fires on MORE than the prose enumerates. On issue_source.rb it also flags project (threaded through resolve_and_persist, tracker_from_config, tracker_from_git_remote), and runner's chain includes build (4 methods, not 3). The rule is correct; the ticket narrative undercounted its own hits. Cop implemented as Sdlc/ParameterThreadedThroughPrivateMethods.

Implementation note (2026-07-22): the detection algorithm as specified correctly fires on MORE than the prose enumerates. On issue_source.rb it also flags `project` (threaded through resolve_and_persist, tracker_from_config, tracker_from_git_remote), and `runner`'s chain includes `build` (4 methods, not 3). The rule is correct; the ticket narrative undercounted its own hits. Cop implemented as Sdlc/ParameterThreadedThroughPrivateMethods.

jared referenced this issue

2026-07-23 11:30:54 +00:00

aidd-lint rule: workflow-war-story #100

jared closed this issue

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

jared commented

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

Author

Owner

Covered: Sdlc/Structural/ParameterThreadedThroughPrivateMethods shipped and enabled in .rubocop.yml with tests. ADR-0061 finalizes the os-sdlc lint program; closing per commit 8e88bbe.

Covered: Sdlc/Structural/ParameterThreadedThroughPrivateMethods shipped and enabled in .rubocop.yml with tests. ADR-0061 finalizes the os-sdlc lint program; closing per commit 8e88bbe.

jared referenced this issue

2026-07-27 17:12:40 +00:00

workflow-war-story: #\d+ matches cross-references, not incident history (13 of 15 remaining offenses) #168

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

jared/cc-os#87

No description provided.

Rows
Columns