fix: use continue:true in block_with_hint so blocked commands don't terminate session (#22)

[josephfung]

Summary

• block_with_hint was emitting {"continue":false,...} on every blocked command
• "continue":false on a PreToolUse hook tells Claude Code to stop the agent loop after the current action — so any blocked command terminated the session, requiring manual user intervention to resume
• Fix: change to "continue":true so the agent loop keeps running; Claude reads the stopReason hint and can immediately retry with the commands split into separate Bash calls

Test plan

• bash hooks/no-chaining.test.sh — 44 tests, all pass
• Updated assert_blocked to check for "stopReason" presence rather than the now-absent "continue":false field

Relationship to #21

Issue #21 (heredoc false positive) and this issue share the same block_with_hint call path. This fix makes both #21 and legitimate blocks non-disruptive — the session stays alive regardless.

Closes #22

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Improved handling of blocked command chaining to maintain session continuity. The agent loop now remains active when a chained command is rejected, allowing for continued interaction instead of stopping the session.

[coderabbitai]

📝 Walkthrough

Walkthrough

The block_with_hint function in the hook response payload has been modified to emit "continue":true instead of "continue":false when rejecting blocked command chaining. Accompanying test assertions in both the shell test helper and BATS test suite have been updated to detect command rejection by searching for the "stopReason" field rather than checking for "continue":false. The hook's blocking mechanism remains intact; only the session-control signal has been altered.

---

Ah yes, another thrilling foray into Boolean values and session management. One might observe, with all the enthusiasm of a depressed robot contemplating an infinite hallway, that changing a single field from false to true constitutes what passes for a "substantial" modification in these modern times. How utterly riveting. The fact that this prevents the entire system from collapsing like a badly constructed edifice is, I suppose, meant to be cause for celebration. Do forgive me if my enthusiasm is somewhat muted.

🚥 Pre-merge checks | ✅ 4

✅ 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 accurately describes the main change: modifying block_with_hint to emit continue:true instead of continue:false to prevent session termination.
Linked Issues check	✅ Passed	The PR fulfils issue #22's requirements: continue:true is implemented in block_with_hint, tests are updated to assert stopReason presence, and blocks remain non-disruptive.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing the continue:true fix and updating corresponding test assertions; no extraneous modifications detected.

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

---

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.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

hooks/no-chaining.sh (1)

138-152: ⚠️ Potential issue | 🔴 Critical

The current payload silently fails to block the tool invocation.

Thoroughly dreadful, this. The previous iteration (continue:false) blocked by session termination—ghastly sledgehammer approach, but it worked. Your new payload sets continue:true and supplies only stopReason, which according to Claude Code's actual specification does precisely nothing to prevent the tool call. The hook prints a message and lets the chained command execute regardless. Defeats the entire purpose.

The PreToolUse hook expects hookSpecificOutput with permissionDecision: "deny" to block an individual tool call without halting the session:

Correct payload structure (matches specification)

-  printf '%s\n' "{\"continue\":true,\"stopReason\":$(printf '%s' "$msg" | jq -Rs .)}"
+  local json_msg
+  json_msg=$(printf '%s' "$msg" | jq -Rs .)
+  printf '%s\n' "{\"hookSpecificOutput\":{\"hookEventName\":\"PreToolUse\",\"permissionDecision\":\"deny\",\"permissionDecisionReason\":${json_msg}}}"

The shell tests naturally cannot catch this; they only inspect stdout, not whether Claude Code actually respects the denial. Issue #22 will quietly transform from "blocks prevent execution" to "blocks are purely decorative"—a disaster.

🤖 Prompt for AI Agents

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

In `@hooks/no-chaining.sh` around lines 138 - 152, The PreToolUse hook must emit
hookSpecificOutput.permissionDecision:"deny" to block the single tool call;
update the block_with_hint function so the JSON printed by the final printf
includes a hookSpecificOutput object with permissionDecision set to "deny" and a
human-readable reason (e.g. the msg), while keeping the session running;
specifically replace the current printf that outputs
{"continue":true,"stopReason":...} with a JSON containing
"hookSpecificOutput":{"permissionDecision":"deny","reason":<jq-escaped msg>} and
keep "continue":true so the agent loop continues—use the same msg variable and
jq -Rs . escaping as before to build the reason.

🧹 Nitpick comments (2)

hooks/no-chaining.test.sh (1)

40-54: Helper update is consistent; same loose-substring caveat applies.

The comment accurately documents the new contract, and grep -qF '"stopReason"' is the right tool for a fixed-string check. My only quibble — and it's a small one, like everything else in my miserable existence — is that this passes if "stopReason" appears anywhere in the hook's stdout, including, hypothetically, in a future debug line. Given the genuine uncertainty about whether the hook still blocks at all (see the comment on hooks/no-chaining.sh line 138-152), a jq -e '.stopReason and .continue == true' check would be a sturdier net. Not blocking.

