os-sdlc lint rule: format validation raising in initialize #94

New issue

Closed

opened 2026-07-22 20:16:50 +00:00 by jared · 2 comments

jared commented

2026-07-22 20:16:50 +00:00

Owner

Context

Sandi Metz blind critique: initialize does regexp-format validation of a parameter and
raises inline — primitive obsession, not a self-validating value object.

Problem

plugins/os-sdlc/lib/os_sdlc/workspace.rb:16-21:

@ticket_number = ticket_number.to_s
unless @ticket_number.match?(TICKET_NUMBER)
  raise HarnessError,
        "invalid ticket number #{ticket_number.inspect} -- letters, digits, " \
        "underscores, and hyphens only"
end

Detection

Inputs: the AST of a class's initialize method body and its parameter list.
Algorithm:
1. Collect initialize's parameter names.
2. Map ivars/lvars assigned in the body whose RHS is a parameter, directly or via one
  .to_s (e.g. @ticket_number = ticket_number.to_s) — count as parameter-derived too.
3. Find every if/unless node directly in the body.
4. Flag if a node's condition is (or contains via &&/||) a match?/=~ send whose
  receiver/argument is a parameter or parameter-derived var (one further .to_s
  allowed), AND that node's branch body contains a raise.
5. Out of scope: nil?/is_a?/respond_to? guards raising a plain ArgumentError —
  presence/type, not format validation.
6. Out of scope (deliberate): raise must sit inside the branch. A guard-clause return if <valid> then an unconditional raise doesn't match step 4 (no branch wraps it) — lets
  a value object's own validating initialize (the recommended correction) pass.
Failure message shown to the pipeline model (verbatim):
"initialize validates ticket_number against a format regexp (TICKET_NUMBER) and raises inline -- this is primitive obsession: a plain String is re-validated by hand. Extract a value object (e.g. TicketNumber) that validates its own format in its own initialize and raises there, then call TicketNumber.new(ticket_number).to_s here."

Correction

     def initialize(ticket_number, project, issue_source: IssueSource.for(project))
-      @ticket_number = ticket_number.to_s
-      unless @ticket_number.match?(TICKET_NUMBER)
-        raise HarnessError,
-              "invalid ticket number #{ticket_number.inspect} -- letters, digits, " \
-              "underscores, and hyphens only"
-      end
-
+      @ticket_number = TicketNumber.new(ticket_number).to_s
       @project = project
       ...
     end
+
+    class TicketNumber
+      FORMAT = /\A[a-zA-Z0-9_-]+\z/
+      def initialize(value)
+        @value = value.to_s
+        return if @value.match?(FORMAT)
+
+        raise HarnessError, "invalid ticket number #{value.inspect}"
+      end
+      def to_s = @value
+    end

Workspace#initialize has no if/unless node with a match?/=~ condition and a raise
in its branch — it clears. The rule also inspects TicketNumber#initialize (every
initialize is in scope); that passes too: its guard is return if <valid> then an
unconditional raise, not nested in a branch, so step 4 never matches (guard-clause
exemption, step 6).

Pass/fail examples

Must fail: current Workspace#initialize — @ticket_number = ticket_number.to_s
(step 2), then unless @ticket_number.match?(TICKET_NUMBER) (parameter-derived) wraps
raise HarnessError inline (step 4: raise inside branch).
Must pass: check/raise moved into TicketNumber#initialize (guard-clause shaped, no
wrapped raise); Workspace#initialize only calls TicketNumber.new(...).
Must pass (control case): raise ArgumentError, "project required" if project.nil? —
presence guard, no match?/=~.

Provenance

Sandi Metz blind critique of workspace.rb, cc-os mine-lints session 2026-07-22 (organic
finding — not a council/run finding).

Implementation plan

Custom RuboCop cop under plugins/os-sdlc/lib/os_sdlc/cops/, registered in
plugins/os-sdlc/.rubocop.yml; implemented by a subagent from this ticket per
references/implement-cop.md, with review before merge.

