Bug: Discrepancy between lint failures in `autocorrect` and `force_update`?

[pboling]

My CI failed with a RuboCop violation. So locally I ran:

❯ bundle exec rake rubocop_gradual:autocorrect
Running RuboCop Gradual...
Inspecting 181 file(s) for autocorrection...
.....................................................................................................................................................................................
Fixed 0 file(s).
.......................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 258 files with 5513 issue(s).
Processing results...
RuboCop Gradual got no changes.
noglob bundle exec rake rubocop_gradual:autocorrect  53.01s user 8.48s system 228% cpu 26.863 total

This is a surprising and invalid result, since I saw the result of running rubocop on CI (which runs check).

Instead of saying it "got no changes" it should have reported the new failures we are about to see below...

So I ran force_update:

❯ bundle exec rake rubocop_gradual:force_update
Running RuboCop Gradual...
.......................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 258 files with 5515 issue(s).
Processing results...
Uh oh, RuboCop Gradual got worse:
-> spec/channels/modified_relays_channel_spec.rb (1 new issues)
    (line 7) "Betterment/UnscopedFind: Records are being retrieved directly using user input.
Please query for the associated record in a way that enforces authorization (e.g. "trust-root chaining").

INSTEAD OF THIS:
Post.find(params[:post_id])

DO THIS:
current_user.posts.find(params[:post_id])

