Supersede #911: compiler_spec call-count refactor with lint unblock

[justin808]

Supersedes #911.

Includes:

• original compiler_spec call-count refactor
• .prettierignore workflow-file ignore update to avoid unrelated lint failures

Closes #855

---

Note

Low Risk
Test-only refactor that changes how mocks detect the hook call; production behavior is unchanged, with minimal risk beyond potential spec brittleness.

Overview
Updates spec/shakapacker/compiler_spec.rb to remove manual call_count logic in several precompile_hook examples and instead branch the Open3.capture3 stub based on the invoked command/executable, using shared hook_command/hook_executable variables.

This makes the tests align with the real Open3.capture3(env, executable, *args, chdir: ...) signature and improves robustness for cases like quoted paths, missing executables, command chaining, and env-var-prefixed hook commands.

^{Written by Cursor Bugbot for commit 3df53f2. Configure here.}

Summary by CodeRabbit

• Tests
  • Expanded test coverage for precompile hook handling: special-character/quote cases, executable-vs-argument behavior to prevent shell interpretation, path traversal and security checks, environment variable extraction/merging, malformed command handling, and skip-logic scenarios.
• Documentation
  • Clarified triage flow to always offer optional rationale replies for skipped/declined items, require explicit user selection before posting, and added prompt guidance for rationale replies.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 11dc29ed-13c8-40ea-b051-ea97a38e9dd4

📥 Commits

Reviewing files that changed from the base of the PR and between 9c8cabc and c4c06d9.

📒 Files selected for processing (2)

• .claude/commands/address-review.md
• spec/shakapacker/compiler_spec.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• spec/shakapacker/compiler_spec.rb

---

Walkthrough

Expands and hardens precompile_hook tests in spec/shakapacker/compiler_spec.rb (handling spaces, quotes, env vars, security checks, malformed commands, and skip logic) and updates guidance in .claude/commands/address-review.md to add explicit rationale-reply flows during triage.

Changes

Cohort / File(s)	Summary
Precompile Hook Tests spec/shakapacker/compiler_spec.rb	Adds/extends tests covering shell-free invocation, quoted/space-containing commands, env extraction/merging, path-traversal and partial-match security checks, malformed command errors, SHAKAPACKER_SKIP_PRECOMPILE_HOOK behaviors, and refactors mocks to explicit have_received/not_to have_received assertions. Review focus: mock dispatching, exact argv expectations, and security assertions.
Triage / Address-Review Guidance .claude/commands/address-review.md	Introduces optional rationale-reply flow: always offer rationale replies for skipped/declined DISCUSS items, gate posting on explicit user selection, and extends prompts/step flow accordingly. Review focus: prompt wording and gating logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐇 I dug through tests both neat and wide,
I chased each quote and space aside,
No shell to trick, no path to slip,
Env vars merged from every clip.
I hop with joy — the specs abide!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The .claude/commands/address-review.md changes appear unrelated to issue #855 objectives about refactoring compiler_spec.rb call-count patterns.	Remove changes to .claude/commands/address-review.md or explain their necessity as related to the PR's objectives in the PR description.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Supersede #911: compiler_spec call-count refactor with lint unblock' is specific and relates to the main changes in the PR.
Linked Issues check	✅ Passed	The PR addresses issue #855 by refactoring call_count patterns to use explicit have_received assertions in compiler_spec.rb tests, which aligns with all stated objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/supersede-911-call-count-lint

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR refactors compiler_spec.rb to replace indirect call_count patterns with more explicit test logic, aligning with the project's RSpec best practices outlined in CLAUDE.md. It also adds .prettierignore entries to prevent lint failures on workflow files during PR validation.

Key changes:

• Removed call_count variables and replaced with direct argument matching in mock stubs
• Extracted hook commands into named variables for clarity
• Added prettier ignore rules for workflow files

Issues found:

• Three test blocks have incorrect argument indexing due to inconsistent block signatures (|*args| vs |env, *args|), which will cause test failures when the hook command matching logic doesn't work as expected

Confidence Score: 2/5

