os-sdlc: POODR/standards cleanup of phase-3 runner code

jared commented

2026-08-11 22:12:47 +00:00

Owner

Context

Post-#341 review (standards axis, opus) found the new runner code under plugins/os-sdlc/lib/os_sdlc/runner/ violates the plugin's own Forbidden Moves and several POODR rules. Behavior is correct (797 tests green); this is a refactor-only ticket. All findings are in code added by commits 651695f..f692b7a.

Findings to fix (each with location)

ADR-narrative comments in lib/ code (os-sdlc/CLAUDE.md Forbidden Move #1 — "the ADRs do their own talking"): engine.rb:4-6, engine_factory.rb:6-10, ticket_flow.rb:20-21 and :33-36, transition_table.rb:59-60, plus the "Holds X as instance state so it isn't threaded through parameter lists" refactor-rationale sentence copy-pasted into ~10 class comments (brief_assembler, event_handler, gate_evaluator, gate_failure_flow, stage_brief_writer, transition_applier, row_builder, row_validator, terminal_reachability, cycle_checker). Delete them all from lib/; ADR-citing headers in TEST files are accepted house style and stay. Also delete the false comment on Engine#handle_event claiming a parked ticket "never returns nil" (the parking event itself returns nil).
Hardcoded collaborators → constructor injection (POODR ch.3): TransitionTable#reaches_terminal_via_success? news TerminalReachability per call; GateFailureFlow#retry_ledger news RetryLedger per call; Resumer/StageBriefWriter inline BriefAssembler.new. Inject with sensible defaults (def initialize(..., ledger: RetryLedger.new(db))-style) so tests can substitute.
TransitionTable ↔ RowValidator cycle: validate! builds RowValidator.new(row:, table: self) which calls back into table.known_state?/row_lookup/reaches_terminal_via_success?. Pass the validator the data (state set, lookup, reachability checker) instead of the whole table.
on_failure Primitive Obsession: the {route:, retries:, exhausted:} Hash is poked in RetryLedger, GateFailureFlow, RowValidator, TransitionTable#edges_from. Introduce a small OnFailure value type (Data.define) with route/retries/exhausted readers; construct it in RowBuilder.
Dead public surface (Speculative Generality): Brief.read, BriefAssembler#read, TransitionTable#skippable?, attr_reader :entry_state, Engine#parked_info have no production caller. Delete each unless a test legitimately depends on it as the observable seam (e.g. entry_state is asserted in map_loader_test — keep anything a spec-level test observes; delete the rest).
Engine.for(**options) + options.fetch(:table): restore an explicit keyword signature.
db.rb Shotgun Surgery: map_name declared in both add_ticket_columns and ensure_column!(ColumnSpec.new(:tickets, :map_name, String)) (db.rb:44,73) — one declaration.
bin/os-sdlc-runner:37-47: extract_flag's bare second args.delete_at(idx) as the return value is unreadable; make the return explicit, and fold the extract_map_flag middle-man.

NOTE: the shared-brief-writer duplication (resumer.rb:56-83 / stage_brief_writer.rb:14-33) is deliberately EXCLUDED — it is fixed by the brief-seq ticket filed alongside this one. Skip it here to avoid a collision; if that ticket lands first, rebase on its extraction.

Scope

IN: the items above, refactor-only, under lib/os_sdlc/runner/ and bin/os-sdlc-runner; test updates only where a constructor signature changes.
OUT: any behavior change (green suite is the invariant), the #342 adapter work (NullTracker/NullNotifier, SkeletonTable .equal? branches — already tracked there), Repeated-Switches row.kind polymorphism (judgement call — leave unless it falls out naturally of item 4), GateEvaluator CQS restructuring (flag if trivial, else leave).

Acceptance criteria

Zero narrative/rationale comments in lib/os_sdlc/runner/ (verifiable: grep -rn "so .* isn't threaded\|legacy\|per ADR\|ADR-01" plugins/os-sdlc/lib/ returns nothing that is narrative — test files exempt).
ast-grep run --lang ruby -p '$C.new($$$A)' over lib/os_sdlc/runner shows no collaborator construction inside public method bodies of TransitionTable/GateFailureFlow/Resumer/StageBriefWriter (defaults in initialize are fine).
RowValidator no longer receives table: self.
on_failure[:route]-style Hash access is gone; all readers go through the OnFailure type.
Full suite green with NO assertion changes except constructor-signature fallout; rubocop clean with no new cop disables.

## Context Post-#341 review (standards axis, opus) found the new runner code under `plugins/os-sdlc/lib/os_sdlc/runner/` violates the plugin's own Forbidden Moves and several POODR rules. Behavior is correct (797 tests green); this is a refactor-only ticket. All findings are in code added by commits 651695f..f692b7a. ## Findings to fix (each with location) 1. **ADR-narrative comments in lib/ code** (os-sdlc/CLAUDE.md Forbidden Move #1 — "the ADRs do their own talking"): `engine.rb:4-6`, `engine_factory.rb:6-10`, `ticket_flow.rb:20-21` and `:33-36`, `transition_table.rb:59-60`, plus the "Holds X as instance state so it isn't threaded through parameter lists" refactor-rationale sentence copy-pasted into ~10 class comments (brief_assembler, event_handler, gate_evaluator, gate_failure_flow, stage_brief_writer, transition_applier, row_builder, row_validator, terminal_reachability, cycle_checker). Delete them all from lib/; ADR-citing headers in TEST files are accepted house style and stay. Also delete the false comment on `Engine#handle_event` claiming a parked ticket "never returns nil" (the parking event itself returns nil). 2. **Hardcoded collaborators → constructor injection** (POODR ch.3): `TransitionTable#reaches_terminal_via_success?` news `TerminalReachability` per call; `GateFailureFlow#retry_ledger` news `RetryLedger` per call; `Resumer`/`StageBriefWriter` inline `BriefAssembler.new`. Inject with sensible defaults (`def initialize(..., ledger: RetryLedger.new(db))`-style) so tests can substitute. 3. **TransitionTable ↔ RowValidator cycle**: `validate!` builds `RowValidator.new(row:, table: self)` which calls back into `table.known_state?`/`row_lookup`/`reaches_terminal_via_success?`. Pass the validator the data (state set, lookup, reachability checker) instead of the whole table. 4. **`on_failure` Primitive Obsession**: the `{route:, retries:, exhausted:}` Hash is poked in `RetryLedger`, `GateFailureFlow`, `RowValidator`, `TransitionTable#edges_from`. Introduce a small `OnFailure` value type (Data.define) with `route`/`retries`/`exhausted` readers; construct it in `RowBuilder`. 5. **Dead public surface** (Speculative Generality): `Brief.read`, `BriefAssembler#read`, `TransitionTable#skippable?`, `attr_reader :entry_state`, `Engine#parked_info` have no production caller. Delete each unless a test legitimately depends on it as the observable seam (e.g. `entry_state` is asserted in map_loader_test — keep anything a spec-level test observes; delete the rest). 6. **`Engine.for(**options)` + `options.fetch(:table)`**: restore an explicit keyword signature. 7. **db.rb Shotgun Surgery**: `map_name` declared in both `add_ticket_columns` and `ensure_column!(ColumnSpec.new(:tickets, :map_name, String))` (`db.rb:44,73`) — one declaration. 8. **bin/os-sdlc-runner:37-47**: `extract_flag`'s bare second `args.delete_at(idx)` as the return value is unreadable; make the return explicit, and fold the `extract_map_flag` middle-man. NOTE: the shared-brief-writer duplication (resumer.rb:56-83 / stage_brief_writer.rb:14-33) is deliberately EXCLUDED — it is fixed by the brief-seq ticket filed alongside this one. Skip it here to avoid a collision; if that ticket lands first, rebase on its extraction. ## Scope - IN: the items above, refactor-only, under lib/os_sdlc/runner/ and bin/os-sdlc-runner; test updates only where a constructor signature changes. - OUT: any behavior change (green suite is the invariant), the #342 adapter work (NullTracker/NullNotifier, SkeletonTable `.equal?` branches — already tracked there), Repeated-Switches `row.kind` polymorphism (judgement call — leave unless it falls out naturally of item 4), GateEvaluator CQS restructuring (flag if trivial, else leave). ## Acceptance criteria 1. Zero narrative/rationale comments in lib/os_sdlc/runner/ (verifiable: `grep -rn "so .* isn't threaded\|legacy\|per ADR\|ADR-01" plugins/os-sdlc/lib/` returns nothing that is narrative — test files exempt). 2. `ast-grep run --lang ruby -p '$C.new($$$A)'` over lib/os_sdlc/runner shows no collaborator construction inside public method bodies of TransitionTable/GateFailureFlow/Resumer/StageBriefWriter (defaults in `initialize` are fine). 3. `RowValidator` no longer receives `table: self`. 4. `on_failure[:route]`-style Hash access is gone; all readers go through the OnFailure type. 5. Full suite green with NO assertion changes except constructor-signature fallout; rubocop clean with no new cop disables.

jared added the

P2

label

2026-08-11 22:12:47 +00:00

jared commented

2026-08-11 23:16:24 +00:00

Author

Owner

Shipped in e988002: narrative comments stripped, collaborators injected, OnFailure value type, RowValidator decoupled from table, dead surface removed (Brief.read kept — house cop seam). 799 runs green, rubocop clean.

Shipped in e988002: narrative comments stripped, collaborators injected, OnFailure value type, RowValidator decoupled from table, dead surface removed (Brief.read kept — house cop seam). 799 runs green, rubocop clean.

jared added the

review

label

2026-08-11 23:16:28 +00:00

jared commented

2026-08-12 14:02:13 +00:00

Author

Owner

Restored coverage for escalation clearing on resume (retry/abort/skip) lost in e988002's cleanup, via Escalation.fetch returning nil post-deactivate. Commit e6523b6. GateFailureFlow's guarded retry-counter reset checked and confirmed live (it doesn't route through TransitionApplier), so left as-is.

Restored coverage for escalation clearing on resume (retry/abort/skip) lost in e988002's cleanup, via Escalation.fetch returning nil post-deactivate. Commit e6523b6. GateFailureFlow's guarded retry-counter reset checked and confirmed live (it doesn't route through TransitionApplier), so left as-is.

jared removed the

review

label

2026-08-12 14:24:14 +00:00

jared commented

2026-08-12 14:24:15 +00:00

Author

Owner

Resolution

Done: POODR cleanup: narrative comments stripped, collaborators injected, OnFailure value type introduced, RowValidator decoupled from table, dead public surface removed (Brief.read retained as house seam). Escalation clearing on resume restored. 799 tests green, rubocop clean.

Evidence: e988002, e6523b6

Follow-ups: none

Approved-by: jared

## Resolution **Done:** POODR cleanup: narrative comments stripped, collaborators injected, OnFailure value type introduced, RowValidator decoupled from table, dead public surface removed (Brief.read retained as house seam). Escalation clearing on resume restored. 799 tests green, rubocop clean. **Evidence:** e988002, e6523b6 **Follow-ups:** none Approved-by: jared

jared closed this issue

2026-08-12 14:24:15 +00:00

jared referenced this issue from a commit

2026-08-13 15:08:12 +00:00

refactor(os-sdlc): POODR cleanup — inject collaborators, OnFailure value type, drop narrative comments and dead surface (#347)

jared referenced this issue from a commit

2026-08-13 15:08:12 +00:00

test(os-sdlc): re-assert escalation clearing on resume (#347 coverage gap)