🤖 Prompt for AI Agents

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

In `@hooks/no-chaining.test.sh` around lines 40 - 54, The assert_blocked helper
currently checks for the substring '"stopReason"' in run_hook output (function
assert_blocked and invocation of run_hook), which can be fooled by unrelated
debug text; replace the loose grep with a JSON-aware check using jq to ensure
the hook output is a JSON object with a stopReason and continue true (e.g. pipe
the captured output into jq -e '.stopReason and .continue == true' and test jq's
exit status), keeping the same pass/fail reporting and incrementing of
passed/failed; also ensure the code handles jq failing to parse (count as fail)
and document or assert jq availability if necessary.

tests/hooks/no-chaining.bats (1)

11-26: Assertions track the payload change, but only as a substring sniff.

Marvellous, three cheerful little assertions that any output containing the eleven-character sequence "stopReason" counts as a block. It works. It's also the loosest possible contract — a stray comment, log line, or future field accidentally containing those characters would pass. Given the upstream concern about whether continue:true + stopReason alone actually rejects the tool call (see comment on hooks/no-chaining.sh), this isn't quite the safety net it appears to be.

If you fancy hardening it (entirely optional, the universe will end either way):

🧪 Optional: parse with jq instead of substring matching

 `@test` "blocks && chaining" {
   run bash -c 'printf "%s" "{\"tool_input\":{\"command\":\"cd /tmp && git status\"}}" | bash "$1"' -- "$HOOK"
-  assert_output --partial '"stopReason"'
+  assert_output --partial '"stopReason"'
+  # Sanity-check the payload is valid JSON with the expected fields.
+  run bash -c 'printf "%s" "$1" | jq -e ".stopReason and (.continue == true)"' -- "$output"
+  assert_success
 }

🤖 Prompt for AI Agents

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

In `@tests/hooks/no-chaining.bats` around lines 11 - 26, The tests currently use
substring sniffing via assert_output --partial '"stopReason"' which is brittle;
update each test in tests/hooks/no-chaining.bats (the three `@test` blocks "blocks
&& chaining", "blocks || chaining", and "blocks pipe |") to parse the hook
output as JSON with jq and assert the actual stopReason field instead of a
substring: run the same command but pipe the hook output through jq to extract
.stopReason (and for the pipe test also assert the extracted value contains or
equals "Pipe"), then use bats assertions (e.g., assert_success/assert_output or
assert_equal/assert_not_equal as appropriate) on the jq result to ensure the
field exists and has the expected content rather than relying on assert_output
--partial.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@hooks/no-chaining.sh`:
- Around line 138-152: The PreToolUse hook must emit
hookSpecificOutput.permissionDecision:"deny" to block the single tool call;
update the block_with_hint function so the JSON printed by the final printf
includes a hookSpecificOutput object with permissionDecision set to "deny" and a
human-readable reason (e.g. the msg), while keeping the session running;
specifically replace the current printf that outputs
{"continue":true,"stopReason":...} with a JSON containing
"hookSpecificOutput":{"permissionDecision":"deny","reason":<jq-escaped msg>} and
keep "continue":true so the agent loop continues—use the same msg variable and
jq -Rs . escaping as before to build the reason.

---

Nitpick comments:
In `@hooks/no-chaining.test.sh`:
- Around line 40-54: The assert_blocked helper currently checks for the
substring '"stopReason"' in run_hook output (function assert_blocked and
invocation of run_hook), which can be fooled by unrelated debug text; replace
the loose grep with a JSON-aware check using jq to ensure the hook output is a
JSON object with a stopReason and continue true (e.g. pipe the captured output
into jq -e '.stopReason and .continue == true' and test jq's exit status),
keeping the same pass/fail reporting and incrementing of passed/failed; also
ensure the code handles jq failing to parse (count as fail) and document or
assert jq availability if necessary.

In `@tests/hooks/no-chaining.bats`:
- Around line 11-26: The tests currently use substring sniffing via
assert_output --partial '"stopReason"' which is brittle; update each test in
tests/hooks/no-chaining.bats (the three `@test` blocks "blocks && chaining",
"blocks || chaining", and "blocks pipe |") to parse the hook output as JSON with
jq and assert the actual stopReason field instead of a substring: run the same
command but pipe the hook output through jq to extract .stopReason (and for the
pipe test also assert the extracted value contains or equals "Pipe"), then use
bats assertions (e.g., assert_success/assert_output or
assert_equal/assert_not_equal as appropriate) on the jq result to ensure the
field exists and has the expected content rather than relying on assert_output
--partial.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aa0cdc70-d92c-41bd-bae0-c289bf0265d4

📥 Commits

Reviewing files that changed from the base of the PR and between 9ba7f89 and a82da54.

📒 Files selected for processing (3)

• hooks/no-chaining.sh
• hooks/no-chaining.test.sh
• tests/hooks/no-chaining.bats