From 75917e0e3017086687a1d268d87e803a540de675 Mon Sep 17 00:00:00 2001 From: Moyom96 Date: Mon, 22 Jun 2026 09:21:42 -0600 Subject: [PATCH 1/3] feat: add pr-delta-workflow --- Gemfile.lock | 24 +++ README.md | 33 ++++ exe/wall-e | 25 ++- .../wall_e/install/install_generator.rb | 7 +- .../install/templates/wall_e_pr_delta.yml | 29 ++++ .../install/templates/wall_e_settings.yml | 7 + .../collectors/complexity_collector.rb | 11 +- lib/tech_debt/collectors/debride_collector.rb | 5 +- lib/tech_debt/collectors/flay_collector.rb | 4 +- lib/tech_debt/collectors/layer_collector.rb | 15 +- lib/tech_debt/config.rb | 7 + lib/tech_debt/delta/pr_delta.rb | 162 ++++++++++++++++++ lib/tech_debt/github/client.rb | 39 +++++ lib/tech_debt/github/comment_upserter.rb | 33 ++++ lib/tech_debt/github/issue_manager.rb | 8 +- lib/tech_debt/github/pull_request.rb | 64 +++++++ lib/tech_debt/verification/pr_verifier.rb | 33 +--- lib/tech_debt/verification/result_reporter.rb | 12 +- .../collectors/complexity_collector_spec.rb | 43 +++++ .../collectors/debride_collector_spec.rb | 35 ++++ .../collectors/flay_collector_spec.rb | 5 + .../collectors/layer_collector_spec.rb | 31 ++++ spec/tech_debt/delta/pr_delta_spec.rb | 111 ++++++++++++ spec/tech_debt/github/client_spec.rb | 64 +++++++ .../tech_debt/github/comment_upserter_spec.rb | 39 +++++ spec/tech_debt/github/pull_request_spec.rb | 67 ++++++++ 26 files changed, 871 insertions(+), 42 deletions(-) create mode 100644 lib/generators/wall_e/install/templates/wall_e_pr_delta.yml create mode 100644 lib/tech_debt/delta/pr_delta.rb create mode 100644 lib/tech_debt/github/client.rb create mode 100644 lib/tech_debt/github/comment_upserter.rb create mode 100644 lib/tech_debt/github/pull_request.rb create mode 100644 spec/tech_debt/collectors/complexity_collector_spec.rb create mode 100644 spec/tech_debt/collectors/debride_collector_spec.rb create mode 100644 spec/tech_debt/collectors/layer_collector_spec.rb create mode 100644 spec/tech_debt/delta/pr_delta_spec.rb create mode 100644 spec/tech_debt/github/client_spec.rb create mode 100644 spec/tech_debt/github/comment_upserter_spec.rb create mode 100644 spec/tech_debt/github/pull_request_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index 78ffb11..42ee0d8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -4,6 +4,7 @@ PATH wall-e (0.1.0) debride (~> 1.12) faraday-retry + flay (~> 2.13) flog (~> 4.8) octokit (~> 9.0) railties (>= 7.0) @@ -55,6 +56,7 @@ GEM path_expander (~> 2.0) prism (~> 1.7) sexp_processor (~> 4.17) + diff-lcs (1.6.2) drb (2.2.3) erb (6.0.2) erubi (1.13.1) @@ -69,6 +71,11 @@ GEM net-http (~> 0.5) faraday-retry (2.4.0) faraday (~> 2.0) + flay (2.14.4) + erubi (~> 1.10) + path_expander (~> 2.0) + prism (~> 1.7) + sexp_processor (~> 4.0) flog (4.9.4) path_expander (~> 2.0) prism (~> 1.7) @@ -94,6 +101,8 @@ GEM multipart-post (2.4.1) net-http (0.9.1) uri (>= 0.11.1) + nokogiri (1.19.1-arm64-darwin) + racc (~> 1.4) nokogiri (1.19.1-x86_64-darwin) racc (~> 1.4) octokit (9.2.0) @@ -146,6 +155,19 @@ GEM regexp_parser (2.11.3) reline (0.6.3) io-console (~> 0.5) + rspec (3.13.2) + rspec-core (~> 3.13.0) + rspec-expectations (~> 3.13.0) + rspec-mocks (~> 3.13.0) + rspec-core (3.13.6) + rspec-support (~> 3.13.0) + rspec-expectations (3.13.5) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-mocks (3.13.8) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-support (3.13.7) rubocop (1.84.2) json (~> 2.3) language_server-protocol (~> 3.17.0.2) @@ -199,10 +221,12 @@ GEM zeitwerk (2.7.5) PLATFORMS + arm64-darwin-23 x86_64-darwin DEPENDENCIES rake + rspec standard wall-e! diff --git a/README.md b/README.md index a0d65bc..ebdec84 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ Running the install generator adds these project files: | -------------------------------------- | -------------------------------------------- | | `.github/workflows/wall_e_scan.yml` | Scheduled/manual scan | | `.github/workflows/wall_e_verify.yml` | Optional PR verification on `pull_request` | +| `.github/workflows/wall_e_pr_delta.yml` | Per-PR debt delta comment on `pull_request` | | `config/wall_e_settings.yml` | Scanner, LLM, GitHub, auto-assign, verification | | `.github/prompts/wall_e_analysis.md` | System prompt for semantic triage | | `.github/prompts/wall_e_issue_writer.md` | Second-pass prompt: criteria + baselines | @@ -191,6 +192,35 @@ verification: Install the **`wall_e_verify`** workflow (via `rails g wall_e:install`) to run verification on `pull_request` events. If no linked issues contain wall-e verification metadata, the run exits early with minimal work. +### Per-PR debt delta (`--pr-delta`) + +Comment on a pull request with the debt it **adds on its changed lines**. This is a fast, static-only feedback loop that runs on every PR, complementary to the scheduled scan (which files tracked issues) and `--verify-pr` (which checks fixes for existing issues). + +```sh +bundle exec wall-e --pr-delta 42 +bundle exec wall-e --pr-delta 42 --dry-run +``` + +How it works: + +1. Fetches the PR's changed Ruby files and diffs. +2. Runs the static collectors (`debride`, `flog`, `flay`, layer checks) scoped to those files. +3. Keeps only findings whose line range overlaps the lines the PR actually added. +4. Posts (or **updates**) a single summary comment marked with ``, so repeated pushes edit one comment instead of stacking duplicates. + +No LLM is called and no issues are created. The comment is informational unless you opt into gating. + +Configure in `config/wall_e_settings.yml`: + +```yaml +pr_delta: + enabled: true + fail_on: "none" # "none" (comment only) | "high" (fail check on high-severity) | "any" + debt_types: [] # empty = all debt types +``` + +With `fail_on: "high"` or `"any"`, the command exits non-zero when matching findings exist, turning the PR check red. Install the **`wall_e_pr_delta`** workflow (via `rails g wall_e:install`) to run it on `pull_request` events; it only needs `GITHUB_TOKEN`. + ## GitHub Actions usage The **scan** workflow supports: @@ -200,6 +230,8 @@ The **scan** workflow supports: The **verify** workflow (`wall_e_verify.yml`) runs on `pull_request` (`opened`, `synchronize`) and executes `bundle exec wall-e --verify-pr `. +The **pr_delta** workflow (`wall_e_pr_delta.yml`) runs on the same `pull_request` events and executes `bundle exec wall-e --pr-delta `. It is static-only (no `OPENAI_API_KEY` needed) and uses `concurrency` to cancel superseded runs. + Manual scan example: 1. Open **Actions** in your repo @@ -313,6 +345,7 @@ bundle exec wall-e [options] | `--skip-llm` | Skip triage and issue writer; static collectors only | | `--max-issues N` | Override max issues to create (for testing) | | `--verify-pr NUMBER` | Run PR verification instead of a repo scan | +| `--pr-delta NUMBER` | Comment on a PR with debt added on its changed lines (static-only) | ## Troubleshooting diff --git a/exe/wall-e b/exe/wall-e index bf2e751..31dc032 100755 --- a/exe/wall-e +++ b/exe/wall-e @@ -17,7 +17,8 @@ options = { dry_run: false, skip_llm: false, max_issues: nil, - verify_pr: nil + verify_pr: nil, + pr_delta: nil } OptionParser.new do |opts| @@ -54,6 +55,10 @@ OptionParser.new do |opts| opts.on("--verify-pr NUMBER", Integer, "Verify a PR against linked wall-e issues (skips normal scan)") do |value| options[:verify_pr] = value end + + opts.on("--pr-delta NUMBER", Integer, "Comment on a PR with debt added on its changed lines (skips normal scan)") do |value| + options[:pr_delta] = value + end end.parse! config = TechDebt::Config.load(options[:config_path]) @@ -77,6 +82,24 @@ if options[:verify_pr] puts JSON.pretty_generate(summary) exit 1 if summary["status"] == "error" +elsif options[:pr_delta] + require "tech_debt/delta/pr_delta" + begin + summary = TechDebt::Delta::PrDelta.new( + config, + pr_number: options[:pr_delta], + dry_run: options[:dry_run] + ).run + rescue Octokit::NotFound => e + warn "[wall-e] #{e.message}" + exit 1 + rescue ArgumentError => e + warn "[wall-e] #{e.message}" + exit 1 + end + + puts JSON.pretty_generate(summary) + exit 1 if summary["status"] == "fail" else summary = TechDebt::Analyzer.new( config, diff --git a/lib/generators/wall_e/install/install_generator.rb b/lib/generators/wall_e/install/install_generator.rb index 92f52db..cf32c21 100644 --- a/lib/generators/wall_e/install/install_generator.rb +++ b/lib/generators/wall_e/install/install_generator.rb @@ -20,6 +20,11 @@ def copy_verify_workflow copy_file "wall_e_verify.yml", ".github/workflows/wall_e_verify.yml" end + def copy_pr_delta_workflow + say "Adding PR debt-delta workflow...", :green + copy_file "wall_e_pr_delta.yml", ".github/workflows/wall_e_pr_delta.yml" + end + def copy_config say "Adding wall-e settings...", :green copy_file "wall_e_settings.yml", "config/wall_e_settings.yml" @@ -50,7 +55,7 @@ def print_next_steps say "" say " 1. Add OPENAI_API_KEY as a GitHub Actions secret" say " 2. (Optional) Add AGENT_ASSIGN_TOKEN for auto-assign (falls back to GITHUB_TOKEN)" - say " 3. Review .github/workflows/wall_e_scan.yml and wall_e_verify.yml triggers" + say " 3. Review .github/workflows/ triggers (scan, verify, pr_delta)" say " 4. Adjust analysis.paths and flog_threshold in config/wall_e_settings.yml if needed" say " 5. Optional: set verification.close_on_pass in config/wall_e_settings.yml" say " 6. Test locally:" diff --git a/lib/generators/wall_e/install/templates/wall_e_pr_delta.yml b/lib/generators/wall_e/install/templates/wall_e_pr_delta.yml new file mode 100644 index 0000000..571e1d0 --- /dev/null +++ b/lib/generators/wall_e/install/templates/wall_e_pr_delta.yml @@ -0,0 +1,29 @@ +name: wall-e debt delta + +on: + pull_request: + types: [opened, synchronize] + +# Cancel superseded runs when a PR is updated quickly. +concurrency: + group: wall-e-pr-delta-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + pr_delta: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@v4 + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + bundler-cache: true + + - name: Comment debt delta on PR + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: bundle exec wall-e --pr-delta ${{ github.event.pull_request.number }} diff --git a/lib/generators/wall_e/install/templates/wall_e_settings.yml b/lib/generators/wall_e/install/templates/wall_e_settings.yml index c397261..41c114b 100644 --- a/lib/generators/wall_e/install/templates/wall_e_settings.yml +++ b/lib/generators/wall_e/install/templates/wall_e_settings.yml @@ -51,3 +51,10 @@ auto_assign: # Optional: PR verification (--verify-pr) can auto-close issues when all checks pass (use with care). verification: close_on_pass: false + +# Optional: per-PR debt delta (--pr-delta). Static-only; runs collectors on changed Ruby +# files and comments with debt found on the lines the PR added. No issues are created. +pr_delta: + enabled: true + fail_on: "none" # "none" (comment only) | "high" (fail check on high-severity) | "any" + debt_types: [] # empty = all debt types diff --git a/lib/tech_debt/collectors/complexity_collector.rb b/lib/tech_debt/collectors/complexity_collector.rb index 3a263f9..3bf1154 100644 --- a/lib/tech_debt/collectors/complexity_collector.rb +++ b/lib/tech_debt/collectors/complexity_collector.rb @@ -36,18 +36,21 @@ def parse_output(output, threshold) score = match[:score].to_f next if score < threshold - file = rest[%r{(?[\w\/\.\-]+\.rb):\d+(?:-\d+)?}, :path] - next unless file + location = rest.match(%r{(?[\w/.\-]+\.rb):(?\d+)(?:-(?\d+))?}) + next unless location identifier = rest.sub(%r{\s+[\w\/\.\-]+\.rb:\d+(?:-\d+)?\s*$}, "") next if identifier =~ /\Amain#none\z/i + start_line = location[:start].to_i { - file: file, + file: location[:path], identifier: identifier, type: "high_complexity", detail: "Method complexity score #{score} exceeds threshold #{threshold}", - score: score + score: score, + line: start_line, + end_line: location[:end] ? location[:end].to_i : start_line } end end diff --git a/lib/tech_debt/collectors/debride_collector.rb b/lib/tech_debt/collectors/debride_collector.rb index 9741688..f1652be 100644 --- a/lib/tech_debt/collectors/debride_collector.rb +++ b/lib/tech_debt/collectors/debride_collector.rb @@ -29,12 +29,15 @@ def parse_output(output) match = line.match(%r{^(?[^:]+):(?\d+)\s+(?\S+)\s+is not called from anywhere}) next unless match + line = match[:line].to_i { file: match[:file], identifier: match[:identifier], type: "dead_code", detail: "Method appears to be uncalled (debride)", - score: 1 + score: 1, + line: line, + end_line: line } end end diff --git a/lib/tech_debt/collectors/flay_collector.rb b/lib/tech_debt/collectors/flay_collector.rb index 4a8c8a7..c972740 100644 --- a/lib/tech_debt/collectors/flay_collector.rb +++ b/lib/tech_debt/collectors/flay_collector.rb @@ -77,7 +77,9 @@ def candidates_for_group(lines, target_set) identifier: "#{loc[:file]}:#{loc[:line]}", type: "structural_duplication", detail: build_detail(match_type, node_type, mass, other_refs), - score: mass + score: mass, + line: loc[:line], + end_line: loc[:line] } end end diff --git a/lib/tech_debt/collectors/layer_collector.rb b/lib/tech_debt/collectors/layer_collector.rb index dbbd025..2a343bb 100644 --- a/lib/tech_debt/collectors/layer_collector.rb +++ b/lib/tech_debt/collectors/layer_collector.rb @@ -36,6 +36,7 @@ def job_file?(file) def current_attribute_violations(file, content) return [] unless content.match?(/Current\.\w+/) + line = line_of(content, /Current\.\w+/) [{ file: file, identifier: extract_class_name(content) || File.basename(file, ".rb"), @@ -43,7 +44,9 @@ def current_attribute_violations(file, content) detail: "References Current.* inside a model — layer violation. " \ "Current context is unavailable in background jobs and rake tasks, " \ "causing silent nil failures. Pass the value as an explicit parameter instead.", - score: 8 + score: 8, + line: line, + end_line: line }] end @@ -54,6 +57,7 @@ def anemic_job_signals(file, content) return [] if body.nil? || body.size != 1 return [] unless body[0].match?(/\A\w+\.\w+[\w!?]*(\(.*\))?\z/) + line = line_of(content, /def perform/) [{ file: file, identifier: "#{extract_class_name(content)}#perform", @@ -61,10 +65,17 @@ def anemic_job_signals(file, content) detail: "Job perform delegates entirely to a single model method with no added logic — " \ "anemic job. Consider using the active_job-performs gem to eliminate the " \ "separate job class and declare background execution directly on the model.", - score: 5 + score: 5, + line: line, + end_line: line }] end + def line_of(content, regex) + index = content.each_line.find_index { |l| l.match?(regex) } + index ? index + 1 : 1 + end + def extract_perform_body(content) match = content.match(/def perform\([^)]*\)\n(.*?)\n\s*end/m) return nil unless match diff --git a/lib/tech_debt/config.rb b/lib/tech_debt/config.rb index 7597669..8a15000 100644 --- a/lib/tech_debt/config.rb +++ b/lib/tech_debt/config.rb @@ -64,6 +64,13 @@ def close_issues_on_verification_pass? verification.fetch("close_on_pass", false) end + def pr_delta + value = raw["pr_delta"] + return { "enabled" => false } unless value.is_a?(Hash) + + { "enabled" => false }.merge(value) + end + private def validate! diff --git a/lib/tech_debt/delta/pr_delta.rb b/lib/tech_debt/delta/pr_delta.rb new file mode 100644 index 0000000..0e5f207 --- /dev/null +++ b/lib/tech_debt/delta/pr_delta.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +require_relative "../github/client" +require_relative "../github/pull_request" +require_relative "../github/comment_upserter" +require_relative "../collectors/debride_collector" +require_relative "../collectors/complexity_collector" +require_relative "../collectors/flay_collector" +require_relative "../collectors/layer_collector" + +module TechDebt + module Delta + # Per-PR debt delta: runs the static collectors against only the Ruby files + # a PR changes, keeps findings that land on lines the PR added, and posts a + # single (upserted) summary comment. No issues are created. + class PrDelta + MARKER = "" + SEVERITY_EMOJI = {"high" => "🔴", "medium" => "🟡", "low" => "🟢"}.freeze + SEVERITY_RANK = {"high" => 2, "medium" => 1, "low" => 0}.freeze + # A finding is "high" once its score reaches this multiple of the collector's threshold. + HIGH_SEVERITY_MULTIPLIER = 2 + # Score at/above which a leaked-business-logic finding counts as high severity. + LEAKED_HIGH_SCORE = 8 + + def initialize(config, pr_number:, dry_run: false) + @config = config + @pr_number = pr_number.to_i + @dry_run = dry_run + @repo = Github::Client.repo(config) + @client = Github::Client.build + @pull_request = Github::PullRequest.new(@client, @repo, @pr_number) + end + + def run + changed_files, patches = @pull_request.ruby_file_index + findings = changed_files.empty? ? [] : delta_findings(changed_files, patches) + post_comment(findings) + summary(changed_files, findings) + end + + private + + def delta_findings(changed_files, patches) + added_by_file = patches.transform_values { |patch| Github::PullRequest.added_lines(patch) } + collect(changed_files) + .select { |candidate| touches_changed_lines?(candidate, added_by_file) } + .select { |candidate| allowed_debt_type?(candidate) } + .map { |candidate| candidate.merge(severity: severity_for(candidate)) } + end + + def collect(files) + [ + Collectors::DebrideCollector, + Collectors::ComplexityCollector, + Collectors::FlayCollector, + Collectors::LayerCollector + ].flat_map { |collector| collector.new(@config, files: files).call } + end + + def touches_changed_lines?(candidate, added_by_file) + added = added_by_file[candidate[:file]] + return false if added.nil? || added.empty? + + first = candidate.fetch(:line, 0) + last = candidate.fetch(:end_line, first) + added.any? { |line| line.between?(first, last) } + end + + def allowed_debt_type?(candidate) + allowed = Array(@config.pr_delta["debt_types"]).map(&:to_s) + allowed.empty? || allowed.include?(candidate[:type].to_s) + end + + def post_comment(findings) + body = format_comment(findings) + if @dry_run + warn "[wall-e] Dry run — PR delta comment would be:\n#{body}" + return + end + + Github::CommentUpserter + .new(client: @client, repo: @repo, pr_number: @pr_number) + .upsert(body, marker: MARKER) + end + + def format_comment(findings) + return no_findings_comment if findings.empty? + + rows = findings.sort_by { |f| -severity_rank(f) }.map { |f| finding_row(f) } + [ + "## 🤖 wall-e debt delta", + "", + "Found **#{findings.size}** debt signal(s) on the lines this PR changed.", + "", + "| Severity | Type | Location | Detail |", + "| --- | --- | --- | --- |", + *rows, + "", + "_Static analysis on changed lines only. Run a full wall-e scan to file tracked issues._", + "", + MARKER + ].join("\n") + end + + def no_findings_comment + [ + "## 🤖 wall-e debt delta", + "", + "✅ No new debt signals on the lines this PR changed.", + "", + MARKER + ].join("\n") + end + + def finding_row(finding) + severity = finding[:severity] + file = finding[:file].to_s.tr("|`", "/") + location = "`#{file}:#{finding[:line]}`" + detail = finding[:detail].to_s.tr("|", "/").gsub(/\s+/, " ").strip + "| #{SEVERITY_EMOJI.fetch(severity, "🟡")} #{severity} | #{finding[:type]} | #{location} | #{detail} |" + end + + def severity_rank(finding) + SEVERITY_RANK.fetch(finding[:severity], 1) + end + + def severity_for(finding) + case finding[:type].to_s + when "high_complexity" + (finding[:score].to_f >= (@config.flog_threshold * HIGH_SEVERITY_MULTIPLIER)) ? "high" : "medium" + when "structural_duplication" + (finding[:score].to_f >= (@config.flay_threshold * HIGH_SEVERITY_MULTIPLIER)) ? "high" : "medium" + when "dead_code" + "low" + else + (finding[:score].to_i >= LEAKED_HIGH_SCORE) ? "high" : "medium" + end + end + + def summary(changed_files, findings) + gated = findings.select { |f| gates?(f[:severity]) } + { + "mode" => @dry_run ? "dry_run" : "live", + "pull_request" => @pr_number, + "changed_ruby_files" => changed_files.size, + "finding_count" => findings.size, + "gating_count" => gated.size, + "status" => gated.empty? ? "pass" : "fail", + "findings" => findings + } + end + + def gates?(severity) + case @config.pr_delta.fetch("fail_on", "none").to_s + when "any" then true + when "high" then severity == "high" + else false + end + end + end + end +end diff --git a/lib/tech_debt/github/client.rb b/lib/tech_debt/github/client.rb new file mode 100644 index 0000000..8566ca2 --- /dev/null +++ b/lib/tech_debt/github/client.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "octokit" + +module TechDebt + module Github + # Builds Octokit clients and resolves the target repo from config/env. + # Centralizes token handling shared by issue creation, verification, and + # PR delta reporting. + module Client + module_function + + # Builds an Octokit client. When +token_env+ is given and set, that token + # is used; otherwise it falls back to GITHUB_TOKEN (raising if absent). + # auto_paginate is enabled so comment/file/commit listings are complete + # (the default 30-per-page cap would silently truncate large PRs and break + # comment upsert dedup). + def build(token_env: nil) + client = Octokit::Client.new(access_token: resolve_token(token_env)) + client.auto_paginate = true + client + end + + def repo(config) + value = config.github["repo"] || ENV["GITHUB_REPOSITORY"] + raise ArgumentError, "github.repo or GITHUB_REPOSITORY is required" if value.nil? || value.to_s.empty? + + value + end + + def resolve_token(token_env) + explicit = token_env && ENV[token_env] + return explicit unless explicit.to_s.strip.empty? + + ENV.fetch("GITHUB_TOKEN") + end + end + end +end diff --git a/lib/tech_debt/github/comment_upserter.rb b/lib/tech_debt/github/comment_upserter.rb new file mode 100644 index 0000000..18b8181 --- /dev/null +++ b/lib/tech_debt/github/comment_upserter.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "octokit" + +module TechDebt + module Github + # Posts or updates a single PR comment identified by a hidden marker, so + # repeated runs (e.g. on every `synchronize`) edit one comment instead of + # stacking duplicates. + class CommentUpserter + def initialize(client:, repo:, pr_number:) + @client = client + @repo = repo + @pr_number = pr_number + end + + def upsert(body, marker:) + existing = find_existing(marker) + if existing + @client.update_comment(@repo, existing.id, body) + else + @client.add_comment(@repo, @pr_number, body) + end + end + + private + + def find_existing(marker) + @client.issue_comments(@repo, @pr_number).find { |comment| comment.body.to_s.include?(marker) } + end + end + end +end diff --git a/lib/tech_debt/github/issue_manager.rb b/lib/tech_debt/github/issue_manager.rb index 168c4a7..edbb3b2 100644 --- a/lib/tech_debt/github/issue_manager.rb +++ b/lib/tech_debt/github/issue_manager.rb @@ -2,6 +2,7 @@ require 'json' require 'octokit' +require_relative 'client' require_relative 'fingerprint' module TechDebt @@ -21,12 +22,9 @@ class IssueManager attr_reader :repo def initialize(config) - token = ENV.fetch('GITHUB_TOKEN') - @repo = config.github['repo'] || ENV['GITHUB_REPOSITORY'] - raise ArgumentError, 'github.repo or GITHUB_REPOSITORY is required' if @repo.nil? || @repo.empty? - + @repo = Github::Client.repo(config) @config = config - @client = Octokit::Client.new(access_token: token) + @client = Github::Client.build end # Ensure that the labels are created in the repository diff --git a/lib/tech_debt/github/pull_request.rb b/lib/tech_debt/github/pull_request.rb new file mode 100644 index 0000000..4a1ac59 --- /dev/null +++ b/lib/tech_debt/github/pull_request.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module TechDebt + module Github + # Read-only view over a pull request's changed Ruby files and diffs. + class PullRequest + HUNK_HEADER = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/ + + def initialize(client, repo, number) + @client = client + @repo = repo + @number = number + end + + # Returns [changed_rb_files, patches_by_file]. + def ruby_file_index + files = [] + patches = {} + @client.pull_request_files(@repo, @number).each do |file| + next unless file.filename.end_with?(".rb") + + files << file.filename + patches[file.filename] = file.patch.to_s + end + [files.uniq, patches] + end + + # Issue references (Fixes/Closes/Resolves #N) from the PR body and commits. + def linked_issue_numbers(pattern) + nums = [] + @client.pull_request(@repo, @number).body.to_s.scan(pattern) { nums << Regexp.last_match(1).to_i } + @client.pull_request_commits(@repo, @number).each do |commit| + commit.commit&.message.to_s.scan(pattern) { nums << Regexp.last_match(1).to_i } + end + nums.uniq.sort + end + + # Set of new-file line numbers added by the diff in +patch+. + def self.added_lines(patch) + added = Set.new + new_line = nil + patch.to_s.each_line do |line| + if (match = line.match(HUNK_HEADER)) + new_line = match[1].to_i + elsif new_line + new_line = advance(added, line, new_line) + end + end + added + end + + def self.advance(added, line, new_line) + if line.start_with?("+") && !line.start_with?("+++") + added << new_line + new_line + 1 + elsif line.start_with?("-") && !line.start_with?("---") + new_line + else + new_line + 1 + end + end + end + end +end diff --git a/lib/tech_debt/verification/pr_verifier.rb b/lib/tech_debt/verification/pr_verifier.rb index be7b985..44edc5b 100644 --- a/lib/tech_debt/verification/pr_verifier.rb +++ b/lib/tech_debt/verification/pr_verifier.rb @@ -1,7 +1,8 @@ # frozen_string_literal: true -require "octokit" +require_relative "../github/client" require_relative "../github/issue_manager" +require_relative "../github/pull_request" require_relative "llm_verifier" require_relative "result_reporter" require_relative "static_verifier" @@ -16,10 +17,9 @@ def initialize(config, pr_number:, verification_prompt_path:, dry_run: false) @pr_number = pr_number.to_i @verification_prompt_path = verification_prompt_path @dry_run = dry_run - @repo = config.github["repo"] || ENV["GITHUB_REPOSITORY"] - raise ArgumentError, "github.repo or GITHUB_REPOSITORY is required" if @repo.nil? || @repo.empty? - - @client = Octokit::Client.new(access_token: ENV.fetch("GITHUB_TOKEN")) + @repo = Github::Client.repo(config) + @client = Github::Client.build + @pull_request = Github::PullRequest.new(@client, @repo, @pr_number) end def run @@ -34,7 +34,7 @@ def run } end - rb_files, patches = pull_request_file_index + rb_files, patches = @pull_request.ruby_file_index static = StaticVerifier.new(@config, changed_rb_files: rb_files) llm_verifier = nil @@ -72,14 +72,7 @@ def init_llm_verifier end def extract_issue_numbers - pr = @client.pull_request(@repo, @pr_number) - nums = [] - pr.body.to_s.scan(ISSUE_REF) { nums << Regexp.last_match(1).to_i } - @client.pull_request_commits(@repo, @pr_number).each do |c| - msg = c.commit&.message - msg.to_s.scan(ISSUE_REF) { nums << Regexp.last_match(1).to_i } - end - nums.uniq.sort + @pull_request.linked_issue_numbers(ISSUE_REF) end def load_verified_payload(issue_number) @@ -90,18 +83,6 @@ def load_verified_payload(issue_number) { issue_number: issue_number, payload: payload } end - def pull_request_file_index - rb_files = [] - patches = {} - @client.pull_request_files(@repo, @pr_number).each do |f| - next unless f.filename.end_with?(".rb") - - rb_files << f.filename - patches[f.filename] = f.patch.to_s - end - [rb_files.uniq, patches] - end - def build_static_result(issue_number, payload, static_out) passed = static_out["passed"] { diff --git a/lib/tech_debt/verification/result_reporter.rb b/lib/tech_debt/verification/result_reporter.rb index 186da95..f5309ce 100644 --- a/lib/tech_debt/verification/result_reporter.rb +++ b/lib/tech_debt/verification/result_reporter.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true require "octokit" +require_relative "../github/comment_upserter" module TechDebt module Verification class ResultReporter + MARKER = "" + def initialize(client:, repo:, pr_number:, dry_run: false) @client = client @repo = repo @@ -19,7 +22,7 @@ def post!(results, close_on_pass: false) return end - @client.add_comment(@repo, @pr_number, body) + upserter.upsert(body, marker: MARKER) return unless close_on_pass && all_pass?(results) close_linked_issues(results) @@ -27,6 +30,10 @@ def post!(results, close_on_pass: false) private + def upserter + Github::CommentUpserter.new(client: @client, repo: @repo, pr_number: @pr_number) + end + def all_pass?(results) results.all? { |r| r["verdict"] == "pass" } end @@ -48,6 +55,8 @@ def format_comment(results) lines = ["## wall-e PR verification", ""] if results.empty? lines << "_No verification results._" + lines << "" + lines << MARKER return lines.join("\n") end @@ -73,6 +82,7 @@ def format_comment(results) lines << "" end + lines << MARKER lines.join("\n").strip end diff --git a/spec/tech_debt/collectors/complexity_collector_spec.rb b/spec/tech_debt/collectors/complexity_collector_spec.rb new file mode 100644 index 0000000..4c059fc --- /dev/null +++ b/spec/tech_debt/collectors/complexity_collector_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require "spec_helper" +require "tech_debt/config" +require "tech_debt/collectors/complexity_collector" + +RSpec.describe TechDebt::Collectors::ComplexityCollector do + let(:config) { instance_double(TechDebt::Config, analysis: {"paths" => [], "exclude_paths" => []}, flog_threshold: 25.0) } + + subject(:collector) { described_class.new(config, files: ["app/models/user.rb"]) } + + before { allow(File).to receive(:file?).and_return(true) } + + def stub_flog(line) + allow(Open3).to receive(:capture3).and_return([line, "", double(success?: true, exitstatus: 0)]) + end + + describe "#call" do + it "parses a line range into line/end_line" do + stub_flog(" 40.0: User#big app/models/user.rb:12-29\n") + expect(collector.call.first).to include( + file: "app/models/user.rb", identifier: "User#big", + type: "high_complexity", line: 12, end_line: 29 + ) + end + + it "sets end_line equal to line when no range is present" do + stub_flog(" 40.0: User#m app/models/user.rb:12\n") + candidate = collector.call.first + expect([candidate[:line], candidate[:end_line]]).to eq([12, 12]) + end + + it "drops methods below the flog threshold" do + stub_flog(" 10.0: User#small app/models/user.rb:5\n") + expect(collector.call).to eq([]) + end + + it "ignores the main#none summary row" do + stub_flog(" 99.0: main#none app/models/user.rb:1\n") + expect(collector.call).to eq([]) + end + end +end diff --git a/spec/tech_debt/collectors/debride_collector_spec.rb b/spec/tech_debt/collectors/debride_collector_spec.rb new file mode 100644 index 0000000..a966d8c --- /dev/null +++ b/spec/tech_debt/collectors/debride_collector_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require "spec_helper" +require "tech_debt/config" +require "tech_debt/collectors/debride_collector" + +RSpec.describe TechDebt::Collectors::DebrideCollector do + let(:config) { instance_double(TechDebt::Config, analysis: {"paths" => [], "exclude_paths" => []}) } + + subject(:collector) { described_class.new(config, files: ["app/models/user.rb"]) } + + before { allow(File).to receive(:file?).and_return(true) } + + describe "#call" do + it "extracts the line number and mirrors it to end_line" do + output = "app/models/user.rb:42 User#unused is not called from anywhere\n" + allow(Open3).to receive(:capture3).and_return([output, "", double(success?: true, exitstatus: 0)]) + + expect(collector.call.first).to include( + file: "app/models/user.rb", identifier: "User#unused", + type: "dead_code", line: 42, end_line: 42 + ) + end + + it "skips lines that don't match the debride format" do + allow(Open3).to receive(:capture3).and_return(["nothing useful here\n", "", double(success?: true, exitstatus: 0)]) + expect(collector.call).to eq([]) + end + + it "returns an empty array without running debride when there are no targets" do + expect(Open3).not_to receive(:capture3) + expect(described_class.new(config, files: []).call).to eq([]) + end + end +end diff --git a/spec/tech_debt/collectors/flay_collector_spec.rb b/spec/tech_debt/collectors/flay_collector_spec.rb index 1956d86..15498cc 100644 --- a/spec/tech_debt/collectors/flay_collector_spec.rb +++ b/spec/tech_debt/collectors/flay_collector_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "spec_helper" +require "tech_debt/config" require "tech_debt/collectors/flay_collector" RSpec.describe TechDebt::Collectors::FlayCollector do @@ -17,6 +18,10 @@ # Returns a collector with an explicit file list so we bypass glob expansion. let(:files) { ["app/models/order.rb", "app/models/invoice.rb"] } + # The fixture paths don't exist on disk; treat the explicit file list as present + # so BaseCollector#target_files keeps them. + before { allow(File).to receive(:file?).and_return(true) } + describe "#call" do context "when flay finds no output" do before do diff --git a/spec/tech_debt/collectors/layer_collector_spec.rb b/spec/tech_debt/collectors/layer_collector_spec.rb new file mode 100644 index 0000000..ad3eadc --- /dev/null +++ b/spec/tech_debt/collectors/layer_collector_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require "spec_helper" +require "tech_debt/config" +require "tech_debt/collectors/layer_collector" + +RSpec.describe TechDebt::Collectors::LayerCollector do + let(:config) { instance_double(TechDebt::Config, analysis: {"paths" => [], "exclude_paths" => []}) } + + subject(:collector) { described_class.new(config, files: []) } + + # These exercise the line-number extraction directly; #model_file?/#job_file? + # gating is covered by the collector's own path filtering. + describe "line extraction" do + it "reports the line of a Current.* reference" do + content = "class Order\n def total\n Current.user.id\n end\nend\n" + result = collector.send(:current_attribute_violations, "app/models/order.rb", content).first + expect(result).to include(type: "leaked_business_logic", identifier: "Order", line: 3, end_line: 3) + end + + it "reports the line of an anemic perform" do + content = "class SyncJob\n def perform(id)\n Model.sync(id)\n end\nend\n" + result = collector.send(:anemic_job_signals, "app/jobs/sync_job.rb", content).first + expect(result).to include(identifier: "SyncJob#perform", line: 2, end_line: 2) + end + + it "falls back to line 1 when the pattern is absent" do + expect(collector.send(:line_of, "class Order\nend\n", /nope/)).to eq(1) + end + end +end diff --git a/spec/tech_debt/delta/pr_delta_spec.rb b/spec/tech_debt/delta/pr_delta_spec.rb new file mode 100644 index 0000000..58a0d06 --- /dev/null +++ b/spec/tech_debt/delta/pr_delta_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require "spec_helper" +require "tech_debt/config" +require "tech_debt/delta/pr_delta" + +RSpec.describe TechDebt::Delta::PrDelta do + let(:pr_delta_settings) { {"enabled" => true, "fail_on" => "none", "debt_types" => []} } + + let(:config) do + instance_double( + TechDebt::Config, + github: {"repo" => "acme/app"}, + pr_delta: pr_delta_settings, + flog_threshold: 25.0, + flay_threshold: 25 + ) + end + + let(:client) { instance_double(Octokit::Client) } + let(:pull_request) { instance_double(TechDebt::Github::PullRequest) } + + let(:changed_files) { ["app/models/order.rb"] } + # Added lines 10, 11, 12 in order.rb + let(:patches) { {"app/models/order.rb" => "@@ -1,1 +10,3 @@\n+a\n+b\n+c\n"} } + + subject(:delta) { described_class.new(config, pr_number: 42, dry_run: true) } + + before do + allow(TechDebt::Github::Client).to receive(:build).and_return(client) + allow(TechDebt::Github::PullRequest).to receive(:new).and_return(pull_request) + allow(pull_request).to receive(:ruby_file_index).and_return([changed_files, patches]) + + stub_collector(TechDebt::Collectors::DebrideCollector, in_range_candidate) + stub_collector(TechDebt::Collectors::ComplexityCollector, out_of_range_candidate) + stub_collector(TechDebt::Collectors::FlayCollector, []) + stub_collector(TechDebt::Collectors::LayerCollector, []) + end + + def stub_collector(klass, results) + instance = instance_double(klass.to_s, call: results) + allow(klass).to receive(:new).and_return(instance) + end + + let(:in_range_candidate) do + [{file: "app/models/order.rb", identifier: "Order#dead", type: "dead_code", + detail: "uncalled", score: 1, line: 11, end_line: 11}] + end + + let(:out_of_range_candidate) do + [{file: "app/models/order.rb", identifier: "Order#big", type: "high_complexity", + detail: "complex", score: 80, line: 50, end_line: 60}] + end + + describe "#run" do + it "keeps only findings on changed lines" do + summary = delta.run + expect(summary["finding_count"]).to eq(1) + expect(summary["findings"].map { |f| f[:type] }).to eq(["dead_code"]) + end + + it "keeps a multi-line finding when its range overlaps a changed line" do + out_of_range_candidate[0][:line] = 8 + out_of_range_candidate[0][:end_line] = 11 + expect(delta.run["finding_count"]).to eq(2) + end + + it "reports the changed Ruby file count" do + expect(delta.run["changed_ruby_files"]).to eq(1) + end + + context "when no Ruby files changed" do + let(:changed_files) { [] } + + it "skips collection and reports zero findings" do + expect(TechDebt::Collectors::DebrideCollector).not_to receive(:new) + expect(delta.run["finding_count"]).to eq(0) + end + end + + context "with a debt_types filter" do + let(:pr_delta_settings) { {"enabled" => true, "fail_on" => "none", "debt_types" => ["high_complexity"]} } + + it "drops findings whose type is not allowed" do + expect(delta.run["finding_count"]).to eq(0) + end + end + + describe "gating status" do + it "passes when fail_on is none" do + expect(delta.run["status"]).to eq("pass") + end + + context "when fail_on is any" do + let(:pr_delta_settings) { {"enabled" => true, "fail_on" => "any", "debt_types" => []} } + + it "fails because a finding exists" do + expect(delta.run["status"]).to eq("fail") + end + end + + context "when fail_on is high" do + let(:pr_delta_settings) { {"enabled" => true, "fail_on" => "high", "debt_types" => []} } + + it "passes when the only finding is low severity" do + expect(delta.run["status"]).to eq("pass") + end + end + end + end +end diff --git a/spec/tech_debt/github/client_spec.rb b/spec/tech_debt/github/client_spec.rb new file mode 100644 index 0000000..034f2aa --- /dev/null +++ b/spec/tech_debt/github/client_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require "spec_helper" +require "tech_debt/config" +require "tech_debt/github/client" + +RSpec.describe TechDebt::Github::Client do + describe ".repo" do + it "returns the configured repo" do + config = instance_double(TechDebt::Config, github: {"repo" => "acme/app"}) + expect(described_class.repo(config)).to eq("acme/app") + end + + it "falls back to GITHUB_REPOSITORY when config repo is absent" do + config = instance_double(TechDebt::Config, github: {}) + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("GITHUB_REPOSITORY").and_return("acme/env") + expect(described_class.repo(config)).to eq("acme/env") + end + + it "raises when neither config repo nor GITHUB_REPOSITORY is set" do + config = instance_double(TechDebt::Config, github: {}) + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("GITHUB_REPOSITORY").and_return(nil) + expect { described_class.repo(config) }.to raise_error(ArgumentError, /GITHUB_REPOSITORY/) + end + end + + describe ".resolve_token" do + before do + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:fetch).and_call_original + end + + it "prefers an explicit token_env when set" do + allow(ENV).to receive(:[]).with("AGENT_TOKEN").and_return("explicit") + expect(described_class.resolve_token("AGENT_TOKEN")).to eq("explicit") + end + + it "falls back to GITHUB_TOKEN when token_env is blank" do + allow(ENV).to receive(:[]).with("AGENT_TOKEN").and_return(" ") + allow(ENV).to receive(:fetch).with("GITHUB_TOKEN").and_return("ghtoken") + expect(described_class.resolve_token("AGENT_TOKEN")).to eq("ghtoken") + end + + it "falls back to GITHUB_TOKEN when token_env is nil" do + allow(ENV).to receive(:fetch).with("GITHUB_TOKEN").and_return("ghtoken") + expect(described_class.resolve_token(nil)).to eq("ghtoken") + end + + it "raises KeyError when GITHUB_TOKEN is absent" do + allow(ENV).to receive(:fetch).with("GITHUB_TOKEN").and_raise(KeyError) + expect { described_class.resolve_token(nil) }.to raise_error(KeyError) + end + end + + describe ".build" do + it "enables auto_paginate so listings are not truncated at 30 per page" do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("GITHUB_TOKEN").and_return("ghtoken") + expect(described_class.build.auto_paginate).to be(true) + end + end +end diff --git a/spec/tech_debt/github/comment_upserter_spec.rb b/spec/tech_debt/github/comment_upserter_spec.rb new file mode 100644 index 0000000..752fe76 --- /dev/null +++ b/spec/tech_debt/github/comment_upserter_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "spec_helper" +require "octokit" +require "tech_debt/github/comment_upserter" + +RSpec.describe TechDebt::Github::CommentUpserter do + let(:client) { instance_double(Octokit::Client) } + let(:marker) { "" } + + subject(:upserter) { described_class.new(client: client, repo: "acme/app", pr_number: 42) } + + describe "#upsert" do + it "updates the existing comment that contains the marker" do + existing = double(id: 7, body: "old body\n") + noise = double(id: 1, body: "unrelated comment") + allow(client).to receive(:issue_comments).with("acme/app", 42).and_return([noise, existing]) + + expect(client).to receive(:update_comment).with("acme/app", 7, "new body") + expect(client).not_to receive(:add_comment) + upserter.upsert("new body", marker: marker) + end + + it "creates a new comment when none contain the marker" do + allow(client).to receive(:issue_comments).with("acme/app", 42).and_return([double(id: 1, body: "noise")]) + + expect(client).to receive(:add_comment).with("acme/app", 42, "new body") + expect(client).not_to receive(:update_comment) + upserter.upsert("new body", marker: marker) + end + + it "creates a new comment when there are no comments" do + allow(client).to receive(:issue_comments).and_return([]) + + expect(client).to receive(:add_comment).with("acme/app", 42, "body") + upserter.upsert("body", marker: marker) + end + end +end diff --git a/spec/tech_debt/github/pull_request_spec.rb b/spec/tech_debt/github/pull_request_spec.rb new file mode 100644 index 0000000..b3a944a --- /dev/null +++ b/spec/tech_debt/github/pull_request_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "spec_helper" +require "octokit" +require "tech_debt/github/pull_request" + +RSpec.describe TechDebt::Github::PullRequest do + let(:client) { instance_double(Octokit::Client) } + + describe "#ruby_file_index" do + it "returns unique .rb files and their patches, skipping non-Ruby files" do + files = [ + double(filename: "app/a.rb", patch: "p1"), + double(filename: "app/b.js", patch: "p2"), + double(filename: "app/a.rb", patch: "p1") + ] + allow(client).to receive(:pull_request_files).with("acme/app", 1).and_return(files) + + rb, patches = described_class.new(client, "acme/app", 1).ruby_file_index + expect(rb).to eq(["app/a.rb"]) + expect(patches).to eq({"app/a.rb" => "p1"}) + end + end + + describe "#linked_issue_numbers" do + let(:pattern) { /\b(?:fix(?:es)?|close[sd]?|resolve[sd]?)\s*#(\d+)\b/i } + + it "collects, dedups, and sorts references from the PR body and commit messages" do + allow(client).to receive(:pull_request).with("acme/app", 1).and_return(double(body: "Fixes #4 and closes #2")) + commit = double(commit: double(message: "resolve #2\nfixes #9")) + allow(client).to receive(:pull_request_commits).with("acme/app", 1).and_return([commit]) + + expect(described_class.new(client, "acme/app", 1).linked_issue_numbers(pattern)).to eq([2, 4, 9]) + end + end + + describe ".added_lines" do + it "returns new-file line numbers for added lines only" do + patch = +"" + patch << "@@ -1,3 +10,4 @@\n" + patch << " context\n" + patch << "+added_eleven\n" + patch << "+added_twelve\n" + patch << "-removed\n" + patch << " trailing\n" + + expect(described_class.added_lines(patch).to_a.sort).to eq([11, 12]) + end + + it "tracks line numbers across multiple hunks" do + patch = +"" + patch << "@@ -1,1 +1,1 @@\n+first\n" + patch << "@@ -20,2 +30,2 @@\n context\n+thirty_one\n" + + expect(described_class.added_lines(patch).to_a.sort).to eq([1, 31]) + end + + it "ignores the +++ file header" do + patch = "+++ b/app/models/order.rb\n@@ -1,0 +5,1 @@\n+real_add\n" + expect(described_class.added_lines(patch).to_a).to eq([5]) + end + + it "returns an empty set for an empty patch" do + expect(described_class.added_lines("")).to be_empty + end + end +end From 1a815818eb045c6ac006559555ae2ff9aa1b9772 Mon Sep 17 00:00:00 2001 From: Moyom96 Date: Mon, 22 Jun 2026 09:41:00 -0600 Subject: [PATCH 2/3] fix: make LayerCollector match relative scan paths model_file?/job_file? matched %r{/app/models/} and %r{/app/jobs/}, which require a leading slash that relative scan paths (app/models/...) never have, so the collector never fired in a standard Rails layout. Anchor the match to a path-segment boundary so it fires on relative and nested engine paths without matching unrelated dirs (e.g. lib/myapp/models). Add LayerCollector specs covering line/end_line extraction. --- lib/tech_debt/collectors/layer_collector.rb | 4 +- .../collectors/layer_collector_spec.rb | 42 ++++++++++++++----- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/lib/tech_debt/collectors/layer_collector.rb b/lib/tech_debt/collectors/layer_collector.rb index 2a343bb..0984464 100644 --- a/lib/tech_debt/collectors/layer_collector.rb +++ b/lib/tech_debt/collectors/layer_collector.rb @@ -26,11 +26,11 @@ def analyze_file(file) end def model_file?(file) - file.match?(%r{/app/models/}) + file.match?(%r{(?:\A|/)app/models/}) end def job_file?(file) - file.match?(%r{/app/jobs/}) + file.match?(%r{(?:\A|/)app/jobs/}) end def current_attribute_violations(file, content) diff --git a/spec/tech_debt/collectors/layer_collector_spec.rb b/spec/tech_debt/collectors/layer_collector_spec.rb index ad3eadc..b70bdbf 100644 --- a/spec/tech_debt/collectors/layer_collector_spec.rb +++ b/spec/tech_debt/collectors/layer_collector_spec.rb @@ -7,24 +7,46 @@ RSpec.describe TechDebt::Collectors::LayerCollector do let(:config) { instance_double(TechDebt::Config, analysis: {"paths" => [], "exclude_paths" => []}) } - subject(:collector) { described_class.new(config, files: []) } + before { allow(File).to receive(:file?).and_return(true) } - # These exercise the line-number extraction directly; #model_file?/#job_file? - # gating is covered by the collector's own path filtering. - describe "line extraction" do - it "reports the line of a Current.* reference" do + def collect(file, content) + allow(File).to receive(:read).with(file).and_return(content) + described_class.new(config, files: [file]).call + end + + describe "#call" do + it "flags a Current.* reference in a model at relative path and reports its line" do content = "class Order\n def total\n Current.user.id\n end\nend\n" - result = collector.send(:current_attribute_violations, "app/models/order.rb", content).first - expect(result).to include(type: "leaked_business_logic", identifier: "Order", line: 3, end_line: 3) + expect(collect("app/models/order.rb", content).first).to include( + type: "leaked_business_logic", identifier: "Order", line: 3, end_line: 3 + ) end - it "reports the line of an anemic perform" do + it "flags an anemic perform in a job at relative path and reports its line" do content = "class SyncJob\n def perform(id)\n Model.sync(id)\n end\nend\n" - result = collector.send(:anemic_job_signals, "app/jobs/sync_job.rb", content).first - expect(result).to include(identifier: "SyncJob#perform", line: 2, end_line: 2) + expect(collect("app/jobs/sync_job.rb", content).first).to include( + identifier: "SyncJob#perform", line: 2, end_line: 2 + ) + end + + it "also matches nested engine paths" do + content = "class Order\n def total\n Current.user.id\n end\nend\n" + expect(collect("engines/billing/app/models/order.rb", content)).not_to be_empty end + it "does not treat a non-app/models path as a model file" do + content = "class Order\n Current.user\nend\n" + expect(collect("lib/myapp/models/order.rb", content)).to eq([]) + end + + it "returns nothing for a model without a Current.* reference" do + expect(collect("app/models/order.rb", "class Order\nend\n")).to eq([]) + end + end + + describe "#line_of" do it "falls back to line 1 when the pattern is absent" do + collector = described_class.new(config, files: []) expect(collector.send(:line_of, "class Order\nend\n", /nope/)).to eq(1) end end From 5a0aa2005d048baa35b3f94a9826eeb7c40f6aa9 Mon Sep 17 00:00:00 2001 From: Moyom96 Date: Mon, 22 Jun 2026 19:12:18 -0600 Subject: [PATCH 3/3] chore: normalize lockfile platforms (unversioned darwin + linux) Replace the version-pinned arm64-darwin-23 with the conventional unversioned arm64-darwin, and add x86_64-linux so the lockfile resolves on Linux CI/containers without a frozen-bundle platform error. --- Gemfile.lock | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 42ee0d8..901953a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -105,6 +105,8 @@ GEM racc (~> 1.4) nokogiri (1.19.1-x86_64-darwin) racc (~> 1.4) + nokogiri (1.19.1-x86_64-linux-gnu) + racc (~> 1.4) octokit (9.2.0) faraday (>= 1, < 3) sawyer (~> 0.9) @@ -221,8 +223,9 @@ GEM zeitwerk (2.7.5) PLATFORMS - arm64-darwin-23 + arm64-darwin x86_64-darwin + x86_64-linux DEPENDENCIES rake