Conversation
Add `rake react_on_rails:scan_licenses` that scans Ruby gems (via Bundler) and JS packages (via pnpm/yarn/npm) for disallowed copyleft licenses (GPL-2.0, GPL-3.0, AGPL-3.0). Multi-licensed packages with a permissive option are reported as warnings, not violations. Also add .cursor/BUGBOT.md configuration for automated PR license scanning via Cursor's BugBot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis pull request adds a comprehensive dependency license scanning system for React on Rails. It includes a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a The core scanning logic in
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["rake react_on_rails:scan_licenses"] --> B["LicenseScanner#scan"]
B --> C["scan_ruby_gems\n(Bundler.definition.resolve)"]
B --> D["scan_js_packages"]
D --> E["find_js_lockfile\n(pnpm/yarn/npm)"]
E -->|"pnpm-lock.yaml"| F["pnpm licenses list --json"]
E -->|"yarn.lock"| G["yarn licenses list --json"]
E -->|"package-lock.json"| H["npx license-checker --json"]
F --> I["parse_pnpm_licenses"]
G --> J["parse_yarn_licenses"]
H --> K["parse_npm_licenses"]
C --> L["check_licenses"]
I --> L
J --> L
K --> L
L --> M{{"disallowed?"}}
M -->|"No"| N["skip"]
M -->|"Yes + permissive also present"| O["@warnings"]
M -->|"Yes, no permissive"| P["@violations"]
O --> Q["Result"]
P --> Q
N --> Q
Q --> R{{"FORMAT=json?"}}
R -->|"Yes"| S["print_json_result"]
R -->|"No"| T["print_text_result\n⚠️ Rainbow fallback bug here"]
Q --> U{{"violations.any?"}}
U -->|"Yes"| V["exit(1)"]
U -->|"No"| W["exit(0)"]
Last reviewed commit: 97049c7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| begin | ||
| require "rainbow" | ||
| rescue LoadError | ||
| # Fallback if Rainbow is not available | ||
| module Kernel | ||
| def Rainbow(text) # rubocop:disable Naming/MethodName | ||
| text | ||
| end | ||
| end |
There was a problem hiding this comment.
Rainbow fallback crashes on chained method calls
When rainbow is not available, this fallback returns a plain String from Rainbow(text). However, the code immediately chains Rainbow-specific methods on the result — for example Rainbow("\nReact on Rails License Scan").bold (line 54), Rainbow("=" * 40).cyan (line 55), Rainbow("VIOLATIONS…").red.bold (line 64), etc. Plain String in Ruby has no .bold, .red, .cyan, or .yellow methods, so any run without the Rainbow gem will immediately raise NoMethodError.
Compare with doctor.rake, which uses a SimpleColorWrapper class that absorbs all chained method calls via method_missing and returns self, then produces the original text via to_s. The same pattern should be used here:
begin
require "rainbow"
rescue LoadError
class Rainbow
def self.method_missing(_method, text)
SimpleColorWrapper.new(text)
end
def self.respond_to_missing?(_method, _include_private = false)
true
end
end
class SimpleColorWrapper
def initialize(text)
@text = text
end
def method_missing(_method, *_args)
self
end
def respond_to_missing?(_method, _include_private = false)
true
end
def to_s
@text
end
end
end| def print_json_result(result) | ||
| require "json" | ||
| output = { | ||
| status: result.violations.any? ? "fail" : "pass", | ||
| scanned: result.scanned_count, | ||
| violations: result.violations.map { |v| violation_hash(v) }, | ||
| warnings: result.warnings.map { |v| violation_hash(v) } | ||
| } | ||
| puts JSON.pretty_generate(output) | ||
| end | ||
|
|
||
| def violation_hash(violation) | ||
| { | ||
| name: violation.name, | ||
| version: violation.version, | ||
| licenses: violation.licenses, | ||
| source: violation.source | ||
| } | ||
| end | ||
|
|
||
| def print_text_result(result) | ||
| puts Rainbow("\nReact on Rails License Scan").bold | ||
| puts Rainbow("=" * 40).cyan | ||
| puts "Scanned #{result.scanned_count} dependencies\n\n" | ||
|
|
||
| print_violations(result.violations) if result.violations.any? | ||
| print_warnings(result.warnings) if result.warnings.any? | ||
| print_summary(result.violations) | ||
| end | ||
|
|
||
| def print_violations(violations) | ||
| puts Rainbow("VIOLATIONS (disallowed licenses):").red.bold | ||
| violations.each do |v| | ||
| puts Rainbow(" ✗ #{v.name} #{v.version} (#{v.source})").red | ||
| puts " Licenses: #{v.licenses.join(', ')}" | ||
| end | ||
| puts | ||
| end | ||
|
|
||
| def print_warnings(warnings) | ||
| puts Rainbow("WARNINGS (multi-licensed with copyleft option):").yellow.bold | ||
| warnings.each do |v| | ||
| puts Rainbow(" ⚠ #{v.name} #{v.version} (#{v.source})").yellow | ||
| puts " Licenses: #{v.licenses.join(', ')} (permissive option available)" | ||
| end | ||
| puts | ||
| end | ||
|
|
||
| def print_summary(violations) | ||
| if violations.empty? | ||
| puts Rainbow("✓ No disallowed licenses found").green.bold | ||
| else | ||
| puts Rainbow("✗ #{violations.size} violation(s) found").red.bold | ||
| puts " Disallowed: #{ReactOnRails::LicenseScanner::DISALLOWED_LICENSES.join(', ')}" | ||
| end | ||
| puts | ||
| end |
There was a problem hiding this comment.
Helper methods defined in global Object scope
The methods print_json_result, violation_hash, print_text_result, print_violations, print_warnings, and print_summary are defined at the top level of the file, which means they land on Ruby's Object class. This makes them available everywhere and can silently conflict with identically named methods in other rake files or gems. In particular, print_summary is a very generic name.
Consider wrapping these in a private module or namespace, similar to how doctor.rake could be refactored, or use a plain Ruby class/module:
module ReactOnRails
module LicenseScanFormatter
module_function
def print_json_result(result)
# ...
end
# ...
end
endand call them as ReactOnRails::LicenseScanFormatter.print_json_result(result) inside the task block. This matches the module-namespaced style used throughout the rest of the codebase.
| def scan | ||
| scan_ruby_gems | ||
| scan_js_packages | ||
| Result.new(violations: @violations, warnings: @warnings, scanned_count: @scanned_count) | ||
| end |
There was a problem hiding this comment.
scan accumulates state on repeated calls
@violations, @warnings, and @scanned_count are initialized in initialize but never reset before scanning. If scan is called more than once on the same instance, results will be doubled. This is not a problem in the current rake task (one instance, one call), but it makes the API surprising and could cause flaky tests if a test helper reuses a scanner instance.
Consider resetting at the start of scan:
def scan
@violations = []
@warnings = []
@scanned_count = 0
scan_ruby_gems
scan_js_packages
Result.new(violations: @violations, warnings: @warnings, scanned_count: @scanned_count)
end| # frozen_string_literal: true | ||
|
|
||
| require_relative "spec_helper" | ||
| require "react_on_rails/license_scanner" | ||
|
|
||
| RSpec.describe ReactOnRails::LicenseScanner do | ||
| subject(:scanner) { described_class.new } | ||
|
|
||
| describe "#scan" do | ||
| it "scans ruby gems and returns a result" do | ||
| result = scanner.scan | ||
| expect(result).to be_a(described_class::Result) | ||
| expect(result.scanned_count).to be_positive | ||
| expect(result.violations).to be_an(Array) | ||
| expect(result.warnings).to be_an(Array) | ||
| end | ||
|
|
||
| it "reports no violations for the react_on_rails dependency tree" do | ||
| result = scanner.scan | ||
| expect(result.violations).to be_empty | ||
| end | ||
| end | ||
|
|
||
| describe "license classification" do | ||
| let(:result_struct) { described_class::Result } | ||
|
|
||
| describe "DISALLOWED_LICENSES" do | ||
| it "includes GPL and AGPL variants" do | ||
| expect(described_class::DISALLOWED_LICENSES).to include("GPL-2.0", "GPL-3.0", "AGPL-3.0") | ||
| end | ||
| end | ||
|
|
||
| describe "PERMISSIVE_LICENSES" do | ||
| it "includes common permissive licenses" do | ||
| expect(described_class::PERMISSIVE_LICENSES).to include("MIT", "Apache-2.0", "BSD-2-Clause") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe "multi-licensed gems" do | ||
| it "treats dual MIT/GPL as a warning, not a violation" do | ||
| result = scanner.scan | ||
|
|
||
| gpl_violations = result.violations.select { |v| v.licenses.any? { |l| l.include?("GPL") } } | ||
| expect(gpl_violations).to be_empty | ||
|
|
||
| diff_lcs_warning = result.warnings.find { |w| w.name == "diff-lcs" } | ||
| expect(diff_lcs_warning.licenses).to include("MIT") if diff_lcs_warning | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
All tests are integration tests with no unit coverage for parsing logic
Every example in this spec calls scanner.scan, which hits the live Bundler definition and the real gem specs. This means:
- JS package parsing (
parse_pnpm_licenses,parse_npm_licenses,parse_yarn_licenses) has zero test coverage. - The
check_licensesclassification logic (violation vs. warning) is only covered indirectly through the real gem tree. - The suite will fail or produce different results on a machine where
diff-lcsis not installed (thediff_lcs_warningassertion is guarded byif diff_lcs_warning, effectively skipping the check when the gem isn't present).
Adding unit tests that feed crafted data to parse_* helpers and check_licenses directly would give reproducible, fast coverage of the actual logic.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97049c7187
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd = LICENSE_COMMANDS[package_manager] | ||
|
|
||
| output = `#{cmd}` | ||
| return nil unless $CHILD_STATUS&.success? |
There was a problem hiding this comment.
$? to detect JS command failures
The JS scan path never runs because fetch_js_licenses checks $CHILD_STATUS, which is nil unless English is explicitly required; in normal Ruby/Rails execution only $? is updated after backticks. As written, return nil unless $CHILD_STATUS&.success? always returns early, so JS dependencies are silently skipped and the task can report a passing scan while missing disallowed npm/pnpm/yarn licenses.
Useful? React with 👍 / 👎.
| DISALLOWED_LICENSES.any? { |d| license.start_with?(d) || license.casecmp(d).zero? } | ||
| end | ||
|
|
||
| def permissive_license?(license) | ||
| PERMISSIVE_LICENSES.any? { |p| license.casecmp(p).zero? } |
There was a problem hiding this comment.
License classification assumes each entry is a single token, but npm/yarn data commonly includes SPDX expressions like MIT OR GPL-3.0 or GPL-2.0 OR MIT. With the current start_with?/exact-match checks, GPL may be missed when it is not the first token, and permissive alternatives are not recognized when bundled in one expression, causing false negatives or incorrect hard violations.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| def Rainbow(text) # rubocop:disable Naming/MethodName | ||
| text | ||
| end | ||
| end |
There was a problem hiding this comment.
Rainbow fallback crashes on method chaining calls
Low Severity
The Rainbow fallback returns a raw String from Rainbow(text), but the rake task chains methods like .bold, .red, .cyan, .green, and .yellow on the result. String doesn't have these methods, so this would raise NoMethodError if Rainbow is unavailable. The existing codebase already solves this correctly in doctor.rb and doctor.rake using a SimpleColorWrapper class with method_missing that returns self for chaining.
| end | ||
|
|
||
| def disallowed_license?(license) | ||
| DISALLOWED_LICENSES.any? { |d| license.start_with?(d) || license.casecmp(d).zero? } |
There was a problem hiding this comment.
Compound SPDX expressions cause order-dependent license detection
Medium Severity
The disallowed_license? method uses start_with? which causes inconsistent handling of compound SPDX expressions from JS packages. "GPL-3.0 OR MIT" is flagged as a violation (not a warning, since permissive_license? won't match the compound string), while "MIT OR GPL-3.0" passes undetected entirely. Detection depends on license order within the expression, and matched compounds are misclassified as violations instead of warnings.
Additional Locations (1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
react_on_rails/spec/react_on_rails/license_scanner_spec.rb (1)
47-49: Make the multi-license assertion strict to validate intended behavior.Line 48 is conditional, so this example passes even when
diff-lcswarning is missing. Assert presence first, then assert licenses.💡 Proposed fix
diff_lcs_warning = result.warnings.find { |w| w.name == "diff-lcs" } - expect(diff_lcs_warning.licenses).to include("MIT") if diff_lcs_warning + expect(diff_lcs_warning).not_to be_nil + expect(diff_lcs_warning.licenses).to include("MIT")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/license_scanner_spec.rb` around lines 47 - 49, The test currently uses a conditional that lets it pass when the "diff-lcs" warning is missing; change it to first assert that a warning for "diff-lcs" exists (e.g., expect(result.warnings.find { |w| w.name == "diff-lcs" }).not_to be_nil or a similar presence assertion on result.warnings) and then assert that that found object's licenses include "MIT" (use the same diff_lcs_warning variable to perform expect(diff_lcs_warning.licenses).to include("MIT")), removing the conditional so the test fails when the warning is absent..cursor/BUGBOT.md (1)
8-8: Align BugBot rule wording with scanner coverage to avoid policy drift.Line 8 only names base IDs, while the scanner also blocks
-only/-or-latervariants. Consider mirroring the full set (or referencing scanner constants) so automation/docs stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cursor/BUGBOT.md at line 8, The rule text currently lists only base SPDX IDs ("GPL-2.0, GPL-3.0, AGPL-3.0") but the scanner also matches "-only" and "-or-later" variants, so update the wording in the line that currently reads "If any new or upgraded dependency has a license in {GPL-2.0, GPL-3.0, AGPL-3.0}, then:" to either expand the set to explicitly include GPL-2.0-only, GPL-2.0-or-later, GPL-3.0-only, GPL-3.0-or-later, AGPL-3.0-only, AGPL-3.0-or-later, or replace it with a reference to the scanner's license constants so the doc and automation stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails/lib/react_on_rails/license_scanner.rb`:
- Line 77: The JS license scan currently silently skips on parsing failures:
replace the early return on licenses_json (the "return if licenses_json.nil?" in
license_scanner.rb) with raising a descriptive error so a parse failure surfaces
as a failed scan, and change the rescue that swallows the parser exception
(around the code that handles parser errors at the later rescue block) to either
re-raise the original exception or raise a new ParsingError that includes the
original exception message and context; ensure the exception propagates to the
caller so CI can detect the failure rather than reporting a false pass.
- Around line 159-181: The current matching in normalize_license /
disallowed_license? / permissive_license? treats the license string as a single
token and misses SPDX expressions like "MIT OR GPL-2.0"; update normalization to
return a normalized set of SPDX license identifiers by splitting/tokenizing on
SPDX operators (OR, AND, WITH, "+", "+", commas, whitespace) and extracting
identifier tokens (case-insensitive), then change disallowed_license? and
permissive_license? to check any token against DISALLOWED_LICENSES /
PERMISSIVE_LICENSES (using case-insensitive equality or starts_with where
appropriate for license exceptions) so composite expressions correctly detect
copyleft or disallowed components.
- Around line 107-112: The subprocess success check in fetch_js_licenses is
using $CHILD_STATUS without requiring the English library which makes it
unreliable; change the check to use the builtin $? instead of $CHILD_STATUS
(e.g., replace "$CHILD_STATUS&.success?" with "$?.success?") so the method
correctly detects command success after running the
LICENSE_COMMANDS[package_manager] backtick invocation; alternatively, if you
prefer the $CHILD_STATUS name, require 'English' at the top of the file, but do
not rely on safe navigation with a possibly nil alias.
In `@react_on_rails/lib/tasks/license_scan.rake`:
- Around line 9-13: The fallback Rainbow method in Kernel currently returns a
plain String which breaks callers that chain color methods (e.g.,
Rainbow(...).bold); update Kernel#Rainbow to return an object that responds to
the color/format chain (at least the methods used: bold, red, yellow, green,
underline, etc.) and delegates to the underlying string, returning the plain
string when converted (to_s/inspect); implement this via a small wrapper (e.g.,
a simple class or struct with defined color methods or method_missing that
returns self or the wrapped string) so chained calls do not raise NoMethodError
and ultimately yield the original text.
---
Nitpick comments:
In @.cursor/BUGBOT.md:
- Line 8: The rule text currently lists only base SPDX IDs ("GPL-2.0, GPL-3.0,
AGPL-3.0") but the scanner also matches "-only" and "-or-later" variants, so
update the wording in the line that currently reads "If any new or upgraded
dependency has a license in {GPL-2.0, GPL-3.0, AGPL-3.0}, then:" to either
expand the set to explicitly include GPL-2.0-only, GPL-2.0-or-later,
GPL-3.0-only, GPL-3.0-or-later, AGPL-3.0-only, AGPL-3.0-or-later, or replace it
with a reference to the scanner's license constants so the doc and automation
stay in sync.
In `@react_on_rails/spec/react_on_rails/license_scanner_spec.rb`:
- Around line 47-49: The test currently uses a conditional that lets it pass
when the "diff-lcs" warning is missing; change it to first assert that a warning
for "diff-lcs" exists (e.g., expect(result.warnings.find { |w| w.name ==
"diff-lcs" }).not_to be_nil or a similar presence assertion on result.warnings)
and then assert that that found object's licenses include "MIT" (use the same
diff_lcs_warning variable to perform expect(diff_lcs_warning.licenses).to
include("MIT")), removing the conditional so the test fails when the warning is
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0255f90-3f4d-4cfe-b5f5-6cdb62750fe3
📒 Files selected for processing (4)
.cursor/BUGBOT.mdreact_on_rails/lib/react_on_rails/license_scanner.rbreact_on_rails/lib/tasks/license_scan.rakereact_on_rails/spec/react_on_rails/license_scanner_spec.rb
|
|
||
| package_manager = detect_package_manager(lockfile) | ||
| licenses_json = fetch_js_licenses(package_manager) | ||
| return if licenses_json.nil? |
There was a problem hiding this comment.
JS scan currently fails open on parse errors, which can produce false “pass” results.
Line 77 returns early when parsing fails, and Lines 115-117 swallow the parser error. That skips JS dependency checks without surfacing a failure signal.
💡 Proposed fix
- rescue JSON::ParserError
- nil
+ rescue JSON::ParserError => e
+ raise "Unable to parse #{package_manager} license output: #{e.message}"
endAlso applies to: 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react_on_rails/lib/react_on_rails/license_scanner.rb` at line 77, The JS
license scan currently silently skips on parsing failures: replace the early
return on licenses_json (the "return if licenses_json.nil?" in
license_scanner.rb) with raising a descriptive error so a parse failure surfaces
as a failed scan, and change the rescue that swallows the parser exception
(around the code that handles parser errors at the later rescue block) to either
re-raise the original exception or raise a new ParsingError that includes the
original exception message and context; ensure the exception propagates to the
caller so CI can detect the failure rather than reporting a false pass.
| def fetch_js_licenses(package_manager) | ||
| cmd = LICENSE_COMMANDS[package_manager] | ||
|
|
||
| output = `#{cmd}` | ||
| return nil unless $CHILD_STATUS&.success? | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Ruby $CHILD_STATUS vs $? global variable subprocess status require English
💡 Result:
In Ruby, $CHILD_STATUS and $? are the same value: they both refer to the exit status of the last executed child process, exposed as a Process::Status object (or nil if there isn’t one). (ruby-doc.org)
The difference is naming and availability:
$?is the built-in “cryptic” special global variable (always available). (ruby-doc.org)$CHILD_STATUSis the English alias for$?, and it’s only available after you load the standard libraryEnglish:(ruby-doc.org)require "English" p $CHILD_STATUS # same as $?
Citations:
- 1: https://ruby-doc.org/3.2/Process/Status.html?utm_source=openai
- 2: https://ruby-doc.org/docs/ruby-doc-bundle/UsersGuide/rg/globalvars.html?utm_source=openai
- 3: https://ruby-doc.org/stdlib-2.5.3/libdoc/English/rdoc/English.html?utm_source=openai
🏁 Script executed:
# Locate and examine the license_scanner.rb file
fd "license_scanner.rb" -x cat -n {}Repository: shakacode/react_on_rails
Length of output: 6170
Use $? for subprocess success checks—$CHILD_STATUS requires English.
Line 111 uses $CHILD_STATUS without loading the English standard library, where it is defined as a readable alias. Without it, $CHILD_STATUS is nil, making the check unreliable. The safe navigation operator masks but doesn't fix the issue: nil&.success? returns nil, causing the function to silently skip license scanning even when the command succeeds.
Proposed fix
- return nil unless $CHILD_STATUS&.success?
+ return nil unless $?.success?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react_on_rails/lib/react_on_rails/license_scanner.rb` around lines 107 - 112,
The subprocess success check in fetch_js_licenses is using $CHILD_STATUS without
requiring the English library which makes it unreliable; change the check to use
the builtin $? instead of $CHILD_STATUS (e.g., replace "$CHILD_STATUS&.success?"
with "$?.success?") so the method correctly detects command success after
running the LICENSE_COMMANDS[package_manager] backtick invocation;
alternatively, if you prefer the $CHILD_STATUS name, require 'English' at the
top of the file, but do not rely on safe navigation with a possibly nil alias.
| normalized = licenses.map { |l| normalize_license(l) } | ||
|
|
||
| disallowed = normalized.select { |l| disallowed_license?(l) } | ||
| return if disallowed.empty? | ||
|
|
||
| has_permissive = normalized.any? { |l| permissive_license?(l) } | ||
| if has_permissive | ||
| @warnings << Violation.new(name: name, version: version, licenses: licenses, source: source) | ||
| else | ||
| @violations << Violation.new(name: name, version: version, licenses: licenses, source: source) | ||
| end | ||
| end | ||
|
|
||
| def normalize_license(license) | ||
| license.to_s.strip | ||
| end | ||
|
|
||
| def disallowed_license?(license) | ||
| DISALLOWED_LICENSES.any? { |d| license.start_with?(d) || license.casecmp(d).zero? } | ||
| end | ||
|
|
||
| def permissive_license?(license) | ||
| PERMISSIVE_LICENSES.any? { |p| license.casecmp(p).zero? } |
There was a problem hiding this comment.
Current license matching misses SPDX expressions and can misclassify copyleft packages.
Lines 159-181 compare full strings, so expressions like MIT OR GPL-2.0 are not tokenized correctly. This can hide disallowed licenses or classify permissive+copyleft combos incorrectly.
💡 Proposed fix
- normalized = licenses.map { |l| normalize_license(l) }
+ normalized = licenses
+ .flat_map { |l| split_license_expression(normalize_license(l)) }
+ .uniq
@@
def normalize_license(license)
license.to_s.strip
end
+
+ def split_license_expression(license)
+ license
+ .gsub(/[()]/, " ")
+ .split(/\s+(?:OR|AND|WITH)\s+|,|\/|\|/i)
+ .map(&:strip)
+ .reject(&:empty?)
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react_on_rails/lib/react_on_rails/license_scanner.rb` around lines 159 - 181,
The current matching in normalize_license / disallowed_license? /
permissive_license? treats the license string as a single token and misses SPDX
expressions like "MIT OR GPL-2.0"; update normalization to return a normalized
set of SPDX license identifiers by splitting/tokenizing on SPDX operators (OR,
AND, WITH, "+", "+", commas, whitespace) and extracting identifier tokens
(case-insensitive), then change disallowed_license? and permissive_license? to
check any token against DISALLOWED_LICENSES / PERMISSIVE_LICENSES (using
case-insensitive equality or starts_with where appropriate for license
exceptions) so composite expressions correctly detect copyleft or disallowed
components.
| module Kernel | ||
| def Rainbow(text) # rubocop:disable Naming/MethodName | ||
| text | ||
| end | ||
| end |
There was a problem hiding this comment.
Rainbow fallback is broken: it returns String but callers chain color methods.
When rainbow is unavailable, Line 10 returns plain text; later calls like Line 54 (Rainbow(...).bold) will raise NoMethodError.
💡 Proposed fix
rescue LoadError
# Fallback if Rainbow is not available
module Kernel
+ class PlainRainbow
+ def initialize(text)
+ `@text` = text
+ end
+
+ def bold = self
+ def cyan = self
+ def red = self
+ def yellow = self
+ def green = self
+
+ def to_s
+ `@text`
+ end
+ end
+
def Rainbow(text) # rubocop:disable Naming/MethodName
- text
+ PlainRainbow.new(text)
end
end
endAlso applies to: 54-55, 64-65, 73-75, 83-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react_on_rails/lib/tasks/license_scan.rake` around lines 9 - 13, The fallback
Rainbow method in Kernel currently returns a plain String which breaks callers
that chain color methods (e.g., Rainbow(...).bold); update Kernel#Rainbow to
return an object that responds to the color/format chain (at least the methods
used: bold, red, yellow, green, underline, etc.) and delegates to the underlying
string, returning the plain string when converted (to_s/inspect); implement this
via a small wrapper (e.g., a simple class or struct with defined color methods
or method_missing that returns self or the wrapped string) so chained calls do
not raise NoMethodError and ultimately yield the original text.
PR Review: Add dependency license scanning rake taskOverall this is a useful feature, but there are several issues ranging from a silent bug to test quality and code organization concerns. Bug: Missing
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| cmd = LICENSE_COMMANDS[package_manager] | ||
|
|
||
| output = `#{cmd}` | ||
| return nil unless $CHILD_STATUS&.success? |
There was a problem hiding this comment.
$CHILD_STATUS is defined by require "English" which is missing from this file. Without it, $CHILD_STATUS is always nil, so this guard always returns nil (falsy) and JS license scanning is silently skipped on every run.
Every other file in this codebase that uses $CHILD_STATUS includes require "English" at line 3. Add it here too, or use $? directly.
| begin | ||
| require "rainbow" | ||
| rescue LoadError | ||
| # Fallback if Rainbow is not available | ||
| module Kernel | ||
| def Rainbow(text) # rubocop:disable Naming/MethodName | ||
| text | ||
| end | ||
| end |
There was a problem hiding this comment.
Patching Kernel is a very invasive fallback — it affects all objects in the process, not just this rake task. If the user's app already defines a Kernel#Rainbow method for something else, this will conflict.
A less invasive approach is a local lambda or a simple method defined inside the task's closure.
| print_text_result(result) | ||
| end | ||
|
|
||
| exit(1) if result.violations.any? |
There was a problem hiding this comment.
exit(1) bypasses Rake's error handling. The idiomatic approach for a rake task failure is abort "message" which prints to stderr and exits with code 1, or raise "message" which participates in Rake's task dependency error propagation.
| def print_json_result(result) | ||
| require "json" | ||
| output = { | ||
| status: result.violations.any? ? "fail" : "pass", | ||
| scanned: result.scanned_count, | ||
| violations: result.violations.map { |v| violation_hash(v) }, | ||
| warnings: result.warnings.map { |v| violation_hash(v) } | ||
| } | ||
| puts JSON.pretty_generate(output) | ||
| end | ||
|
|
||
| def violation_hash(violation) | ||
| { | ||
| name: violation.name, | ||
| version: violation.version, | ||
| licenses: violation.licenses, | ||
| source: violation.source | ||
| } | ||
| end | ||
|
|
||
| def print_text_result(result) | ||
| puts Rainbow("\nReact on Rails License Scan").bold | ||
| puts Rainbow("=" * 40).cyan | ||
| puts "Scanned #{result.scanned_count} dependencies\n\n" | ||
|
|
||
| print_violations(result.violations) if result.violations.any? | ||
| print_warnings(result.warnings) if result.warnings.any? | ||
| print_summary(result.violations) | ||
| end | ||
|
|
||
| def print_violations(violations) | ||
| puts Rainbow("VIOLATIONS (disallowed licenses):").red.bold | ||
| violations.each do |v| | ||
| puts Rainbow(" ✗ #{v.name} #{v.version} (#{v.source})").red | ||
| puts " Licenses: #{v.licenses.join(', ')}" | ||
| end | ||
| puts | ||
| end | ||
|
|
||
| def print_warnings(warnings) | ||
| puts Rainbow("WARNINGS (multi-licensed with copyleft option):").yellow.bold | ||
| warnings.each do |v| | ||
| puts Rainbow(" ⚠ #{v.name} #{v.version} (#{v.source})").yellow | ||
| puts " Licenses: #{v.licenses.join(', ')} (permissive option available)" | ||
| end | ||
| puts | ||
| end | ||
|
|
||
| def print_summary(violations) | ||
| if violations.empty? | ||
| puts Rainbow("✓ No disallowed licenses found").green.bold | ||
| else | ||
| puts Rainbow("✗ #{violations.size} violation(s) found").red.bold | ||
| puts " Disallowed: #{ReactOnRails::LicenseScanner::DISALLOWED_LICENSES.join(', ')}" | ||
| end | ||
| puts | ||
| end |
There was a problem hiding this comment.
These helper methods are defined at the top level of the rake file, polluting the global Ruby namespace. If any host application (or another gem) defines methods with the same names (print_text_result, print_summary, etc.), one will silently override the other.
Wrap them in a module or inline them directly into the task.
| end | ||
|
|
||
| def disallowed_license?(license) | ||
| DISALLOWED_LICENSES.any? { |d| license.start_with?(d) || license.casecmp(d).zero? } |
There was a problem hiding this comment.
Two issues with this check:
-
Case inconsistency:
start_with?is case-sensitive whilecasecmpis case-insensitive. A license string"gpl-2.0-foo"won't be caught bystart_with?("GPL-2.0")even though the intent seems to be to catch it. -
Redundant DISALLOWED_LICENSES entries: Since
start_with?("GPL-2.0")already matches"GPL-2.0-only"and"GPL-2.0-or-later", those entries in the constant are redundant. Thestart_with?approach also inadvertently flags"GPL-2.0-WITH-Classpath-exception"and similar SPDX exception identifiers.
Consider using only casecmp exact matching and making the DISALLOWED_LICENSES list the explicit source of truth for which identifiers are blocked.
| expect(gpl_violations).to be_empty | ||
|
|
||
| diff_lcs_warning = result.warnings.find { |w| w.name == "diff-lcs" } | ||
| expect(diff_lcs_warning.licenses).to include("MIT") if diff_lcs_warning |
There was a problem hiding this comment.
This assertion is conditional on diff_lcs_warning being present, so if the gem isn't in warnings (e.g., its license metadata changes or it isn't installed), the test passes vacuously — providing no safety net.
Use expect(diff_lcs_warning).not_to be_nil before the licenses assertion, or better, stub the gem data to make the test deterministic.
Keep only the .cursor/BUGBOT.md config, which is specific to this repo's PR workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justin808
changed the title
|
|
||
| ## License Scanning | ||
|
|
||
| If the PR modifies dependency files (package.json, pnpm-lock.yaml, yarn.lock, Gemfile, Gemfile.lock, gemspec), then: |
There was a problem hiding this comment.
The trigger pattern gemspec won't match any actual file in this repo — the files are named react_on_rails.gemspec and react_on_rails_pro.gemspec. Consider using a glob pattern like *.gemspec so gemspec changes are caught.
| If the PR modifies dependency files (package.json, pnpm-lock.yaml, yarn.lock, Gemfile, Gemfile.lock, gemspec), then: | |
| If the PR modifies dependency files (package.json, pnpm-lock.yaml, yarn.lock, Gemfile, Gemfile.lock, *.gemspec), then: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d53e7d29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ## License Scanning | ||
|
|
||
| If the PR modifies dependency files (package.json, pnpm-lock.yaml, yarn.lock, Gemfile, Gemfile.lock, gemspec), then: |
There was a problem hiding this comment.
*
The dependency-change condition currently lists gemspec as a literal filename, but this repo’s Ruby dependency manifests are *.gemspec files (for example react_on_rails.gemspec), so PRs that only update those files can skip this BugBot rule and miss the license scan entirely. Updating this trigger to explicitly cover *.gemspec prevents silent gaps in compliance checks for gem dependency changes.
Useful? React with 👍 / 👎.
Review of BugBot License Scanning ConfigOverall this is a lightweight, sensible addition. One functional issue and a couple of notes worth considering: Bug:
|
…-renderer-duplicate-error-reporting * origin/master: Consolidate changelog to keep only rc6 prerelease (#2533) Fix CSS module class name divergence with rspack SSR (#2489) Bump version to 16.4.0.rc.6 Add BugBot license scanning config (#2515) Fix buildVM promise cleanup ordering (#2483) (#2484) # Conflicts: # CHANGELOG.md
* origin/master: Fix private_output_path not configured on fresh Shakapacker installs (#2289) Bump the npm-security group across 1 directory with 3 updates (#2387) docs: use https links (#2518) Consolidate changelog to keep only rc6 prerelease (#2533) Fix CSS module class name divergence with rspack SSR (#2489) Bump version to 16.4.0.rc.6 Add BugBot license scanning config (#2515) Fix buildVM promise cleanup ordering (#2483) (#2484) # Conflicts: # CHANGELOG.md
* origin/master: Fix private_output_path not configured on fresh Shakapacker installs (#2289) Bump the npm-security group across 1 directory with 3 updates (#2387) docs: use https links (#2518) Consolidate changelog to keep only rc6 prerelease (#2533) Fix CSS module class name divergence with rspack SSR (#2489) Bump version to 16.4.0.rc.6 Add BugBot license scanning config (#2515) Fix buildVM promise cleanup ordering (#2483) (#2484) # Conflicts: # CHANGELOG.md
## Summary - Add `.cursor/BUGBOT.md` to configure Cursor BugBot for automated license scanning on PRs - When a PR modifies dependency files (package.json, Gemfile, etc.), BugBot will flag any new/upgraded dependency with GPL-2.0, GPL-3.0, or AGPL-3.0 licenses The open-source ROR dependency tree is clean (all MIT/Apache-2.0/BSD). This config ensures it stays that way. ## Test plan - [x] BugBot config follows documented format - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…upport * origin/master: (38 commits) fix: use env-var-driven ports in Procfile templates to support multiple worktrees (#2539) Fix prettier formatting in auto-bundling doc Docs: Clarify .client/.server suffixes vs use client RSC directive (#2406) Warn against using .server/.client variants with RSC features Docs: move internal-only docs out of published docs trees (#2414) Fix crash when HTTPX::ErrorResponse is returned in get_form_body_for_file (#2532) Skip flaky external URLs in lychee checks (#2547) Update mise docs: prefer shell-level shims over conductor-exec (#2537) Document compression middleware compatibility with streaming SSR (#2544) Fix duplicate node-renderer message reporting in render failures (#2531) Fix private_output_path not configured on fresh Shakapacker installs (#2289) Bump the npm-security group across 1 directory with 3 updates (#2387) docs: use https links (#2518) Consolidate changelog to keep only rc6 prerelease (#2533) Fix CSS module class name divergence with rspack SSR (#2489) Bump version to 16.4.0.rc.6 Add BugBot license scanning config (#2515) Fix buildVM promise cleanup ordering (#2483) (#2484) Fix streaming SSR hangs and silent error absorption in RSC payload injection (#2407) Ensure lefthook uses mise tools in non-interactive shells (#2512) ... # Conflicts: # CHANGELOG.md
1 participant
Summary
.cursor/BUGBOT.mdto configure Cursor BugBot for automated license scanning on PRsThe open-source ROR dependency tree is clean (all MIT/Apache-2.0/BSD). This config ensures it stays that way.
Test plan
🤖 Generated with Claude Code