• This PR has critical test logic errors that will cause test failures
• The refactor introduces argument indexing bugs in three test cases where the block signature doesn't match the argument access pattern. These will cause tests to fail because the condition args[1] == hook_command will never be true when the env hash is at args[0]
• Pay close attention to spec/shakapacker/compiler_spec.rb lines 121-122, 177-178, and 202-203 where argument indexing is incorrect

Important Files Changed

Filename	Overview
.prettierignore	Added workflow file ignores to prevent lint failures during PR validation
spec/shakapacker/compiler_spec.rb	Refactored tests to use explicit assertion patterns instead of call counters, with minor logic issues in test conditions

_{Last reviewed commit: ad0410f}

[greptile-apps]

_{2 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[claude]

PR Review\n\nThis is a clean test-quality improvement.

[claude]

PR Review

This is a clean test-quality improvement that aligns with the project preferred spy assertion patterns. The core refactor is sound and the logic is correct.

What works well

• Replacing call_count counter patterns with args[0] == hook_executable dispatch is semantically clearer and more robust against execution-order changes.
• Consistently using the env-splat block signature is correct, since Open3.capture3 receives the env hash as a distinct first argument.
• Adding have_received(:capture3) spy assertions to several tests improves confidence that the hook is actually invoked, not just that compile returns true.
• Introducing hook_executable in all tests makes the pattern consistent and self-documenting.

Issues

Minor: have_received assertion missing from the nonexistent-hook test

The 'warns when hook executable does not exist' test had call_count removed and hook_executable added for consistency, but unlike the other refactored tests it did not gain an explicit have_received(:capture3) assertion. The CLAUDE.md guidelines call for have_received over counter patterns. Adding one would complete the consistency story.

Minor: hash_including without arguments is overly permissive

In the new assertions such as with(hash_including, hook_executable, hash_including), hash_including with no arguments matches any hash. For the last arg (which always contains chdir:), using hash_including(chdir: anything) would document intent and catch regressions where chdir is accidentally dropped.

Nit: duplicate conditional in shell-injection test

In the 'prevents shell injection' test, args[0] == hook_executable is evaluated twice in successive lines - once for captured_args and once for the if/else. A combined expression would be slightly cleaner, though readable as-is.

Verdict

Approve with the suggestion to add the missing have_received assertion for the nonexistent-hook test and consider tightening the hash_including matchers. The .prettierignore change mentioned in the PR description does not appear in the diff - worth confirming if it was intentionally dropped.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

spec/shakapacker/compiler_spec.rb (1)

253-274: Use direct have_received(...).once assertions in the remaining hook spy examples.

These examples still infer the hook call through captured_args / captured_env. They fail if the hook never runs, but they will still pass if it runs more than once. A direct spy assertion keeps the refactor consistent and preserves the call-count protection you removed elsewhere.

♻️ Prefer explicit spy assertions here too

-      captured_args = []
       allow(Open3).to receive(:capture3) do |env, *args|
-        captured_args << args if args[0] == hook_executable
         if args[0] == hook_executable
           ["", "", hook_status]
         else
           ["", "", webpack_status]
         end
       end
@@
       expect(Shakapacker.compiler.compile).to be true
-      expect(captured_args[0][0]).to eq("bin/prepare")
-      expect(captured_args[0][1..4]).to eq(["&&", "rm", "-rf", "/"])
+      expect(Open3).to have_received(:capture3)
+        .with(hash_including, hook_executable, "&&", "rm", "-rf", "/", hash_including)
+        .once
@@
-      captured_env = nil
       allow(Open3).to receive(:capture3) do |env, *args|
-        captured_env = env if args[0] == hook_executable
         if args[0] == hook_executable
           ["", "", hook_status]
         else
           ["", "", webpack_status]
         end
       end
@@
       expect(Shakapacker.compiler.compile).to be true
-      expect(captured_env["FOO"]).to eq("bar")
-      expect(captured_env["BAZ"]).to eq("qux")
+      expect(Open3).to have_received(:capture3)
+        .with(hash_including("FOO" => "bar", "BAZ" => "qux"), hook_executable, "--arg", hash_including)
+        .once

As per coding guidelines: **/*_spec.rb: Use explicit RSpec spy assertions with have_received/not_to have_received instead of indirect counter patterns.

Also applies to: 286-304

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@spec/shakapacker/compiler_spec.rb` around lines 253 - 274, The test is using
an indirect captured_args pattern to assert the precompile hook ran and with
specific arguments; replace those with direct RSpec spy assertions: keep the
allow(Open3).to receive(:capture3) spy setup but remove the captured_args
inspection and instead add explicit expectations like expect(Open3).to
have_received(:capture3).once.with(kind_of(Hash), include(hook_executable),
anything) or a more specific .with(...) matching the exact argument array (e.g.,
"bin/prepare", "&&", "rm", "-rf", "/") after calling
Shakapacker.compiler.compile; apply the same replacement for the similar hook
assertions around lines 286-304 so the tests assert the hook was invoked exactly
once and with the intended arguments.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@spec/shakapacker/compiler_spec.rb`:
- Around line 176-193: The test currently only asserts two trailing args, which
doesn't verify correct parsing of the quoted hook path; update the spec around
Shakapacker.compiler.compile/Open3.capture3 to assert the exact parsed argv:
when precompile_hook is "'bin/my script' --arg1 --arg2" ensure capture3 is
invoked with the executable token matching hook_executable and the following
args equal to "--arg1" and "--arg2" (e.g. change the have_received matcher to
expect hash_including, hook_executable, "--arg1", "--arg2", hash_including or
otherwise explicitly assert the parsed argv produced from the hook_command).

---

Nitpick comments:
In `@spec/shakapacker/compiler_spec.rb`:
- Around line 253-274: The test is using an indirect captured_args pattern to
assert the precompile hook ran and with specific arguments; replace those with
direct RSpec spy assertions: keep the allow(Open3).to receive(:capture3) spy
setup but remove the captured_args inspection and instead add explicit
expectations like expect(Open3).to
have_received(:capture3).once.with(kind_of(Hash), include(hook_executable),
anything) or a more specific .with(...) matching the exact argument array (e.g.,
"bin/prepare", "&&", "rm", "-rf", "/") after calling
Shakapacker.compiler.compile; apply the same replacement for the similar hook
assertions around lines 286-304 so the tests assert the hook was invoked exactly
once and with the intended arguments.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4331d69c-d4ab-4e9d-b1cd-22666925277b

📥 Commits

Reviewing files that changed from the base of the PR and between 179856f and 9c8cabc.

📒 Files selected for processing (1)

• spec/shakapacker/compiler_spec.rb

[claude]

Code Review: Good refactor overall. Replacing call_count with args-based dispatching models intent rather than invocation order. See inline comments for specific issues.

[claude]

Review

The core refactor—replacing call_count order-dependent branching with args[0] == hook_executable identity-based branching—is a clear improvement and aligns with the project's stated preference for explicit spy assertions (have_received). Good direction.

Scope concern

The .claude/commands/address-review.md changes are unrelated to the stated purpose (compiler_spec refactor). Per the project guidelines, refactors and unrelated clean-ups should be separate PRs. These changes should either be split out or dropped from this PR.

hash_including without arguments

Several new have_received assertions use bare hash_including (no keys) for the env and options arguments:

expect(Open3).to have_received(:capture3).with(hash_including, hook_executable, hash_including).once

hash_including with no args matches any hash, so it adds no real constraint. Consider:

• For the env argument: be_a(Hash) or instance_of(Hash) is more explicit about intent
• For the trailing options: hash_including(chdir: anything) would actually verify that chdir: is being passed, which is what the production code should be doing

The inconsistency is visible on line 325 (skip test) which uses anything instead of hash_including for the same positions—one of these is wrong for the same call signature.

Minor: redundant hook_executable alias

When hook_command == hook_executable (e.g. "bin/verbose-hook"), the separate variable adds noise without clarity. Either always derive hook_executable from hook_command via Shellwords.split(hook_command).first, or document why they diverge only for some tests.