refactor: architecture refactor with dependency injection#22
Merged
garrettdimon merged 53 commits intomainfrom Feb 3, 2026
Merged
refactor: architecture refactor with dependency injection#22garrettdimon merged 53 commits intomainfrom
garrettdimon merged 53 commits intomainfrom
Conversation
Add history: keyword param to Tool#initialize with default Reviewer.history. Replace all 4 Reviewer.history calls with @history instance variable. This is the first step toward full dependency injection — callers can now provide their own history store.
Move 5 timing methods (last_prepared_at, last_prepared_at=, average_time, get_timing, record_timing) and stale? logic into Tool::Timing. Tool delegates via Forwardable. Timing receives history and key through constructor — no global access.
Reviewer::Conversions → Reviewer::Tool::Conversions since the module only converts to Tool. Delete old lib/reviewer/conversions.rb. Remove unused Conversions include from Command::String. Reorder requires in reviewer.rb so Tool loads before Command.
Add arguments: and history: keyword params to Command#initialize with defaults to Reviewer globals. Replace all Reviewer.arguments (3 calls) and Reviewer.history (3 calls) with instance variables.
Add arguments: and history: keyword params to Tools#initialize. Replace Reviewer.arguments (3 calls) and Reviewer.history (1 call) with instance variables.
Add arguments: keyword param to Runner#initialize. Pass arguments through to Command. Replace Reviewer.arguments.streaming? with the injected arguments instance.
Reviewer::Guidance → Reviewer::Runner::Guidance since it is only used by Runner. Delete old lib/reviewer/guidance.rb.
Reviewer::FailedFiles → Reviewer::Runner::FailedFiles since it only parses runner output. Update reference in Batch. Delete old file.
Move result-building logic (skipped_result, missing_result, executed_result) from Runner into Runner::ResultBuilder. Runner's to_result delegates to the builder, which reads tool, command, and shell state to construct immutable Result objects.
…ents Add keyword params to Batch#initialize. Strategy is determined at init time from arguments (or injected directly). Replace all Reviewer.history (5 calls) and Reviewer.arguments (1 call) with instance variables. Pass output and arguments through to Runner.
New module provides format_duration, status_mark, status_style, pluralize, CHECKMARK, and XMARK. Will be included by all domain formatters to share common display patterns.
Create domain-specific formatters that own their display logic: - Runner::Formatter (tool_summary, success, failure, skipped, etc.) - Batch::Formatter (batch_summary, run_summary, missing_tools, etc.) - Doctor::Formatter (diagnostic report rendering) - Setup::Formatter (first-run setup messages) - Session::Formatter (lifecycle warnings) Output becomes a thin facade delegating to domain formatters while retaining primitives (clear, newline, divider, help, unfiltered). Report::Formatter now uses shared Output::Formatting module. Fold Token class into Printer as pre-computed STYLES constant. Delete Output::Doctor mixin, Output::Setup mixin, and Token class.
Tests that modify Reviewer.configuration now save/restore inline. Duplicated lifecycle tests removed (covered by session_test.rb). test_helper simplified to one-time boot configuration.
Handle --capabilities at Reviewer module level (like init/doctor) instead of inside Arguments' Slop parser. This avoids the circular dependency: Arguments.new → Capabilities.new → Reviewer.tools → Reviewer.arguments → Arguments.new. Capabilities now accepts injected tools parameter for testability.
Structural fixes to eliminate code smells: simplify ToolBlock#quote, extract Tool#matches_tags?, refactor Report::Formatter#print_truncated_output, destructure Doctor::Formatter#print_finding, rename Batch#execute_tool to run_tool, and extract Command#run_summary. Configure .reek.yml thresholds for DI constructors instead of suppressing individual classes. Add RuboCop CountKeywordArgs: false for parameter lists. Add YARD documentation across all public APIs to reach zero inch warnings.
Replace ~30 .reek.yml exclusions with idiomatic Ruby fixes: - NilCheck (14): .to_s, truthiness, guard clauses instead of .nil? - Attribute (4): attr_accessor to attr_reader where setters unused - BooleanParameter (6): move clear_screen from Session to exe/ entry points - ControlParameter (1): default lambda in Files#initialize parameter - DuplicateMethodCall (3): cache repeated accessor calls to locals - TooManyStatements (1): restore exclusion after failed extraction Remaining ~57 exclusions are structural: DI constructors, writable config attributes, and clear linear orchestration methods.
Cover previously untested branches: - Session: unrecognized keywords, JSON empty tools, failed-with- no-previous-run vs no-failures-to-retry, streaming failure path - Arguments::Files: unstaged, modified, untracked git keywords - Prompt: EOF/empty input returns false - Runner::Result: executed?, detail_summary for all tool types Branch coverage: 87.76% -> 88.83% (350/394) Line coverage: 99.39% -> 99.54% (4786/4808)
Session, Setup, Arguments, and Reviewer module now construct and call domain formatters (Session::Formatter, Setup::Formatter, Doctor::Formatter) directly instead of going through Output delegation methods. Output is now a pure primitives class with 6 public methods.
Keywords now accepts tools: parameter instead of privately calling Reviewer.tools. Default preserves backward compatibility.
Move prompt and configuration to review/format method parameters since they're only used in the first-run setup path.
Eliminates the DataClump warning by having the Reviewer entry point handle the first-run configuration check directly, keeping Session focused on running tools.
Bundle arguments, output, and history into a single Context that flows through Session → Batch → Runner → Command. Defaults to Reviewer globals so entry points create it once and pass it downstream.
Move build logic into Result as a factory method with private helpers (build_skipped, build_missing, build_executed). Eliminates single-purpose builder class in favor of a method on the value object itself.
Replace separate arguments/output/history parameters with single context: parameter across Command, Runner, Batch, Session, and Guidance. Each class receives Context and extracts needed dependencies internally.
Remove on_git_error lambda from Arguments::Files, inline the error handler directly. Update .reek.yml to remove stale ResultBuilder and reduced-ivar exclusions after Context refactor.
Removes the last Reviewer.output reference in application code (outside of Context defaults and Setup entry point).
Removes hardcoded tool_key matching from Result#detail_summary. Summary patterns are now configured per-tool in .reviewer.yml and carried through the Result struct.
Removes 9 Struct stubs (EmptyFiles, StubKeywords, StubKeywordsWithProvided, StubKeywordsFailed, StubArgs, StubArgsWithKeywords, StubArgsWithFailed, StubKeywordsUnrecognized, StubArgsWithUnrecognized) and replaces them with real Arguments.new(...) calls.
Centralizes Context construction in tests. Replaces bare Context.new calls in 6 test files with default_context, and removes duplicate context_with helpers from runner_test and command_test.
- Loader requires file: keyword arg (no global config access) - Tools accepts config_file: and passes it through to Loader - Tools computes matching_tags from its own data (keyword → tag) - Arguments wires keywords.reserved into Files constructor - Files uses on_git_error: callback instead of Session::Formatter 596 tests, 0 Reek, 0 RuboCop, 99.71% line / 92.62% branch. No singleton references remain outside lib/reviewer.rb.
There was a problem hiding this comment.
Pull request overview
Large refactor to make Reviewer’s runtime architecture more modular and testable by introducing dependency injection, extracting domain-specific formatter classes, and centralizing lifecycle orchestration in a Session object.
Changes:
- Introduce
ContextandSessionto own review/format lifecycles with explicit dependency injection (arguments/output/history). - Extract domain-specific formatters (Runner/Batch/Setup/Doctor/Session) and move related concerns under their namespaces.
- Update tests to remove reliance on global/module state and add coverage for new components (timing, formatting, failed-file extraction, etc.).
Reviewed changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_helper.rb | Centralizes test configuration setup for predictable fixture-based runs |
| test/reviewer_test.rb | Updates top-level CLI behavior tests for new Session/DI lifecycle |
| test/reviewer/tools_test.rb | Adapts tool selection/history tests to injected dependencies |
| test/reviewer/tool_test.rb | Adds tag matching tests for new Tool#matches_tags? behavior |
| test/reviewer/tool/timing_test.rb | Adds coverage for new Tool::Timing persistence/logic |
| test/reviewer/setup_test.rb | Updates setup tests to restore configuration after temp file edits |
| test/reviewer/session_test.rb | Adds comprehensive Session lifecycle coverage (formats, guidance, edge cases) |
| test/reviewer/runner_test.rb | Updates runner tests for required Context + adds failed_files tests |
| test/reviewer/runner/strategies/passthrough_test.rb | Updates runner creation to pass Context |
| test/reviewer/runner/strategies/captured_test.rb | Updates runner creation to pass Context |
| test/reviewer/runner/result_test.rb | Adds coverage for Result factories and new helpers (executed?/detail_summary) |
| test/reviewer/runner/formatter_test.rb | Adds coverage for new Runner::Formatter output behavior |
| test/reviewer/report_test.rb | Adds coverage for Report#missing_tools |
| test/reviewer/prompt_test.rb | Adds EOF handling test for Prompt |
| test/reviewer/output_test.rb | Shrinks Output tests to primitives; moves domain output tests to formatters |
| test/reviewer/output/printer_test.rb | Updates tests for new Printer implementation and style map constant |
| test/reviewer/output/formatting_test.rb | Adds tests for shared Output::Formatting helpers |
| test/reviewer/output/doctor_test.rb | Updates doctor output tests to use Doctor::Formatter |
| test/reviewer/loader_test.rb | Moves Loader tests under Configuration namespace and updates error expectations |
| test/reviewer/keywords/git_test.rb | Removes standalone Git keyword tests (logic moved/rewired) |
| test/reviewer/history_test.rb | Updates API usage from clear! to clear |
| test/reviewer/guidance_test.rb | Updates Guidance tests to new Runner::Guidance + Context injection |
| test/reviewer/failed_files_test.rb | Moves FailedFiles tests under Runner namespace and expands rejection cases |
| test/reviewer/doctor_test.rb | Updates doctor tests to restore configuration via helper |
| test/reviewer/doctor/opportunity_check_test.rb | Updates config mutation to use Reviewer.configuration directly |
| test/reviewer/doctor/config_check_test.rb | Updates temp config helper to restore Reviewer.configuration.file |
| test/reviewer/conversions_test.rb | Updates to include Tool::Conversions instead of removed module |
| test/reviewer/context_test.rb | Adds tests for new Context defaults and accessors |
| test/reviewer/configuration_test.rb | Refactors config tests to use Configuration instance (less global mutation) |
| test/reviewer/command_test.rb | Updates Command tests for Context injection + adds run_summary behavior tests |
| test/reviewer/batch_test.rb | Updates Batch tests for Context injection and history usage |
| test/reviewer/batch/formatter_test.rb | Adds coverage for new Batch::Formatter output behavior |
| test/reviewer/arguments_test.rb | Updates capabilities behavior test and adds runner_strategy unit tests |
| test/reviewer/arguments/keywords_test.rb | Adds coverage for injected tools in keyword recognition |
| test/reviewer/arguments/files_test.rb | Replaces Git stubbing with Open3 stubs and adds coverage for more git keywords |
| lib/reviewer/tools.rb | Adds DI for arguments/history and updates filtering/tag logic |
| lib/reviewer/tool/timing.rb | Adds new Tool::Timing class for prepare timestamps and timing history |
| lib/reviewer/tool/settings.rb | Documentation improvements and equality semantics commentary |
| lib/reviewer/tool/conversions.rb | Adds Tool-scoped conversion helpers (replacing removed module) |
| lib/reviewer/tool.rb | Refactors timing logic into Tool::Timing and adds matches_tags? |
| lib/reviewer/shell/timer.rb | Tightens Timer immutability and updates docs + prepped? semantics |
| lib/reviewer/shell/result.rb | Makes stdout/stderr/status read-only (except exit_status) |
| lib/reviewer/setup/tool_block.rb | Extracts tool YAML rendering into a dedicated class |
| lib/reviewer/setup/generator.rb | Delegates per-tool YAML to ToolBlock; keeps js runner orchestration |
| lib/reviewer/setup/gemfile_lock.rb | Doc improvements |
| lib/reviewer/setup/formatter.rb | Adds Setup::Formatter (moves setup output out of Output) |
| lib/reviewer/setup/detector.rb | Improves Result documentation and detector docs |
| lib/reviewer/setup.rb | Injects output and uses Setup::Formatter for messages |
| lib/reviewer/session/formatter.rb | Adds Session::Formatter for lifecycle warnings and formatting errors |
| lib/reviewer/session.rb | Adds Session lifecycle orchestrator for review/format with DI |
| lib/reviewer/runner/strategies/passthrough.rb | Routes output through Runner::Formatter rather than Output |
| lib/reviewer/runner/strategies/captured.rb | Refines progress bar logic and routes output through Runner::Formatter |
| lib/reviewer/runner/result.rb | Adds Result.from_runner factories and detail_summary helper |
| lib/reviewer/runner/guidance.rb | Moves Guidance under Runner namespace and uses Runner::Formatter |
| lib/reviewer/runner/formatter.rb | Adds Runner::Formatter (tool summary, success/failure, guidance, etc.) |
| lib/reviewer/runner/failed_files.rb | Moves FailedFiles under Runner namespace and documents regex behavior |
| lib/reviewer/runner.rb | Requires new runner components; enforces Context injection; stream-guarded output |
| lib/reviewer/report/formatter.rb | Uses shared Output::Formatting + Result.detail_summary |
| lib/reviewer/report.rb | Adds missing_tools convenience accessor |
| lib/reviewer/prompt.rb | Doc improvements |
| lib/reviewer/output/token.rb | Removes Token in favor of Printer styling constants |
| lib/reviewer/output/setup.rb | Removes setup output module (moved to Setup::Formatter) |
| lib/reviewer/output/printer.rb | Reworks styling as constants + adds write_raw |
| lib/reviewer/output/formatting.rb | Adds shared formatting helpers/constants for all formatters |
| lib/reviewer/output/doctor.rb | Removes doctor output module (moved to Doctor::Formatter) |
| lib/reviewer/output.rb | Reduces Output to primitives + scrub/help/unfiltered |
| lib/reviewer/loader.rb | Removes old top-level Loader (replaced by Configuration::Loader) |
| lib/reviewer/keywords/git.rb | Removes old Git keyword module (moved into Arguments::Files via Open3) |
| lib/reviewer/keywords.rb | Removes old keywords namespace file |
| lib/reviewer/history.rb | Refactors store access and renames clear! to clear |
| lib/reviewer/guidance.rb | Removes old top-level Guidance (replaced by Runner::Guidance) |
| lib/reviewer/failed_files.rb | Removes old top-level FailedFiles (replaced by Runner::FailedFiles) |
| lib/reviewer/doctor/tool_inventory.rb | Adds DI for configuration/tools usage |
| lib/reviewer/doctor/report.rb | Doc improvements for Finding |
| lib/reviewer/doctor/opportunity_check.rb | Adds DI for configuration/tools usage |
| lib/reviewer/doctor/formatter.rb | Adds Doctor::Formatter (moves doctor output out of Output) |
| lib/reviewer/doctor/environment_check.rb | Doc improvements |
| lib/reviewer/doctor/config_check.rb | Uses Configuration::Loader and supports injected configuration |
| lib/reviewer/doctor.rb | Requires new Doctor::Formatter |
| lib/reviewer/conversions.rb | Removes old conversions module (moved under Tool) |
| lib/reviewer/context.rb | Adds new Context dependency container |
| lib/reviewer/configuration/loader.rb | Adds new Configuration::Loader replacing old Loader |
| lib/reviewer/configuration.rb | Makes printer read-only (accessor changes) |
| lib/reviewer/command/string.rb | Removes include of removed Conversions module |
| lib/reviewer/command.rb | Adds Context injection; removes globals for args/history; adds run_summary |
| lib/reviewer/capabilities.rb | Adds tools injection for capability reporting |
| lib/reviewer/batch/formatter.rb | Adds Batch::Formatter (moves batch display out of Output) |
| lib/reviewer/batch.rb | Adds Context injection; uses Command/Runner via Context; stores failed files via Runner |
| lib/reviewer/arguments/tags.rb | Makes provided/keywords read-only; doc improvements |
| lib/reviewer/arguments/keywords.rb | Adds injected tools; makes provided read-only |
| lib/reviewer/arguments/files.rb | Inlines git calls via Open3 and routes errors through Session::Formatter |
| lib/reviewer/arguments.rb | Supports output injection; moves capabilities handling to Reviewer; adds runner_strategy |
| lib/reviewer.rb | Switches to Session-based lifecycle and new formatters; adds capabilities handling |
| exe/rvw | Changes CLI entry to clear + run Reviewer.review without params |
| exe/fmt | Changes CLI entry to clear + run Reviewer.format without params |
| .rubocop.yml | Tweaks parameter list metric to ignore keyword args in counting |
| .reviewer.yml | Comments out multiple skip_in_batch entries (behavior change) |
| .reek.yml | Reworks reek configuration toward targeted, documented exclusions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix YARD placeholder [type] → Array<String> on Tools#tool_names - Guard Printer#initialize sync= for non-IO streams - Skip terminal clear when stdout is not a TTY (fixes piped/JSON output) - Fix json_output? to check arguments.format instead of json? flag so --format json routes through the JSON path
Tests: - Printer accepts StringIO streams without raising - Output#clear is a no-op when not a TTY - Tools#all passes injected history to Tool instances TTY standardization: - Move TTY guard into Output#clear (printer.tty?) so callers don't need to check — same pattern as Printer#style_enabled? - Simplify exe/rvw and exe/fmt back to plain Reviewer.output.clear Documentation fixes: - Remove duplicate comment block in Context - Fix @param type for Arguments#initialize (Array, not Hash) - Fix "an colelction/an collection" → "a collection" (3 occurrences) - Fix "Proves" → "Provides" in Keywords#to_a - Fix "file arguments" → "keyword arguments" in Keywords#to_s - Fix "can recognized" → "can recognize" in Keywords#possible - Fix "instace" → "instance" in Tags - Fix "it's prepration" → "its preparation" in Tool#prepare? - Fix "tag values" → "file values" in Files#to_s - Add missing @param docs for Files (output, on_git_error) - Add missing @param config_file to Tools#initialize
Shell, Runner, and Captured strategy all bypassed the Output/Printer infrastructure by writing directly to $stdout and embedding raw ANSI escape codes without TTY checks. This wires the printer's stream through Shell (via DI) and Runner, and guards all 256-color ANSI codes in Captured behind style_enabled? so non-TTY environments get clean output.
…p bar erase - Session exits early with message when file keywords (staged, modified) resolve to no files, instead of silently running zero tools - Session::Formatter#no_reviewable_files displays the scoping message - Doctor::KeywordCheck warns when tool names or tags shadow reserved keywords or collide with each other - Keywords#tools= wires the tools collection post-construction so Session can break the circular dependency - failed_with_nothing_to_run? also checks for_tags to avoid short-circuiting when tags are present alongside `failed` - Captured strategy erases the prep progress bar before starting the run bar, preventing stale output in TTY mode - Runner::Formatter#current_command adds trailing newline for spacing
Session#run_tools now branches on JSON/text format before checking early exit conditions. When --json is requested with `failed` (nothing to retry) or file-scoping keywords (no matching files), the response is a valid JSON object with success, message, summary, and tools fields instead of plain text that breaks JSON consumers. Refactored keyword_check_test and formatter_test to address reek warnings structurally rather than suppressing them: - Extract memoized `configuration` accessor to eliminate duplicate Reviewer.configuration calls and give helpers instance state - Extract `with_config_file` to share config swap/restore pattern - Convert `warnings_matching` to `assert_single_conflict_warning` backed by memoized `conflicts_report`, so helpers use self - Replace setup ivars with memoized `formatter` method in formatter_test to resolve InstanceVariableAssumption
Slop's on-parse handlers used `output.help(...) && exit` which short-circuited because help() returns nil. Moved flag handling out of Slop blocks into explicit conditionals in review/format with help? and version? predicates on Arguments.
Combine tool name, key, and command summary into a single message line instead of splitting across message and detail fields.
Pad tool names to uniform width and right-justify durations so columns line up across tools with different name lengths.
Enables running focused test files when specific files are passed via the --files flag instead of always running the full suite.
- Replace bare Slop help with structured --help showing commands, keywords, flags, and examples. Options section delegates to Slop so flag changes stay in sync automatically. - Remove terminal screen clear from exe/rvw and exe/fmt - Expand gemspec description to explain what the gem does - Document exit codes in README CI section - Replace `disabled` with `skip_in_batch` in README options reference - Rewrite CHANGELOG [Unreleased] to reflect full 1.0 scope - Extract shared guard clauses into handle_early_exits - Fix test ordering bug: config-swapping tests now clear memoized tools to prevent stale config paths from leaking across tests - Extract with_swapped_config test helper to DRY up 5 test files
Same root cause as the earlier test ordering fix—swapping the config file without clearing @tools left stale references to temp directories. Use the shared with_swapped_config helper instead. Fixes CI failures on Ruby 3.2 and 3.4.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
bundle exec ./exe/rvw— all tools passbundle exec ./exe/fmt— formatter passes