hotfix: append "do not commit your changes." to sandbox prompts#142
Merged
sweetmantech merged 1 commit intomainfrom Apr 15, 2026
Merged
hotfix: append "do not commit your changes." to sandbox prompts#142sweetmantech merged 1 commit intomainfrom
sweetmantech merged 1 commit intomainfrom
Conversation
run-sandbox-command now rewrites the --message value passed to openclaw so it always ends with "do not commit your changes." regardless of what the caller sends. The helper is idempotent and a no-op when --message isn't in the args, so other consumers of the task are unaffected. The task pushes sandbox state to GitHub after the run; we don't want the agent creating its own commits during execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughA new module exports a constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/sandboxes/__tests__/appendNoCommitInstruction.test.ts (1)
23-31: Add a regression test for “instruction present but not suffixed”.Current coverage doesn’t catch cases where the phrase exists mid-message. Add one test to enforce true suffix behavior.
Suggested test addition
+ it("appends when instruction appears in the middle but is not the suffix", () => { + const original = [ + "agent", + "--message", + `${NO_COMMIT_INSTRUCTION} and then continue`, + ]; + + expect(appendNoCommitInstruction(original)).toEqual([ + "agent", + "--message", + `${NO_COMMIT_INSTRUCTION} and then continue ${NO_COMMIT_INSTRUCTION}`, + ]); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/__tests__/appendNoCommitInstruction.test.ts` around lines 23 - 31, Add a new unit test to cover the case where NO_COMMIT_INSTRUCTION appears inside a message but is not the trailing suffix: create an original args array whose last string contains `${NO_COMMIT_INSTRUCTION}` in the middle (e.g., "do something ${NO_COMMIT_INSTRUCTION} please"), call appendNoCommitInstruction(original) and assert that the returned array has the instruction appended (i.e., the function treats the mid-message occurrence as not-suffixed and adds the NO_COMMIT_INSTRUCTION as the proper trailing element). This ensures appendNoCommitInstruction correctly checks for a true suffix rather than any substring match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sandboxes/appendNoCommitInstruction.ts`:
- Around line 24-26: The early return uses
current.includes(NO_COMMIT_INSTRUCTION) which matches substrings anywhere;
change the check to ensure the message actually ends with the instruction (e.g.,
use current.trim().endsWith(NO_COMMIT_INSTRUCTION) or an anchored regex) so you
only skip when the instruction is the suffix. Keep the append logic that sets
out[valueIdx] = `${current} ${NO_COMMIT_INSTRUCTION}` but ensure you trim/trail
whitespace appropriately so you don't introduce double spaces when adding the
suffix.
In `@src/tasks/runSandboxCommandTask.ts`:
- Around line 78-80: The code currently applies appendNoCommitInstruction to
every sandbox.runCommand invocation, which can mutate unrelated commands; change
the args passed to sandbox.runCommand so appendNoCommitInstruction is only
applied when invoking the OpenClaw agent (check the command and first arg).
Concretely, compute finalArgs = (command === 'openclaw' && Array.isArray(args)
&& args[0] === 'agent') ? appendNoCommitInstruction(args) : (args || []), then
pass finalArgs into sandbox.runCommand instead of always calling
appendNoCommitInstruction(args || []).
---
Nitpick comments:
In `@src/sandboxes/__tests__/appendNoCommitInstruction.test.ts`:
- Around line 23-31: Add a new unit test to cover the case where
NO_COMMIT_INSTRUCTION appears inside a message but is not the trailing suffix:
create an original args array whose last string contains
`${NO_COMMIT_INSTRUCTION}` in the middle (e.g., "do something
${NO_COMMIT_INSTRUCTION} please"), call appendNoCommitInstruction(original) and
assert that the returned array has the instruction appended (i.e., the function
treats the mid-message occurrence as not-suffixed and adds the
NO_COMMIT_INSTRUCTION as the proper trailing element). This ensures
appendNoCommitInstruction correctly checks for a true suffix rather than any
substring match.
🪄 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: a2da7669-b5d5-4a12-81cd-2567d1f84625
📒 Files selected for processing (3)
src/sandboxes/__tests__/appendNoCommitInstruction.test.tssrc/sandboxes/appendNoCommitInstruction.tssrc/tasks/runSandboxCommandTask.ts
Comment on lines
+24
to
+26
| if (current.includes(NO_COMMIT_INSTRUCTION)) return out; | ||
|
|
||
| out[valueIdx] = `${current} ${NO_COMMIT_INSTRUCTION}`; |
There was a problem hiding this comment.
includes() does not enforce the required suffix behavior.
At Line [24], the early return triggers even when the instruction is only a substring, so the message may still not end with the required text.
Proposed fix
- const current = out[valueIdx];
- if (current.includes(NO_COMMIT_INSTRUCTION)) return out;
-
- out[valueIdx] = `${current} ${NO_COMMIT_INSTRUCTION}`;
+ const current = out[valueIdx].trimEnd();
+ if (current.endsWith(NO_COMMIT_INSTRUCTION)) return out;
+
+ out[valueIdx] = current.length
+ ? `${current} ${NO_COMMIT_INSTRUCTION}`
+ : NO_COMMIT_INSTRUCTION;
return out;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (current.includes(NO_COMMIT_INSTRUCTION)) return out; | |
| out[valueIdx] = `${current} ${NO_COMMIT_INSTRUCTION}`; | |
| const current = out[valueIdx].trimEnd(); | |
| if (current.endsWith(NO_COMMIT_INSTRUCTION)) return out; | |
| out[valueIdx] = current.length | |
| ? `${current} ${NO_COMMIT_INSTRUCTION}` | |
| : NO_COMMIT_INSTRUCTION; | |
| return out; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/appendNoCommitInstruction.ts` around lines 24 - 26, The early
return uses current.includes(NO_COMMIT_INSTRUCTION) which matches substrings
anywhere; change the check to ensure the message actually ends with the
instruction (e.g., use current.trim().endsWith(NO_COMMIT_INSTRUCTION) or an
anchored regex) so you only skip when the instruction is the suffix. Keep the
append logic that sets out[valueIdx] = `${current} ${NO_COMMIT_INSTRUCTION}` but
ensure you trim/trail whitespace appropriately so you don't introduce double
spaces when adding the suffix.
Comment on lines
78
to
+80
| const commandResult = await sandbox.runCommand({ | ||
| cmd: command, | ||
| args: args || [], | ||
| args: appendNoCommitInstruction(args || []), |
There was a problem hiding this comment.
Limit prompt mutation to OpenClaw agent invocations only.
At Line [80], appendNoCommitInstruction is applied to every command’s args, not just openclaw agent. This can unexpectedly rewrite unrelated --message flags.
Proposed fix
- const commandResult = await sandbox.runCommand({
+ const normalizedArgs = args ?? [];
+ const shouldAppendNoCommit =
+ command === "openclaw" && normalizedArgs[0] === "agent";
+
+ const commandResult = await sandbox.runCommand({
cmd: command,
- args: appendNoCommitInstruction(args || []),
+ args: shouldAppendNoCommit
+ ? appendNoCommitInstruction(normalizedArgs)
+ : normalizedArgs,
cwd,
env: getSandboxEnv(accountId),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const commandResult = await sandbox.runCommand({ | |
| cmd: command, | |
| args: args || [], | |
| args: appendNoCommitInstruction(args || []), | |
| const normalizedArgs = args ?? []; | |
| const shouldAppendNoCommit = | |
| command === "openclaw" && normalizedArgs[0] === "agent"; | |
| const commandResult = await sandbox.runCommand({ | |
| cmd: command, | |
| args: shouldAppendNoCommit | |
| ? appendNoCommitInstruction(normalizedArgs) | |
| : normalizedArgs, | |
| cwd, | |
| env: getSandboxEnv(accountId), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/runSandboxCommandTask.ts` around lines 78 - 80, The code currently
applies appendNoCommitInstruction to every sandbox.runCommand invocation, which
can mutate unrelated commands; change the args passed to sandbox.runCommand so
appendNoCommitInstruction is only applied when invoking the OpenClaw agent
(check the command and first arg). Concretely, compute finalArgs = (command ===
'openclaw' && Array.isArray(args) && args[0] === 'agent') ?
appendNoCommitInstruction(args) : (args || []), then pass finalArgs into
sandbox.runCommand instead of always calling appendNoCommitInstruction(args ||
[]).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
The task pushes sandbox state to GitHub after the run (`pushSandboxToGithub`). We don't want the agent itself creating commits during execution — it breaks the snapshot flow and can cause force-push / divergence issues.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests
Summary by cubic
Hotfix guardrail: sandbox commands now always append "do not commit your changes." to
--messageprompts so the agent doesn’t create commits during execution and break snapshot flow.appendNoCommitInstruction(args)helper (idempotent; no-op without--message; returns a copy).runSandboxCommandTaskto rewrite args beforesandbox.runCommand.Written for commit a364400. Summary will update on new commits.