See here for more information on this error:
https://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind
 (https://github.com/Betterment/betterlint#bettermentunscopedfind)"
-> spec/channels/sensor_streams_channel_spec.rb (1 new issues)
    (line 7) "Betterment/UnscopedFind: Records are being retrieved directly using user input.
Please query for the associated record in a way that enforces authorization (e.g. "trust-root chaining").

INSTEAD OF THIS:
Post.find(params[:post_id])

DO THIS:
current_user.posts.find(params[:post_id])

See here for more information on this error:
https://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind
 (https://github.com/Betterment/betterlint#bettermentunscopedfind)"
Force updating lock file...
noglob bundle exec rake rubocop_gradual:force_update  20.56s user 4.62s system 226% cpu 11.099 total

The force_update caught the change somehow, when the autocorrect did not.

[pboling]

Autocorrect should run the complete rubocop linting. It should still fail, even if it can't fix anything. Regular rubocop fails the same way with or without the -a flag.

[pboling]

@skryukov any thoughts on this?

[skryukov]

Hey, @pboling! Sorry for the delay.

Autocorrect should run the complete rubocop linting. It should still fail, even if it can't fix anything.

Yes, that's exactly our approach—we initially run autocorrect, followed by the default rubocop-gradual command:

rubocop-gradual/lib/rubocop/gradual/commands/autocorrect.rb

Lines 10 to 21 in dd23f28

	class Autocorrect
	def call
	runner = RuboCop::CLI::Command::ExecuteRunner.new(
	RuboCop::CLI::Environment.new(
	Configuration.rubocop_options.merge(formatters: [[Formatters::Autocorrect, nil]]),
	Configuration.rubocop_config_store,
	lint_paths
	)
	)
	runner.run
	Base.new.call
	end

Based on the logs you shared, the number of inspected files is consistent, yet the number of detected issues varies, the question is why 🤔

I tried to reproduce the issue, but failed. To get more info we could try adding a debug option to the Rakefile. This will add awful lot of stuff to the output, including RuboCop debug information:

RuboCop::Gradual::RakeTask.new.tap do |t|
  t.options = ["--debug"]
end

[pboling]

One thing I have noticed is that most of the issues I've had with discrepancies have come from Betterment/betterlint or from various Bundler rules (not sure which gem they come from). I'll add debug and see if it sheds any more light.

[pboling]

IMO - it is important that autocorrect run the same rules as a regular check, because that seems to be the behavior of RuboCop, and developers have been trained over many years to just run rubocop -a, and have the expectation that if there are no failures then they can push and expect it to pass the CI build (which doesn't run -a).

Since gradual allows hijacking of rubocop to run rubocop_gradual in it's place, it makes sense to support the existing expectations and workflow of developers.

We have the hijack in place, so people run rubocop and get rubocop_gradual, and because of this workflow discrepancy our developers frequently push code that appears to pass linting (via autocorrect), but fails the build on CI. It's a a frustrating loop.

[pboling]

I was confused about your comment:

Yes, that's exactly our approach—we initially run autocorrect, followed by the default rubocop-gradual command:

You're saying that this is what you expect to happen for everyone, as that is what the code in rubocop_gradual is intended to do?

That is definitely nothing like our experience, if so.

[pboling]

More info, just saw this again, and this time (thankfully) it is in a fully open source repo.

Without changing any files in between these commands:

❯ bundle exec rake rubocop_gradual:check
Running RuboCop Gradual...
...................................
Found 1 files with 6 issue(s).
Processing results...
RuboCop Gradual got no changes.

❯ bundle exec rake rubocop_gradual:autocorrect
Running RuboCop Gradual...
Inspecting 34 file(s) for autocorrection...
..................................
Fixed 0 file(s).
...................................
Found 1 files with 6 issue(s).
Processing results...
Uh oh, RuboCop Gradual got worse:
-> bin/bundle (1 new issues)
    (line 64) "ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods."
RuboCop Gradual failed!

❯ bundle exec rake rubocop_gradual:force_update
Running RuboCop Gradual...
...................................
Found 1 files with 6 issue(s).
Processing results...
RuboCop Gradual got no changes.

https://github.com/VitalConnectInc/rack-openid2

[pboling]

@skryukov
Just ran into this again on another private project. I tried the debug idea you posted earlier.

Starting with a clean git working tree. I ran force_udpate to make sure I had no uncommitted changes initially, then check, then autocorrect, and what I found was very interesting.

My ENV

force_update

❯ bundle exec rake rubocop_gradual:force_update
Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: force_update
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3896, :left=>3896}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.077933999942616 seconds
noglob bundle exec rake rubocop_gradual:force_update  37.51s user 7.49s system 257% cpu 17.451 total

❯ git status
On branch main

nothing to commit, working tree clean

check

❯ bundle exec rake rubocop_gradual:check
Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: check
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3896, :left=>3896}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.030246999929659 seconds
noglob bundle exec rake rubocop_gradual:check  38.42s user 8.82s system 263% cpu 17.951 total

❯ git status
On branch main

nothing to commit, working tree clean

autocorrect

Running RuboCop Gradual...
Inspecting 338 file(s) for autocorrection...
..................................................................................................................................................................................................................................................................................................................................................
Fixed 0 file(s).
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3890 issue(s).
Processing results...
RuboCop Gradual got 6 issue(s) fixed, 3890 left. Keep going!
Running RuboCop Gradual...
Gradual mode: update
Inspecting 338 file(s) for autocorrection...
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.127219000016339 seconds
noglob bundle exec rake rubocop_gradual:autocorrect  98.46s user 15.90s system 223% cpu 51.254 total

❯ git status
On branch main
...

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   .rubocop_gradual.lock

no changes added to commit (use "git add" and/or "git commit -a")

This is where the discrepancy occurs. The autocorrect output says there were no changes... but that's not true.

{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.

The lockfile was changed. It should have failed, but it reported success with not changes found despite making actual changes.

This is born out by running check a 2nd time, as this time it will note the failure.

2nd check

Running RuboCop Gradual...
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3896 issue(s).
Processing results...
Uh oh, RuboCop Gradual got worse:
...
Unexpected Changes!

RuboCop Gradual lock file is outdated, to fix this message:
- Run `rubocop-gradual` locally and commit the results, or
- EVEN BETTER: before doing the above, try to fix the remaining issues in those files!

`.rubocop_gradual.lock` diff:

-    [153, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
-    [171, 5, 59, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 3742851078],
-    [181, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+    [153, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
-    [161, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
-    [186, 5, 59, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 3742851078],
-    [196, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+    [161, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
-    [62, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
-    [77, 5, 63, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 1476888842]
+    [62, 9, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]
-    [229, 5, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545],
-    [262, 5, 54, "Betterment/UnscopedFind: Records are being retrieved directly using user input.\nPlease query for the associated record in a way that enforces authorization (e.g. \"trust-root chaining\").\n\nINSTEAD OF THIS:\nPost.find(params[:post_id])\n\nDO THIS:\ncurrent_user.posts.find(params[:post_id])\n\nSee here for more information on this error:\nhttps://github.com/Betterment/betterlint/blob/main/README.md#bettermentunscopedfind\n (https://github.com/Betterment/betterlint#bettermentunscopedfind)", 113652520]
+    [229, 5, 3, "Rails/OutputSafety: Tagging a string as html safe may be a security risk.", 193432545]

RuboCop Gradual failed!
noglob bundle exec rake rubocop_gradual:check  19.91s user 4.76s system 211% cpu 11.689 total

2nd autocorrect

Strangely, and as evidence of the discrepancy, if I run autocorrect again (still with the changed lockfile that just failed the 2nd check above), it will pass saying there are no changes.

This means that check and autocorrect are not parsing rules config, or the lockfile, or both, the same way.

Running RuboCop Gradual...
Inspecting 338 file(s) for autocorrection...
..................................................................................................................................................................................................................................................................................................................................................
Fixed 0 file(s).
..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Found 264 files with 3890 issue(s).
Processing results...
RuboCop Gradual got no changes.
Running RuboCop Gradual...
Gradual mode: update
Inspecting 338 file(s) for autocorrection...
...
{:fixed=>0, :moved=>0, :new=>0, :unchanged=>3890, :left=>3890}
RuboCop Gradual got no changes.
Finished Gradual processing in 5.472328000003472 seconds
noglob bundle exec rake rubocop_gradual:autocorrect  107.44s user 17.65s system 214% cpu 58.262 total

Because of this discrepancy I can no longer use autocorrect on this project, since it will always fail check in CI.

[skryukov]

Thanks @pboling I'll try to look into it next week

[pboling]

I should add that so far all of the discrepancy in rules that I've paid enough attention to remember has been from one single rule:
Betterment/UnscopedFind
It comes from the betterlint gem, which is an extension of RuboCop rules specifically for Rails projects.

It is possible that it is an issue of interplay between various plugins, as I do load many rubocop-* gems.

[pboling]

Oh! I think I figured it out partially. I had been referencing Betterment rules in my local rubocop config file, but had failed to use the include_gem directive.

inherit_gem:
  betterlint:
    - config/default.yml

Now that I've added that, it waffles between running force_update and check and autocorrect consistently giving the same result, and, without changing any code, giving inconsistent results.

I think a bug in the unscoped find rule may be to blame. Perhaps it is not deterministic?
Betterment/betterlint#51

Anyhow, now that the config is loaded properly, it seems I can turn off the rule (Betterment/UnscopedFind) and have it stay off reliably, so my lockfile remains consistent. If I turn the rule on I can't get consistent results.

[pboling]

I'm going to close this for now. I never had time to fully investigate the specific conditions of what looked like a bug. The gem is generally working as expected for a long time now.