fix(task-planner): add verify command shell safety rules#126
fix(task-planner): add verify command shell safety rules#126kavanaghpatrick wants to merge 1 commit intotzachbon:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a mandatory "Verify Command Shell Safety" section to task-planner docs requiring the task-completed-gate sanitizer to validate all Verify commands, allow only safe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/ralph-specum/agents/task-planner.md (2)
302-304: Add language specification to the fenced code block.The static analysis tool flagged this code block as missing a language specification. Add
shellorbashto improve syntax highlighting and documentation clarity.📝 Proposed fix
-``` +```shell pnpm lint && pnpm test && pnpm build</details> As per static analysis hints (markdownlint-cli2). <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/ralph-specum/agents/task-planner.mdaround lines 302 - 304, The
fenced code block containing the shell command "pnpm lint && pnpm test && pnpm
build" is missing a language hint; update that fenced block to specify a shell
language (e.g., change the opening fence fromtoshell orbash) so the block becomesshell followed by the existing command and the closing ```,
which satisfies markdownlint-cli2 and improves syntax highlighting.</details> --- `286-286`: **Document the sanitizer version or commit to prevent documentation drift.** The rules reference a "task-completed-gate sanitizer" that exists in an external repository (kavanaghpatrick/ralph-parallel per PR objectives). Without version pinning or validation, this documentation could become outdated if the sanitizer implementation changes. Consider: 1. **Add a reference** to the specific sanitizer commit/version these rules apply to 2. **Add a verification mechanism** - e.g., automated tests that validate these rules match the sanitizer's actual behavior 3. **Add a maintenance note** - remind maintainers to update both the sanitizer and this documentation together <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/agents/task-planner.md` at line 286, Add explicit versioning and maintenance guidance for the "task-completed-gate sanitizer": update the documentation line that mentions the task-completed-gate sanitizer to reference the exact commit SHA or release tag from kavanaghpatrick/ralph-parallel that these Verify rules were validated against; add a short note suggesting an automated verification step (e.g., a unit/integration test that runs the sanitizer against a representative set of Verify commands to ensure behavior matches the rules) and include a maintenance reminder to update this doc whenever the task-completed-gate sanitizer (or its file/class name) changes so documentation and sanitizer remain in sync. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@plugins/ralph-specum/agents/task-planner.md:
- Around line 302-304: The fenced code block containing the shell command "pnpm
lint && pnpm test && pnpm build" is missing a language hint; update that fenced
block to specify a shell language (e.g., change the opening fence fromtoshell orbash) so the block becomesshell followed by the existing
command and the closing ```, which satisfies markdownlint-cli2 and improves
syntax highlighting.- Line 286: Add explicit versioning and maintenance guidance for the
"task-completed-gate sanitizer": update the documentation line that mentions the
task-completed-gate sanitizer to reference the exact commit SHA or release tag
from kavanaghpatrick/ralph-parallel that these Verify rules were validated
against; add a short note suggesting an automated verification step (e.g., a
unit/integration test that runs the sanitizer against a representative set of
Verify commands to ensure behavior matches the rules) and include a maintenance
reminder to update this doc whenever the task-completed-gate sanitizer (or its
file/class name) changes so documentation and sanitizer remain in sync.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `ae693df8-965a-47da-b7ab-5525e87a7b44` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 67ba64ac7983ba562b1943f6af9f0be5164be03a and a94f4435ff22776ef8ae045ee90a4cb125d5b4e4. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `plugins/ralph-specum/agents/task-planner.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The task-completed-gate sanitizer validates all Verify commands, but the task-planner had no awareness of which shell operators are allowed vs forbidden. This caused generated verify commands with ;, |, or || to be rejected at task completion time. Adds a <mandatory> block documenting that && is allowed while ;, |, ||, backticks, $(), and .. are forbidden. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
kavanaghpatrick
force-pushed
the
fix/task-planner-shell-safety
branch
from
a94f443 to
d26fade
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ralph-specum/agents/task-planner.md`:
- Around line 557-579: The new mandatory task-completed-gate sanitizer forbids
pipes, semicolons, `||`, backticks, `$()`, and `..` in Verify commands, but this
file still contains legacy Verify examples using those operators; find
occurrences of Verify example blocks and replace any use of `;`, `|`, `||`,
backticks, `$()`, or `..` with safe `&&`-chained commands (e.g., convert `cmd1
|| cmd2`, `cmd1; cmd2`, or `cmd1 | cmd2` to `cmd1 && cmd2` and remove command
substitution/backticks), ensure monorepo examples use `cd <subdir> &&
<command>`, and update or remove any examples that cannot be expressed with only
`&&` so the document and the task-completed-gate sanitizer are consistent.
- Around line 574-576: Add a language tag to the fenced code block containing
the shell command "pnpm lint && pnpm test && pnpm build" by changing the opening
backticks to include a language (e.g., bash) so the block reads as a bash code
block; update the fence that currently is "```" to "```bash" to satisfy
markdownlint MD040 for the fenced block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c3733e5-e88e-4b79-b4d8-b4ce2f7d0b2f
📒 Files selected for processing (1)
plugins/ralph-specum/agents/task-planner.md
| <mandatory> | ||
| The task-completed-gate sanitizer validates all **Verify** commands before execution. | ||
|
|
||
| **ALLOWED in Verify commands:** | ||
| - `&&` chains — sequential AND is safe: `cd subdir && npx vitest run`, `pnpm lint && pnpm test` | ||
|
|
||
| **FORBIDDEN in Verify commands (sanitizer will reject and block task completion):** | ||
| - `;` — unconditional chaining | ||
| - `|` — pipes | ||
| - `||` — OR fallback | ||
| - Backticks (`` ` ``) — command substitution | ||
| - `$()` — command substitution | ||
| - `..` — path traversal | ||
|
|
||
| **Monorepo pattern:** Use `cd <subdir> && <command>` to run commands in subdirectories. | ||
|
|
||
| **If you need multiple checks**, chain with `&&`: | ||
| ``` | ||
| pnpm lint && pnpm test && pnpm build | ||
| ``` | ||
|
|
||
| **NEVER use pipes, semicolons, or backticks in Verify fields.** | ||
| </mandatory> |
There was a problem hiding this comment.
Inconsistent policy rollout: mandatory safety rules conflict with existing Verify examples in this doc
This new mandatory block is correct, but the file still contains earlier Verify examples using now-forbidden operators (notably pipes/OR/semicolon-style patterns). That contradiction can still cause the planner to emit sanitizer-rejected commands.
Please update or rewrite those legacy examples to comply with this policy (only && chaining), so the document is internally consistent end-to-end.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 574-574: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ralph-specum/agents/task-planner.md` around lines 557 - 579, The new
mandatory task-completed-gate sanitizer forbids pipes, semicolons, `||`,
backticks, `$()`, and `..` in Verify commands, but this file still contains
legacy Verify examples using those operators; find occurrences of Verify example
blocks and replace any use of `;`, `|`, `||`, backticks, `$()`, or `..` with
safe `&&`-chained commands (e.g., convert `cmd1 || cmd2`, `cmd1; cmd2`, or `cmd1
| cmd2` to `cmd1 && cmd2` and remove command substitution/backticks), ensure
monorepo examples use `cd <subdir> && <command>`, and update or remove any
examples that cannot be expressed with only `&&` so the document and the
task-completed-gate sanitizer are consistent.
| ``` | ||
| pnpm lint && pnpm test && pnpm build | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block
The fence at Line 574 should specify a language to satisfy markdownlint (MD040).
Suggested fix
-```
+```bash
pnpm lint && pnpm test && pnpm build</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 574-574: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @plugins/ralph-specum/agents/task-planner.md around lines 574 - 576, Add a
language tag to the fenced code block containing the shell command "pnpm lint &&
pnpm test && pnpm build" by changing the opening backticks to include a language
(e.g., bash) so the block reads as a bash code block; update the fence that
currently is "" to "bash" to satisfy markdownlint MD040 for the fenced
block.
</details>
<!-- fingerprinting:phantom:triton:hawk:075fe021-873a-47d6-9132-21083df19817 -->
<!-- This is an auto-generated comment by CodeRabbit -->
2 participants
Summary
<mandatory>block totask-planner.mddocumenting which shell operators thetask-completed-gatesanitizer allows vs rejects in Verify commands&&chains are safe and allowed (sequential AND — monorepocd subdir && cmd, multi-checklint && test);,|,||, backticks,$(),..remain forbiddenContext
The
ralph-parallelplugin'stask-completed-gate.shsanitizer validates Verify commands beforeeval. The task-planner agent had no awareness of these constraints, so it generated verify commands using operators the sanitizer rejects. This mismatch caused 30+ task completion failures in monorepo projects.Companion fix (sanitizer side): kavanaghpatrick/ralph-parallel@c12c66f
Test plan
&&for subdirectory patterns;,|,||, or backticks appear in generated Verify fields🤖 Generated with Claude Code
Summary by CodeRabbit