From 0759e94205237229ef8c2eeede3343f7f483ca94 Mon Sep 17 00:00:00 2001 From: crimson-knight Date: Wed, 23 Sep 2026 15:03:20 -0400 Subject: [PATCH 1/3] Add library rule packs to amber-lsp --- docs/rule-packs.md | 101 +++++ spec/amber_lsp/analyzer_spec.cr | 8 + spec/amber_lsp/rule_packs_spec.cr | 423 ++++++++++++++++++ .../controllers/action_return_rule_spec.cr | 57 +++ spec/amber_lsp/spec_helper.cr | 8 +- src/amber_lsp.cr | 12 +- src/amber_lsp/analyzer.cr | 26 ++ src/amber_lsp/controller.cr | 6 +- .../analyze_project_files_with_rule_packs.cr | 264 +++++++++++ .../describe_library_rule_pack.cr | 119 +++++ .../determine_project_rule_pack_state.cr | 136 ++++++ .../load_rule_packs_for_project.cr | 90 ++++ .../print_declared_rule_pack_contexts.cr | 90 ++++ ...t_crystal_calls_outside_required_blocks.cr | 159 +++++++ src/amber_lsp/project_context.cr | 47 +- src/amber_lsp/rules/base_rule.cr | 4 + .../rules/controllers/action_return_rule.cr | 19 +- src/amber_lsp/rules/custom_rule.cr | 4 + src/amber_lsp/rules/rule_registry.cr | 5 + 19 files changed, 1563 insertions(+), 15 deletions(-) create mode 100644 docs/rule-packs.md create mode 100644 spec/amber_lsp/rule_packs_spec.cr create mode 100644 src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr create mode 100644 src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr create mode 100644 src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr create mode 100644 src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr create mode 100644 src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr create mode 100644 src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks.cr diff --git a/docs/rule-packs.md b/docs/rule-packs.md new file mode 100644 index 0000000..2a2b662 --- /dev/null +++ b/docs/rule-packs.md @@ -0,0 +1,101 @@ +# Library rule packs + +Library rule packs let a Crystal library ship opt-in feature guidance and edit-time checks with the same `.claude/` files that Shards distributes. `amber-lsp` reads packs from installed dependencies at `lib/*/.claude/rules/*.yml` and from the project at `.claude/rules/*.yml`; no copy step is required. A project pack with the same `pack` id overrides a dependency pack. A pack whose `library` equals the project's own shard name does not apply, because that pack describes the library's consumer contract. + +## Pack document + +Each YAML file has a pack identity, declared modes, optional evidence patterns, mode-specific context, and rules: + +```yaml +pack: grant/tenancy +library: grant +version: 1.0.0 + +modes: + row: + declared_by: + key_path: grant.tenancy + expected_value: row + evidence: + - '^\s*multitenant\b' + tenant_column: tenant_id + context: | + Scope queries with the library's tenant context. + schema: + declared_by: + key_path: grant.tenancy + expected_value: schema + evidence: + - '^\s*SchemaTenant\.with\b' + context: | + Schema-specific rules are not included yet. + +rules: + - id: grant/example + modes: [row] + severity: warning + applies_to: ["src/**"] + exclude_from: ["src/controllers/**"] + message: "Explain the finding and the repair." + check: + kind: line_regex + pattern: '\bunsafe_call\b' +``` + +`declared_by.key_path` is a dotted path into the application's `shard.yml`; `expected_value` is compared as a string. A pack rule runs only when one of its modes is declared. Evidence patterns scan the application's `.cr` files outside `lib/`, `.git/`, `tmp/`, and `vendor/`; they support mixed-mode detection and the undeclared-feature warning. Evidence alone does not activate mode-specific rules. Context blocks are printed only for declared modes and should stay to about 15 lines or fewer. + +Each rule requires an id, one or more mode names, a severity, file globs, a message, and one check. Severity values are `error`, `warning`, `info`, and `hint`. `exclude_from` is optional. `applies_to` and `exclude_from` use Crystal path globs, including `**` for recursive directories. Rules can be enabled, disabled, or have their severity overridden through the existing `.amber-lsp.yml` `rules` mapping. + +## Check kinds + +### `line_regex` + +Matches a regular expression on each line and uses the existing custom-rule diagnostic matcher. `negate: true` reports once at the start of a file when the pattern does not occur. + +```yaml +check: + kind: line_regex + pattern: '^\s*[^#]*\.unscoped\b' +``` + +### `file_requires` + +Reports on each line matching `trigger_pattern` when the file has no line matching `required_pattern`. If `trigger_pattern` is omitted, `amber-lsp` uses the first mode's `tenant_column` as a Crystal `column ` declaration. + +```yaml +check: + kind: file_requires + required_pattern: '^\s*multitenant\b' +``` + +### `call_outside_block` + +Uses `Crystal::Parser` and a `Crystal::Visitor` to inspect method calls. `source_globs` select the files where `tenant_macro` declarations identify scoped model names. `methods` lists calls on those model constants that can query the database. A dotted `required_call`, such as `Grant::Tenant.with`, permits calls lexically inside its block and nested blocks. `escape_call` permits calls only inside a block on that same model receiver, such as `Todo.unscoped { Todo.where(...) }`. A method definition starts a new lexical scope, so a query written in a method body does not become tenant-scoped just because the method definition appears inside a tenant block. Receiver constants are followed through chained calls, but model instances held in variables are not resolved. The current matcher uses the last constant name, so same-named models in different namespaces can be ambiguous. + +```yaml +check: + kind: call_outside_block + source_globs: ["src/models/**"] + tenant_macro: multitenant + methods: [where, find, all, first, count] + required_call: Grant::Tenant.with + escape_call: unscoped +``` + +`required_call` may be omitted when `escape_call` alone defines the allowed block. The rule intentionally does not infer receiver types or follow aliases stored in local variables. + +### `project_conflict` + +Runs over project state rather than one source line. The default `condition: mixed_modes` reports when every listed mode is declared or has evidence. `condition: evidence_without_declaration` reports when a listed mode has feature evidence but its declaration is absent; use one mode for this warning. + +```yaml +check: + kind: project_conflict + condition: mixed_modes +``` + +## Project activation and context + +Amber's built-in convention rules remain gated on an `amber` dependency. Library packs run in any project when the pack applies, including a project with only a Grant dependency. Pack diagnostics use the ordinary LSP diagnostic channel and `.amber-lsp.yml` severity overrides. + +Run `amber-lsp context [--root DIR]` to print the context blocks for declared pack modes and one warning line per pack whose feature is used without a declaration. It prints nothing when no pack is declared or evidenced. Exit status is 0 for successful context inspection; invalid command arguments or an unreadable project return nonzero. diff --git a/spec/amber_lsp/analyzer_spec.cr b/spec/amber_lsp/analyzer_spec.cr index 13f5f6e..be0d503 100644 --- a/spec/amber_lsp/analyzer_spec.cr +++ b/spec/amber_lsp/analyzer_spec.cr @@ -2,6 +2,10 @@ require "./spec_helper" # A mock rule for testing the analyzer class MockTestRule < AmberLSP::Rules::BaseRule + def requires_amber_project? : Bool + false + end + def id : String "mock/test-rule" end @@ -39,6 +43,10 @@ end # A mock rule that only applies to controller files class MockControllerRule < AmberLSP::Rules::BaseRule + def requires_amber_project? : Bool + false + end + def id : String "mock/controller-rule" end diff --git a/spec/amber_lsp/rule_packs_spec.cr b/spec/amber_lsp/rule_packs_spec.cr new file mode 100644 index 0000000..2d9f4c1 --- /dev/null +++ b/spec/amber_lsp/rule_packs_spec.cr @@ -0,0 +1,423 @@ +require "./spec_helper" +require "../../src/amber_lsp/rules/controllers/action_return_rule" + +GRANT_RULE_PACK_FIXTURE = <<-YAML + pack: grant/tenancy + library: grant + version: 1.0.0 + modes: + row: + declared_by: + key_path: grant.tenancy + expected_value: row + evidence: + - '^\\s*multitenant\\b' + tenant_column: tenant_id + context: | + Scope queries with Grant::Tenant.with. + schema: + declared_by: + key_path: grant.tenancy + expected_value: schema + evidence: + - '^\\s*Grant::SchemaTenant\\.with\\b' + - '^\\s*schema_tenant_excluded\\b' + context: | + Schema-specific rules are not included yet. + rules: + - id: grant/tenant-column-without-multitenant + modes: [row] + severity: error + applies_to: ["src/models/**"] + message: Tenant columns require multitenant. + check: + kind: file_requires + required_pattern: '^\\s*multitenant\\b' + - id: grant/tenancy-modes-mixed + modes: [row, schema] + severity: error + applies_to: ["**/*.cr"] + message: Choose one tenancy mode. + check: + kind: project_conflict + condition: mixed_modes + - id: grant/row-query-outside-tenant + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + exclude_from: ["src/controllers/**"] + message: Query must be tenant-scoped. + check: + kind: call_outside_block + source_globs: ["src/models/**"] + tenant_macro: multitenant + methods: [all, where, find!] + required_call: Grant::Tenant.with + escape_call: unscoped + - id: grant/unscoped-in-request-code + modes: [row] + severity: warning + applies_to: ["src/controllers/**"] + message: Do not use unscoped in request code. + check: + kind: line_regex + pattern: '^\\s*[^#]*\\.unscoped\\b' + - id: grant/raw-sql-on-scoped-model + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + message: Raw SQL must use an unscoped block. + check: + kind: call_outside_block + source_globs: ["src/models/**"] + tenant_macro: multitenant + methods: [exec, query, scalar] + escape_call: unscoped + - id: grant/tenancy-undeclared + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + message: Declare Grant tenancy in shard.yml. + check: + kind: project_conflict + condition: evidence_without_declaration + YAML + +def write_rule_pack_project( + root : String, + shard_content : String = "name: tenant_app\nversion: 0.1.0\ngrant:\n tenancy: row\n", + pack_content : String = GRANT_RULE_PACK_FIXTURE, +) : Nil + Dir.mkdir_p(File.join(root, ".claude", "rules")) + File.write(File.join(root, "shard.yml"), shard_content) + File.write(File.join(root, ".claude", "rules", "tenancy.yml"), pack_content) +end + +def analyze_pack_file(root : String, file_path : String, content : String) : Array(AmberLSP::Rules::Diagnostic) + Dir.mkdir_p(File.dirname(file_path)) + File.write(file_path, content) + project_context = AmberLSP::ProjectContext.detect(root) + analyzer = AmberLSP::Analyzer.new + analyzer.configure(project_context) + analyzer.analyze(file_path, content) +end + +def diagnostic_codes(diagnostics : Array(AmberLSP::Rules::Diagnostic)) : Array(String) + diagnostics.map(&.code) +end + +describe "AmberLSP library rule packs" do + before_each do + AmberLSP::Rules::RuleRegistry.clear + end + + it "loads packs from installed dependencies and project rules" do + with_tempdir do |root| + write_rule_pack_project(root) + dependency_root = File.join(root, "dependency_source") + dependency_pack_path = File.join(dependency_root, ".claude", "rules", "tenancy.yml") + project_pack_path = File.join(root, ".claude", "rules", "project.yml") + Dir.mkdir_p(File.dirname(dependency_pack_path)) + File.write(dependency_pack_path, GRANT_RULE_PACK_FIXTURE) + Dir.mkdir_p(File.join(root, "lib")) + File.symlink(dependency_root, File.join(root, "lib", "grant")) + File.write( + project_pack_path, + GRANT_RULE_PACK_FIXTURE.gsub("grant/tenancy", "project/tenancy").gsub("library: grant", "library: project"), + ) + + project_context = AmberLSP::ProjectContext.detect(root) + packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs + + packs.map(&.pack_id).should eq(["grant/tenancy", "project/tenancy"]) + end + end + + it "keeps a library's own pack inactive while allowing clean LSP checks" do + with_tempdir do |root| + shard_content = "name: grant\nversion: 0.1.0\n" + write_rule_pack_project(root, shard_content) + + project_context = AmberLSP::ProjectContext.detect(root) + packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs + packs.map(&.pack_id).should eq(["grant/tenancy"]) + + file_path = File.join(root, "src", "grant", "scale", "tenant.cr") + content = "multitenant :tenant_id\n" + analyzer = AmberLSP::Analyzer.new + analyzer.configure(project_context) + analyzer.has_applicable_library_rule_pack?(file_path, content).should be_true + analyzer.analyze(file_path, content).should be_empty + end + end + + it "runs a declared dependency pack in a non-Amber project" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "jobs")) + file_path = File.join(root, "src", "jobs", "fixture.cr") + diagnostics = analyze_pack_file(root, file_path, "puts \"unscoped\"\n") + + diagnostics.map(&.code).should eq([] of String) + end + end + + it "runs line_regex checks and ignores full-line comments" do + with_tempdir do |root| + write_rule_pack_project(root) + file_path = File.join(root, "src", "controllers", "todos_controller.cr") + content = "# Todo.unscoped is only documentation\nTodo.unscoped.all\n" + + diagnostics = analyze_pack_file(root, file_path, content) + + diagnostics.map(&.code).should eq(["grant/unscoped-in-request-code"]) + diagnostics.first.range.start.line.should eq(1) + diagnostics.first.severity.should eq(AmberLSP::Rules::Severity::Warning) + end + end + + it "reports a tenant column without multitenant and accepts the declared macro" do + with_tempdir do |root| + write_rule_pack_project(root) + file_path = File.join(root, "src", "models", "account.cr") + Dir.mkdir_p(File.dirname(file_path)) + missing_macro = "class Account\n column tenant_id : Int64\nend\n" + + diagnostics = analyze_pack_file(root, file_path, missing_macro) + + diagnostics.map(&.code).should contain("grant/tenant-column-without-multitenant") + diagnostics.find(&.code.==("grant/tenant-column-without-multitenant")).not_nil!.severity.should eq(AmberLSP::Rules::Severity::Error) + + valid_model = "class Account\n column tenant_id : Int64\n multitenant :tenant_id\nend\n" + valid_diagnostics = analyze_pack_file(root, file_path, valid_model) + + valid_diagnostics.map(&.code).should_not contain("grant/tenant-column-without-multitenant") + end + end + + it "uses the configured tenant column for file_requires" do + with_tempdir do |root| + pack_content = GRANT_RULE_PACK_FIXTURE.gsub("tenant_column: tenant_id", "tenant_column: account_id") + write_rule_pack_project(root, pack_content: pack_content) + file_path = File.join(root, "src", "models", "account.cr") + Dir.mkdir_p(File.dirname(file_path)) + + diagnostics = analyze_pack_file(root, file_path, "class Account\n column account_id : Int64\nend\n") + + diagnostics.map(&.code).should contain("grant/tenant-column-without-multitenant") + end + end + + it "reports a multitenant query outside the required block" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + + diagnostics = analyze_pack_file(root, file_path, "Todo.where(active: true)\n") + + diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + end + end + + it "accepts queries inside the required block and nested blocks" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + content = <<-CRYSTAL + Grant::Tenant.with(7) do + Todo.where(active: true) + run do + Todo.all + end + end + CRYSTAL + + diagnostics = analyze_pack_file(root, file_path, content) + + diagnostics.map(&.code).should_not contain("grant/row-query-outside-tenant") + end + end + + it "does not treat a method defined inside a tenant block as scoped" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + content = <<-CRYSTAL + Grant::Tenant.with(7) do + def load_todos + Todo.all + end + end + CRYSTAL + + diagnostics = analyze_pack_file(root, file_path, content) + + diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + end + end + + it "does not scope a method body just because its call is inside a tenant block" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + content = <<-CRYSTAL + def load_todos + Todo.all + end + Grant::Tenant.with(7) { load_todos } + CRYSTAL + + diagnostics = analyze_pack_file(root, file_path, content) + + diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + end + end + + it "allows a same-model unscoped block but not a different model's query" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + File.write(File.join(root, "src", "models", "invoice.cr"), "class Invoice\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + + same_model = analyze_pack_file(root, file_path, "Todo.unscoped { Todo.where(active: true) }\n") + same_model.map(&.code).should_not contain("grant/row-query-outside-tenant") + + different_model = analyze_pack_file(root, file_path, "Todo.unscoped { Invoice.all }\n") + different_model.map(&.code).should contain("grant/row-query-outside-tenant") + end + end + + it "does not flag raw_all and allows raw SQL inside the same model's unscoped block" do + with_tempdir do |root| + write_rule_pack_project(root) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + file_path = File.join(root, "src", "jobs", "cleanup_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + + raw_all_diagnostics = analyze_pack_file(root, file_path, "Todo.raw_all(\"WHERE active = true\")\n") + raw_all_diagnostics.map(&.code).should_not contain("grant/raw-sql-on-scoped-model") + + scoped_sql = <<-CRYSTAL + Todo.unscoped do + Todo.exec("DELETE FROM todos") + Todo.query("SELECT 1") { } + Todo.scalar("SELECT COUNT(*) FROM todos") { |value| value } + end + CRYSTAL + scoped_sql_diagnostics = analyze_pack_file(root, file_path, scoped_sql) + scoped_sql_diagnostics.map(&.code).should_not contain("grant/raw-sql-on-scoped-model") + + unsafe_sql = "Todo.exec(\"DELETE FROM todos\")\nTodo.query(\"SELECT 1\") { }\nTodo.scalar(\"SELECT 1\") { |value| value }\n" + unsafe_sql_diagnostics = analyze_pack_file(root, file_path, unsafe_sql) + unsafe_sql_diagnostics.count(&.code.==("grant/raw-sql-on-scoped-model")).should eq(3) + end + end + + it "reports mixed modes as an error and undeclared row use as a warning" do + with_tempdir do |root| + write_rule_pack_project(root) + file_path = File.join(root, "src", "jobs", "tenancy_job.cr") + Dir.mkdir_p(File.dirname(file_path)) + mixed_content = "Grant::SchemaTenant.with(\"acme\") { run_job }\n" + + mixed_diagnostics = analyze_pack_file(root, file_path, mixed_content) + mixed_diagnostic = mixed_diagnostics.find(&.code.==("grant/tenancy-modes-mixed")).not_nil! + mixed_diagnostic.severity.should eq(AmberLSP::Rules::Severity::Error) + + shard_content = "name: tenant_app\nversion: 0.1.0\n" + write_rule_pack_project(root, shard_content) + Dir.mkdir_p(File.join(root, "src", "models")) + File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") + undeclared_path = File.join(root, "src", "jobs", "undeclared_job.cr") + undeclared_diagnostics = analyze_pack_file(root, undeclared_path, "Todo.all\n") + undeclared = undeclared_diagnostics.find(&.code.==("grant/tenancy-undeclared")).not_nil! + undeclared.severity.should eq(AmberLSP::Rules::Severity::Warning) + end + end + + it "keeps Amber built-in rules gated on Amber dependencies" do + with_tempdir do |root| + write_rule_pack_project(root) + file_path = File.join(root, "src", "controllers", "home_controller.cr") + Dir.mkdir_p(File.dirname(file_path)) + content = "class HomeController < ApplicationController\n def index\n User.all\n end\nend\n" + AmberLSP::Rules::RuleRegistry.register(AmberLSP::Rules::Controllers::ActionReturnRule.new) + non_amber_analyzer = AmberLSP::Analyzer.new + non_amber_analyzer.configure(AmberLSP::ProjectContext.detect(root)) + + non_amber_diagnostics = non_amber_analyzer.analyze(file_path, content) + non_amber_diagnostics.map(&.code).should_not contain("amber/action-return-type") + + shard_content = <<-YAML + name: tenant_app + version: 0.1.0 + grant: + tenancy: row + dependencies: + amber: + github: amberframework/amber + YAML + File.write(File.join(root, "shard.yml"), shard_content) + amber_analyzer = AmberLSP::Analyzer.new + amber_analyzer.configure(AmberLSP::ProjectContext.detect(root)) + + amber_diagnostics = amber_analyzer.analyze(file_path, content) + amber_diagnostics.map(&.code).should contain("amber/action-return-type") + end + end + + it "prints declared context and warns when feature use has no declaration" do + with_tempdir do |root| + write_rule_pack_project(root) + file_path = File.join(root, "src", "models", "todo.cr") + Dir.mkdir_p(File.dirname(file_path)) + File.write(file_path, "class Todo\n multitenant :tenant_id\nend\n") + binary_path = File.join(Dir.current, "bin", "amber-lsp") + stdout = IO::Memory.new + stderr = IO::Memory.new + status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: stderr) + + status.success?.should be_true + stderr.to_s.should be_empty + stdout.to_s.should contain("grant/tenancy (row)") + stdout.to_s.should contain("Scope queries with Grant::Tenant.with.") + + undeclared_shard = "name: tenant_app\nversion: 0.1.0\n" + write_rule_pack_project(root, undeclared_shard) + stdout = IO::Memory.new + stderr = IO::Memory.new + status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: stderr) + + status.success?.should be_true + stdout.to_s.should contain("warning: grant/tenancy feature is used but shard.yml does not declare grant.tenancy.") + end + end + + it "prints nothing when no pack is declared or evidenced" do + with_tempdir do |root| + File.write(File.join(root, "shard.yml"), "name: empty_app\nversion: 0.1.0\n") + binary_path = File.join(Dir.current, "bin", "amber-lsp") + stdout = IO::Memory.new + status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: Process::Redirect::Close) + + status.success?.should be_true + stdout.to_s.should be_empty + end + end +end diff --git a/spec/amber_lsp/rules/controllers/action_return_rule_spec.cr b/spec/amber_lsp/rules/controllers/action_return_rule_spec.cr index 7a0b6fb..76ff0ee 100644 --- a/spec/amber_lsp/rules/controllers/action_return_rule_spec.cr +++ b/spec/amber_lsp/rules/controllers/action_return_rule_spec.cr @@ -80,6 +80,63 @@ describe AmberLSP::Rules::Controllers::ActionReturnRule do diagnostics.should be_empty end + it "produces no diagnostics when actions use application response helpers" do + content = <<-CRYSTAL + class HomeController < ApplicationController + def index + respond_json(200, {ok: true}) + end + end + CRYSTAL + + rule = AmberLSP::Rules::Controllers::ActionReturnRule.new + diagnostics = rule.check("src/controllers/home_controller.cr", content) + diagnostics.should be_empty + end + + it "produces no diagnostics when actions write the response directly" do + content = <<-CRYSTAL + class HomeController < ApplicationController + def index + context.response.print("ok") + end + end + CRYSTAL + + rule = AmberLSP::Rules::Controllers::ActionReturnRule.new + diagnostics = rule.check("src/controllers/home_controller.cr", content) + diagnostics.should be_empty + end + + it "produces no diagnostics when actions hand the response to a writer helper" do + content = <<-CRYSTAL + class HomeController < ApplicationController + def connect + send_event(context.response, "endpoint") + end + end + CRYSTAL + + rule = AmberLSP::Rules::Controllers::ActionReturnRule.new + diagnostics = rule.check("src/controllers/home_controller.cr", content) + diagnostics.should be_empty + end + + it "skips class methods and controller support files" do + content = <<-CRYSTAL + class HomeController < ApplicationController + def self.build + "helper" + end + end + CRYSTAL + + rule = AmberLSP::Rules::Controllers::ActionReturnRule.new + rule.check("src/controllers/home_controller.cr", content).should be_empty + rule.check("src/controllers/application_controller.cr", content).should be_empty + rule.check("src/controllers/concerns/session_state.cr", content).should be_empty + end + it "reports warning when action does not call any response method" do content = <<-CRYSTAL class HomeController < ApplicationController diff --git a/spec/amber_lsp/spec_helper.cr b/spec/amber_lsp/spec_helper.cr index 93c6d9f..822b2f0 100644 --- a/spec/amber_lsp/spec_helper.cr +++ b/spec/amber_lsp/spec_helper.cr @@ -9,6 +9,12 @@ require "../../src/amber_lsp/rules/custom_rule" require "../../src/amber_lsp/document_store" require "../../src/amber_lsp/project_context" require "../../src/amber_lsp/configuration" +require "../../src/amber_lsp/library_rule_packs/describe_library_rule_pack" +require "../../src/amber_lsp/library_rule_packs/load_rule_packs_for_project" +require "../../src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks" +require "../../src/amber_lsp/library_rule_packs/determine_project_rule_pack_state" +require "../../src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs" +require "../../src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts" require "../../src/amber_lsp/analyzer" require "../../src/amber_lsp/controller" require "../../src/amber_lsp/server" @@ -29,7 +35,7 @@ def format_lsp_message(message) : String end def run_lsp_session(messages : Array) : Array(JSON::Any) - input_data = messages.map { |m| format_lsp_message(m) }.join + input_data = messages.map { |message| format_lsp_message(message) }.join input = IO::Memory.new(input_data) output = IO::Memory.new diff --git a/src/amber_lsp.cr b/src/amber_lsp.cr index 472f055..d4ae72e 100644 --- a/src/amber_lsp.cr +++ b/src/amber_lsp.cr @@ -21,8 +21,18 @@ require "./amber_lsp/rules/custom_rule" require "./amber_lsp/document_store" require "./amber_lsp/project_context" require "./amber_lsp/configuration" +require "./amber_lsp/library_rule_packs/describe_library_rule_pack" +require "./amber_lsp/library_rule_packs/load_rule_packs_for_project" +require "./amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks" +require "./amber_lsp/library_rule_packs/determine_project_rule_pack_state" +require "./amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs" +require "./amber_lsp/library_rule_packs/print_declared_rule_pack_contexts" require "./amber_lsp/analyzer" require "./amber_lsp/controller" require "./amber_lsp/server" -AmberLSP::Server.new(STDIN, STDOUT).run +if ARGV.first? == "context" + exit AmberLSP::LibraryRulePacks::PrintDeclaredRulePackContexts.new(ARGV[1..].to_a).perform +else + AmberLSP::Server.new(STDIN, STDOUT).run +end diff --git a/src/amber_lsp/analyzer.cr b/src/amber_lsp/analyzer.cr index 58186a4..30c8140 100644 --- a/src/amber_lsp/analyzer.cr +++ b/src/amber_lsp/analyzer.cr @@ -2,18 +2,36 @@ module AmberLSP class Analyzer getter configuration : Configuration @project_root : String? + @project_context : ProjectContext? + @library_rule_pack_analyzer : LibraryRulePacks::AnalyzeProjectFilesWithRulePacks? def initialize @configuration = Configuration.new @project_root = nil + @project_context = nil + @library_rule_pack_analyzer = nil end def configure(project_context : ProjectContext) : Nil @configuration = Configuration.load(project_context.root_path) @project_root = project_context.root_path + @project_context = project_context + list_of_rule_packs = LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs + @library_rule_pack_analyzer = LibraryRulePacks::AnalyzeProjectFilesWithRulePacks.new( + project_context, + @configuration, + list_of_rule_packs, + ) register_custom_rules end + def has_applicable_library_rule_pack?(file_path : String, content : String) : Bool + analyzer = @library_rule_pack_analyzer + return false unless analyzer + + analyzer.has_applicable_pack?(file_path, content) + end + private def register_custom_rules : Nil @configuration.custom_rules.each do |custom_config| severity = case custom_config.severity @@ -48,6 +66,10 @@ module AmberLSP rules.each do |rule| next unless @configuration.rule_enabled?(rule.id) + if rule.requires_amber_project? + project_context = @project_context + next unless project_context && project_context.amber_project? + end rule_diagnostics = rule.check(file_path, content) severity = @configuration.rule_severity(rule.id, rule.default_severity) @@ -67,6 +89,10 @@ module AmberLSP end end + if analyzer = @library_rule_pack_analyzer + diagnostics.concat(analyzer.list_of_diagnostics_for(file_path, content)) + end + diagnostics end diff --git a/src/amber_lsp/controller.cr b/src/amber_lsp/controller.cr index 328ead6..61204cb 100644 --- a/src/amber_lsp/controller.cr +++ b/src/amber_lsp/controller.cr @@ -168,9 +168,9 @@ module AmberLSP # Only analyze Crystal files return unless file_path.ends_with?(".cr") - # Only run if we detected an Amber project ctx = @project_context - return unless ctx && ctx.amber_project? + return unless ctx + return unless ctx.amber_project? || @analyzer.has_applicable_library_rule_pack?(file_path, content) diagnostics = @analyzer.analyze(file_path, content) publish_diagnostics(uri, diagnostics, server) @@ -184,7 +184,7 @@ module AmberLSP "method" => JSON::Any.new("textDocument/publishDiagnostics"), "params" => JSON::Any.new({ "uri" => JSON::Any.new(uri), - "diagnostics" => JSON::Any.new(lsp_diagnostics.map { |d| JSON::Any.new(d) }), + "diagnostics" => JSON::Any.new(lsp_diagnostics.map { |diagnostic| JSON::Any.new(diagnostic) }), }), } diff --git a/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr b/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr new file mode 100644 index 0000000..da0595d --- /dev/null +++ b/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr @@ -0,0 +1,264 @@ +module AmberLSP::LibraryRulePacks + class AnalyzeProjectFilesWithRulePacks + def initialize( + @project_context : AmberLSP::ProjectContext, + @configuration : AmberLSP::Configuration, + @list_of_rule_packs : Array(DescribeLibraryRulePack), + ) + end + + def has_applicable_pack?(file_path : String, content : String) : Bool + @list_of_rule_packs.any? do |rule_pack| + next true if library_is_project_root?(rule_pack) + + DetermineProjectRulePackState.new(@project_context, rule_pack, file_path, content) + .has_any_applicable_mode? + end + end + + def list_of_diagnostics_for(file_path : String, content : String) : Array(Rules::Diagnostic) + relative_file_path = project_relative_path(file_path) + list_of_diagnostics = [] of Rules::Diagnostic + + @list_of_rule_packs.each do |rule_pack| + next if library_is_project_root?(rule_pack) + + project_state = DetermineProjectRulePackState.new( + @project_context, + rule_pack, + file_path, + content, + ) + + rule_pack.list_of_rules.each do |rule| + next unless @configuration.rule_enabled?(rule.rule_id) + next unless rule_applies_to_file?(rule, relative_file_path) + + if rule.check.check_kind == "project_conflict" + list_of_diagnostics.concat( + project_conflict_diagnostics_for(rule_pack, rule, project_state) + ) + next + end + + next unless project_state.is_rule_mode_declared?(rule.list_of_mode_names) + + list_of_diagnostics.concat( + file_diagnostics_for(rule_pack, rule, project_state, file_path, relative_file_path, content) + ) + end + end + + apply_configured_severity(list_of_diagnostics) + end + + private def library_is_project_root?(rule_pack : DescribeLibraryRulePack) : Bool + @project_context.shard_name == rule_pack.library_shard_name + end + + private def rule_applies_to_file?(rule : DescribeLibraryRulePack::Rule, relative_file_path : String) : Bool + applies = rule.list_of_applicable_globs.any? do |pattern| + Rules::RuleRegistry.file_matches_pattern?(relative_file_path, pattern) + end + return false unless applies + + rule.list_of_excluded_globs.none? do |pattern| + Rules::RuleRegistry.file_matches_pattern?(relative_file_path, pattern) + end + end + + private def project_conflict_diagnostics_for( + rule_pack : DescribeLibraryRulePack, + rule : DescribeLibraryRulePack::Rule, + project_state : DetermineProjectRulePackState, + ) : Array(Rules::Diagnostic) + conflict_found = case rule.check.project_condition + when "mixed_modes" + project_state.all_modes_are_present?(rule.list_of_mode_names) + when "evidence_without_declaration" + project_state.has_undeclared_mode_evidence?(rule.list_of_mode_names) + else + false + end + return [] of Rules::Diagnostic unless conflict_found + + [diagnostic_at_start_of_file(rule, severity_for(rule.severity_name))] + end + + private def file_diagnostics_for( + rule_pack : DescribeLibraryRulePack, + rule : DescribeLibraryRulePack::Rule, + project_state : DetermineProjectRulePackState, + file_path : String, + relative_file_path : String, + content : String, + ) : Array(Rules::Diagnostic) + case rule.check.check_kind + when "line_regex" + line_regex_diagnostics_for(rule, relative_file_path, content) + when "file_requires" + file_requires_diagnostics_for(rule_pack, rule, content) + when "call_outside_block" + call_outside_block_diagnostics_for(rule, project_state, file_path, content) + else + [] of Rules::Diagnostic + end + end + + private def line_regex_diagnostics_for( + rule : DescribeLibraryRulePack::Rule, + relative_file_path : String, + content : String, + ) : Array(Rules::Diagnostic) + custom_rule = Rules::CustomRule.new( + id: rule.rule_id, + description: rule.diagnostic_message, + default_severity: severity_for(rule.severity_name), + applies_to: rule.list_of_applicable_globs, + pattern: Regex.new(rule.check.regex_pattern), + message_template: rule.diagnostic_message, + negate: rule.check.negates_pattern?, + ) + custom_rule.check(relative_file_path, content) + end + + private def file_requires_diagnostics_for( + rule_pack : DescribeLibraryRulePack, + rule : DescribeLibraryRulePack::Rule, + content : String, + ) : Array(Rules::Diagnostic) + required_pattern = Regex.new(rule.check.required_regex_pattern) + return [] of Rules::Diagnostic if content.each_line.any? { |line| required_pattern.matches?(line) } + + trigger_pattern = trigger_pattern_for(rule_pack, rule) + return [] of Rules::Diagnostic unless trigger_pattern + + list_of_diagnostics = [] of Rules::Diagnostic + content.each_line.with_index do |line, line_number| + match = trigger_pattern.match(line) + next unless match + + start_character = (match.begin(0) || 0).to_i32 + end_character = (match.end(0) || line.size).to_i32 + range = Rules::TextRange.new( + Rules::Position.new(line_number.to_i32, start_character), + Rules::Position.new(line_number.to_i32, end_character), + ) + list_of_diagnostics << Rules::Diagnostic.new( + range, + severity_for(rule.severity_name), + rule.rule_id, + rule.diagnostic_message, + ) + end + + list_of_diagnostics + end + + private def trigger_pattern_for( + rule_pack : DescribeLibraryRulePack, + rule : DescribeLibraryRulePack::Rule, + ) : Regex? + unless rule.check.trigger_regex_pattern.empty? + return Regex.new(rule.check.trigger_regex_pattern) + end + + mode = rule_pack.modes_by_name[rule.list_of_mode_names.first]? + tenant_column_name = mode.try(&.tenant_column_name) + return nil unless tenant_column_name + return nil unless tenant_column_name.matches?(/\A[a-zA-Z_][a-zA-Z0-9_]*\z/) + + Regex.new("^\\s*column\\s+#{tenant_column_name}\\b") + end + + private def call_outside_block_diagnostics_for( + rule : DescribeLibraryRulePack::Rule, + project_state : DetermineProjectRulePackState, + file_path : String, + content : String, + ) : Array(Rules::Diagnostic) + check = rule.check + list_of_scoped_model_names = project_state.list_of_scoped_model_names_for( + check.list_of_source_globs, + check.model_macro_name, + ) + return [] of Rules::Diagnostic if list_of_scoped_model_names.empty? + + visitor = VisitCrystalCallsOutsideRequiredBlocks.new( + list_of_scoped_model_names, + check.list_of_query_method_names, + check.required_block_call_name, + check.escape_block_call_name, + check.model_macro_name, + ) + ast = Crystal::Parser.new(content).parse + visitor.accept(ast) + + visitor.list_of_calls_outside_required_blocks.compact_map do |call| + location = call.name_location || call.location + next unless location + + line = (location.line_number - 1).to_i32 + start_character = (location.column_number - 1).to_i32 + range = Rules::TextRange.new( + Rules::Position.new(line, start_character), + Rules::Position.new(line, start_character + call.name.size), + ) + Rules::Diagnostic.new( + range, + severity_for(rule.severity_name), + rule.rule_id, + rule.diagnostic_message, + ) + end + rescue Crystal::SyntaxException + [] of Rules::Diagnostic + end + + private def diagnostic_at_start_of_file( + rule : DescribeLibraryRulePack::Rule, + severity : Rules::Severity, + ) : Rules::Diagnostic + range = Rules::TextRange.new( + Rules::Position.new(0, 0), + Rules::Position.new(0, 0), + ) + Rules::Diagnostic.new(range, severity, rule.rule_id, rule.diagnostic_message) + end + + private def apply_configured_severity( + list_of_diagnostics : Array(Rules::Diagnostic), + ) : Array(Rules::Diagnostic) + list_of_diagnostics.map do |diagnostic| + configured_severity = @configuration.rule_severity(diagnostic.code, diagnostic.severity) + next diagnostic if configured_severity == diagnostic.severity + + Rules::Diagnostic.new( + diagnostic.range, + configured_severity, + diagnostic.code, + diagnostic.message, + diagnostic.source, + ) + end + end + + private def severity_for(severity_name : String) : Rules::Severity + case severity_name.downcase + when "error" then Rules::Severity::Error + when "info" then Rules::Severity::Information + when "hint" then Rules::Severity::Hint + else Rules::Severity::Warning + end + end + + private def project_relative_path(file_path : String) : String + project_root = File.expand_path(@project_context.root_path) + absolute_file_path = File.expand_path(file_path) + prefix = project_root.ends_with?(File::SEPARATOR) ? project_root : "#{project_root}#{File::SEPARATOR}" + return file_path unless absolute_file_path.starts_with?(prefix) + + absolute_file_path[prefix.size..] + end + end +end diff --git a/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr b/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr new file mode 100644 index 0000000..7d1a550 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr @@ -0,0 +1,119 @@ +require "yaml" + +module AmberLSP::LibraryRulePacks + class DescribeLibraryRulePack + include YAML::Serializable + + @[YAML::Field(key: "pack")] + property pack_id : String + + @[YAML::Field(key: "library")] + property library_shard_name : String + + @[YAML::Field(key: "version")] + property pack_version : String + + @[YAML::Field(key: "modes")] + property modes_by_name : Hash(String, Mode) = {} of String => Mode + + @[YAML::Field(key: "rules")] + property list_of_rules : Array(Rule) = [] of Rule + + def is_valid? : Bool + return false if @pack_id.empty? || @library_shard_name.empty? || @pack_version.empty? + return false if @modes_by_name.empty? + + @list_of_rules.all? do |rule| + !rule.rule_id.empty? && + !rule.list_of_mode_names.empty? && + rule.list_of_mode_names.all? { |mode_name| @modes_by_name.has_key?(mode_name) } && + !rule.list_of_applicable_globs.empty? && + !rule.check.check_kind.empty? + end + end + + class Declaration + include YAML::Serializable + + property key_path : String + property expected_value : String + end + + class Mode + include YAML::Serializable + + @[YAML::Field(key: "declared_by")] + property declaration : Declaration + + @[YAML::Field(key: "evidence")] + property list_of_evidence_patterns : Array(String) = [] of String + + @[YAML::Field(key: "tenant_column")] + property tenant_column_name : String? = nil + + @[YAML::Field(key: "context")] + property guidance_text : String = "" + end + + class Rule + include YAML::Serializable + + @[YAML::Field(key: "id")] + property rule_id : String + + @[YAML::Field(key: "modes")] + property list_of_mode_names : Array(String) = [] of String + + @[YAML::Field(key: "severity")] + property severity_name : String = "warning" + + @[YAML::Field(key: "applies_to")] + property list_of_applicable_globs : Array(String) = ["**/*.cr"] + + @[YAML::Field(key: "exclude_from")] + property list_of_excluded_globs : Array(String) = [] of String + + @[YAML::Field(key: "message")] + property diagnostic_message : String = "" + + property check : Check + end + + class Check + include YAML::Serializable + + @[YAML::Field(key: "kind")] + property check_kind : String + + @[YAML::Field(key: "pattern")] + property regex_pattern : String = "" + + @[YAML::Field(key: "trigger_pattern")] + property trigger_regex_pattern : String = "" + + @[YAML::Field(key: "required_pattern")] + property required_regex_pattern : String = "" + + @[YAML::Field(key: "negate")] + property? negates_pattern : Bool = false + + @[YAML::Field(key: "source_globs")] + property list_of_source_globs : Array(String) = [] of String + + @[YAML::Field(key: "tenant_macro")] + property model_macro_name : String = "multitenant" + + @[YAML::Field(key: "methods")] + property list_of_query_method_names : Array(String) = [] of String + + @[YAML::Field(key: "required_call")] + property required_block_call_name : String = "" + + @[YAML::Field(key: "escape_call")] + property escape_block_call_name : String = "" + + @[YAML::Field(key: "condition")] + property project_condition : String = "mixed_modes" + end + end +end diff --git a/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr b/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr new file mode 100644 index 0000000..dd7ccc8 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr @@ -0,0 +1,136 @@ +require "set" + +module AmberLSP::LibraryRulePacks + class DetermineProjectRulePackState + @is_mode_declared_by_name = {} of String => Bool + @has_mode_evidence_by_name = {} of String => Bool + @project_source_content_by_path = {} of String => String + + def initialize( + @project_context : AmberLSP::ProjectContext, + @rule_pack : DescribeLibraryRulePack, + @file_path : String, + @file_content : String, + ) + load_project_source_content + determine_mode_declarations + determine_mode_evidence + end + + def is_mode_declared?(mode_name : String) : Bool + @is_mode_declared_by_name[mode_name]? || false + end + + def has_evidence_for_mode?(mode_name : String) : Bool + @has_mode_evidence_by_name[mode_name]? || false + end + + def is_mode_present?(mode_name : String) : Bool + is_mode_declared?(mode_name) || has_evidence_for_mode?(mode_name) + end + + def has_any_applicable_mode? : Bool + @rule_pack.modes_by_name.keys.any? do |mode_name| + is_mode_declared?(mode_name) || has_evidence_for_mode?(mode_name) + end + end + + def all_modes_are_present?(list_of_mode_names : Array(String)) : Bool + !list_of_mode_names.empty? && list_of_mode_names.all? { |mode_name| is_mode_present?(mode_name) } + end + + def has_undeclared_mode_evidence?(list_of_mode_names : Array(String)) : Bool + list_of_mode_names.any? do |mode_name| + has_evidence_for_mode?(mode_name) && !is_mode_declared?(mode_name) + end + end + + def is_rule_mode_declared?(list_of_mode_names : Array(String)) : Bool + list_of_mode_names.any? { |mode_name| is_mode_declared?(mode_name) } + end + + def list_of_scoped_model_names_for( + source_globs : Array(String), + model_macro_name : String, + ) : Set(String) + list_of_model_names = Set(String).new + + @project_source_content_by_path.each do |file_path, content| + relative_path = project_relative_path(file_path) + next unless source_globs.any? { |pattern| Rules::RuleRegistry.file_matches_pattern?(relative_path, pattern) } + + list_of_model_names.concat( + VisitCrystalCallsOutsideRequiredBlocks.find_multitenant_model_names(content, model_macro_name) + ) + end + + list_of_model_names + end + + private def load_project_source_content : Nil + project_root = File.expand_path(@project_context.root_path) + + Dir.glob(File.join(project_root, "**", "*.cr")).each do |file_path| + next unless project_file_is_application_source?(file_path, project_root) + + content = if File.expand_path(file_path) == File.expand_path(@file_path) + @file_content + else + File.read(file_path) + end + @project_source_content_by_path[File.expand_path(file_path)] = content + rescue + next + end + + current_file_path = File.expand_path(@file_path) + if project_file_is_application_source?(current_file_path, project_root) + @project_source_content_by_path[current_file_path] = @file_content + end + end + + private def project_file_is_application_source?(file_path : String, project_root : String) : Bool + prefix = project_root.ends_with?(File::SEPARATOR) ? project_root : "#{project_root}#{File::SEPARATOR}" + return false unless file_path.starts_with?(prefix) + + relative_path = file_path[prefix.size..] + return false if relative_path.starts_with?("lib/") + return false if relative_path.starts_with?(".git/") + return false if relative_path.starts_with?("tmp/") + return false if relative_path.starts_with?("vendor/") + + true + end + + private def determine_mode_declarations : Nil + @rule_pack.modes_by_name.each do |mode_name, mode| + @is_mode_declared_by_name[mode_name] = @project_context.has_shard_declaration?( + mode.declaration.key_path, + mode.declaration.expected_value, + ) + end + end + + private def determine_mode_evidence : Nil + @rule_pack.modes_by_name.each do |mode_name, mode| + evidence_found = mode.list_of_evidence_patterns.any? do |pattern| + regex = Regex.new(pattern) + @project_source_content_by_path.values.any? do |content| + content.each_line.any? { |line| regex.matches?(line) } + end + end + @has_mode_evidence_by_name[mode_name] = evidence_found + end + rescue ArgumentError + @rule_pack.modes_by_name.each_key { |mode_name| @has_mode_evidence_by_name[mode_name] = false } + end + + private def project_relative_path(file_path : String) : String + project_root = File.expand_path(@project_context.root_path) + prefix = project_root.ends_with?(File::SEPARATOR) ? project_root : "#{project_root}#{File::SEPARATOR}" + return file_path unless file_path.starts_with?(prefix) + + file_path[prefix.size..] + end + end +end diff --git a/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr new file mode 100644 index 0000000..1becfd1 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr @@ -0,0 +1,90 @@ +module AmberLSP::LibraryRulePacks + class LoadRulePacksForProject + def initialize(@project_context : AmberLSP::ProjectContext) + end + + def load_rule_packs : Array(DescribeLibraryRulePack) + list_of_packs_by_id = {} of String => DescribeLibraryRulePack + + dependency_pack_paths.each do |pack_path| + load_rule_pack(pack_path).try do |rule_pack| + list_of_packs_by_id[rule_pack.pack_id] = rule_pack + end + end + + project_pack_paths.each do |pack_path| + load_rule_pack(pack_path).try do |rule_pack| + list_of_packs_by_id[rule_pack.pack_id] = rule_pack + end + end + + list_of_packs_by_id.values.sort_by(&.pack_id) + end + + private def dependency_pack_paths : Array(String) + list_of_library_paths = Dir.glob(File.join(@project_context.root_path, "lib", "*")).sort + list_of_library_paths.flat_map do |library_path| + Dir.glob(File.join(library_path, ".claude", "rules", "*.yml")) + end.sort + end + + private def project_pack_paths : Array(String) + Dir.glob(File.join(@project_context.root_path, ".claude", "rules", "*.yml")).sort + end + + private def load_rule_pack(pack_path : String) : DescribeLibraryRulePack? + rule_pack = DescribeLibraryRulePack.from_yaml(File.read(pack_path)) + return rule_pack if rule_pack_is_valid?(rule_pack) + + STDERR.puts "WARNING: Ignoring invalid amber-lsp rule pack at #{pack_path}." + nil + rescue ex + STDERR.puts "WARNING: Could not load amber-lsp rule pack at #{pack_path}: #{ex.message}" + nil + end + + private def rule_pack_is_valid?(rule_pack : DescribeLibraryRulePack) : Bool + return false unless rule_pack.is_valid? + + rule_pack.modes_by_name.each_value do |mode| + mode.list_of_evidence_patterns.each { |pattern| Regex.new(pattern) } + end + + rule_pack.list_of_rules.all? do |rule| + next false unless {"error", "warning", "info", "hint"}.includes?(rule.severity_name.downcase) + + case rule.check.check_kind + when "line_regex" + Regex.new(rule.check.regex_pattern) + true + when "file_requires" + !rule.check.required_regex_pattern.empty? && + (rule.check.trigger_regex_pattern.empty? || begin + Regex.new(rule.check.trigger_regex_pattern) + true + end) && begin + Regex.new(rule.check.required_regex_pattern) + true + end + when "call_outside_block" + !rule.check.list_of_source_globs.empty? && + !rule.check.list_of_query_method_names.empty? && + (!rule.check.required_block_call_name.empty? || !rule.check.escape_block_call_name.empty?) + when "project_conflict" + case rule.check.project_condition + when "mixed_modes" + rule.list_of_mode_names.size > 1 + when "evidence_without_declaration" + rule.list_of_mode_names.size == 1 + else + false + end + else + false + end + end + rescue ArgumentError + false + end + end +end diff --git a/src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr b/src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr new file mode 100644 index 0000000..1d17509 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr @@ -0,0 +1,90 @@ +module AmberLSP::LibraryRulePacks + class PrintDeclaredRulePackContexts + def initialize(@list_of_arguments : Array(String)) + end + + def perform : Int32 + project_context = AmberLSP::ProjectContext.detect(project_root_from_arguments) + list_of_rule_packs = LoadRulePacksForProject.new(project_context).load_rule_packs + list_of_output_lines = [] of String + + list_of_rule_packs.each do |rule_pack| + next if project_context.shard_name == rule_pack.library_shard_name + + project_state = DetermineProjectRulePackState.new(project_context, rule_pack, "", "") + next unless project_state.has_any_applicable_mode? + + append_declared_mode_contexts(list_of_output_lines, rule_pack, project_state) + append_undeclared_feature_warning(list_of_output_lines, rule_pack, project_state) + end + + STDOUT.puts(list_of_output_lines.join('\n')) unless list_of_output_lines.empty? + 0 + rescue ex + STDERR.puts "amber-lsp context failed: #{ex.message}" + 1 + end + + private def project_root_from_arguments : String + project_root = Dir.current + arguments = @list_of_arguments.dup + + until arguments.empty? + argument = arguments.shift + case argument + when "--root" + root_argument = arguments.shift? + raise ArgumentError.new("--root requires a directory") unless root_argument + project_root = File.expand_path(root_argument) + else + raise ArgumentError.new("unexpected argument #{argument.inspect}") + end + end + + find_project_root(project_root) + end + + private def find_project_root(start_path : String) : String + current_path = File.expand_path(start_path) + current_path = File.dirname(current_path) unless File.directory?(current_path) + + loop do + return current_path if File.file?(File.join(current_path, "shard.yml")) + + parent_path = File.dirname(current_path) + return current_path if parent_path == current_path + current_path = parent_path + end + end + + private def append_declared_mode_contexts( + list_of_output_lines : Array(String), + rule_pack : DescribeLibraryRulePack, + project_state : DetermineProjectRulePackState, + ) : Nil + rule_pack.modes_by_name.each do |mode_name, mode| + next unless project_state.is_mode_declared?(mode_name) + next if mode.guidance_text.strip.empty? + + list_of_output_lines << "#{rule_pack.pack_id} (#{mode_name})" + list_of_output_lines.concat(mode.guidance_text.lines.map(&.rstrip)) + end + end + + private def append_undeclared_feature_warning( + list_of_output_lines : Array(String), + rule_pack : DescribeLibraryRulePack, + project_state : DetermineProjectRulePackState, + ) : Nil + list_of_undeclared_mode_names = rule_pack.modes_by_name.keys.select do |mode_name| + project_state.has_evidence_for_mode?(mode_name) && !project_state.is_mode_declared?(mode_name) + end + return if list_of_undeclared_mode_names.empty? + + list_of_key_paths = list_of_undeclared_mode_names.compact_map do |mode_name| + rule_pack.modes_by_name[mode_name]?.try(&.declaration.key_path) + end + list_of_output_lines << "warning: #{rule_pack.pack_id} feature is used but shard.yml does not declare #{list_of_key_paths.join(", ")}." + end + end +end diff --git a/src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks.cr b/src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks.cr new file mode 100644 index 0000000..d5d7d5b --- /dev/null +++ b/src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks.cr @@ -0,0 +1,159 @@ +require "set" +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks + class VisitCrystalCallsOutsideRequiredBlocks < Crystal::Visitor + getter list_of_calls_outside_required_blocks : Array(Crystal::Call) + getter list_of_multitenant_model_names : Set(String) + + @list_of_class_names = [] of String + @required_block_depth = 0 + @list_of_unscoped_model_names = [] of String + @list_of_block_scope_changes = [] of {Bool, String?} + @list_of_method_scope_snapshots = [] of {Int32, Array(String)} + + def initialize( + @list_of_scoped_model_names : Set(String), + @list_of_query_method_names : Array(String), + @required_block_call_name : String, + @escape_block_call_name : String, + @model_macro_name : String, + ) + @list_of_calls_outside_required_blocks = [] of Crystal::Call + @list_of_multitenant_model_names = Set(String).new + end + + def self.find_multitenant_model_names(content : String, model_macro_name : String) : Set(String) + visitor = new( + Set(String).new, + [] of String, + "", + "", + model_macro_name, + ) + visitor.accept(Crystal::Parser.new(content).parse) + visitor.list_of_multitenant_model_names + rescue Crystal::SyntaxException + Set(String).new + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::ClassDef) : Bool + @list_of_class_names << node.name.names.last + true + end + + def end_visit(node : Crystal::ClassDef) : Nil + @list_of_class_names.pop + end + + def visit(node : Crystal::Def) : Bool + @list_of_method_scope_snapshots << {@required_block_depth, @list_of_unscoped_model_names.dup} + @required_block_depth = 0 + @list_of_unscoped_model_names.clear + true + end + + def end_visit(node : Crystal::Def) : Nil + previous_scope = @list_of_method_scope_snapshots.pop? + return unless previous_scope + + @required_block_depth = previous_scope[0] + @list_of_unscoped_model_names = previous_scope[1] + end + + def visit(node : Crystal::Call) : Bool + if node.name == @model_macro_name && !@list_of_class_names.empty? + @list_of_multitenant_model_names << @list_of_class_names.last + end + + if query_call_is_outside_required_blocks?(node) + @list_of_calls_outside_required_blocks << node + end + + true + end + + def visit(node : Crystal::Block) : Bool + call = node.call + enters_required_block = call.try { |block_call| is_required_block_call?(block_call) } || false + if enters_required_block + @required_block_depth += 1 + end + + escape_model_name = call.try { |block_call| escaped_model_name(block_call) } + if escape_model_name + @list_of_unscoped_model_names << escape_model_name + end + + @list_of_block_scope_changes << {enters_required_block, escape_model_name} + true + end + + def end_visit(node : Crystal::Block) : Nil + scope_change = @list_of_block_scope_changes.pop? + return unless scope_change + + @required_block_depth -= 1 if scope_change[0] + if scope_change[1] + @list_of_unscoped_model_names.pop + end + end + + private def query_call_is_outside_required_blocks?(call : Crystal::Call) : Bool + return false unless @list_of_query_method_names.includes?(call.name) + + model_name = model_name_for_receiver(call.obj) + return false unless model_name && @list_of_scoped_model_names.includes?(model_name) + return false if @required_block_depth > 0 + return false if @list_of_unscoped_model_names.includes?(model_name) + + true + end + + private def is_required_block_call?(call : Crystal::Call) : Bool + return false if @required_block_call_name.empty? + + if @required_block_call_name.includes?('.') + full_call_name(call) == @required_block_call_name + else + call.name == @required_block_call_name + end + end + + private def escaped_model_name(call : Crystal::Call) : String? + return nil if @escape_block_call_name.empty? + return nil unless call.name == @escape_block_call_name + + model_name = model_name_for_receiver(call.obj) + return nil unless model_name && @list_of_scoped_model_names.includes?(model_name) + + model_name + end + + private def model_name_for_receiver(node : Crystal::ASTNode?) : String? + case node + when Crystal::Path + node.names.last? + when Crystal::Call + model_name_for_receiver(node.obj) + else + nil + end + end + + private def full_call_name(call : Crystal::Call) : String + case receiver = call.obj + when Crystal::Path + "#{receiver.names.join("::")}.#{call.name}" + when Crystal::Call + "#{full_call_name(receiver)}.#{call.name}" + else + call.name + end + end + end +end diff --git a/src/amber_lsp/project_context.cr b/src/amber_lsp/project_context.cr index 10b6d19..3064f9e 100644 --- a/src/amber_lsp/project_context.cr +++ b/src/amber_lsp/project_context.cr @@ -4,8 +4,16 @@ module AmberLSP class ProjectContext getter root_path : String getter? amber_project : Bool + getter shard_name : String? - def initialize(@root_path : String, @amber_project : Bool = false) + @shard_configuration : YAML::Any? + + def initialize( + @root_path : String, + @amber_project : Bool = false, + @shard_name : String? = nil, + @shard_configuration : YAML::Any? = nil, + ) end def self.detect(root_path : String) : ProjectContext @@ -15,20 +23,41 @@ module AmberLSP return ProjectContext.new(root_path, amber_project: false) end - content = File.read(shard_path) - is_amber = has_amber_dependency?(content) + shard_configuration = YAML.parse(File.read(shard_path)) + is_amber = has_amber_dependency?(shard_configuration) + shard_name = shard_configuration["name"]?.try(&.as_s?) + + ProjectContext.new( + root_path, + amber_project: is_amber, + shard_name: shard_name, + shard_configuration: shard_configuration, + ) + rescue YAML::ParseException + ProjectContext.new(root_path, amber_project: false) + end + + def has_shard_declaration?(key_path : String, expected_value : String) : Bool + shard_configuration = @shard_configuration + return false unless shard_configuration - ProjectContext.new(root_path, amber_project: is_amber) + current_value = shard_configuration + key_path.split('.').each do |key| + next_value = current_value[key]? + return false unless next_value + current_value = next_value + end + + current_value.as_s? == expected_value + rescue TypeCastError + false end - private def self.has_amber_dependency?(shard_content : String) : Bool - yaml = YAML.parse(shard_content) - dependencies = yaml["dependencies"]? + private def self.has_amber_dependency?(shard_configuration : YAML::Any) : Bool + dependencies = shard_configuration["dependencies"]? return false unless dependencies dependencies["amber"]? != nil - rescue YAML::ParseException - false end end end diff --git a/src/amber_lsp/rules/base_rule.cr b/src/amber_lsp/rules/base_rule.cr index 4e98963..6b2c1d3 100644 --- a/src/amber_lsp/rules/base_rule.cr +++ b/src/amber_lsp/rules/base_rule.cr @@ -6,6 +6,10 @@ module AmberLSP::Rules abstract def applies_to : Array(String) abstract def check(file_path : String, content : String) : Array(Diagnostic) + def requires_amber_project? : Bool + true + end + # Finds the line and character range for the first occurrence of a pattern. # Returns nil if the pattern is not found. def find_line_range(content : String, pattern : Regex) : TextRange? diff --git a/src/amber_lsp/rules/controllers/action_return_rule.cr b/src/amber_lsp/rules/controllers/action_return_rule.cr index b09a090..00d638c 100644 --- a/src/amber_lsp/rules/controllers/action_return_rule.cr +++ b/src/amber_lsp/rules/controllers/action_return_rule.cr @@ -3,6 +3,7 @@ module AmberLSP::Rules::Controllers RESPONSE_METHODS = ["render", "redirect_to", "redirect_back", "respond_with", "halt!"] SKIPPED_METHODS = ["initialize", "before_action", "after_action", "before_filter", "after_filter"] VISIBILITY_CHANGE = /^\s*(private|protected)\s*$/ + CLASS_METHOD = /^\s{2,4}def\s+self\./ def id : String "amber/action-return-type" @@ -22,6 +23,7 @@ module AmberLSP::Rules::Controllers def check(file_path : String, content : String) : Array(Diagnostic) return [] of Diagnostic unless file_path.includes?("controllers/") + return [] of Diagnostic if action_support_file?(file_path) diagnostics = [] of Diagnostic lines = content.lines @@ -45,6 +47,8 @@ module AmberLSP::Rules::Controllers # Detect method start at standard 2-space indent (methods inside a class) method_match = /^(\s{2,4})def\s+(\w+)/.match(line) if method_match && !in_public_method + next if CLASS_METHOD.matches?(line) + indent = method_match[1].size name = method_match[2] @@ -65,7 +69,7 @@ module AmberLSP::Rules::Controllers if in_public_method # Check for response method calls - if RESPONSE_METHODS.any? { |m| line.includes?(m) } + if response_written_or_returned?(line) has_response_call = true end @@ -93,6 +97,19 @@ module AmberLSP::Rules::Controllers diagnostics end + + private def action_support_file?(file_path : String) : Bool + File.basename(file_path) == "application_controller.cr" || + file_path.includes?("controllers/concerns/") + end + + private def response_written_or_returned?(line : String) : Bool + RESPONSE_METHODS.any? { |method_name| line.includes?(method_name) } || + line.includes?("respond_") || + line.matches?(/\bcontext\.response\.(print|write)\b/) || + line.matches?(/\b\w+\(context\.response\b/) || + line.includes?("String.build") + end end end diff --git a/src/amber_lsp/rules/custom_rule.cr b/src/amber_lsp/rules/custom_rule.cr index aafb447..04b028b 100644 --- a/src/amber_lsp/rules/custom_rule.cr +++ b/src/amber_lsp/rules/custom_rule.cr @@ -20,6 +20,10 @@ module AmberLSP::Rules ) end + def requires_amber_project? : Bool + false + end + def check(file_path : String, content : String) : Array(Diagnostic) return [] of Diagnostic unless applies_to.any? { |pattern| RuleRegistry.file_matches_pattern?(file_path, pattern) diff --git a/src/amber_lsp/rules/rule_registry.cr b/src/amber_lsp/rules/rule_registry.cr index b43490a..d0e1c1e 100644 --- a/src/amber_lsp/rules/rule_registry.cr +++ b/src/amber_lsp/rules/rule_registry.cr @@ -23,6 +23,11 @@ module AmberLSP::Rules def self.file_matches_pattern?(file_path : String, pattern : String) : Bool if pattern == "*" true + elsif File.match?(pattern, file_path) + true + elsif pattern.starts_with?("*") && !pattern.includes?("/") + # Preserve the existing suffix behavior for rules such as *_controller.cr. + file_path.ends_with?(pattern.lchop("*")) elsif pattern.ends_with?("**") # Recursive glob: "src/**" matches anything under "src/" prefix = pattern.rchop("**") From 5a43008bdf1d2326025222df5fa419bc7e1a16a5 Mon Sep 17 00:00:00 2001 From: crimson-knight Date: Wed, 23 Sep 2026 20:47:39 -0400 Subject: [PATCH 2/3] feat(amber-lsp): detect Grant tenancy from AST rule packs --- docs/rule-packs.md | 111 ++- spec/amber_lsp/rule_packs_spec.cr | 707 ++++++++++-------- spec/amber_lsp/spec_helper.cr | 10 +- .../mixed_app/config/tenant_scope.cr | 3 + .../mixed_app/src/models/invoice.cr | 8 + .../no_tenancy_app/src/models/invoice.cr | 6 + .../lib/grant/.amber-lsp/packs/tenancy.yml | 142 ++++ .../row_app/src/models/account.cr | 6 + .../row_app/src/models/custom_document.cr | 5 + .../row_app/src/models/invoice.cr | 8 + .../row_app/src/models/invoice_export.cr | 4 + .../row_app/src/models/ledger_entry.cr | 5 + .../schema_app/config/tenant_scope.cr | 3 + .../schema_app/src/models/country.cr | 7 + .../schema_app/src/models/invoice.cr | 5 + src/amber_lsp.cr | 12 +- .../analyze_project_files_with_rule_packs.cr | 135 +++- .../describe_library_rule_pack.cr | 19 +- .../determine_project_rule_pack_state.cr | 204 +++-- ...lect_project_grant_tenancy_declarations.cr | 157 ++++ .../grant_tenant_model_declaration.cr | 50 ++ .../grant_tenancy/source_node.cr | 79 ++ .../visit_chainable_unscoped_model_calls.cr | 73 ++ ...nt_schema_queries_outside_tenant_blocks.cr | 77 ++ .../visit_grant_tenant_clear_calls.cr | 22 + .../visit_raw_connection_sql_call_sites.cr | 123 +++ ..._spawn_calls_inside_grant_tenant_blocks.cr | 62 ++ .../load_rule_packs_for_project.cr | 31 +- ...r => print_detected_rule_pack_contexts.cr} | 25 +- src/amber_lsp/project_context.cr | 20 - 30 files changed, 1615 insertions(+), 504 deletions(-) create mode 100644 spec/fixtures/rule_pack_apps/mixed_app/config/tenant_scope.cr create mode 100644 spec/fixtures/rule_pack_apps/mixed_app/src/models/invoice.cr create mode 100644 spec/fixtures/rule_pack_apps/no_tenancy_app/src/models/invoice.cr create mode 100644 spec/fixtures/rule_pack_apps/row_app/lib/grant/.amber-lsp/packs/tenancy.yml create mode 100644 spec/fixtures/rule_pack_apps/row_app/src/models/account.cr create mode 100644 spec/fixtures/rule_pack_apps/row_app/src/models/custom_document.cr create mode 100644 spec/fixtures/rule_pack_apps/row_app/src/models/invoice.cr create mode 100644 spec/fixtures/rule_pack_apps/row_app/src/models/invoice_export.cr create mode 100644 spec/fixtures/rule_pack_apps/row_app/src/models/ledger_entry.cr create mode 100644 spec/fixtures/rule_pack_apps/schema_app/config/tenant_scope.cr create mode 100644 spec/fixtures/rule_pack_apps/schema_app/src/models/country.cr create mode 100644 spec/fixtures/rule_pack_apps/schema_app/src/models/invoice.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/source_node.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr create mode 100644 src/amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks.cr rename src/amber_lsp/library_rule_packs/{print_declared_rule_pack_contexts.cr => print_detected_rule_pack_contexts.cr} (68%) diff --git a/docs/rule-packs.md b/docs/rule-packs.md index 2a2b662..834b1bb 100644 --- a/docs/rule-packs.md +++ b/docs/rule-packs.md @@ -1,92 +1,133 @@ # Library rule packs -Library rule packs let a Crystal library ship opt-in feature guidance and edit-time checks with the same `.claude/` files that Shards distributes. `amber-lsp` reads packs from installed dependencies at `lib/*/.claude/rules/*.yml` and from the project at `.claude/rules/*.yml`; no copy step is required. A project pack with the same `pack` id overrides a dependency pack. A pack whose `library` equals the project's own shard name does not apply, because that pack describes the library's consumer contract. +Library rule packs let a Crystal library ship feature guidance and edit-time +checks for `amber-lsp`, a harness-neutral tool. The pack uses amber-lsp's YAML +format and lives in the library at `.amber-lsp/packs/.yml`. The engine +discovers installed dependency packs at `lib/*/.amber-lsp/packs/*.yml` and +project packs at `.amber-lsp/packs/*.yml`. A project pack with the same `pack` +id overrides a dependency pack. A pack whose `library` equals the project's +own shard name does not apply, because that pack describes the library's +consumer contract. ## Pack document -Each YAML file has a pack identity, declared modes, optional evidence patterns, mode-specific context, and rules: +Each YAML file has a pack identity, source-detected modes with context, and +rules: ```yaml pack: grant/tenancy library: grant -version: 1.0.0 +version: 2.0.0 modes: row: - declared_by: - key_path: grant.tenancy - expected_value: row - evidence: - - '^\s*multitenant\b' - tenant_column: tenant_id context: | - Scope queries with the library's tenant context. + Row tenancy was detected from the app's multitenant model macros. schema: - declared_by: - key_path: grant.tenancy - expected_value: schema - evidence: - - '^\s*SchemaTenant\.with\b' context: | - Schema-specific rules are not included yet. + Schema tenancy was detected from the app's schema-tenant declarations. rules: - id: grant/example modes: [row] severity: warning applies_to: ["src/**"] - exclude_from: ["src/controllers/**"] + exclude_from: ["spec/**"] message: "Explain the finding and the repair." check: - kind: line_regex - pattern: '\bunsafe_call\b' + kind: crystal_ast + operation: chained_unscoped_on_tenant_model ``` -`declared_by.key_path` is a dotted path into the application's `shard.yml`; `expected_value` is compared as a string. A pack rule runs only when one of its modes is declared. Evidence patterns scan the application's `.cr` files outside `lib/`, `.git/`, `tmp/`, and `vendor/`; they support mixed-mode detection and the undeclared-feature warning. Evidence alone does not activate mode-specific rules. Context blocks are printed only for declared modes and should stay to about 15 lines or fewer. - -Each rule requires an id, one or more mode names, a severity, file globs, a message, and one check. Severity values are `error`, `warning`, `info`, and `hint`. `exclude_from` is optional. `applies_to` and `exclude_from` use Crystal path globs, including `**` for recursive directories. Rules can be enabled, disabled, or have their severity overridden through the existing `.amber-lsp.yml` `rules` mapping. +For Grant, `amber-lsp` parses Crystal source under the app's `src/` and +`config/` directories to detect tenancy. It does not scan installed `lib/` code +or `spec/` files for mode declarations. Row mode comes from each class body's +`multitenant :column` macro, and the engine captures that tenant column per +model. Schema mode comes from a `Grant::SchemaTenant.with(...)` call or a +`schema_tenant_excluded` macro in a class body. Comments and string contents do +not count as declarations, and an unparsable source file is skipped. + +Rules run only when one of their listed modes is detected. Context blocks are +printed by `amber-lsp context [--root DIR]` for detected modes. There is no +shard.yml feature declaration or evidence-without-declaration warning. The +engine reads shard.yml only for package identity and installed dependency +names. + +Each rule requires an id, one or more mode names, a severity, file globs, a +message, and one check. Severity values are `error`, `warning`, `info`, and +`hint`. `exclude_from` is optional. `applies_to` and `exclude_from` use Crystal +path globs, including `**` for recursive directories. Rules can be enabled, +disabled, or have their severity overridden through the existing +`.amber-lsp.yml` `rules` mapping. ## Check kinds ### `line_regex` -Matches a regular expression on each line and uses the existing custom-rule diagnostic matcher. `negate: true` reports once at the start of a file when the pattern does not occur. +Matches a regular expression on each line using the existing custom-rule +diagnostic matcher. `negate: true` reports once at the start of a file when the +pattern does not occur. ```yaml check: kind: line_regex - pattern: '^\s*[^#]*\.unscoped\b' + pattern: '^\\s*[^#]*\\.unsafe_call\\b' ``` ### `file_requires` -Reports on each line matching `trigger_pattern` when the file has no line matching `required_pattern`. If `trigger_pattern` is omitted, `amber-lsp` uses the first mode's `tenant_column` as a Crystal `column ` declaration. +Reports on each line matching `trigger_pattern` when the file has no line +matching `required_pattern`. Both are explicit regular expressions. ```yaml check: kind: file_requires - required_pattern: '^\s*multitenant\b' + trigger_pattern: '^\\s*column\\s+account_id\\b' + required_pattern: '^\\s*multitenant\\b' ``` ### `call_outside_block` -Uses `Crystal::Parser` and a `Crystal::Visitor` to inspect method calls. `source_globs` select the files where `tenant_macro` declarations identify scoped model names. `methods` lists calls on those model constants that can query the database. A dotted `required_call`, such as `Grant::Tenant.with`, permits calls lexically inside its block and nested blocks. `escape_call` permits calls only inside a block on that same model receiver, such as `Todo.unscoped { Todo.where(...) }`. A method definition starts a new lexical scope, so a query written in a method body does not become tenant-scoped just because the method definition appears inside a tenant block. Receiver constants are followed through chained calls, but model instances held in variables are not resolved. The current matcher uses the last constant name, so same-named models in different namespaces can be ambiguous. +Uses `Crystal::Parser` and a `Crystal::Visitor` to inspect method calls. +`source_globs` select the files where `tenant_macro` declarations identify +scoped model names. `methods` lists calls on those model constants that can +query the database. A dotted `required_call`, such as `Grant::Tenant.with`, +permits calls lexically inside its block and nested blocks. `escape_call` +permits calls only inside a block on that same model receiver, such as +`Todo.unscoped { Todo.where(...) }`. A method definition starts a new lexical +scope. Receiver constants are followed through chained calls, but model +instances held in variables are not resolved. ```yaml check: kind: call_outside_block source_globs: ["src/models/**"] tenant_macro: multitenant - methods: [where, find, all, first, count] + methods: [where, find, all] required_call: Grant::Tenant.with escape_call: unscoped ``` -`required_call` may be omitted when `escape_call` alone defines the allowed block. The rule intentionally does not infer receiver types or follow aliases stored in local variables. +### `crystal_ast` + +Runs one of the named AST operations used by Grant's tenancy pack. The parser +visitors inspect Crystal call, class, block, and literal nodes rather than +matching source lines. Supported operations are `chained_unscoped_in_request_code`, +`chained_unscoped_bulk_write`, `chained_unscoped_on_tenant_model`, +`unscoped_block_in_request_code`, `spawn_inside_tenant_block`, +`tenant_column_without_multitenant`, `raw_connection_sql_on_tenant_table`, +`tenant_clear_in_app_code`, and `schema_query_outside_tenant`. + +```yaml +check: + kind: crystal_ast + operation: tenant_column_without_multitenant +``` ### `project_conflict` -Runs over project state rather than one source line. The default `condition: mixed_modes` reports when every listed mode is declared or has evidence. `condition: evidence_without_declaration` reports when a listed mode has feature evidence but its declaration is absent; use one mode for this warning. +Runs over project state. The supported `condition: mixed_modes` reports when +both listed modes are detected in the app's Crystal source. ```yaml check: @@ -96,6 +137,12 @@ check: ## Project activation and context -Amber's built-in convention rules remain gated on an `amber` dependency. Library packs run in any project when the pack applies, including a project with only a Grant dependency. Pack diagnostics use the ordinary LSP diagnostic channel and `.amber-lsp.yml` severity overrides. +Amber's built-in convention rules remain gated on an `amber` dependency. +Library packs run in any project when a pack mode is detected, including a +non-Amber app with Grant installed. Pack diagnostics use the ordinary LSP +diagnostic channel and `.amber-lsp.yml` severity overrides. -Run `amber-lsp context [--root DIR]` to print the context blocks for declared pack modes and one warning line per pack whose feature is used without a declaration. It prints nothing when no pack is declared or evidenced. Exit status is 0 for successful context inspection; invalid command arguments or an unreadable project return nonzero. +Run `amber-lsp context [--root DIR]` to print the context blocks for detected +pack modes. It prints nothing when no mode is detected. Exit status is 0 for +successful context inspection; invalid command arguments or an unreadable +project return nonzero. diff --git a/spec/amber_lsp/rule_packs_spec.cr b/spec/amber_lsp/rule_packs_spec.cr index 2d9f4c1..51ebe49 100644 --- a/spec/amber_lsp/rule_packs_spec.cr +++ b/spec/amber_lsp/rule_packs_spec.cr @@ -1,423 +1,492 @@ require "./spec_helper" -require "../../src/amber_lsp/rules/controllers/action_return_rule" - -GRANT_RULE_PACK_FIXTURE = <<-YAML - pack: grant/tenancy - library: grant - version: 1.0.0 - modes: - row: - declared_by: - key_path: grant.tenancy - expected_value: row - evidence: - - '^\\s*multitenant\\b' - tenant_column: tenant_id - context: | - Scope queries with Grant::Tenant.with. - schema: - declared_by: - key_path: grant.tenancy - expected_value: schema - evidence: - - '^\\s*Grant::SchemaTenant\\.with\\b' - - '^\\s*schema_tenant_excluded\\b' - context: | - Schema-specific rules are not included yet. - rules: - - id: grant/tenant-column-without-multitenant - modes: [row] - severity: error - applies_to: ["src/models/**"] - message: Tenant columns require multitenant. - check: - kind: file_requires - required_pattern: '^\\s*multitenant\\b' - - id: grant/tenancy-modes-mixed - modes: [row, schema] - severity: error - applies_to: ["**/*.cr"] - message: Choose one tenancy mode. - check: - kind: project_conflict - condition: mixed_modes - - id: grant/row-query-outside-tenant - modes: [row] - severity: warning - applies_to: ["**/*.cr"] - exclude_from: ["src/controllers/**"] - message: Query must be tenant-scoped. - check: - kind: call_outside_block - source_globs: ["src/models/**"] - tenant_macro: multitenant - methods: [all, where, find!] - required_call: Grant::Tenant.with - escape_call: unscoped - - id: grant/unscoped-in-request-code - modes: [row] - severity: warning - applies_to: ["src/controllers/**"] - message: Do not use unscoped in request code. - check: - kind: line_regex - pattern: '^\\s*[^#]*\\.unscoped\\b' - - id: grant/raw-sql-on-scoped-model - modes: [row] - severity: warning - applies_to: ["**/*.cr"] - message: Raw SQL must use an unscoped block. - check: - kind: call_outside_block - source_globs: ["src/models/**"] - tenant_macro: multitenant - methods: [exec, query, scalar] - escape_call: unscoped - - id: grant/tenancy-undeclared - modes: [row] - severity: warning - applies_to: ["**/*.cr"] - message: Declare Grant tenancy in shard.yml. - check: - kind: project_conflict - condition: evidence_without_declaration - YAML - -def write_rule_pack_project( - root : String, - shard_content : String = "name: tenant_app\nversion: 0.1.0\ngrant:\n tenancy: row\n", - pack_content : String = GRANT_RULE_PACK_FIXTURE, -) : Nil - Dir.mkdir_p(File.join(root, ".claude", "rules")) - File.write(File.join(root, "shard.yml"), shard_content) - File.write(File.join(root, ".claude", "rules", "tenancy.yml"), pack_content) + +GRANT_TENANCY_PACK_FIXTURE_PATH = File.join( + Dir.current, + "spec", + "fixtures", + "rule_pack_apps", + "row_app", + "lib", + "grant", + ".amber-lsp", + "packs", + "tenancy.yml", +) + +GRANT_TENANCY_REQUEST_PATHS = [ + "src/controllers/invoices_controller.cr", + "src/channels/invoice_channel.cr", + "src/sockets/invoice_socket.cr", + "src/pipes/tenant_pipe.cr", +] + +def install_tenancy_fixture_app(root : String, fixture_name : String) : Nil + fixture_root = File.join(Dir.current, "spec", "fixtures", "rule_pack_apps", fixture_name) + + ["src", "config", "lib"].each do |directory_name| + FileUtils.rm_rf(File.join(root, directory_name)) + end + + File.write(File.join(root, "shard.yml"), "name: #{fixture_name}\nversion: 0.1.0\n") + + ["src", "config"].each do |source_directory_name| + source_directory = File.join(fixture_root, source_directory_name) + next unless Dir.exists?(source_directory) + + FileUtils.cp_r(source_directory, root) + end + + pack_path = File.join(root, "lib", "grant", ".amber-lsp", "packs", "tenancy.yml") + Dir.mkdir_p(File.dirname(pack_path)) + File.write(pack_path, File.read(GRANT_TENANCY_PACK_FIXTURE_PATH)) end -def analyze_pack_file(root : String, file_path : String, content : String) : Array(AmberLSP::Rules::Diagnostic) +def analyze_tenancy_fixture_source( + root : String, + relative_file_path : String, + content : String, +) : Array(AmberLSP::Rules::Diagnostic) + file_path = File.join(root, relative_file_path) Dir.mkdir_p(File.dirname(file_path)) File.write(file_path, content) + project_context = AmberLSP::ProjectContext.detect(root) analyzer = AmberLSP::Analyzer.new analyzer.configure(project_context) analyzer.analyze(file_path, content) end -def diagnostic_codes(diagnostics : Array(AmberLSP::Rules::Diagnostic)) : Array(String) +def has_tenancy_diagnostic_code?(diagnostics : Array(AmberLSP::Rules::Diagnostic), code : String) : Bool + diagnostics.any? { |diagnostic| diagnostic.code == code } +end + +def tenancy_diagnostic_codes(diagnostics : Array(AmberLSP::Rules::Diagnostic)) : Array(String) diagnostics.map(&.code) end -describe "AmberLSP library rule packs" do +describe "AmberLSP Grant tenancy rule pack v2" do before_each do AmberLSP::Rules::RuleRegistry.clear end - it "loads packs from installed dependencies and project rules" do + it "loads the library pack from the harness-neutral dependency path" do with_tempdir do |root| - write_rule_pack_project(root) - dependency_root = File.join(root, "dependency_source") - dependency_pack_path = File.join(dependency_root, ".claude", "rules", "tenancy.yml") - project_pack_path = File.join(root, ".claude", "rules", "project.yml") - Dir.mkdir_p(File.dirname(dependency_pack_path)) - File.write(dependency_pack_path, GRANT_RULE_PACK_FIXTURE) - Dir.mkdir_p(File.join(root, "lib")) - File.symlink(dependency_root, File.join(root, "lib", "grant")) - File.write( - project_pack_path, - GRANT_RULE_PACK_FIXTURE.gsub("grant/tenancy", "project/tenancy").gsub("library: grant", "library: project"), - ) + install_tenancy_fixture_app(root, "row_app") project_context = AmberLSP::ProjectContext.detect(root) - packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs + list_of_rule_packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs - packs.map(&.pack_id).should eq(["grant/tenancy", "project/tenancy"]) + list_of_rule_packs.map(&.pack_id).should eq(["grant/tenancy"]) + File.exists?(File.join(root, "lib", "grant", ".amber-lsp", "packs", "tenancy.yml")).should be_true + File.exists?(File.join(root, "lib", "grant", ".claude", "rules", "tenancy.yml")).should be_false end end - it "keeps a library's own pack inactive while allowing clean LSP checks" do + it "gives agents the Grant runtime context for detected modes" do with_tempdir do |root| - shard_content = "name: grant\nversion: 0.1.0\n" - write_rule_pack_project(root, shard_content) - + install_tenancy_fixture_app(root, "row_app") project_context = AmberLSP::ProjectContext.detect(root) - packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs - packs.map(&.pack_id).should eq(["grant/tenancy"]) - - file_path = File.join(root, "src", "grant", "scale", "tenant.cr") - content = "multitenant :tenant_id\n" - analyzer = AmberLSP::Analyzer.new - analyzer.configure(project_context) - analyzer.has_applicable_library_rule_pack?(file_path, content).should be_true - analyzer.analyze(file_path, content).should be_empty + rule_pack = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs.first + + rule_pack.should_not be_nil + if loaded_rule_pack = rule_pack + row_context = loaded_rule_pack.modes_by_name["row"].guidance_text + row_context.should contain("detected from the app's `multitenant` model macros") + row_context.should contain("ScopedRawSqlError") + row_context.should contain("fiber-local") + schema_context = loaded_rule_pack.modes_by_name["schema"].guidance_text + schema_context.should contain("default search path without an error") + schema_context.should contain("PostgreSQL") + end end end - it "runs a declared dependency pack in a non-Amber project" do + it "loads exactly ten rules with only the two intended errors" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "jobs")) - file_path = File.join(root, "src", "jobs", "fixture.cr") - diagnostics = analyze_pack_file(root, file_path, "puts \"unscoped\"\n") - - diagnostics.map(&.code).should eq([] of String) + install_tenancy_fixture_app(root, "row_app") + project_context = AmberLSP::ProjectContext.detect(root) + loaded_rule_pack = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs.first + + loaded_rule_pack.should_not be_nil + if rule_pack = loaded_rule_pack + rule_pack.list_of_rules.size.should eq(10) + rule_pack.list_of_rules.count { |rule| rule.severity_name == "error" }.should eq(2) + rule_pack.list_of_rules.count { |rule| rule.severity_name == "warning" }.should eq(8) + end end end - it "runs line_regex checks and ignores full-line comments" do + it "runs a pack in a non-Amber app and detects the account_id macro argument" do with_tempdir do |root| - write_rule_pack_project(root) - file_path = File.join(root, "src", "controllers", "todos_controller.cr") - content = "# Todo.unscoped is only documentation\nTodo.unscoped.all\n" + install_tenancy_fixture_app(root, "row_app") + project_context = AmberLSP::ProjectContext.detect(root) + project_context.amber_project?.should be_false - diagnostics = analyze_pack_file(root, file_path, content) + diagnostics = analyze_tenancy_fixture_source( + root, + "src/controllers/invoices_controller.cr", + "Invoice.unscoped.all\n", + ) - diagnostics.map(&.code).should eq(["grant/unscoped-in-request-code"]) - diagnostics.first.range.start.line.should eq(1) - diagnostics.first.severity.should eq(AmberLSP::Rules::Severity::Warning) + has_tenancy_diagnostic_code?(diagnostics, "grant/chained-unscoped-in-request-code").should be_true + diagnostics.any? do |diagnostic| + diagnostic.code == "grant/chained-unscoped-in-request-code" && + diagnostic.severity == AmberLSP::Rules::Severity::Error + end.should be_true end end - it "reports a tenant column without multitenant and accepts the declared macro" do + it "reports chainable unscoped calls in each request path and allows block-form unscoped" do with_tempdir do |root| - write_rule_pack_project(root) - file_path = File.join(root, "src", "models", "account.cr") - Dir.mkdir_p(File.dirname(file_path)) - missing_macro = "class Account\n column tenant_id : Int64\nend\n" + install_tenancy_fixture_app(root, "row_app") + + GRANT_TENANCY_REQUEST_PATHS.each do |relative_path| + diagnostics = analyze_tenancy_fixture_source(root, relative_path, "Invoice.unscoped.all\n") + has_tenancy_diagnostic_code?(diagnostics, "grant/chained-unscoped-in-request-code").should be_true + + block_diagnostics = analyze_tenancy_fixture_source( + root, + relative_path, + "Invoice.unscoped { Invoice.all }\n", + ) + has_tenancy_diagnostic_code?(block_diagnostics, "grant/chained-unscoped-in-request-code").should be_false + end + end + end - diagnostics = analyze_pack_file(root, file_path, missing_macro) + it "reports unscoped bulk writes and leaves scoped writes clean" do + with_tempdir do |root| + install_tenancy_fixture_app(root, "row_app") - diagnostics.map(&.code).should contain("grant/tenant-column-without-multitenant") - diagnostics.find(&.code.==("grant/tenant-column-without-multitenant")).not_nil!.severity.should eq(AmberLSP::Rules::Severity::Error) + bulk_write_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/delete_invoices_job.cr", + "Invoice.unscoped.delete_all\n", + ) + has_tenancy_diagnostic_code?(bulk_write_diagnostics, "grant/chained-unscoped-bulk-write").should be_true + bulk_write_diagnostics.any? do |diagnostic| + diagnostic.code == "grant/chained-unscoped-bulk-write" && diagnostic.severity == AmberLSP::Rules::Severity::Error + end.should be_true + + scoped_write_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/delete_invoices_job.cr", + "Invoice.where(id: 1).delete_all\n", + ) + has_tenancy_diagnostic_code?(scoped_write_diagnostics, "grant/chained-unscoped-bulk-write").should be_false - valid_model = "class Account\n column tenant_id : Int64\n multitenant :tenant_id\nend\n" - valid_diagnostics = analyze_pack_file(root, file_path, valid_model) + spec_diagnostics = analyze_tenancy_fixture_source( + root, + "spec/delete_invoices_spec.cr", + "Invoice.unscoped.update_all({\"status\" => \"archived\"})\n", + ) + has_tenancy_diagnostic_code?(spec_diagnostics, "grant/chained-unscoped-bulk-write").should be_false - valid_diagnostics.map(&.code).should_not contain("grant/tenant-column-without-multitenant") + db_diagnostics = analyze_tenancy_fixture_source( + root, + "db/backfill.cr", + "Invoice.unscoped.delete_all\n", + ) + has_tenancy_diagnostic_code?(db_diagnostics, "grant/chained-unscoped-bulk-write").should be_false end end - it "uses the configured tenant column for file_requires" do + it "warns about chainable unscoped reads outside request code and excludes bulk writes" do with_tempdir do |root| - pack_content = GRANT_RULE_PACK_FIXTURE.gsub("tenant_column: tenant_id", "tenant_column: account_id") - write_rule_pack_project(root, pack_content: pack_content) - file_path = File.join(root, "src", "models", "account.cr") - Dir.mkdir_p(File.dirname(file_path)) + install_tenancy_fixture_app(root, "row_app") + + read_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/read_invoices_job.cr", "Invoice.unscoped.all\n") + has_tenancy_diagnostic_code?(read_diagnostics, "grant/chained-unscoped-on-tenant-model").should be_true + + request_diagnostics = analyze_tenancy_fixture_source( + root, + "src/controllers/invoices_controller.cr", + "Invoice.unscoped.all\n", + ) + has_tenancy_diagnostic_code?(request_diagnostics, "grant/chained-unscoped-on-tenant-model").should be_false - diagnostics = analyze_pack_file(root, file_path, "class Account\n column account_id : Int64\nend\n") + write_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/delete_invoices_job.cr", + "Invoice.unscoped.delete_all\n", + ) + has_tenancy_diagnostic_code?(write_diagnostics, "grant/chained-unscoped-on-tenant-model").should be_false - diagnostics.map(&.code).should contain("grant/tenant-column-without-multitenant") + scoped_read_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/read_invoices_job.cr", "Invoice.all\n") + has_tenancy_diagnostic_code?(scoped_read_diagnostics, "grant/chained-unscoped-on-tenant-model").should be_false end end - it "reports a multitenant query outside the required block" do + it "warns about block-form unscoped in request code and allows the default scope" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) + install_tenancy_fixture_app(root, "row_app") - diagnostics = analyze_pack_file(root, file_path, "Todo.where(active: true)\n") + unsafe_diagnostics = analyze_tenancy_fixture_source( + root, + "src/controllers/invoices_controller.cr", + "Invoice.unscoped { Invoice.all }\n", + ) + has_tenancy_diagnostic_code?(unsafe_diagnostics, "grant/unscoped-block-in-request-code").should be_true - diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + safe_diagnostics = analyze_tenancy_fixture_source( + root, + "src/controllers/invoices_controller.cr", + "Invoice.all\n", + ) + has_tenancy_diagnostic_code?(safe_diagnostics, "grant/unscoped-block-in-request-code").should be_false end end - it "accepts queries inside the required block and nested blocks" do + it "warns when spawn is inside either tenant block and allows spawn outside" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - content = <<-CRYSTAL - Grant::Tenant.with(7) do - Todo.where(active: true) - run do - Todo.all - end - end - CRYSTAL + install_tenancy_fixture_app(root, "row_app") + row_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/invoice_job.cr", + "Grant::Tenant.with(7) { spawn { Invoice.all } }\n", + ) + has_tenancy_diagnostic_code?(row_diagnostics, "grant/spawn-inside-tenant-block").should be_true - diagnostics = analyze_pack_file(root, file_path, content) + outside_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/invoice_job.cr", + "spawn { Invoice.all }\n", + ) + has_tenancy_diagnostic_code?(outside_diagnostics, "grant/spawn-inside-tenant-block").should be_false - diagnostics.map(&.code).should_not contain("grant/row-query-outside-tenant") + install_tenancy_fixture_app(root, "schema_app") + schema_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/invoice_job.cr", + "Grant::SchemaTenant.with(\"acme\") { spawn { Invoice.all } }\n", + ) + has_tenancy_diagnostic_code?(schema_diagnostics, "grant/spawn-inside-tenant-block").should be_true end end - it "does not treat a method defined inside a tenant block as scoped" do + it "uses captured tenant columns and skips a model whose table the column references" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - content = <<-CRYSTAL - Grant::Tenant.with(7) do - def load_todos - Todo.all - end - end - CRYSTAL - - diagnostics = analyze_pack_file(root, file_path, content) - - diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + install_tenancy_fixture_app(root, "row_app") + + missing_macro = File.read(File.join( + Dir.current, + "spec", + "fixtures", + "rule_pack_apps", + "row_app", + "src", + "models", + "invoice_export.cr", + )) + missing_diagnostics = analyze_tenancy_fixture_source(root, "src/models/invoice_export.cr", missing_macro) + has_tenancy_diagnostic_code?(missing_diagnostics, "grant/tenant-column-without-multitenant").should be_true + missing_diagnostics.any? do |diagnostic| + diagnostic.code == "grant/tenant-column-without-multitenant" && diagnostic.message.includes?("account_id") + end.should be_true + + declared_macro = missing_macro.sub("column account_id : Int64", "column account_id : Int64\n multitenant :account_id") + declared_diagnostics = analyze_tenancy_fixture_source(root, "src/models/invoice_export.cr", declared_macro) + has_tenancy_diagnostic_code?(declared_diagnostics, "grant/tenant-column-without-multitenant").should be_false + + referenced_table = File.read(File.join( + Dir.current, + "spec", + "fixtures", + "rule_pack_apps", + "row_app", + "src", + "models", + "account.cr", + )) + referenced_diagnostics = analyze_tenancy_fixture_source(root, "src/models/account.cr", referenced_table) + has_tenancy_diagnostic_code?(referenced_diagnostics, "grant/tenant-column-without-multitenant").should be_false + + unrelated_column = missing_macro.sub("account_id", "owner_id") + unrelated_diagnostics = analyze_tenancy_fixture_source(root, "src/models/invoice_export.cr", unrelated_column) + has_tenancy_diagnostic_code?(unrelated_diagnostics, "grant/tenant-column-without-multitenant").should be_false end end - it "does not scope a method body just because its call is inside a tenant block" do + it "finds raw connection SQL naming a row tenant table but ignores model raw SQL" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - content = <<-CRYSTAL - def load_todos - Todo.all + install_tenancy_fixture_app(root, "row_app") + + connection_sql = <<-CRYSTAL + Invoice.adapter.open do |db| + db.exec("SELECT * FROM invoices WHERE account_id = 7") end - Grant::Tenant.with(7) { load_todos } CRYSTAL + connection_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/raw_report_job.cr", connection_sql) + has_tenancy_diagnostic_code?(connection_diagnostics, "grant/raw-connection-sql-on-tenant-table").should be_true - diagnostics = analyze_pack_file(root, file_path, content) + model_raw_sql = <<-CRYSTAL + Invoice.unscoped do + Invoice.exec("SELECT * FROM invoices") + end + CRYSTAL + model_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/raw_report_job.cr", model_raw_sql) + has_tenancy_diagnostic_code?(model_diagnostics, "grant/raw-connection-sql-on-tenant-table").should be_false - diagnostics.map(&.code).should contain("grant/row-query-outside-tenant") + other_table_sql = <<-CRYSTAL + Invoice.adapter.open do |db| + db.exec("SELECT * FROM audit_events") + end + CRYSTAL + other_table_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/raw_report_job.cr", other_table_sql) + has_tenancy_diagnostic_code?(other_table_diagnostics, "grant/raw-connection-sql-on-tenant-table").should be_false + + default_table_model = File.read(File.join( + Dir.current, + "spec", + "fixtures", + "rule_pack_apps", + "row_app", + "src", + "models", + "ledger_entry.cr", + )) + analyze_tenancy_fixture_source(root, "src/models/ledger_entry.cr", default_table_model) + default_table_sql = <<-CRYSTAL + Grant::Connections["primary"][:writer].open do |db| + db.scalar("SELECT COUNT(*) FROM ledger_entry") + end + CRYSTAL + default_table_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/raw_report_job.cr", default_table_sql) + has_tenancy_diagnostic_code?(default_table_diagnostics, "grant/raw-connection-sql-on-tenant-table").should be_true + + annotated_table_model = File.read(File.join( + Dir.current, + "spec", + "fixtures", + "rule_pack_apps", + "row_app", + "src", + "models", + "custom_document.cr", + )) + analyze_tenancy_fixture_source(root, "src/models/custom_document.cr", annotated_table_model) + annotated_table_sql = <<-CRYSTAL + Grant::Connections["primary"][:writer].open do |db| + db.query("SELECT * FROM custom_documents") { } + end + CRYSTAL + annotated_table_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/raw_report_job.cr", annotated_table_sql) + has_tenancy_diagnostic_code?(annotated_table_diagnostics, "grant/raw-connection-sql-on-tenant-table").should be_true end end - it "allows a same-model unscoped block but not a different model's query" do + it "warns about Tenant.clear outside specs and allows it in specs" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - File.write(File.join(root, "src", "models", "invoice.cr"), "class Invoice\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - - same_model = analyze_pack_file(root, file_path, "Todo.unscoped { Todo.where(active: true) }\n") - same_model.map(&.code).should_not contain("grant/row-query-outside-tenant") - - different_model = analyze_pack_file(root, file_path, "Todo.unscoped { Invoice.all }\n") - different_model.map(&.code).should contain("grant/row-query-outside-tenant") + install_tenancy_fixture_app(root, "row_app") + + app_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/reset_tenant_job.cr", + "Grant::Tenant.clear\n", + ) + has_tenancy_diagnostic_code?(app_diagnostics, "grant/tenant-clear-in-app-code").should be_true + + spec_diagnostics = analyze_tenancy_fixture_source( + root, + "spec/reset_tenant_spec.cr", + "Grant::Tenant.clear\n", + ) + has_tenancy_diagnostic_code?(spec_diagnostics, "grant/tenant-clear-in-app-code").should be_false + + other_clear = analyze_tenancy_fixture_source(root, "src/jobs/reset_tenant_job.cr", "Grant::SchemaTenant.clear\n") + has_tenancy_diagnostic_code?(other_clear, "grant/tenant-clear-in-app-code").should be_false end end - it "does not flag raw_all and allows raw SQL inside the same model's unscoped block" do + it "warns about schema queries outside a tenant block and skips excluded models" do with_tempdir do |root| - write_rule_pack_project(root) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - file_path = File.join(root, "src", "jobs", "cleanup_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - - raw_all_diagnostics = analyze_pack_file(root, file_path, "Todo.raw_all(\"WHERE active = true\")\n") - raw_all_diagnostics.map(&.code).should_not contain("grant/raw-sql-on-scoped-model") - - scoped_sql = <<-CRYSTAL - Todo.unscoped do - Todo.exec("DELETE FROM todos") - Todo.query("SELECT 1") { } - Todo.scalar("SELECT COUNT(*) FROM todos") { |value| value } - end - CRYSTAL - scoped_sql_diagnostics = analyze_pack_file(root, file_path, scoped_sql) - scoped_sql_diagnostics.map(&.code).should_not contain("grant/raw-sql-on-scoped-model") + install_tenancy_fixture_app(root, "schema_app") + + unsafe_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/rebuild_invoice_job.cr", "Invoice.all\n") + has_tenancy_diagnostic_code?(unsafe_diagnostics, "grant/schema-query-outside-tenant").should be_true + + scoped_diagnostics = analyze_tenancy_fixture_source( + root, + "src/jobs/rebuild_invoice_job.cr", + "Grant::SchemaTenant.with(\"acme\") { Invoice.all }\n", + ) + has_tenancy_diagnostic_code?(scoped_diagnostics, "grant/schema-query-outside-tenant").should be_false + + excluded_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/rebuild_invoice_job.cr", "Country.all\n") + has_tenancy_diagnostic_code?(excluded_diagnostics, "grant/schema-query-outside-tenant").should be_false - unsafe_sql = "Todo.exec(\"DELETE FROM todos\")\nTodo.query(\"SELECT 1\") { }\nTodo.scalar(\"SELECT 1\") { |value| value }\n" - unsafe_sql_diagnostics = analyze_pack_file(root, file_path, unsafe_sql) - unsafe_sql_diagnostics.count(&.code.==("grant/raw-sql-on-scoped-model")).should eq(3) + request_diagnostics = analyze_tenancy_fixture_source( + root, + "src/controllers/invoices_controller.cr", + "Invoice.all\n", + ) + has_tenancy_diagnostic_code?(request_diagnostics, "grant/schema-query-outside-tenant").should be_false end end - it "reports mixed modes as an error and undeclared row use as a warning" do + it "detects schema mode from schema_tenant_excluded without requiring a with call" do with_tempdir do |root| - write_rule_pack_project(root) - file_path = File.join(root, "src", "jobs", "tenancy_job.cr") - Dir.mkdir_p(File.dirname(file_path)) - mixed_content = "Grant::SchemaTenant.with(\"acme\") { run_job }\n" - - mixed_diagnostics = analyze_pack_file(root, file_path, mixed_content) - mixed_diagnostic = mixed_diagnostics.find(&.code.==("grant/tenancy-modes-mixed")).not_nil! - mixed_diagnostic.severity.should eq(AmberLSP::Rules::Severity::Error) - - shard_content = "name: tenant_app\nversion: 0.1.0\n" - write_rule_pack_project(root, shard_content) - Dir.mkdir_p(File.join(root, "src", "models")) - File.write(File.join(root, "src", "models", "todo.cr"), "class Todo\n multitenant :tenant_id\nend\n") - undeclared_path = File.join(root, "src", "jobs", "undeclared_job.cr") - undeclared_diagnostics = analyze_pack_file(root, undeclared_path, "Todo.all\n") - undeclared = undeclared_diagnostics.find(&.code.==("grant/tenancy-undeclared")).not_nil! - undeclared.severity.should eq(AmberLSP::Rules::Severity::Warning) + install_tenancy_fixture_app(root, "schema_app") + File.delete(File.join(root, "config", "tenant_scope.cr")) + + diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/rebuild_invoice_job.cr", "Invoice.all\n") + + has_tenancy_diagnostic_code?(diagnostics, "grant/schema-query-outside-tenant").should be_true end end - it "keeps Amber built-in rules gated on Amber dependencies" do + it "warns when both source-detected modes appear and leaves either mode alone" do with_tempdir do |root| - write_rule_pack_project(root) - file_path = File.join(root, "src", "controllers", "home_controller.cr") - Dir.mkdir_p(File.dirname(file_path)) - content = "class HomeController < ApplicationController\n def index\n User.all\n end\nend\n" - AmberLSP::Rules::RuleRegistry.register(AmberLSP::Rules::Controllers::ActionReturnRule.new) - non_amber_analyzer = AmberLSP::Analyzer.new - non_amber_analyzer.configure(AmberLSP::ProjectContext.detect(root)) - - non_amber_diagnostics = non_amber_analyzer.analyze(file_path, content) - non_amber_diagnostics.map(&.code).should_not contain("amber/action-return-type") - - shard_content = <<-YAML - name: tenant_app - version: 0.1.0 - grant: - tenancy: row - dependencies: - amber: - github: amberframework/amber - YAML - File.write(File.join(root, "shard.yml"), shard_content) - amber_analyzer = AmberLSP::Analyzer.new - amber_analyzer.configure(AmberLSP::ProjectContext.detect(root)) - - amber_diagnostics = amber_analyzer.analyze(file_path, content) - amber_diagnostics.map(&.code).should contain("amber/action-return-type") + install_tenancy_fixture_app(root, "mixed_app") + + mixed_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/check_tenant_job.cr", "Invoice.all\n") + has_tenancy_diagnostic_code?(mixed_diagnostics, "grant/tenancy-modes-mixed").should be_true + mixed_diagnostics.any? do |diagnostic| + diagnostic.code == "grant/tenancy-modes-mixed" && diagnostic.message.includes?("docs/schema_tenancy.md") + end.should be_true + + install_tenancy_fixture_app(root, "row_app") + row_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/check_tenant_job.cr", "Invoice.all\n") + has_tenancy_diagnostic_code?(row_diagnostics, "grant/tenancy-modes-mixed").should be_false + + install_tenancy_fixture_app(root, "schema_app") + schema_diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/check_tenant_job.cr", "Invoice.all\n") + has_tenancy_diagnostic_code?(schema_diagnostics, "grant/tenancy-modes-mixed").should be_false end end - it "prints declared context and warns when feature use has no declaration" do + it "ignores tenancy keys in shard.yml and reports no diagnostics for an app with no tenancy" do with_tempdir do |root| - write_rule_pack_project(root) - file_path = File.join(root, "src", "models", "todo.cr") - Dir.mkdir_p(File.dirname(file_path)) - File.write(file_path, "class Todo\n multitenant :tenant_id\nend\n") - binary_path = File.join(Dir.current, "bin", "amber-lsp") - stdout = IO::Memory.new - stderr = IO::Memory.new - status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: stderr) - - status.success?.should be_true - stderr.to_s.should be_empty - stdout.to_s.should contain("grant/tenancy (row)") - stdout.to_s.should contain("Scope queries with Grant::Tenant.with.") - - undeclared_shard = "name: tenant_app\nversion: 0.1.0\n" - write_rule_pack_project(root, undeclared_shard) - stdout = IO::Memory.new - stderr = IO::Memory.new - status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: stderr) - - status.success?.should be_true - stdout.to_s.should contain("warning: grant/tenancy feature is used but shard.yml does not declare grant.tenancy.") + install_tenancy_fixture_app(root, "no_tenancy_app") + File.write( + File.join(root, "shard.yml"), + "name: no_tenancy_app\nversion: 0.1.0\ngrant:\n tenancy: row\n", + ) + Dir.mkdir_p(File.join(root, "spec")) + File.write(File.join(root, "spec", "fake_tenancy.cr"), "Grant::SchemaTenant.with(\"spec\") { nil }\n") + Dir.mkdir_p(File.join(root, "lib", "other")) + File.write(File.join(root, "lib", "other", "fake_tenancy.cr"), "Grant::SchemaTenant.with(\"lib\") { nil }\n") + File.write( + File.join(root, "src", "models", "commented_macro.cr"), + "# multitenant :account_id\nMESSAGE = \"schema_tenant_excluded\"\nclass Invoice\n def configure\n multitenant :account_id\n end\nend\n", + ) + + diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/clean_job.cr", "Grant::Tenant.clear\nInvoice.all\n") + + diagnostics.should be_empty + File.read(File.join(root, "shard.yml")).should contain("grant:") end end - it "prints nothing when no pack is declared or evidenced" do + it "skips malformed Crystal files without crashing or inventing another mode" do with_tempdir do |root| - File.write(File.join(root, "shard.yml"), "name: empty_app\nversion: 0.1.0\n") - binary_path = File.join(Dir.current, "bin", "amber-lsp") - stdout = IO::Memory.new - status = Process.run(binary_path, ["context", "--root", root], output: stdout, error: Process::Redirect::Close) + install_tenancy_fixture_app(root, "row_app") + Dir.mkdir_p(File.join(root, "config")) + File.write(File.join(root, "config", "broken.cr"), "Grant::SchemaTenant.with(\"broken\") do\n") + + diagnostics = analyze_tenancy_fixture_source(root, "src/jobs/clean_job.cr", "Invoice.all\n") - status.success?.should be_true - stdout.to_s.should be_empty + diagnostics.should be_empty end end end diff --git a/spec/amber_lsp/spec_helper.cr b/spec/amber_lsp/spec_helper.cr index 822b2f0..6a2a665 100644 --- a/spec/amber_lsp/spec_helper.cr +++ b/spec/amber_lsp/spec_helper.cr @@ -12,9 +12,17 @@ require "../../src/amber_lsp/configuration" require "../../src/amber_lsp/library_rule_packs/describe_library_rule_pack" require "../../src/amber_lsp/library_rule_packs/load_rule_packs_for_project" require "../../src/amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/source_node" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations" require "../../src/amber_lsp/library_rule_packs/determine_project_rule_pack_state" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites" +require "../../src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks" require "../../src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs" -require "../../src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts" +require "../../src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts" require "../../src/amber_lsp/analyzer" require "../../src/amber_lsp/controller" require "../../src/amber_lsp/server" diff --git a/spec/fixtures/rule_pack_apps/mixed_app/config/tenant_scope.cr b/spec/fixtures/rule_pack_apps/mixed_app/config/tenant_scope.cr new file mode 100644 index 0000000..6568adb --- /dev/null +++ b/spec/fixtures/rule_pack_apps/mixed_app/config/tenant_scope.cr @@ -0,0 +1,3 @@ +Grant::SchemaTenant.with("acme") do + Invoice.all +end diff --git a/spec/fixtures/rule_pack_apps/mixed_app/src/models/invoice.cr b/spec/fixtures/rule_pack_apps/mixed_app/src/models/invoice.cr new file mode 100644 index 0000000..54a057d --- /dev/null +++ b/spec/fixtures/rule_pack_apps/mixed_app/src/models/invoice.cr @@ -0,0 +1,8 @@ +class Invoice < Grant::Base + table :invoices + + column id : Int64, primary: true + column account_id : Int64 + + multitenant :account_id +end diff --git a/spec/fixtures/rule_pack_apps/no_tenancy_app/src/models/invoice.cr b/spec/fixtures/rule_pack_apps/no_tenancy_app/src/models/invoice.cr new file mode 100644 index 0000000..95839d3 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/no_tenancy_app/src/models/invoice.cr @@ -0,0 +1,6 @@ +class Invoice < Grant::Base + table :invoices + + column id : Int64, primary: true + column account_id : Int64 +end diff --git a/spec/fixtures/rule_pack_apps/row_app/lib/grant/.amber-lsp/packs/tenancy.yml b/spec/fixtures/rule_pack_apps/row_app/lib/grant/.amber-lsp/packs/tenancy.yml new file mode 100644 index 0000000..48a0431 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/lib/grant/.amber-lsp/packs/tenancy.yml @@ -0,0 +1,142 @@ +pack: grant/tenancy +library: grant +version: 2.0.0 + +modes: + row: + context: | + Grant row tenancy was detected from the app's `multitenant` model macros. + Each model captures the tenant column passed to its own macro. Grant adds a + default scope that raises `Grant::NoTenantError` when no tenant is set, and + writes using another tenant's id raise `Grant::TenantMismatchError`. + `Model.exec/query/scalar` on a default-scoped model raises + `Grant::Querying::ScopedRawSqlError` outside `Model.unscoped { }`. + Block-form `unscoped` temporarily disables scoping. Chainable `Model.unscoped` + returns an unguarded builder, so chained reads and writes can cross tenants. + `Grant::Tenant.with` is fiber-local: a spawned fiber starts without the + tenant. Carry the tenant id into that fiber and wrap its work. `Tenant.clear` + exists mainly for tests. + schema: + context: | + Grant schema tenancy was detected from the app's `Grant::SchemaTenant.with` + call or `schema_tenant_excluded` model macro. `SchemaTenant.with` pins a + PostgreSQL connection with the tenant search path; excluded models use + `public`. A query on a non-excluded model outside the block runs against the + default search path without an error. The request pipe covers controllers; + jobs and other non-request work need their own block. The row and schema + modes can coexist, but review `docs/schema_tenancy.md` for their tradeoffs + and migration path. + +rules: + - id: grant/chained-unscoped-in-request-code + modes: [row] + severity: error + applies_to: + - "src/controllers/**" + - "src/channels/**" + - "src/sockets/**" + - "src/pipes/**" + message: "Remove chainable unscoped from request code and rely on the model's tenant scope." + check: + kind: crystal_ast + operation: chained_unscoped_in_request_code + + - id: grant/chained-unscoped-bulk-write + modes: [row] + severity: error + applies_to: ["**/*.cr"] + exclude_from: ["spec/**", "db/**"] + message: "Replace the unscoped bulk write with a tenant-scoped write or an explicit tenant predicate." + check: + kind: crystal_ast + operation: chained_unscoped_bulk_write + + - id: grant/chained-unscoped-on-tenant-model + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + exclude_from: + - "src/controllers/**" + - "src/channels/**" + - "src/sockets/**" + - "src/pipes/**" + - "spec/**" + - "db/**" + message: "Use the tenant-scoped model query; reserve chainable unscoped for an explicitly justified cross-tenant operation." + check: + kind: crystal_ast + operation: chained_unscoped_on_tenant_model + + - id: grant/unscoped-block-in-request-code + modes: [row] + severity: warning + applies_to: + - "src/controllers/**" + - "src/channels/**" + - "src/sockets/**" + - "src/pipes/**" + message: "Remove the unscoped block from request code and rely on the model's tenant scope." + check: + kind: crystal_ast + operation: unscoped_block_in_request_code + + - id: grant/spawn-inside-tenant-block + modes: [row, schema] + severity: warning + applies_to: ["**/*.cr"] + exclude_from: ["spec/**"] + message: "Carry the tenant id or schema into the fiber and wrap its work in the matching Grant tenant block." + check: + kind: crystal_ast + operation: spawn_inside_tenant_block + + - id: grant/tenant-column-without-multitenant + modes: [row] + severity: warning + applies_to: ["src/**"] + message: "This model declares {tenant_column}, which is used as a tenant key elsewhere; add multitenant :{tenant_column}, or rename/remove it if unrelated." + check: + kind: crystal_ast + operation: tenant_column_without_multitenant + + - id: grant/raw-connection-sql-on-tenant-table + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + exclude_from: ["spec/**"] + message: "Raw connection SQL bypasses model scopes; use the tenant-scoped model API or add an explicit tenant predicate." + check: + kind: crystal_ast + operation: raw_connection_sql_on_tenant_table + + - id: grant/tenant-clear-in-app-code + modes: [row] + severity: warning + applies_to: ["**/*.cr"] + exclude_from: ["spec/**"] + message: "Remove Tenant.clear from app code; scope work with Grant::Tenant.with and reserve clear for tests." + check: + kind: crystal_ast + operation: tenant_clear_in_app_code + + - id: grant/schema-query-outside-tenant + modes: [schema] + severity: warning + applies_to: + - "src/jobs/**" + - "src/lib/**" + - "src/domain/**" + - "src/process_managers/**" + message: "Wrap non-request work in Grant::SchemaTenant.with(schema); otherwise this query uses the default search path." + check: + kind: crystal_ast + operation: schema_query_outside_tenant + + - id: grant/tenancy-modes-mixed + modes: [row, schema] + severity: warning + applies_to: ["**/*.cr"] + message: "Both tenancy modes were detected; review docs/schema_tenancy.md and document why both are needed." + check: + kind: project_conflict + condition: mixed_modes diff --git a/spec/fixtures/rule_pack_apps/row_app/src/models/account.cr b/spec/fixtures/rule_pack_apps/row_app/src/models/account.cr new file mode 100644 index 0000000..3d8f17b --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/src/models/account.cr @@ -0,0 +1,6 @@ +class Account < Grant::Base + table :accounts + + column id : Int64, primary: true + column account_id : Int64 +end diff --git a/spec/fixtures/rule_pack_apps/row_app/src/models/custom_document.cr b/spec/fixtures/rule_pack_apps/row_app/src/models/custom_document.cr new file mode 100644 index 0000000..d09e19f --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/src/models/custom_document.cr @@ -0,0 +1,5 @@ +@[Grant::Table(name: :custom_documents)] +class CustomDocument < Grant::Base + column account_id : Int64 + multitenant :account_id +end diff --git a/spec/fixtures/rule_pack_apps/row_app/src/models/invoice.cr b/spec/fixtures/rule_pack_apps/row_app/src/models/invoice.cr new file mode 100644 index 0000000..54a057d --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/src/models/invoice.cr @@ -0,0 +1,8 @@ +class Invoice < Grant::Base + table :invoices + + column id : Int64, primary: true + column account_id : Int64 + + multitenant :account_id +end diff --git a/spec/fixtures/rule_pack_apps/row_app/src/models/invoice_export.cr b/spec/fixtures/rule_pack_apps/row_app/src/models/invoice_export.cr new file mode 100644 index 0000000..c49b0a7 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/src/models/invoice_export.cr @@ -0,0 +1,4 @@ +class InvoiceExport < Grant::Base + table :invoice_exports + column account_id : Int64 +end diff --git a/spec/fixtures/rule_pack_apps/row_app/src/models/ledger_entry.cr b/spec/fixtures/rule_pack_apps/row_app/src/models/ledger_entry.cr new file mode 100644 index 0000000..eece661 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/row_app/src/models/ledger_entry.cr @@ -0,0 +1,5 @@ +class LedgerEntry < Grant::Base + column id : Int64, primary: true + column account_id : Int64 + multitenant :account_id +end diff --git a/spec/fixtures/rule_pack_apps/schema_app/config/tenant_scope.cr b/spec/fixtures/rule_pack_apps/schema_app/config/tenant_scope.cr new file mode 100644 index 0000000..6568adb --- /dev/null +++ b/spec/fixtures/rule_pack_apps/schema_app/config/tenant_scope.cr @@ -0,0 +1,3 @@ +Grant::SchemaTenant.with("acme") do + Invoice.all +end diff --git a/spec/fixtures/rule_pack_apps/schema_app/src/models/country.cr b/spec/fixtures/rule_pack_apps/schema_app/src/models/country.cr new file mode 100644 index 0000000..8180127 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/schema_app/src/models/country.cr @@ -0,0 +1,7 @@ +class Country < Grant::Base + table :countries + + column id : Int64, primary: true + + schema_tenant_excluded +end diff --git a/spec/fixtures/rule_pack_apps/schema_app/src/models/invoice.cr b/spec/fixtures/rule_pack_apps/schema_app/src/models/invoice.cr new file mode 100644 index 0000000..b235c74 --- /dev/null +++ b/spec/fixtures/rule_pack_apps/schema_app/src/models/invoice.cr @@ -0,0 +1,5 @@ +class Invoice < Grant::Base + table :invoices + + column id : Int64, primary: true +end diff --git a/src/amber_lsp.cr b/src/amber_lsp.cr index d4ae72e..85d511f 100644 --- a/src/amber_lsp.cr +++ b/src/amber_lsp.cr @@ -24,15 +24,23 @@ require "./amber_lsp/configuration" require "./amber_lsp/library_rule_packs/describe_library_rule_pack" require "./amber_lsp/library_rule_packs/load_rule_packs_for_project" require "./amber_lsp/library_rule_packs/visit_crystal_calls_outside_required_blocks" +require "./amber_lsp/library_rule_packs/grant_tenancy/source_node" +require "./amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration" +require "./amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations" require "./amber_lsp/library_rule_packs/determine_project_rule_pack_state" +require "./amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls" +require "./amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks" +require "./amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls" +require "./amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites" +require "./amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks" require "./amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs" -require "./amber_lsp/library_rule_packs/print_declared_rule_pack_contexts" +require "./amber_lsp/library_rule_packs/print_detected_rule_pack_contexts" require "./amber_lsp/analyzer" require "./amber_lsp/controller" require "./amber_lsp/server" if ARGV.first? == "context" - exit AmberLSP::LibraryRulePacks::PrintDeclaredRulePackContexts.new(ARGV[1..].to_a).perform + exit AmberLSP::LibraryRulePacks::PrintDetectedRulePackContexts.new(ARGV[1..].to_a).perform else AmberLSP::Server.new(STDIN, STDOUT).run end diff --git a/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr b/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr index da0595d..109a347 100644 --- a/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr +++ b/src/amber_lsp/library_rule_packs/analyze_project_files_with_rule_packs.cr @@ -36,15 +36,15 @@ module AmberLSP::LibraryRulePacks if rule.check.check_kind == "project_conflict" list_of_diagnostics.concat( - project_conflict_diagnostics_for(rule_pack, rule, project_state) + project_conflict_diagnostics_for(rule, project_state) ) next end - next unless project_state.is_rule_mode_declared?(rule.list_of_mode_names) + next unless project_state.rule_mode_detected?(rule.list_of_mode_names) list_of_diagnostics.concat( - file_diagnostics_for(rule_pack, rule, project_state, file_path, relative_file_path, content) + file_diagnostics_for(rule, project_state, file_path, relative_file_path, content) ) end end @@ -68,15 +68,12 @@ module AmberLSP::LibraryRulePacks end private def project_conflict_diagnostics_for( - rule_pack : DescribeLibraryRulePack, rule : DescribeLibraryRulePack::Rule, project_state : DetermineProjectRulePackState, ) : Array(Rules::Diagnostic) conflict_found = case rule.check.project_condition when "mixed_modes" project_state.all_modes_are_present?(rule.list_of_mode_names) - when "evidence_without_declaration" - project_state.has_undeclared_mode_evidence?(rule.list_of_mode_names) else false end @@ -86,7 +83,6 @@ module AmberLSP::LibraryRulePacks end private def file_diagnostics_for( - rule_pack : DescribeLibraryRulePack, rule : DescribeLibraryRulePack::Rule, project_state : DetermineProjectRulePackState, file_path : String, @@ -97,9 +93,11 @@ module AmberLSP::LibraryRulePacks when "line_regex" line_regex_diagnostics_for(rule, relative_file_path, content) when "file_requires" - file_requires_diagnostics_for(rule_pack, rule, content) + file_requires_diagnostics_for(rule, content) when "call_outside_block" call_outside_block_diagnostics_for(rule, project_state, file_path, content) + when "crystal_ast" + crystal_ast_diagnostics_for(rule, project_state, file_path, content) else [] of Rules::Diagnostic end @@ -123,14 +121,13 @@ module AmberLSP::LibraryRulePacks end private def file_requires_diagnostics_for( - rule_pack : DescribeLibraryRulePack, rule : DescribeLibraryRulePack::Rule, content : String, ) : Array(Rules::Diagnostic) required_pattern = Regex.new(rule.check.required_regex_pattern) return [] of Rules::Diagnostic if content.each_line.any? { |line| required_pattern.matches?(line) } - trigger_pattern = trigger_pattern_for(rule_pack, rule) + trigger_pattern = trigger_pattern_for(rule) return [] of Rules::Diagnostic unless trigger_pattern list_of_diagnostics = [] of Rules::Diagnostic @@ -155,20 +152,12 @@ module AmberLSP::LibraryRulePacks list_of_diagnostics end - private def trigger_pattern_for( - rule_pack : DescribeLibraryRulePack, - rule : DescribeLibraryRulePack::Rule, - ) : Regex? + private def trigger_pattern_for(rule : DescribeLibraryRulePack::Rule) : Regex? unless rule.check.trigger_regex_pattern.empty? return Regex.new(rule.check.trigger_regex_pattern) end - mode = rule_pack.modes_by_name[rule.list_of_mode_names.first]? - tenant_column_name = mode.try(&.tenant_column_name) - return nil unless tenant_column_name - return nil unless tenant_column_name.matches?(/\A[a-zA-Z_][a-zA-Z0-9_]*\z/) - - Regex.new("^\\s*column\\s+#{tenant_column_name}\\b") + nil end private def call_outside_block_diagnostics_for( @@ -215,6 +204,112 @@ module AmberLSP::LibraryRulePacks [] of Rules::Diagnostic end + private def crystal_ast_diagnostics_for( + rule : DescribeLibraryRulePack::Rule, + project_state : DetermineProjectRulePackState, + file_path : String, + content : String, + ) : Array(Rules::Diagnostic) + ast = Crystal::Parser.new(content).parse + + case rule.check.operation_name + when "chained_unscoped_in_request_code" + visitor = GrantTenancy::VisitChainableUnscopedModelCalls.new(project_state) + visitor.accept(ast) + diagnostics_for_unscoped_calls(visitor.list_of_chainable_unscoped_calls, rule) + when "chained_unscoped_bulk_write" + visitor = GrantTenancy::VisitChainableUnscopedModelCalls.new(project_state) + visitor.accept(ast) + list_of_bulk_write_calls = visitor.list_of_chainable_unscoped_calls.select(&.has_bulk_write_after?) + diagnostics_for_unscoped_calls(list_of_bulk_write_calls, rule) + when "chained_unscoped_on_tenant_model" + visitor = GrantTenancy::VisitChainableUnscopedModelCalls.new(project_state) + visitor.accept(ast) + list_of_read_calls = visitor.list_of_chainable_unscoped_calls.reject(&.has_bulk_write_after?) + diagnostics_for_unscoped_calls(list_of_read_calls, rule) + when "unscoped_block_in_request_code" + visitor = GrantTenancy::VisitChainableUnscopedModelCalls.new(project_state) + visitor.accept(ast) + diagnostics_for_calls(visitor.list_of_block_unscoped_calls, rule) + when "spawn_inside_tenant_block" + visitor = GrantTenancy::VisitSpawnCallsInsideGrantTenantBlocks.new + visitor.accept(ast) + diagnostics_for_calls(visitor.list_of_spawn_calls_inside_tenant_blocks, rule) + when "tenant_column_without_multitenant" + list_of_findings = project_state.list_of_tenant_column_declarations_for(file_path) + diagnostics_for_tenant_column_findings(list_of_findings, rule) + when "raw_connection_sql_on_tenant_table" + visitor = GrantTenancy::VisitRawConnectionSqlCallSites.new(project_state) + visitor.accept(ast) + list_of_calls = visitor.list_of_raw_connection_sql_call_sites.map(&.call) + diagnostics_for_calls(list_of_calls, rule) + when "tenant_clear_in_app_code" + visitor = GrantTenancy::VisitGrantTenantClearCalls.new + visitor.accept(ast) + diagnostics_for_calls(visitor.list_of_tenant_clear_calls, rule) + when "schema_query_outside_tenant" + visitor = GrantTenancy::VisitGrantSchemaQueriesOutsideTenantBlocks.new(project_state) + visitor.accept(ast) + diagnostics_for_calls(visitor.list_of_schema_queries_outside_tenant_blocks, rule) + else + [] of Rules::Diagnostic + end + rescue Crystal::SyntaxException + [] of Rules::Diagnostic + end + + private def diagnostics_for_unscoped_calls( + list_of_occurrences : Array(GrantTenancy::VisitChainableUnscopedModelCalls::Occurrence), + rule : DescribeLibraryRulePack::Rule, + ) : Array(Rules::Diagnostic) + list_of_occurrences.compact_map { |occurrence| diagnostic_for_source_node(rule, occurrence.call) } + end + + private def diagnostics_for_tenant_column_findings( + list_of_findings : Array(Tuple(String, Crystal::ASTNode)), + rule : DescribeLibraryRulePack::Rule, + ) : Array(Rules::Diagnostic) + list_of_findings.compact_map do |finding| + diagnostic_for_source_node(rule, finding[1], finding[0]) + end + end + + private def diagnostics_for_calls( + list_of_calls : Array(Crystal::Call), + rule : DescribeLibraryRulePack::Rule, + ) : Array(Rules::Diagnostic) + list_of_calls.compact_map { |call| diagnostic_for_source_node(rule, call) } + end + + private def diagnostic_for_source_node( + rule : DescribeLibraryRulePack::Rule, + source_node : Crystal::ASTNode, + source_name : String? = nil, + ) : Rules::Diagnostic? + location = if source_node.is_a?(Crystal::Call) + source_node.name_location || source_node.location + else + source_node.location + end + return nil unless location + + start_character = (location.column_number - 1).to_i32 + display_name = source_name || source_node.as?(Crystal::Call).try(&.name) || "" + end_character = start_character + display_name.size + range = Rules::TextRange.new( + Rules::Position.new((location.line_number - 1).to_i32, start_character), + Rules::Position.new((location.line_number - 1).to_i32, end_character), + ) + diagnostic_message = source_name ? rule.diagnostic_message.gsub("{tenant_column}", source_name) : rule.diagnostic_message + + Rules::Diagnostic.new( + range, + severity_for(rule.severity_name), + rule.rule_id, + diagnostic_message, + ) + end + private def diagnostic_at_start_of_file( rule : DescribeLibraryRulePack::Rule, severity : Rules::Severity, diff --git a/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr b/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr index 7d1a550..3621bc9 100644 --- a/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr +++ b/src/amber_lsp/library_rule_packs/describe_library_rule_pack.cr @@ -32,25 +32,9 @@ module AmberLSP::LibraryRulePacks end end - class Declaration - include YAML::Serializable - - property key_path : String - property expected_value : String - end - class Mode include YAML::Serializable - @[YAML::Field(key: "declared_by")] - property declaration : Declaration - - @[YAML::Field(key: "evidence")] - property list_of_evidence_patterns : Array(String) = [] of String - - @[YAML::Field(key: "tenant_column")] - property tenant_column_name : String? = nil - @[YAML::Field(key: "context")] property guidance_text : String = "" end @@ -114,6 +98,9 @@ module AmberLSP::LibraryRulePacks @[YAML::Field(key: "condition")] property project_condition : String = "mixed_modes" + + @[YAML::Field(key: "operation")] + property operation_name : String = "" end end end diff --git a/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr b/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr index dd7ccc8..0ce69af 100644 --- a/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr +++ b/src/amber_lsp/library_rule_packs/determine_project_rule_pack_state.cr @@ -2,9 +2,12 @@ require "set" module AmberLSP::LibraryRulePacks class DetermineProjectRulePackState - @is_mode_declared_by_name = {} of String => Bool - @has_mode_evidence_by_name = {} of String => Bool + @uses_row_tenancy = false + @uses_schema_tenancy = false @project_source_content_by_path = {} of String => String + @list_of_model_declarations_by_file = {} of String => Array(GrantTenancy::GrantTenantModelDeclaration) + @list_of_model_declarations = [] of GrantTenancy::GrantTenantModelDeclaration + @list_of_row_tenant_column_names = Set(String).new def initialize( @project_context : AmberLSP::ProjectContext, @@ -13,40 +16,76 @@ module AmberLSP::LibraryRulePacks @file_content : String, ) load_project_source_content - determine_mode_declarations - determine_mode_evidence end - def is_mode_declared?(mode_name : String) : Bool - @is_mode_declared_by_name[mode_name]? || false + def mode_detected?(mode_name : String) : Bool + case mode_name + when "row" + @uses_row_tenancy + when "schema" + @uses_schema_tenancy + else + false + end end - def has_evidence_for_mode?(mode_name : String) : Bool - @has_mode_evidence_by_name[mode_name]? || false + def has_any_applicable_mode? : Bool + @rule_pack.modes_by_name.keys.any? { |mode_name| mode_detected?(mode_name) } end - def is_mode_present?(mode_name : String) : Bool - is_mode_declared?(mode_name) || has_evidence_for_mode?(mode_name) + def all_modes_are_present?(list_of_mode_names : Array(String)) : Bool + !list_of_mode_names.empty? && list_of_mode_names.all? { |mode_name| mode_detected?(mode_name) } end - def has_any_applicable_mode? : Bool - @rule_pack.modes_by_name.keys.any? do |mode_name| - is_mode_declared?(mode_name) || has_evidence_for_mode?(mode_name) - end + def rule_mode_detected?(list_of_mode_names : Array(String)) : Bool + list_of_mode_names.any? { |mode_name| mode_detected?(mode_name) } end - def all_modes_are_present?(list_of_mode_names : Array(String)) : Bool - !list_of_mode_names.empty? && list_of_mode_names.all? { |mode_name| is_mode_present?(mode_name) } + def has_row_tenant_model?(model_reference_name : String) : Bool + matching_model_declarations(model_reference_name).any?(&.has_multitenant_macro?) end - def has_undeclared_mode_evidence?(list_of_mode_names : Array(String)) : Bool - list_of_mode_names.any? do |mode_name| - has_evidence_for_mode?(mode_name) && !is_mode_declared?(mode_name) + def has_schema_excluded_model?(model_reference_name : String) : Bool + matching_model_declarations(model_reference_name).any?(&.schema_tenant_excluded?) + end + + def has_non_excluded_schema_model?(model_reference_name : String) : Bool + matching_model_declarations(model_reference_name).any? do |model_declaration| + model_declaration_is_grant_model?(model_declaration) && + !has_schema_excluded_model?(model_declaration.qualified_model_name) end end - def is_rule_mode_declared?(list_of_mode_names : Array(String)) : Bool - list_of_mode_names.any? { |mode_name| is_mode_declared?(mode_name) } + def list_of_row_tenant_table_names : Array(String) + @list_of_model_declarations.compact_map do |model_declaration| + next unless model_declaration.has_multitenant_macro? + + model_declaration.source_table_name + end.uniq + end + + def list_of_tenant_column_declarations_for(file_path : String) : Array(Tuple(String, Crystal::ASTNode)) + absolute_file_path = File.expand_path(file_path) + list_of_current_file_models = @list_of_model_declarations_by_file[absolute_file_path]? + return [] of Tuple(String, Crystal::ASTNode) unless list_of_current_file_models + return [] of Tuple(String, Crystal::ASTNode) if @list_of_row_tenant_column_names.empty? + + list_of_findings = [] of Tuple(String, Crystal::ASTNode) + + list_of_current_file_models.each do |model_declaration| + next unless model_declaration_is_grant_model?(model_declaration) + next if has_row_tenant_model?(model_declaration.qualified_model_name) + + model_declaration.list_of_column_declarations.each do |column_declaration| + tenant_column_name = column_declaration[0] + next unless @list_of_row_tenant_column_names.includes?(tenant_column_name) + next if model_table_is_referenced_by_column?(model_declaration, tenant_column_name) + + list_of_findings << column_declaration + end + end + + list_of_findings end def list_of_scoped_model_names_for( @@ -68,61 +107,110 @@ module AmberLSP::LibraryRulePacks end private def load_project_source_content : Nil - project_root = File.expand_path(@project_context.root_path) + project_source_file_paths.each do |file_path| + content = content_for(file_path) + next unless content - Dir.glob(File.join(project_root, "**", "*.cr")).each do |file_path| - next unless project_file_is_application_source?(file_path, project_root) - - content = if File.expand_path(file_path) == File.expand_path(@file_path) - @file_content - else - File.read(file_path) - end - @project_source_content_by_path[File.expand_path(file_path)] = content - rescue - next + absolute_file_path = File.expand_path(file_path) + @project_source_content_by_path[absolute_file_path] = content + collect_project_declarations_from(absolute_file_path, content) end current_file_path = File.expand_path(@file_path) - if project_file_is_application_source?(current_file_path, project_root) + if project_file_is_application_source?(current_file_path) && + !@project_source_content_by_path.has_key?(current_file_path) @project_source_content_by_path[current_file_path] = @file_content + collect_project_declarations_from(current_file_path, @file_content) + end + end + + private def project_source_file_paths : Array(String) + project_root = File.expand_path(@project_context.root_path) + list_of_source_file_paths = Dir.glob(File.join(project_root, "src", "**", "*.cr")) + list_of_source_file_paths.concat(Dir.glob(File.join(project_root, "config", "**", "*.cr"))) + list_of_source_file_paths.uniq.sort + end + + private def content_for(file_path : String) : String? + if File.expand_path(file_path) == File.expand_path(@file_path) + return @file_content end + + File.read(file_path) + rescue IO::Error + nil end - private def project_file_is_application_source?(file_path : String, project_root : String) : Bool + private def collect_project_declarations_from(file_path : String, content : String) : Nil + collector = GrantTenancy::CollectProjectGrantTenancyDeclarations.for_source(content) + @uses_row_tenancy ||= collector.uses_row_tenancy? + @uses_schema_tenancy ||= collector.uses_schema_tenancy? + @list_of_model_declarations_by_file[file_path] = collector.list_of_model_declarations + @list_of_model_declarations.concat(collector.list_of_model_declarations) + + collector.list_of_model_declarations.each do |model_declaration| + tenant_column_name = model_declaration.tenant_column_name + next unless model_declaration.has_multitenant_macro? && tenant_column_name + + @list_of_row_tenant_column_names.add(tenant_column_name) + end + end + + private def project_file_is_application_source?(file_path : String) : Bool + project_root = File.expand_path(@project_context.root_path) prefix = project_root.ends_with?(File::SEPARATOR) ? project_root : "#{project_root}#{File::SEPARATOR}" return false unless file_path.starts_with?(prefix) relative_path = file_path[prefix.size..] - return false if relative_path.starts_with?("lib/") - return false if relative_path.starts_with?(".git/") - return false if relative_path.starts_with?("tmp/") - return false if relative_path.starts_with?("vendor/") - - true + (relative_path.starts_with?("src/") || relative_path.starts_with?("config/")) && + file_path.ends_with?(".cr") end - private def determine_mode_declarations : Nil - @rule_pack.modes_by_name.each do |mode_name, mode| - @is_mode_declared_by_name[mode_name] = @project_context.has_shard_declaration?( - mode.declaration.key_path, - mode.declaration.expected_value, - ) + private def matching_model_declarations(model_reference_name : String) : Array(GrantTenancy::GrantTenantModelDeclaration) + list_of_exact_matches = @list_of_model_declarations.select do |model_declaration| + model_declaration.qualified_model_name == model_reference_name + end + return list_of_exact_matches unless list_of_exact_matches.empty? + return [] of GrantTenancy::GrantTenantModelDeclaration if model_reference_name.includes?("::") + + list_of_simple_name_matches = @list_of_model_declarations.select do |model_declaration| + model_declaration.class_name == model_reference_name end + unique_qualified_names = list_of_simple_name_matches.map(&.qualified_model_name).uniq + return [] of GrantTenancy::GrantTenantModelDeclaration unless unique_qualified_names.size == 1 + + list_of_simple_name_matches end - private def determine_mode_evidence : Nil - @rule_pack.modes_by_name.each do |mode_name, mode| - evidence_found = mode.list_of_evidence_patterns.any? do |pattern| - regex = Regex.new(pattern) - @project_source_content_by_path.values.any? do |content| - content.each_line.any? { |line| regex.matches?(line) } - end - end - @has_mode_evidence_by_name[mode_name] = evidence_found + private def model_declaration_is_grant_model?( + model_declaration : GrantTenancy::GrantTenantModelDeclaration, + list_of_visited_model_names : Set(String) = Set(String).new, + ) : Bool + return true if model_declaration.grant_model? + return false if list_of_visited_model_names.includes?(model_declaration.qualified_model_name) + + list_of_visited_model_names.add(model_declaration.qualified_model_name) + superclass_reference_name = model_declaration.superclass_reference_name + return false unless superclass_reference_name + return true if superclass_reference_name == "Grant::Base" + + matching_model_declarations(superclass_reference_name).any? do |parent_model_declaration| + model_declaration_is_grant_model?(parent_model_declaration, list_of_visited_model_names) end - rescue ArgumentError - @rule_pack.modes_by_name.each_key { |mode_name| @has_mode_evidence_by_name[mode_name] = false } + end + + private def model_table_is_referenced_by_column?( + model_declaration : GrantTenancy::GrantTenantModelDeclaration, + tenant_column_name : String, + ) : Bool + return false unless tenant_column_name.ends_with?("_id") + + referenced_table_base_name = tenant_column_name[0, tenant_column_name.size - 3] + list_of_referenced_table_names = [referenced_table_base_name, "#{referenced_table_base_name}s"] + normalized_table_name = model_declaration.source_table_name.split('.').last + return false unless normalized_table_name + + list_of_referenced_table_names.includes?(normalized_table_name.downcase) end private def project_relative_path(file_path : String) : String diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations.cr new file mode 100644 index 0000000..eb2acb4 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/collect_project_grant_tenancy_declarations.cr @@ -0,0 +1,157 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class CollectProjectGrantTenancyDeclarations < Crystal::Visitor + getter list_of_model_declarations : Array(GrantTenantModelDeclaration) + getter? uses_row_tenancy : Bool + getter? uses_schema_tenancy : Bool + + @list_of_model_stack = [] of GrantTenantModelDeclaration + @list_of_namespace_segments = [] of String + @list_of_namespace_stack_sizes = [] of Int32 + @method_depth = 0 + @macro_depth = 0 + @next_class_table_name : String? + + def initialize + @list_of_model_declarations = [] of GrantTenantModelDeclaration + @uses_row_tenancy = false + @uses_schema_tenancy = false + end + + def self.for_source(content : String) : CollectProjectGrantTenancyDeclarations + collector = new + collector.accept(Crystal::Parser.new(content).parse) + collector + rescue Crystal::SyntaxException + new + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::ModuleDef) : Bool + @list_of_namespace_stack_sizes << @list_of_namespace_segments.size + @list_of_namespace_segments.concat(node.name.names) + true + end + + def end_visit(node : Crystal::ModuleDef) : Nil + restore_namespace_stack + end + + def visit(node : Crystal::ClassDef) : Bool + class_name = node.name.names.last + qualified_model_name = if node.name.global? + node.name.names.join("::") + else + (@list_of_namespace_segments + node.name.names).join("::") + end + superclass_reference_name = SourceNode.model_name_for_receiver(node.superclass) + model_declaration = GrantTenantModelDeclaration.new( + qualified_model_name, + class_name, + superclass_reference_name, + ) + if table_name = @next_class_table_name + model_declaration.set_source_table_name(table_name) + end + @next_class_table_name = nil + @list_of_model_stack << model_declaration + true + end + + def end_visit(node : Crystal::ClassDef) : Nil + model_declaration = @list_of_model_stack.pop? + @list_of_model_declarations << model_declaration if model_declaration + end + + def visit(node : Crystal::Def) : Bool + @method_depth += 1 + true + end + + def end_visit(node : Crystal::Def) : Nil + @method_depth -= 1 + end + + def visit(node : Crystal::Macro) : Bool + @macro_depth += 1 + true + end + + def end_visit(node : Crystal::Macro) : Nil + @macro_depth -= 1 + end + + def visit(node : Crystal::Annotation) : Bool + if node.path.names == ["Grant", "Table"] + annotation_name = node.named_args.try(&.find { |argument| argument.name == "name" }) + @next_class_table_name = SourceNode.literal_name(annotation_name.try(&.value)) + end + true + end + + def visit(node : Crystal::Call) : Bool + if SourceNode.full_call_name(node) == "Grant::SchemaTenant.with" + @uses_schema_tenancy = true + end + + return true unless current_class_body? + + model_declaration = @list_of_model_stack.last + case node.name + when "multitenant" + return true unless node.obj.nil? + + model_declaration.capture_multitenant_column(SourceNode.literal_name(node.args.first?)) + @uses_row_tenancy = true + when "schema_tenant_excluded" + return true unless node.obj.nil? + + model_declaration.mark_schema_tenant_excluded + @uses_schema_tenancy = true + when "column" + return true unless node.obj.nil? + + column_declaration = column_declaration_from(node.args.first?) + model_declaration.add_column_declaration(*column_declaration) if column_declaration + when "table" + return true unless node.obj.nil? + + table_name = SourceNode.literal_name(node.args.first?) + model_declaration.set_source_table_name(table_name) if table_name + end + + true + end + + private def current_class_body? : Bool + !@list_of_model_stack.empty? && @method_depth == 0 && @macro_depth == 0 + end + + private def column_declaration_from(node : Crystal::ASTNode?) : Tuple(String, Crystal::ASTNode)? + case node + when Crystal::TypeDeclaration + variable = node.var + return {variable.name, variable} if variable.is_a?(Crystal::Var) + when Crystal::SymbolLiteral + return {node.value, node} + when Crystal::Var + return {node.name, node} + when Crystal::Path + return {node.names.last, node} unless node.names.empty? + end + + nil + end + + private def restore_namespace_stack : Nil + previous_size = @list_of_namespace_stack_sizes.pop? + return unless previous_size + + @list_of_namespace_segments = @list_of_namespace_segments[0, previous_size] + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration.cr new file mode 100644 index 0000000..4ae14ce --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/grant_tenant_model_declaration.cr @@ -0,0 +1,50 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class GrantTenantModelDeclaration + getter qualified_model_name : String + getter class_name : String + getter superclass_reference_name : String? + getter source_table_name : String + getter tenant_column_name : String? + getter list_of_column_declarations : Array(Tuple(String, Crystal::ASTNode)) + getter? has_multitenant_macro : Bool + getter? schema_tenant_excluded : Bool + + def initialize( + @qualified_model_name : String, + @class_name : String, + @superclass_reference_name : String?, + ) + @source_table_name = SourceNode.default_table_name(@class_name) + @tenant_column_name = nil + @list_of_column_declarations = [] of Tuple(String, Crystal::ASTNode) + @has_multitenant_macro = false + @schema_tenant_excluded = false + end + + def capture_multitenant_column(column_name : String?) : Nil + @has_multitenant_macro = true + @tenant_column_name = column_name + end + + def mark_schema_tenant_excluded : Nil + @schema_tenant_excluded = true + end + + def set_source_table_name(table_name : String) : Nil + @source_table_name = table_name + end + + def add_column_declaration(column_name : String, source_node : Crystal::ASTNode) : Nil + @list_of_column_declarations << {column_name, source_node} + end + + def grant_model? : Bool + has_multitenant_macro? || schema_tenant_excluded? || + !@list_of_column_declarations.empty? || + @superclass_reference_name == "Grant::Base" || + @source_table_name != SourceNode.default_table_name(@class_name) + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/source_node.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/source_node.cr new file mode 100644 index 0000000..26a0775 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/source_node.cr @@ -0,0 +1,79 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy::SourceNode + def self.full_call_name(call : Crystal::Call) : String + receiver_name = receiver_path_name(call.obj) + return call.name unless receiver_name + + "#{receiver_name}.#{call.name}" + end + + def self.model_name_for_receiver(node : Crystal::ASTNode?) : String? + case node + when Crystal::Path + node.names.join("::") + when Crystal::Call + model_name_for_receiver(node.obj) + else + nil + end + end + + def self.literal_name(node : Crystal::ASTNode?) : String? + case node + when Crystal::SymbolLiteral + node.value + when Crystal::StringLiteral + node.value + when Crystal::Path + node.names.last? + when Crystal::Var + node.name + else + nil + end + end + + def self.default_table_name(class_name : String) : String + characters = class_name.chars + + String.build do |table_name| + characters.each_with_index do |character, character_index| + previous_character = character_index > 0 ? characters[character_index - 1] : nil + next_character = characters[character_index + 1]? + starts_new_word = character_is_uppercase?(character) && previous_character && + (character_is_lowercase_or_digit?(previous_character) || + (character_is_uppercase?(previous_character) && next_character && character_is_lowercase?(next_character))) + + table_name << '_' if starts_new_word && !table_name.empty? + table_name << character.downcase + end + end + end + + private def self.receiver_path_name(node : Crystal::ASTNode?) : String? + case node + when Crystal::Path + node.names.join("::") + when Crystal::Call + receiver_name = receiver_path_name(node.obj) + return node.name unless receiver_name + + "#{receiver_name}.#{node.name}" + else + nil + end + end + + private def self.character_is_uppercase?(character : Char) : Bool + character >= 'A' && character <= 'Z' + end + + private def self.character_is_lowercase?(character : Char) : Bool + character >= 'a' && character <= 'z' + end + + private def self.character_is_lowercase_or_digit?(character : Char) : Bool + character_is_lowercase?(character) || (character >= '0' && character <= '9') + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr new file mode 100644 index 0000000..d517072 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr @@ -0,0 +1,73 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class VisitChainableUnscopedModelCalls < Crystal::Visitor + class Occurrence + getter call : Crystal::Call + getter model_reference_name : String + getter? has_bulk_write_after : Bool + + def initialize(@call : Crystal::Call, @model_reference_name : String, @has_bulk_write_after : Bool) + end + end + + getter list_of_chainable_unscoped_calls : Array(Occurrence) + getter list_of_block_unscoped_calls : Array(Crystal::Call) + + @list_of_call_stack = [] of Crystal::Call + + def initialize(@project_state : AmberLSP::LibraryRulePacks::DetermineProjectRulePackState) + @list_of_chainable_unscoped_calls = [] of Occurrence + @list_of_block_unscoped_calls = [] of Crystal::Call + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::Call) : Bool + if node.name == "unscoped" + if model_reference_name = SourceNode.model_name_for_receiver(node.obj) + if @project_state.has_row_tenant_model?(model_reference_name) + if node.block + @list_of_block_unscoped_calls << node + else + @list_of_chainable_unscoped_calls << Occurrence.new( + node, + model_reference_name, + has_bulk_write_after?(node, model_reference_name), + ) + end + end + end + end + + @list_of_call_stack << node + true + end + + def end_visit(node : Crystal::Call) : Nil + @list_of_call_stack.pop + end + + private def has_bulk_write_after?(unscoped_call : Crystal::Call, model_reference_name : String) : Bool + @list_of_call_stack.any? do |parent_call| + next false unless {"update_all", "delete_all"}.includes?(parent_call.name) + + parent_model_reference_name = SourceNode.model_name_for_receiver(parent_call.obj) + next false unless parent_model_reference_name == model_reference_name + + receiver_chain_has_chainable_unscoped_call?(parent_call.obj) + end + end + + private def receiver_chain_has_chainable_unscoped_call?(node : Crystal::ASTNode?) : Bool + case node + when Crystal::Call + (node.name == "unscoped" && node.block.nil?) || receiver_chain_has_chainable_unscoped_call?(node.obj) + else + false + end + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks.cr new file mode 100644 index 0000000..ca01952 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_schema_queries_outside_tenant_blocks.cr @@ -0,0 +1,77 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class VisitGrantSchemaQueriesOutsideTenantBlocks < Crystal::Visitor + LIST_OF_QUERY_METHOD_NAMES = { + "all", "where", "find", "find!", "find_by", "find_by!", "first", "first?", + "last", "last?", "count", "exists?", "pluck", "sum", "average", "minimum", "maximum", + } + + getter list_of_schema_queries_outside_tenant_blocks : Array(Crystal::Call) + + @schema_tenant_block_depth = 0 + @list_of_call_stack = [] of Crystal::Call + @list_of_block_scope_changes = [] of Bool + @list_of_method_scope_snapshots = [] of Int32 + + def initialize(@project_state : AmberLSP::LibraryRulePacks::DetermineProjectRulePackState) + @list_of_schema_queries_outside_tenant_blocks = [] of Crystal::Call + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::Def) : Bool + @list_of_method_scope_snapshots << @schema_tenant_block_depth + @schema_tenant_block_depth = 0 + true + end + + def end_visit(node : Crystal::Def) : Nil + previous_scope_depth = @list_of_method_scope_snapshots.pop? + @schema_tenant_block_depth = previous_scope_depth if previous_scope_depth + end + + def visit(node : Crystal::Block) : Bool + enters_schema_tenant_block = node.call.try do |block_call| + SourceNode.full_call_name(block_call) == "Grant::SchemaTenant.with" + end || false + @schema_tenant_block_depth += 1 if enters_schema_tenant_block + @list_of_block_scope_changes << enters_schema_tenant_block + true + end + + def end_visit(node : Crystal::Block) : Nil + enters_schema_tenant_block = @list_of_block_scope_changes.pop? + @schema_tenant_block_depth -= 1 if enters_schema_tenant_block + end + + def visit(node : Crystal::Call) : Bool + model_reference_name = SourceNode.model_name_for_receiver(node.obj) + if model_reference_name && + LIST_OF_QUERY_METHOD_NAMES.includes?(node.name) && + @schema_tenant_block_depth == 0 && + @project_state.has_non_excluded_schema_model?(model_reference_name) && + !nested_inside_schema_model_query?(model_reference_name) + @list_of_schema_queries_outside_tenant_blocks << node + end + + @list_of_call_stack << node + true + end + + def end_visit(node : Crystal::Call) : Nil + @list_of_call_stack.pop + end + + private def nested_inside_schema_model_query?(model_reference_name : String) : Bool + @list_of_call_stack.any? do |parent_call| + next false unless LIST_OF_QUERY_METHOD_NAMES.includes?(parent_call.name) + + parent_model_reference_name = SourceNode.model_name_for_receiver(parent_call.obj) + parent_model_reference_name == model_reference_name + end + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls.cr new file mode 100644 index 0000000..b6b813f --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_grant_tenant_clear_calls.cr @@ -0,0 +1,22 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class VisitGrantTenantClearCalls < Crystal::Visitor + getter list_of_tenant_clear_calls : Array(Crystal::Call) + + def initialize + @list_of_tenant_clear_calls = [] of Crystal::Call + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::Call) : Bool + if SourceNode.full_call_name(node) == "Grant::Tenant.clear" + @list_of_tenant_clear_calls << node + end + true + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr new file mode 100644 index 0000000..e4548b4 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr @@ -0,0 +1,123 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class VisitRawConnectionSqlCallSites < Crystal::Visitor + class Occurrence + getter call : Crystal::Call + getter sql_literal : Crystal::StringLiteral + + def initialize(@call : Crystal::Call, @sql_literal : Crystal::StringLiteral) + end + end + + getter list_of_raw_connection_sql_call_sites : Array(Occurrence) + + @list_of_active_database_handle_names = [] of String + @list_of_block_handle_scope_sizes = [] of Int32 + + def initialize(@project_state : AmberLSP::LibraryRulePacks::DetermineProjectRulePackState) + @list_of_raw_connection_sql_call_sites = [] of Occurrence + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::Block) : Bool + list_of_new_handle_names = [] of String + if adapter_open_call?(node.call) + node.args.each { |argument| list_of_new_handle_names << argument.name } + end + + @list_of_active_database_handle_names.concat(list_of_new_handle_names) + @list_of_block_handle_scope_sizes << list_of_new_handle_names.size + true + end + + def end_visit(node : Crystal::Block) : Nil + added_handle_count = @list_of_block_handle_scope_sizes.pop? + return unless added_handle_count && added_handle_count > 0 + + @list_of_active_database_handle_names = @list_of_active_database_handle_names[0, + @list_of_active_database_handle_names.size - added_handle_count] + end + + def visit(node : Crystal::Call) : Bool + return true unless raw_connection_sql_call?(node) + + sql_literal = node.args.first?.as?(Crystal::StringLiteral) + return true unless sql_literal + return true unless sql_literal_names_tenant_table?(sql_literal.value) + + @list_of_raw_connection_sql_call_sites << Occurrence.new(node, sql_literal) + true + end + + private def raw_connection_sql_call?(call : Crystal::Call) : Bool + if {"exec", "query", "scalar"}.includes?(call.name) + return database_handle_call?(call.obj) + end + + if {"execute", "exec_query", "select_all", "select_one", "select_value", + "select_values", "select_rows", "with_result_set"}.includes?(call.name) + return grant_connection_facade_call?(call) + end + + false + end + + private def database_handle_call?(node : Crystal::ASTNode?) : Bool + if node.is_a?(Crystal::Var) + return @list_of_active_database_handle_names.includes?(node.name) + end + + false + end + + private def grant_connection_facade_call?(call : Crystal::Call) : Bool + receiver_call = call.obj.as?(Crystal::Call) + return false unless receiver_call + + receiver_call.name == "connection" && + SourceNode.full_call_name(receiver_call) == "Grant.connection" + end + + private def adapter_open_call?(call : Crystal::Call?) : Bool + return false unless call + return false unless call.name == "open" + + receiver_has_adapter_call?(call.obj) || receiver_has_connection_registry_path?(call.obj) + end + + private def receiver_has_adapter_call?(node : Crystal::ASTNode?) : Bool + case node + when Crystal::Call + node.name == "adapter" || receiver_has_adapter_call?(node.obj) + when Crystal::Var + node.name == "adapter" + else + false + end + end + + private def receiver_has_connection_registry_path?(node : Crystal::ASTNode?) : Bool + case node + when Crystal::Path + node.names == ["Grant", "Connections"] + when Crystal::Call + receiver_has_connection_registry_path?(node.obj) || + node.args.any? { |argument| receiver_has_connection_registry_path?(argument) } + else + false + end + end + + private def sql_literal_names_tenant_table?(sql_literal : String) : Bool + list_of_sql_tokens = sql_literal.downcase.split(/[^a-z0-9_]+/) + @project_state.list_of_row_tenant_table_names.any? do |table_name| + normalized_table_name = table_name.split('.').last + normalized_table_name && list_of_sql_tokens.includes?(normalized_table_name.downcase) + end + end + end +end diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks.cr new file mode 100644 index 0000000..1d942f2 --- /dev/null +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_spawn_calls_inside_grant_tenant_blocks.cr @@ -0,0 +1,62 @@ +require "compiler/crystal/syntax" + +module AmberLSP::LibraryRulePacks::GrantTenancy + class VisitSpawnCallsInsideGrantTenantBlocks < Crystal::Visitor + getter list_of_spawn_calls_inside_tenant_blocks : Array(Crystal::Call) + + @tenant_block_depth = 0 + @list_of_block_scope_changes = [] of Bool + @list_of_method_scope_snapshots = [] of Int32 + @macro_depth = 0 + + def initialize + @list_of_spawn_calls_inside_tenant_blocks = [] of Crystal::Call + end + + def visit(node : Crystal::ASTNode) : Bool + true + end + + def visit(node : Crystal::Def) : Bool + @list_of_method_scope_snapshots << @tenant_block_depth + @tenant_block_depth = 0 + true + end + + def end_visit(node : Crystal::Def) : Nil + previous_scope_depth = @list_of_method_scope_snapshots.pop? + @tenant_block_depth = previous_scope_depth if previous_scope_depth + end + + def visit(node : Crystal::Macro) : Bool + @macro_depth += 1 + true + end + + def end_visit(node : Crystal::Macro) : Nil + @macro_depth -= 1 + end + + def visit(node : Crystal::Block) : Bool + call_is_tenant_scope = node.call.try do |block_call| + {"Grant::Tenant.with", "Grant::SchemaTenant.with"}.includes?(SourceNode.full_call_name(block_call)) + end || false + enters_tenant_scope = call_is_tenant_scope && @macro_depth == 0 + @tenant_block_depth += 1 if enters_tenant_scope + @list_of_block_scope_changes << enters_tenant_scope + true + end + + def end_visit(node : Crystal::Block) : Nil + scope_change = @list_of_block_scope_changes.pop? + @tenant_block_depth -= 1 if scope_change + end + + def visit(node : Crystal::Call) : Bool + if node.name == "spawn" && @tenant_block_depth > 0 && @macro_depth == 0 + @list_of_spawn_calls_inside_tenant_blocks << node + end + true + end + end +end diff --git a/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr index 1becfd1..3da1612 100644 --- a/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr +++ b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr @@ -1,3 +1,5 @@ +require "log" + module AmberLSP::LibraryRulePacks class LoadRulePacksForProject def initialize(@project_context : AmberLSP::ProjectContext) @@ -24,32 +26,28 @@ module AmberLSP::LibraryRulePacks private def dependency_pack_paths : Array(String) list_of_library_paths = Dir.glob(File.join(@project_context.root_path, "lib", "*")).sort list_of_library_paths.flat_map do |library_path| - Dir.glob(File.join(library_path, ".claude", "rules", "*.yml")) + Dir.glob(File.join(library_path, ".amber-lsp", "packs", "*.yml")) end.sort end private def project_pack_paths : Array(String) - Dir.glob(File.join(@project_context.root_path, ".claude", "rules", "*.yml")).sort + Dir.glob(File.join(@project_context.root_path, ".amber-lsp", "packs", "*.yml")).sort end private def load_rule_pack(pack_path : String) : DescribeLibraryRulePack? rule_pack = DescribeLibraryRulePack.from_yaml(File.read(pack_path)) return rule_pack if rule_pack_is_valid?(rule_pack) - STDERR.puts "WARNING: Ignoring invalid amber-lsp rule pack at #{pack_path}." + Log.warn { "Ignoring invalid amber-lsp rule pack at #{pack_path}." } nil rescue ex - STDERR.puts "WARNING: Could not load amber-lsp rule pack at #{pack_path}: #{ex.message}" + Log.warn { "Could not load amber-lsp rule pack at #{pack_path}: #{ex.message}" } nil end private def rule_pack_is_valid?(rule_pack : DescribeLibraryRulePack) : Bool return false unless rule_pack.is_valid? - rule_pack.modes_by_name.each_value do |mode| - mode.list_of_evidence_patterns.each { |pattern| Regex.new(pattern) } - end - rule_pack.list_of_rules.all? do |rule| next false unless {"error", "warning", "info", "hint"}.includes?(rule.severity_name.downcase) @@ -59,23 +57,26 @@ module AmberLSP::LibraryRulePacks true when "file_requires" !rule.check.required_regex_pattern.empty? && - (rule.check.trigger_regex_pattern.empty? || begin + !rule.check.trigger_regex_pattern.empty? && + begin Regex.new(rule.check.trigger_regex_pattern) + Regex.new(rule.check.required_regex_pattern) true - end) && begin - Regex.new(rule.check.required_regex_pattern) - true - end + end when "call_outside_block" !rule.check.list_of_source_globs.empty? && !rule.check.list_of_query_method_names.empty? && (!rule.check.required_block_call_name.empty? || !rule.check.escape_block_call_name.empty?) + when "crystal_ast" + {"chained_unscoped_in_request_code", "chained_unscoped_bulk_write", + "chained_unscoped_on_tenant_model", "unscoped_block_in_request_code", + "spawn_inside_tenant_block", "tenant_column_without_multitenant", + "raw_connection_sql_on_tenant_table", "tenant_clear_in_app_code", + "schema_query_outside_tenant"}.includes?(rule.check.operation_name) when "project_conflict" case rule.check.project_condition when "mixed_modes" rule.list_of_mode_names.size > 1 - when "evidence_without_declaration" - rule.list_of_mode_names.size == 1 else false end diff --git a/src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr b/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr similarity index 68% rename from src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr rename to src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr index 1d17509..88bd929 100644 --- a/src/amber_lsp/library_rule_packs/print_declared_rule_pack_contexts.cr +++ b/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr @@ -1,5 +1,5 @@ module AmberLSP::LibraryRulePacks - class PrintDeclaredRulePackContexts + class PrintDetectedRulePackContexts def initialize(@list_of_arguments : Array(String)) end @@ -14,8 +14,7 @@ module AmberLSP::LibraryRulePacks project_state = DetermineProjectRulePackState.new(project_context, rule_pack, "", "") next unless project_state.has_any_applicable_mode? - append_declared_mode_contexts(list_of_output_lines, rule_pack, project_state) - append_undeclared_feature_warning(list_of_output_lines, rule_pack, project_state) + append_detected_mode_contexts(list_of_output_lines, rule_pack, project_state) end STDOUT.puts(list_of_output_lines.join('\n')) unless list_of_output_lines.empty? @@ -57,34 +56,18 @@ module AmberLSP::LibraryRulePacks end end - private def append_declared_mode_contexts( + private def append_detected_mode_contexts( list_of_output_lines : Array(String), rule_pack : DescribeLibraryRulePack, project_state : DetermineProjectRulePackState, ) : Nil rule_pack.modes_by_name.each do |mode_name, mode| - next unless project_state.is_mode_declared?(mode_name) + next unless project_state.mode_detected?(mode_name) next if mode.guidance_text.strip.empty? list_of_output_lines << "#{rule_pack.pack_id} (#{mode_name})" list_of_output_lines.concat(mode.guidance_text.lines.map(&.rstrip)) end end - - private def append_undeclared_feature_warning( - list_of_output_lines : Array(String), - rule_pack : DescribeLibraryRulePack, - project_state : DetermineProjectRulePackState, - ) : Nil - list_of_undeclared_mode_names = rule_pack.modes_by_name.keys.select do |mode_name| - project_state.has_evidence_for_mode?(mode_name) && !project_state.is_mode_declared?(mode_name) - end - return if list_of_undeclared_mode_names.empty? - - list_of_key_paths = list_of_undeclared_mode_names.compact_map do |mode_name| - rule_pack.modes_by_name[mode_name]?.try(&.declaration.key_path) - end - list_of_output_lines << "warning: #{rule_pack.pack_id} feature is used but shard.yml does not declare #{list_of_key_paths.join(", ")}." - end end end diff --git a/src/amber_lsp/project_context.cr b/src/amber_lsp/project_context.cr index 3064f9e..4b550ee 100644 --- a/src/amber_lsp/project_context.cr +++ b/src/amber_lsp/project_context.cr @@ -6,13 +6,10 @@ module AmberLSP getter? amber_project : Bool getter shard_name : String? - @shard_configuration : YAML::Any? - def initialize( @root_path : String, @amber_project : Bool = false, @shard_name : String? = nil, - @shard_configuration : YAML::Any? = nil, ) end @@ -31,28 +28,11 @@ module AmberLSP root_path, amber_project: is_amber, shard_name: shard_name, - shard_configuration: shard_configuration, ) rescue YAML::ParseException ProjectContext.new(root_path, amber_project: false) end - def has_shard_declaration?(key_path : String, expected_value : String) : Bool - shard_configuration = @shard_configuration - return false unless shard_configuration - - current_value = shard_configuration - key_path.split('.').each do |key| - next_value = current_value[key]? - return false unless next_value - current_value = next_value - end - - current_value.as_s? == expected_value - rescue TypeCastError - false - end - private def self.has_amber_dependency?(shard_configuration : YAML::Any) : Bool dependencies = shard_configuration["dependencies"]? return false unless dependencies From 95f04cfb837db63aca338e7790fc01b8ffad991f Mon Sep 17 00:00:00 2001 From: crimson-knight Date: Wed, 23 Sep 2026 21:07:53 -0400 Subject: [PATCH 3/3] fix(amber-lsp): narrow rule pack rescues --- spec/amber_lsp/rule_packs_spec.cr | 51 +++++++++++++++++++ .../visit_chainable_unscoped_model_calls.cr | 2 +- .../visit_raw_connection_sql_call_sites.cr | 2 +- .../load_rule_packs_for_project.cr | 12 +++-- .../print_detected_rule_pack_contexts.cr | 11 +++- 5 files changed, 71 insertions(+), 7 deletions(-) diff --git a/spec/amber_lsp/rule_packs_spec.cr b/spec/amber_lsp/rule_packs_spec.cr index 51ebe49..6d4244e 100644 --- a/spec/amber_lsp/rule_packs_spec.cr +++ b/spec/amber_lsp/rule_packs_spec.cr @@ -64,6 +64,15 @@ def tenancy_diagnostic_codes(diagnostics : Array(AmberLSP::Rules::Diagnostic)) : diagnostics.map(&.code) end +class UnexpectedRulePackReadError < Exception +end + +class RaiseUnexpectedRulePackReadError < AmberLSP::LibraryRulePacks::LoadRulePacksForProject + protected def read_rule_pack_contents(pack_path : String) : String + raise UnexpectedRulePackReadError.new("unexpected read failure") + end +end + describe "AmberLSP Grant tenancy rule pack v2" do before_each do AmberLSP::Rules::RuleRegistry.clear @@ -82,6 +91,48 @@ describe "AmberLSP Grant tenancy rule pack v2" do end end + it "skips malformed and unreadable pack files" do + with_tempdir do |root| + pack_directory = File.join(root, "lib", "grant", ".amber-lsp", "packs") + Dir.mkdir_p(pack_directory) + File.write(File.join(pack_directory, "malformed.yml"), "pack: [\n") + Dir.mkdir(File.join(pack_directory, "unreadable.yml")) + + project_context = AmberLSP::ProjectContext.new(root) + list_of_rule_packs = AmberLSP::LibraryRulePacks::LoadRulePacksForProject.new(project_context).load_rule_packs + + list_of_rule_packs.should be_empty + end + end + + it "propagates unexpected pack read errors" do + with_tempdir do |root| + pack_path = File.join(root, "lib", "grant", ".amber-lsp", "packs", "tenancy.yml") + Dir.mkdir_p(File.dirname(pack_path)) + File.write(pack_path, "pack: test\n") + + project_context = AmberLSP::ProjectContext.new(root) + + expect_raises(UnexpectedRulePackReadError, "unexpected read failure") do + RaiseUnexpectedRulePackReadError.new(project_context).load_rule_packs + end + end + end + + it "logs the exception class when the context command rejects its arguments" do + log_backend = Log::MemoryBackend.new + Log.setup(:error, log_backend) + + begin + result = AmberLSP::LibraryRulePacks::PrintDetectedRulePackContexts.new(["--unexpected"]).perform + + result.should eq(1) + log_backend.entries.last.message.should contain("ArgumentError") + ensure + Log.setup + end + end + it "gives agents the Grant runtime context for detected modes" do with_tempdir do |root| install_tenancy_fixture_app(root, "row_app") diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr index d517072..26b4dcc 100644 --- a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_chainable_unscoped_model_calls.cr @@ -2,7 +2,7 @@ require "compiler/crystal/syntax" module AmberLSP::LibraryRulePacks::GrantTenancy class VisitChainableUnscopedModelCalls < Crystal::Visitor - class Occurrence + struct Occurrence getter call : Crystal::Call getter model_reference_name : String getter? has_bulk_write_after : Bool diff --git a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr index e4548b4..2a07c9d 100644 --- a/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr +++ b/src/amber_lsp/library_rule_packs/grant_tenancy/visit_raw_connection_sql_call_sites.cr @@ -2,7 +2,7 @@ require "compiler/crystal/syntax" module AmberLSP::LibraryRulePacks::GrantTenancy class VisitRawConnectionSqlCallSites < Crystal::Visitor - class Occurrence + struct Occurrence getter call : Crystal::Call getter sql_literal : Crystal::StringLiteral diff --git a/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr index 3da1612..5fb79f2 100644 --- a/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr +++ b/src/amber_lsp/library_rule_packs/load_rule_packs_for_project.cr @@ -35,16 +35,22 @@ module AmberLSP::LibraryRulePacks end private def load_rule_pack(pack_path : String) : DescribeLibraryRulePack? - rule_pack = DescribeLibraryRulePack.from_yaml(File.read(pack_path)) + rule_pack = DescribeLibraryRulePack.from_yaml(read_rule_pack_contents(pack_path)) return rule_pack if rule_pack_is_valid?(rule_pack) Log.warn { "Ignoring invalid amber-lsp rule pack at #{pack_path}." } nil - rescue ex - Log.warn { "Could not load amber-lsp rule pack at #{pack_path}: #{ex.message}" } + rescue ex : YAML::ParseException | IO::Error + Log.warn(exception: ex) do + "Could not load amber-lsp rule pack at #{pack_path} (#{ex.class}): #{ex.message}" + end nil end + protected def read_rule_pack_contents(pack_path : String) : String + File.read(pack_path) + end + private def rule_pack_is_valid?(rule_pack : DescribeLibraryRulePack) : Bool return false unless rule_pack.is_valid? diff --git a/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr b/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr index 88bd929..f64c0bf 100644 --- a/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr +++ b/src/amber_lsp/library_rule_packs/print_detected_rule_pack_contexts.cr @@ -1,5 +1,10 @@ +require "log" +require "yaml" + module AmberLSP::LibraryRulePacks class PrintDetectedRulePackContexts + Log = ::Log.for(self) + def initialize(@list_of_arguments : Array(String)) end @@ -19,8 +24,10 @@ module AmberLSP::LibraryRulePacks STDOUT.puts(list_of_output_lines.join('\n')) unless list_of_output_lines.empty? 0 - rescue ex - STDERR.puts "amber-lsp context failed: #{ex.message}" + rescue ex : ArgumentError | YAML::ParseException | IO::Error + Log.error(exception: ex) do + "amber-lsp context failed: #{ex.class}: #{ex.message}" + end 1 end