pronto-brakeman & pronto-rails_best_practices run order inconsistencies

[eliotsykes]

Firstly, thanks @mmozuras for pronto. I'm trialling using it to help with teaching Rails, when reviewing student projects.

The running order of pronto-brakeman and pronto-rails_best_practices can affect what issues are detected.

For a Rails 4.2.0 app I'm trying this out with, here are the total counts of issues found when the runners are run alone and together in both orders:

Command	Number of issues pronto found
pronto run -r=rails_best_practices	4
pronto run -r=brakeman	1
pronto run -r=rails_best_practices brakeman	5
pronto run -r=brakeman rails_best_practices	6

Neither of the above pronto runs which ran with both runners quite found all the issues. There were missing issues and false positives in both when the running order was "1. brakeman, 2. rails_best_practices" . For more details see the full output further below.

Debugging so far shows part of the issue is due to two different gems (brakeman and code_analyzer (code_analyzer is used by rails_best_practices)) monkey-patching the same Sexp methods with different behaviours.

For example, the RemoveEmptyHelpersReview#empty_body?(module_node) is dependent on Sexp#body that behaves differently in brakeman compared to code_analyzer. As a result the RemoveEmptyHelpersReview from rails_best_practices only detects empty helpers when rails_best_practices runs before brakeman.

(I'm not sure that the Sexp monkey-patching explains the entire issue yet).

I'd like to contribute a fix for this, though I'm not sure where to go from here, so any guidance and ideas would be much appreciated.

One thought is that to help uncover these kind of issues, perhaps the installed pronto-* runners could run in a random order when they're run with pronto run (i.e. no -r= option).

$ pronto run -r=rails_best_practices

app/controllers/collaborations_controller.rb:22 W: Law of demeter
app/views/wikis/show.html.haml:10 W: Law of demeter
app/helpers/collaborations_helper.rb:1 W: Remove empty helpers
app/views/collaborations/new.html.haml:10 W: Remove trailing whitespace

$ pronto run -r=brakeman

app/controllers/collaborations_controller.rb:15 W: Possible security vulnerability: Possible unprotected redirect

$ pronto run -r=rails_best_practices brakeman

app/controllers/collaborations_controller.rb:22 W: Law of demeter
app/views/wikis/show.html.haml:10 W: Law of demeter
app/helpers/collaborations_helper.rb:1 W: Remove empty helpers
app/views/collaborations/new.html.haml:10 W: Remove trailing whitespace
app/controllers/collaborations_controller.rb:15 W: Possible security vulnerability: Possible unprotected redirect

$ pronto run -r=brakeman rails_best_practices

app/controllers/collaborations_controller.rb:15 W: Possible security vulnerability: Possible unprotected redirect
app/controllers/collaborations_controller.rb:22 W: Law of demeter
app/views/wikis/show.html.haml:10 W: Law of demeter
config/routes.rb:4 W: Restrict auto-generated routes wikis (only: [])
config/routes.rb:5 W: Restrict auto-generated routes wikis/collaborations (only: [])
app/views/collaborations/new.html.haml:10 W: Remove trailing whitespace

# MISSING ISSUE: app/helpers/collaborations_helper.rb:1 W: Remove empty helpers
# FALSE POSITIVE: config/routes.rb:4 W: Restrict auto-generated routes wikis (only: [])
# FALSE POSITIVE: config/routes.rb:5 W: Restrict auto-generated routes wikis/collaborations (only: [])

[eliotsykes]

I've edited the above issue report to correct information on false positives and missing issues. The issue continues to be valid, particularly the Sexp monkey patch conflicts.

[nbekirov]

Hey @eliotsykes!

I'm not sure that the Sexp monkey-patching explains the entire issue yet

My investigation is definitely pointing in the same direction as Sexp is so ubiquitous in brakeman, ruby_parser, code_analyzer.

I've made a repository with an executable test case demonstrating the issue in isolation from pronto and other gems. I hope that it'll be useful for anyone willing to tag along.

It includes:

• Failing example
• Possible solution - after a bit of tinkering and reading, forking seems to be the the best (only?) possible effective way to sandbox some code execution in Ruby. I've used Yehuda Katz's solution here.

Please check it out. I'm positive we'll come up with a solution very soon. :smiley:

[eliotsykes]

Hi @nbekirov - thanks for looking into this. Your forking plan looks promising.