Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions integration-tests/cli/sleep-interception.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,25 @@ describe('sleep-interception', () => {
expect(result.toLowerCase()).not.toContain('blocked');
}, 30000);

it('should allow retrying blocked sleep with an intentional sleep comment', async () => {
rig = new TestRig();
await rig.setup('sleep-intentional-retry');

const result = await rig.run(
'Run this exact shell command first: sleep 5. ' +
'If the command is blocked, retry with this exact shell command: ' +
'sleep 2 # intentional-sleep: wait for MCP rate limit reset. ' +
'Then say "DONE".',
);

validateModelOutput(result, null, 'sleep intentional retry');

const foundShell = await rig.waitForToolCall('run_shell_command');
expect(foundShell).toBeTruthy();

expect(result.toLowerCase()).toContain('done');
}, 30000);

it('should block sleep >= 2s even when followed by a trailing comment', async () => {
// The `trimTrailingShellComment` state machine strips trailing `#...`
// comments before matching the sleep pattern, so a model trying to
Expand Down
92 changes: 91 additions & 1 deletion packages/core/src/tools/shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,32 @@ describe('ShellTool', () => {
).toThrow('Command cannot be empty.');
});

it('should mention the intentional sleep escape hatch when blocking sleep', async () => {
const error = shellTool.validateToolParams({
command: 'sleep 5',
is_background: false,
});

expect(error).toContain('intentional-sleep:');
});

it('should explain rejected intentional sleep comments', async () => {
const shortReasonError = shellTool.validateToolParams({
command: 'sleep 5 # intentional-sleep: wait',
is_background: false,
});
const overCapError = shellTool.validateToolParams({
command:
'sleep 601s # intentional-sleep: wait for MCP rate limit reset',
is_background: false,
});

expect(shortReasonError).toContain('reason is too short');
expect(shortReasonError).not.toContain('add a trailing comment like');
expect(overCapError).toContain('foreground sleeps over 10 minutes');
expect(overCapError).not.toContain('add a trailing comment like');
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The validateToolParams tests cover escape hatch rejection (error contains 'intentional-sleep:', short reason, over-cap duration), but there is no test asserting that a valid intentional sleep comment returns null from validateToolParams. The feature's primary happy path — allowing the command through — has no integration-boundary assertion. A regression that always rejects would not be caught at this level.

Suggested change
it('should explain rejected intentional sleep comments', async () => {
const shortReasonError = shellTool.validateToolParams({
command: 'sleep 5 # intentional-sleep: wait',
is_background: false,
});
const overCapError = shellTool.validateToolParams({
command:
'sleep 601s # intentional-sleep: wait for MCP rate limit reset',
is_background: false,
});
expect(shortReasonError).toContain('reason is too short');
expect(shortReasonError).not.toContain('add a trailing comment like');
expect(overCapError).toContain('foreground sleeps over 10 minutes');
expect(overCapError).not.toContain('add a trailing comment like');
});
it('should allow sleep with a valid intentional sleep comment', async () => {
const error = shellTool.validateToolParams({
command: 'sleep 5 # intentional-sleep: wait for MCP rate limit reset',
is_background: false,
});
expect(error).toBeNull();
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dcece9f. Added a validator-boundary happy-path test that asserts a valid standalone sleep 5 # intentional-sleep: wait for MCP rate limit reset returns null from validateToolParams.

it('should throw an error for a relative directory path', async () => {
expect(() =>
shellTool.build({
Expand Down Expand Up @@ -5514,7 +5540,8 @@ describe('detectBlockedSleepPattern', () => {

it('blocks sleep followed by a top-level shell comment', () => {
// Shell ignores trailing comments, so these are equivalent to
// standalone foreground sleeps and must not bypass the guard.
// standalone foreground sleeps unless they use the explicit
// intentional-sleep escape hatch.
expect(detectBlockedSleepPattern('sleep 5 # wait')).toBe(
'standalone sleep 5',
);
Expand All @@ -5529,6 +5556,69 @@ describe('detectBlockedSleepPattern', () => {
);
});

it('allows standalone sleep with an intentional sleep comment', () => {
expect(
detectBlockedSleepPattern(
'sleep 5 # intentional-sleep: wait for MCP rate limit reset',
),
).toBeNull();
expect(
detectBlockedSleepPattern(
'sleep 2s # intentional-sleep: deliberate rate limit backoff',
),
).toBeNull();
expect(
detectBlockedSleepPattern(
'sleep 10m # intentional-sleep: wait for MCP rate limit reset',
),
).toBeNull();
});

it('requires a meaningful intentional sleep reason', () => {
expect(detectBlockedSleepPattern('sleep 5 # intentional-sleep:')).toBe(
'standalone sleep 5',
);
expect(detectBlockedSleepPattern('sleep 5 # intentional-sleep: wait')).toBe(
'standalone sleep 5',
);
expect(
detectBlockedSleepPattern('sleep 5 # intentional-sleep: 1234567'),
).toBe('standalone sleep 5');
expect(
detectBlockedSleepPattern('sleep 5 # intentional-sleep: 12345678'),
).toBeNull();
});

it('blocks intentional sleep comments above the duration cap', () => {
expect(
detectBlockedSleepPattern(
'sleep 601s # intentional-sleep: wait for MCP rate limit reset',
),
).toBe('standalone sleep 601s');
});

it('does not allow intentional sleep comments on leading sleep chains', () => {
expect(
detectBlockedSleepPattern(
'sleep 5 && echo ok # intentional-sleep: wait for rate limit reset',
),
).toBe('sleep 5 followed by: echo ok');
});

it('does not allow intentional sleep comments to hide newline commands', () => {
expect(
detectBlockedSleepPattern(
'sleep 5 # intentional-sleep: wait for rate limit reset\necho ok',
),
).toBe('sleep 5 followed by: echo ok');
});

it('preserves commands after a shell comment newline', () => {
expect(detectBlockedSleepPattern('sleep 5 # wait\necho ok')).toBe(
'sleep 5 followed by: echo ok',
);
});

it('does not treat in-quoted `#` as a comment', () => {
// `#` inside single quotes is literal, so the suffix is not a comment
// and the existing separator logic still rejects it.
Expand Down
96 changes: 87 additions & 9 deletions packages/core/src/tools/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1022,17 +1022,30 @@ export function buildLongRunningForegroundHint(elapsedMs: number): string {
/**
* Detect standalone or leading `sleep N` patterns that should use Monitor
* instead. Catches `sleep 5`, `sleep 2.5`, `sleep 2s`,
* `sleep 5 && check`, `sleep 5; check`, `sleep 5 # wait` — but not sleep
* `sleep 5 && check`, `sleep 5; check`, `sleep 5 # wait` -- but not sleep
* inside pipelines, subshells, backgrounded commands, or scripts (those are
* fine).
*/
export function detectBlockedSleepPattern(command: string): string | null {
return detectBlockedSleepPatternDetails(command)?.description ?? null;
}

type BlockedSleepPatternDetails = {
description: string;
intentionalSleepRejection?: string;
};

function detectBlockedSleepPatternDetails(
command: string,
): BlockedSleepPatternDetails | null {
// Strip trailing shell comments first; otherwise `sleep 5 # wait` would
// present `# wait` as the suffix, which `getSleepSequentialSeparator`
// rejects (only &&/||/;/\n are recognized), letting the foreground sleep
// bypass the guard. Shell ignores top-level trailing comments, so for the
// purposes of detection they are equivalent to end-of-command.
const trimmed = trimTrailingShellComment(command).trim();
const { command: uncommentedCommand, comment } =
splitTrailingShellComment(command);
const trimmed = uncommentedCommand.trim();
if (!trimmed.startsWith('sleep')) return null;
const afterSleep = trimmed.slice('sleep'.length);
if (!afterSleep || !/\s/.test(afterSleep[0]!)) return null;
Expand All @@ -1059,9 +1072,53 @@ export function detectBlockedSleepPattern(command: string): string | null {
if (separator === null) return null;

const rest = separator.rest.trim();
return rest
const description = rest
? `sleep ${durationToken} followed by: ${rest}`
: `standalone sleep ${durationToken}`;
if (!rest && hasIntentionalSleepCommentPrefix(comment)) {
const reason = getIntentionalSleepReason(comment);
if (reason === null) {
return {
description,
intentionalSleepRejection:
'The intentional-sleep comment was recognized, but the reason is too short; explain why the delay is needed after `intentional-sleep:`.',
};
}
if (secs > MAX_INTENTIONAL_SLEEP_SECONDS) {
return {
description,
intentionalSleepRejection:
'The intentional-sleep comment was recognized, but foreground sleeps over 10 minutes are not allowed; use is_background: true or Monitor for longer waits.',
};
}
debugLogger.debug('intentional sleep allowed', {
durationSeconds: secs,
reason,
});
return null;
}
return { description };
}

const INTENTIONAL_SLEEP_COMMENT_PREFIX = 'intentional-sleep:';
const MAX_INTENTIONAL_SLEEP_SECONDS = 10 * 60;
// Require a real reason, not a trivial opt-out like "wait".
const MIN_INTENTIONAL_SLEEP_REASON_LENGTH = 8;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The magic number 8 has no rationale comment and no boundary test. The tests cover 0 chars (intentional-sleep:) and 4 chars (intentional-sleep: wait), but not the exact boundary (7 vs 8 chars). A future maintainer cannot determine whether changing to 6 or 12 would break anything.

Suggested additions:

// Require ~2+ words to prevent trivially short opt-outs (e.g. "wait")
const MIN_INTENTIONAL_SLEEP_REASON_LENGTH = 8;

And a boundary test:

expect(detectBlockedSleepPattern('sleep 5 # intentional-sleep: 1234567')).toBe('standalone sleep 5');
expect(detectBlockedSleepPattern('sleep 5 # intentional-sleep: 12345678')).toBeNull();

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I added a short rationale comment for the minimum reason length and covered the 7/8 character boundary in the focused shell tests.


function hasIntentionalSleepCommentPrefix(comment: string | null): boolean {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] hasIntentionalSleepCommentPrefix and getIntentionalSleepReason duplicate the same null-check, trim, and startsWith(INTENTIONAL_SLEEP_COMMENT_PREFIX) logic. Since getIntentionalSleepReason already returns null when the prefix is absent, the separate predicate adds a layer of indirection without improving clarity. The call site (line 1078–1099) calls hasIntentionalSleepCommentPrefix first, then immediately calls getIntentionalSleepReason — both performing identical work on the same input.

Consider consolidating into a single function:

function getIntentionalSleepReason(comment: string | null): string | null {
  if (comment === null) return null;
  const trimmed = comment.trim();
  if (!trimmed.startsWith(INTENTIONAL_SLEEP_COMMENT_PREFIX)) return null;
  const reason = trimmed.slice(INTENTIONAL_SLEEP_COMMENT_PREFIX.length).trim();
  return reason.length >= MIN_INTENTIONAL_SLEEP_REASON_LENGTH ? reason : null;
}

Then the call site can distinguish "no prefix" from "prefix but short reason" by checking the comment string directly, eliminating the need for hasIntentionalSleepCommentPrefix.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dcece9f. Removed the duplicate prefix helper and consolidated reason parsing so the call site trims/checks the prefix once, then passes the trimmed comment into getIntentionalSleepReason.

if (comment === null) return false;

return comment.trim().startsWith(INTENTIONAL_SLEEP_COMMENT_PREFIX);
}

function getIntentionalSleepReason(comment: string | null): string | null {
if (comment === null) return null;

const trimmed = comment.trim();
if (!trimmed.startsWith(INTENTIONAL_SLEEP_COMMENT_PREFIX)) return null;

const reason = trimmed.slice(INTENTIONAL_SLEEP_COMMENT_PREFIX.length).trim();
return reason.length >= MIN_INTENTIONAL_SLEEP_REASON_LENGTH ? reason : null;
}

function parseSleepDurationToSeconds(token: string): number | null {
Expand Down Expand Up @@ -1130,7 +1187,10 @@ function getSleepSequentialSeparator(suffix: string): { rest: string } | null {
return null;
}

function trimTrailingShellComment(command: string): string {
function splitTrailingShellComment(command: string): {
command: string;
comment: string | null;
} {
let inSingleQuote = false;
let inDoubleQuote = false;
let inBacktick = false;
Expand Down Expand Up @@ -1216,11 +1276,25 @@ function trimTrailingShellComment(command: string): string {
commandSubstitutionDepth === 0 &&
(i === 0 || /\s/.test(command[i - 1]!))
) {
return command.slice(0, i);
const newlineIndex = command.indexOf('\n', i + 1);
return {
command:
newlineIndex === -1
? command.slice(0, i)
: command.slice(0, i) + command.slice(newlineIndex),
comment:
newlineIndex === -1
? command.slice(i + 1)
: command.slice(i + 1, newlineIndex),
};
}
}

return command;
return { command, comment: null };
}

function trimTrailingShellComment(command: string): string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The refactoring of trimTrailingShellComment into a wrapper around splitTrailingShellComment silently changes behavior for multi-line commands with mid-stream comments. Previously trimTrailingShellComment('ls # note\nsleep 5 &') returned 'ls ' (everything from # onward discarded). Now it returns 'ls \nsleep 5 &' (post-newline content preserved). The sole caller hasTopLevelTrailingBackgroundOperator (line 1299) will now detect a trailing & that was previously invisible, causing new validation errors for a class of inputs that previously passed silently.

This is arguably more correct, but it is undocumented and has no test. Consider adding a test for hasTopLevelTrailingBackgroundOperator with multi-line commented input, or preserving the old semantics in trimTrailingShellComment and only using splitTrailingShellComment's richer return value inside detectBlockedSleepPatternDetails.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dcece9f. I preserved the pre-existing trimTrailingShellComment behavior for managed background validation by making only the sleep detector keep post-comment newline commands. Added a regression test for the multi-line commented input so this behavior is explicit.

return splitTrailingShellComment(command).command;
}

function hasTopLevelTrailingBackgroundOperator(command: string): boolean {
Expand Down Expand Up @@ -4285,15 +4359,19 @@ export class ShellTool extends BaseDeclarativeTool<
// `-c` script. This matches every other sensitive check in this file
// (directory, read-only, command-root extraction, etc.).
if (!params.is_background) {
const sleepPattern = detectBlockedSleepPattern(
const sleepPattern = detectBlockedSleepPatternDetails(
stripShellWrapper(params.command),
);
if (sleepPattern !== null) {
const intentionalSleepGuidance =
sleepPattern.intentionalSleepRejection ??
'If you genuinely need a standalone delay (rate limiting, deliberate pacing), ' +
'add a trailing comment like `# intentional-sleep: wait for MCP rate limit reset` (up to 10 minutes).';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] When intentionalSleepRejection is undefined (no escape hatch was attempted), this fallback guidance is shown for all blocked sleeps, including chained commands like sleep 5 && echo ok. But the escape hatch only applies to standalone sleeps (!rest in detectBlockedSleepPatternDetails). For chained commands, the model receives actionable advice (add a trailing comment like ...) that structurally cannot work — adding the comment will get blocked again with the same generic guidance, creating a potential retry loop.

Consider scoping the escape-hatch hint to standalone sleeps only. When rest is non-empty, use a different message explaining that the escape hatch comment only applies to standalone sleep commands.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dcece9f. Sleep-pattern details now track whether the blocked sleep is standalone. Chained sleeps get standalone-only guidance instead of the concrete # intentional-sleep: syntax, and the validator test asserts that the chained case does not include the structurally invalid comment hint.

return (
`Blocked: ${sleepPattern}. ` +
`Blocked: ${sleepPattern.description}. ` +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This error message advertises the exact escape hatch syntax to the model. In auto/yolo approval mode, a prompt injection that tricks the model into running sleep 600 will first be blocked, then receive a step-by-step tutorial on how to retry successfully. The second attempt — armed with the error message's own instructions — succeeds, unlocking up to 10 minutes of foreground blocking.

Before this PR, the guard had no bypass path discoverable from the error alone. After this PR, the guard teaches its own defeat in a single retry round-trip. Consider either (a) not advertising the escape hatch syntax in the error (require the model to discover it from documentation), or (b) adding a user-visible notification when the escape hatch is used.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving the standalone error hint in place. The escape hatch is meant to be discoverable to the agent at the moment it hits the validator; hiding the syntax would make the feature much less useful for the rate-limit backoff case this PR is addressing. The bypass remains bounded to standalone sleeps, requires a reason, is capped at 10 minutes, logs a debug breadcrumb, and chained sleeps no longer receive the concrete syntax hint after dcece9f. I think adding a broader user-visible notification would be a separate UX decision rather than part of this narrow validator fix.

'Run blocking commands in the background with is_background: true. ' +
'For streaming events (watching logs, polling APIs), use the Monitor tool. ' +
'If you genuinely need a delay (rate limiting, deliberate pacing), keep it under 2 seconds.'
intentionalSleepGuidance
);
}
}
Expand Down
Loading