## Context Sandi Metz blind critique: `initialize` does regexp-format validation of a parameter and raises inline — primitive obsession, not a self-validating value object. ### Problem `plugins/os-sdlc/lib/os_sdlc/workspace.rb:16-21`: ```ruby @ticket_number = ticket_number.to_s unless @ticket_number.match?(TICKET_NUMBER) raise HarnessError, "invalid ticket number #{ticket_number.inspect} -- letters, digits, " \ "underscores, and hyphens only" end ``` ### Detection - **Inputs:** the AST of a class's `initialize` method body and its parameter list. - **Algorithm:** 1. Collect `initialize`'s parameter names. 2. Map ivars/lvars assigned in the body whose RHS is a parameter, directly or via one `.to_s` (e.g. `@ticket_number = ticket_number.to_s`) — count as parameter-derived too. 3. Find every `if`/`unless` node directly in the body. 4. Flag if a node's condition is (or contains via `&&`/`||`) a `match?`/`=~` send whose receiver/argument is a parameter or parameter-derived var (one further `.to_s` allowed), AND that node's *branch body* contains a `raise`. 5. Out of scope: `nil?`/`is_a?`/`respond_to?` guards raising a plain `ArgumentError` — presence/type, not format validation. 6. Out of scope (deliberate): raise must sit inside the branch. A guard-clause `return if <valid>` then an unconditional `raise` doesn't match step 4 (no branch wraps it) — lets a value object's own validating `initialize` (the recommended correction) pass. - **Failure message shown to the pipeline model** (verbatim): `"initialize validates ticket_number against a format regexp (TICKET_NUMBER) and raises inline -- this is primitive obsession: a plain String is re-validated by hand. Extract a value object (e.g. TicketNumber) that validates its own format in its own initialize and raises there, then call TicketNumber.new(ticket_number).to_s here."` ### Correction ```diff def initialize(ticket_number, project, issue_source: IssueSource.for(project)) - @ticket_number = ticket_number.to_s - unless @ticket_number.match?(TICKET_NUMBER) - raise HarnessError, - "invalid ticket number #{ticket_number.inspect} -- letters, digits, " \ - "underscores, and hyphens only" - end - + @ticket_number = TicketNumber.new(ticket_number).to_s @project = project ... end + + class TicketNumber + FORMAT = /\A[a-zA-Z0-9_-]+\z/ + def initialize(value) + @value = value.to_s + return if @value.match?(FORMAT) + + raise HarnessError, "invalid ticket number #{value.inspect}" + end + def to_s = @value + end ``` `Workspace#initialize` has no `if`/`unless` node with a `match?`/`=~` condition and a `raise` in its branch — it clears. The rule also inspects `TicketNumber#initialize` (every `initialize` is in scope); that passes too: its guard is `return if <valid>` then an unconditional `raise`, not nested in a branch, so step 4 never matches (guard-clause exemption, step 6). ### Pass/fail examples - **Must fail:** current `Workspace#initialize` — `@ticket_number` = `ticket_number.to_s` (step 2), then `unless @ticket_number.match?(TICKET_NUMBER)` (parameter-derived) wraps `raise HarnessError` inline (step 4: raise inside branch). - **Must pass:** check/raise moved into `TicketNumber#initialize` (guard-clause shaped, no wrapped raise); `Workspace#initialize` only calls `TicketNumber.new(...)`. - **Must pass (control case):** `raise ArgumentError, "project required" if project.nil?` — presence guard, no `match?`/`=~`. ### Provenance Sandi Metz blind critique of workspace.rb, cc-os mine-lints session 2026-07-22 (organic finding — not a council/run finding). ### Implementation plan Custom RuboCop cop under `plugins/os-sdlc/lib/os_sdlc/cops/`, registered in `plugins/os-sdlc/.rubocop.yml`; implemented by a subagent from this ticket per `references/implement-cop.md`, with review before merge.

jared commented

2026-07-22 20:54:09 +00:00

Author

Owner

Implemented as Sdlc/FormatValidationRaisingInInitialize in commit e0f6139 on branch stepper/1-workspace. Unit tests assert the failure message verbatim; the cop fires on the original workspace.rb snippet; full lib/ scan run report-only. Full os-sdlc suite: 88 runs, 162 assertions, 0 failures. Awaiting human review/merge of the branch.

Implemented as Sdlc/FormatValidationRaisingInInitialize in commit e0f6139 on branch stepper/1-workspace. Unit tests assert the failure message verbatim; the cop fires on the original workspace.rb snippet; full lib/ scan run report-only. Full os-sdlc suite: 88 runs, 162 assertions, 0 failures. Awaiting human review/merge of the branch.

jared referenced this issue

2026-07-22 20:54:33 +00:00

os-sdlc: RunNumber#initialize trips Sdlc/FormatValidationRaisingInInitialize #95

jared closed this issue

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

jared commented

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

Author

Owner

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

Covered: Sdlc/Structural/FormatValidationRaisingInInitialize 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-11-09 17:54:51 +00:00

Retire the git-context and os:worktree names after os-git owns them #1631