diff --git a/API.md b/API.md index d13bff1..79d25b2 100644 --- a/API.md +++ b/API.md @@ -259,6 +259,13 @@ mode participates in graph-cache identity, and `clear_cache: true` rebuilds the Invalid API input raises `ArchUnit::UserError` or `ArgumentError`; library or environment failures raise `ArchUnit::TechnicalError`. Rule failures remain violation data. +Malformed included Ruby raises `ArchUnit::SourceParseError`, a `TechnicalError`, during graph or +AST-metric extraction. No partial graph is returned or cached. `error.diagnostics` is an immutable +array of `ArchUnit::ParseDiagnostic` values with `path`, one-based `line`/`column`, and `message`; +each value supports `to_h`. RSpec/Minitest assertions propagate this technical failure instead of +reporting a pass or an ordinary architecture violation. Excluded files are not parsed. The profile's +`malformed_files` counter records a rejected file; existing cache-clearing rules still apply. + See the [main guide](index.html) for installation and complete examples, [SUPPORT.md](https://github.com/LukasNiessen/ArchUnitRuby/blob/main/SUPPORT.md) for help, and [SECURITY.md](https://github.com/LukasNiessen/ArchUnitRuby/blob/main/SECURITY.md) for responsible vulnerability reporting. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc8edc..b45703b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 0.0.2 (unreleased) +- Fail closed on malformed included Ruby in dependency graphs and AST metrics, with structured + source diagnostics and profile counters instead of silently treating incomplete analysis as clean. + - Add development-only locked dependency scanning with advisory refresh, fail-closed health checks, Windows path normalization, and retained CI reports. diff --git a/README.md b/README.md index e90d73f..3a429eb 100644 --- a/README.md +++ b/README.md @@ -517,7 +517,14 @@ For diagnostics, pass an `ArchUnit::Extraction::ExtractionProfile` to `ArchUnit::Extraction.extract_graph(root, options: options, profile: profile)`. Its counters include `unresolved_imports`, `unresolved_constant_references`, and `ambiguous_constant_references`; unknown third-party constants do not become architecture violations. These counts describe a cold analysis; -a cache hit does not rerun diagnostics. Malformed source is skipped, as in import-only mode. +a cache hit does not rerun diagnostics. Included malformed Ruby now raises +`ArchUnit::SourceParseError < ArchUnit::TechnicalError` in both modes and in AST metrics, rather +than silently returning a partial graph or zero counts. Its immutable `diagnostics` contain +`path`, one-based `line` and `column`, and the parser `message`; the extraction profile increments +`malformed_files`. Fix or explicitly exclude invalid files using the project source policy. +Fluent rule selectors do not limit graph discovery. Failed graph builds are not cached; after +editing previously cached valid source, the normal explicit `clear_cache: true` policy still applies. +This intentionally changes the prototype's permissive behavior before the stable release. Expanded analysis uses a bounded Prism index and adds no runtime dependency. The design spike considered [RubyDEX](https://github.com/Shopify/rubydex), whose semantic index is broader but adds a diff --git a/lib/archunit.rb b/lib/archunit.rb index 1a886f9..2a01f2b 100644 --- a/lib/archunit.rb +++ b/lib/archunit.rb @@ -2,6 +2,7 @@ require_relative 'archunit/version' require_relative 'archunit/error/technical_error' +require_relative 'archunit/error/source_parse_error' require_relative 'archunit/error/user_error' require_relative 'archunit/common/pattern' require_relative 'archunit/common/filter' @@ -54,6 +55,7 @@ module ArchUnit ImportKind = Common::Extraction::ImportKind Edge = Common::Extraction::Edge DependencyEvidence = Common::Extraction::DependencyEvidence + ParseDiagnostic = Common::Extraction::ParseDiagnostic Graph = Common::Extraction::Graph MappedEdge = Common::Projection::MappedEdge ProjectedEdge = Common::Projection::ProjectedEdge diff --git a/lib/archunit/common/extraction/parse_diagnostic.rb b/lib/archunit/common/extraction/parse_diagnostic.rb new file mode 100644 index 0000000..82e3711 --- /dev/null +++ b/lib/archunit/common/extraction/parse_diagnostic.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module ArchUnit + module Common + module Extraction + # Immutable parser failure location; line and column are both one-based. + ParseDiagnostic = Data.define(:path, :line, :column, :message) do + def initialize(path:, line:, column:, message:) + unless [path, message].all? { |value| value.is_a?(String) && !value.empty? } + raise ArgumentError, 'parse diagnostic requires a non-empty path and message' + end + unless [line, column].all? { |value| value.is_a?(Integer) && value.positive? } + raise ArgumentError, 'parse diagnostic coordinates must be positive integers' + end + + path = path.tr('\\', '/').freeze + message = message.dup.freeze + super + end + end + end + end +end diff --git a/lib/archunit/error/source_parse_error.rb b/lib/archunit/error/source_parse_error.rb new file mode 100644 index 0000000..33b6928 --- /dev/null +++ b/lib/archunit/error/source_parse_error.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require_relative 'technical_error' +require_relative '../common/extraction/parse_diagnostic' + +module ArchUnit + # An included source could not be analyzed; never an ordinary architecture violation. + class SourceParseError < TechnicalError + # @return [Array] immutable source diagnostics + attr_reader :diagnostics + + # @return [Prism::ParseResult] the complete result, or a technical failure + def self.validate!(result, path:) + return result if result.success? + + diagnostics = result.errors.map do |error| + Common::Extraction::ParseDiagnostic.new( + path: path.to_s, line: error.location.start_line, + column: error.location.start_column + 1, message: error.message + ) + end + raise SourceParseError, diagnostics + end + + def initialize(diagnostics) + unless diagnostics.is_a?(Array) && !diagnostics.empty? && + diagnostics.all?(Common::Extraction::ParseDiagnostic) + raise ArgumentError, 'source parse error requires non-empty parse diagnostics' + end + + @diagnostics = diagnostics.dup.freeze + details = @diagnostics.map do |item| + "#{item.path}:#{item.line}:#{item.column}: #{item.message}" + end + super("Cannot analyze malformed Ruby source:\n#{details.join("\n")}") + end + end +end diff --git a/lib/archunit/extraction/constant_index.rb b/lib/archunit/extraction/constant_index.rb index 6a99aa2..36e0ddf 100644 --- a/lib/archunit/extraction/constant_index.rb +++ b/lib/archunit/extraction/constant_index.rb @@ -42,7 +42,7 @@ def stabilized_visitors def collect(namespace_names: nil) @analyses.to_h do |source, result| visitor = ConstantVisitor.new(namespace_names:) - visitor.visit(result.value) if result.success? + visitor.visit(result.value) [source, visitor] end end diff --git a/lib/archunit/extraction/extract_imports.rb b/lib/archunit/extraction/extract_imports.rb index c079405..57385e7 100644 --- a/lib/archunit/extraction/extract_imports.rb +++ b/lib/archunit/extraction/extract_imports.rb @@ -2,6 +2,7 @@ require 'prism' require_relative '../error/technical_error' +require_relative '../error/source_parse_error' require_relative '../error/user_error' require_relative 'extraction_profile' require_relative 'import_resolver' @@ -57,17 +58,17 @@ def resolve_imports(imports, resolver) def parse_source(source_path, profile) source = ExtractionProfile.measure(profile, :file_read) { File.binread(source_path) } - ExtractionProfile.measure(profile, :prism_parse) do + result = ExtractionProfile.measure(profile, :prism_parse) do Prism.parse(source, filepath: source_path.to_s) end + profile&.increment(:malformed_files) unless result.success? + SourceParseError.validate!(result, path: source_path) rescue SystemCallError => e raise TechnicalError, "could not parse Ruby source file: #{e.message}" end private_class_method :parse_source def parsed_imports(parse_result, profile) - return [] unless parse_result.success? - ExtractionProfile.measure(profile, :import_extraction) do visitor = ImportVisitor.new visitor.visit(parse_result.value) diff --git a/lib/archunit/extraction/extraction_profile.rb b/lib/archunit/extraction/extraction_profile.rb index 2adf0f6..2b82a99 100644 --- a/lib/archunit/extraction/extraction_profile.rb +++ b/lib/archunit/extraction/extraction_profile.rb @@ -20,6 +20,7 @@ class ExtractionProfile ].freeze COUNTERS = %i[ source_files + malformed_files imports unresolved_imports unresolved_constant_references diff --git a/lib/archunit/metrics/extraction/extract_project_info.rb b/lib/archunit/metrics/extraction/extract_project_info.rb index 8f8d2e2..04e01b9 100644 --- a/lib/archunit/metrics/extraction/extract_project_info.rb +++ b/lib/archunit/metrics/extraction/extract_project_info.rb @@ -3,6 +3,7 @@ require 'pathname' require 'prism' require_relative '../../error/technical_error' +require_relative '../../error/source_parse_error' require_relative '../../extraction/enumerate_source_files' require_relative '../../extraction/locate_project' require_relative 'metric_info' @@ -26,15 +27,15 @@ def extract_file_info(source_file, relative_path: nil) path = source_path(source_file) identifier = (relative_path || path.basename.to_s).to_s.tr('\\', '/') source = path.read - build_file_info(identifier, source) + result = SourceParseError.validate!(Prism.parse(source, filepath: path.to_s), path:) + build_file_info(identifier, source, result) rescue SystemCallError => e raise TechnicalError, "could not extract Ruby source metrics: #{e.message}" end - def build_file_info(identifier, source) - parse_result = Prism.parse(source) + def build_file_info(identifier, source, parse_result) visitor = SourceMetricsVisitor.new(identifier) - visitor.visit(parse_result.value) if parse_result.success? + visitor.visit(parse_result.value) FileInfo.new( path: identifier, lines_of_code: lines_of_code(source), **visitor_counts(visitor) ) diff --git a/scripts/check_dependency_analysis.rb b/scripts/check_dependency_analysis.rb index f0d2eb1..fa1c7b0 100644 --- a/scripts/check_dependency_analysis.rb +++ b/scripts/check_dependency_analysis.rb @@ -21,6 +21,29 @@ def verify_project_exclusions(root, rule) abort 'ignored file remained in metrics' unless remaining == ['app/api/request.rb'] end +def reject_malformed_source(rule) + rule.check + abort 'malformed source unexpectedly passed' +rescue ArchUnit::SourceParseError => e + diagnostic = e.diagnostics.first + abort 'parser location missing' unless diagnostic.path.end_with?('request.rb') && + diagnostic.line.positive? && diagnostic.column.positive? +end + +def verify_parse_diagnostics(request, rule) + File.write(request, "class Broken <\n") + ArchUnit.clear_graph_cache + reject_malformed_source(rule) + File.write(request, "path = '../retrieval/store'\nrequire_relative path\n") + ArchUnit.clear_graph_cache +end + +def verify_rule_extensions(root, request, rule) + verify_rationale(rule) + verify_parse_diagnostics(request, rule) + verify_project_exclusions(root, rule) +end + # Also run after installing the built gem, outside Bundler's source checkout. Dir.mktmpdir('archunit-installed-analysis') do |root| File.write(File.join(root, 'Gemfile'), "raise 'must never execute project source'\n") @@ -45,8 +68,7 @@ def verify_project_exclusions(root, rule) abort 'local-string dependency missing' unless rule.check(refreshed).length == 1 ArchUnit.assert_passes(allowed) - verify_rationale(rule) - verify_project_exclusions(root, rule) + verify_rule_extensions(root, request, rule) puts 'Installed dependency analysis, rule rationale, and project exclusions passed.' ensure ArchUnit.clear_graph_cache diff --git a/spec/common/extraction/parse_diagnostic_spec.rb b/spec/common/extraction/parse_diagnostic_spec.rb new file mode 100644 index 0000000..c168b5b --- /dev/null +++ b/spec/common/extraction/parse_diagnostic_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +RSpec.describe ArchUnit::ParseDiagnostic do + it 'normalizes and freezes source data without freezing caller strings' do + path = +'app\\broken.rb' + message = +'unexpected token' + diagnostic = described_class.new(path:, line: 2, column: 3, message:) + path.replace('changed.rb') + message.replace('changed message') + expect(diagnostic.to_h).to eq(path: 'app/broken.rb', line: 2, column: 3, + message: 'unexpected token') + expect(diagnostic.path).to be_frozen + expect(diagnostic.message).to be_frozen + end + + it 'rejects invalid text, coordinates, and malformed diagnostic collections' do + valid = { path: 'file.rb', line: 1, column: 1, message: 'syntax error' } + [{ path: nil }, { path: '' }, { message: nil }, { message: '' }, + { line: 0 }, { column: -1 }, { line: 1.5 }].each do |invalid| + expect { described_class.new(**valid, **invalid) }.to raise_error(ArgumentError) + end + [nil, [], ['not a diagnostic']].each do |invalid| + expect { ArchUnit::SourceParseError.new(invalid) }.to raise_error(ArgumentError) + end + end +end diff --git a/spec/extraction/extract_dependencies_spec.rb b/spec/extraction/extract_dependencies_spec.rb index 637369e..f9682ad 100644 --- a/spec/extraction/extract_dependencies_spec.rb +++ b/spec/extraction/extract_dependencies_spec.rb @@ -94,14 +94,14 @@ def create_file(relative_path, contents = '# fixture') ) end - it 'skips malformed source files without losing dependencies from valid files' do + it 'rejects malformed source instead of returning only dependencies from valid files' do create_file('lib/broken.rb', "require 'hidden'\ndef broken(\n") create_file('lib/valid.rb', "require_relative 'support'\n") create_file('lib/support.rb') - sources = described_class.extract_dependencies(@project_root).map(&:source) - - expect(sources).to eq(['lib/valid.rb']) + expect do + described_class.extract_dependencies(@project_root) + end.to raise_error(ArchUnit::SourceParseError, /broken.rb/) end it 'returns an immutable edge collection' do diff --git a/spec/extraction/extract_graph_spec.rb b/spec/extraction/extract_graph_spec.rb index bef9229..aa24d55 100644 --- a/spec/extraction/extract_graph_spec.rb +++ b/spec/extraction/extract_graph_spec.rb @@ -23,14 +23,10 @@ def extract described_class.extract_graph(@project_root) end - it 'emits a self-edge for every Ruby source, including isolated and malformed files' do + it 'emits a self-edge for every valid Ruby source, including isolated files' do create_file('lib/isolated.rb') - create_file('lib/broken.rb', "def broken(\n") expect(extract).to contain_exactly( - ArchUnit::Edge.new( - source: 'lib/broken.rb', target: 'lib/broken.rb', external: false - ), ArchUnit::Edge.new( source: 'lib/isolated.rb', target: 'lib/isolated.rb', external: false ) diff --git a/spec/extraction/extract_imports_spec.rb b/spec/extraction/extract_imports_spec.rb index b54c3aa..dd98288 100644 --- a/spec/extraction/extract_imports_spec.rb +++ b/spec/extraction/extract_imports_spec.rb @@ -364,11 +364,10 @@ def self.load_dependency expect(extract.map(&:module_name)).to eq(%w[json set]) end - it 'skips a file that Prism cannot parse' do + it 'rejects a file that Prism cannot parse' do create_file('lib/example.rb', "require 'json'\ndef broken(\n") - expect(extract).to eq([]) - expect(extract).to be_frozen + expect { extract }.to raise_error(ArchUnit::SourceParseError, /example.rb/) end it 'returns immutable import collections and values' do diff --git a/spec/extraction/parse_diagnostics_spec.rb b/spec/extraction/parse_diagnostics_spec.rb new file mode 100644 index 0000000..95d013f --- /dev/null +++ b/spec/extraction/parse_diagnostics_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'fileutils' +require 'minitest' +require 'stringio' +require 'tmpdir' + +RSpec.describe 'strict source parsing' do + around do |example| + Dir.mktmpdir('archunit-parse-diagnostics') do |directory| + @root = File.realpath(directory) + File.write(File.join(@root, 'Gemfile'), "raise 'must never execute'\n") + source('app/db/store.rb', 'module Db; class Store; end; end') + source('app/api/request.rb', "require_relative '../db/store'\nclass Broken <\n") + ArchUnit.clear_graph_cache + example.run + ensure + ArchUnit.clear_graph_cache + end + end + + def source(path, contents) + absolute = File.join(@root, path) + FileUtils.mkdir_p(File.dirname(absolute)) + File.write(absolute, contents) + end + + def rule + ArchUnit.project_files(@root).in_path('app/api/**') + .should_not.depend_on_files.in_path('app/db/**') + end + + %i[imports imports_and_constants].each do |mode| + it "rejects the exact malformed-source reproducer in #{mode} mode" do + options = ArchUnit::CheckOptions.new(dependency_analysis: mode) + expect { rule.check(options) }.to raise_error(ArchUnit::TechnicalError, /request.rb/) + end + + it "reports structured diagnostics and a malformed-file counter in #{mode} mode" do + options = ArchUnit::CheckOptions.new(dependency_analysis: mode) + profile = ArchUnit::Extraction::ExtractionProfile.new + expect do + ArchUnit::Extraction.extract_graph(@root, options:, profile:) + end.to raise_error(ArchUnit::SourceParseError) { |error| + expect(error.diagnostics).not_to be_empty + expect(error.diagnostics).to be_frozen + attributes = { path: File.join(@root, 'app/api/request.rb').tr('\\', '/'), + line: be >= 2, column: be >= 1, message: be_a(String) } + expect(error.diagnostics).to all(have_attributes(attributes)) + expect(error.diagnostics).to all(be_frozen) + expect(error.diagnostics.first.to_h.keys).to eq(%i[path line column message]) + } + expect(profile.counters.fetch(:malformed_files)).to eq(1) + expect(profile).not_to be_cache_hit + end + end + + it 'propagates technical failure through assertions and both framework adapters' do + ArchUnit::Testing::MinitestAdapter.install! + expect { ArchUnit.assert_passes(rule) }.to raise_error(ArchUnit::TechnicalError) + expect { expect(rule).to pass }.to raise_error(ArchUnit::TechnicalError) + context = Class.new do + include Minitest::Assertions + + attr_accessor :assertions + end.new + context.assertions = 0 + expect { context.assert_passes(rule) }.to raise_error(ArchUnit::TechnicalError) + expect(context.assertions).to eq(0) + end + + it 'does not cache failed builds and rejects new errors after explicit cache clearing' do + expect { rule.check }.to raise_error(ArchUnit::TechnicalError) + source('app/api/request.rb', "require_relative '../db/store'\n") + expect(rule.check.length).to eq(1) + source('app/db/store.rb', 'class Broken <') + expect(rule.check.length).to eq(1) # Source edits retain the documented explicit-clear policy. + expect do + rule.check(ArchUnit::CheckOptions.new(clear_cache: true)) + end.to raise_error(ArchUnit::SourceParseError, /store.rb/) + expect { rule.check }.to raise_error(ArchUnit::SourceParseError) + end + + it 'does not inspect malformed files excluded by the project source policy' do + source('.archignore', "app/api/\n") + graph = ArchUnit::Extraction.extract_graph(@root) + expect(graph.map(&:source).uniq).to eq(['app/db/store.rb']) + expect(ArchUnit.metrics(@root).count.classes.measure.length).to eq(1) + end + + it 'prevents partial graph reports and misleading zero-valued AST metrics' do + expect { ArchUnit.project_graph(@root).to_json }.to raise_error(ArchUnit::TechnicalError) + expect { ArchUnit.metrics(@root).count.classes.measure }.to raise_error(ArchUnit::TechnicalError) + end + + it 'logs the parser failure rather than a successful check' do + output = StringIO.new + options = ArchUnit::CheckOptions.new(logging: ArchUnit::LoggingOptions.new(io: output)) + expect { rule.check(options) }.to raise_error(ArchUnit::TechnicalError) + expect(output.string).to include('request.rb', 'SourceParseError') + end +end diff --git a/spec/metrics/extraction/extract_project_info_spec.rb b/spec/metrics/extraction/extract_project_info_spec.rb index c2c52cc..38e12f7 100644 --- a/spec/metrics/extraction/extract_project_info_spec.rb +++ b/spec/metrics/extraction/extract_project_info_spec.rb @@ -146,15 +146,13 @@ def perform end end - it 'returns physical code lines but no AST counts for malformed Ruby' do + it 'rejects malformed Ruby instead of reporting misleading zero AST counts' do Dir.mktmpdir do |root| path = write_source(root, 'broken.rb', "class Broken\n def nope(\nend\n") - info = described_class.extract_file_info(path) - - expect(info.lines_of_code).to eq(3) - expect([info.statement_count, info.class_count, info.function_count]).to eq([0, 0, 0]) - expect(info.class_infos).to be_empty + expect do + described_class.extract_file_info(path) + end.to raise_error(ArchUnit::SourceParseError, /broken.rb/) end end