-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(core): allow intentional foreground sleep for backoff #4708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6f8e97b
846a02a
e95fa62
dcece9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1022,17 +1022,31 @@ 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; | ||
| isStandalone: boolean; | ||
| 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; | ||
|
|
@@ -1059,9 +1073,51 @@ export function detectBlockedSleepPattern(command: string): string | null { | |
| if (separator === null) return null; | ||
|
|
||
| const rest = separator.rest.trim(); | ||
| return rest | ||
| const isStandalone = !rest; | ||
| const description = rest | ||
| ? `sleep ${durationToken} followed by: ${rest}` | ||
| : `standalone sleep ${durationToken}`; | ||
| const trimmedComment = comment?.trim(); | ||
| if ( | ||
| isStandalone && | ||
| trimmedComment?.startsWith(INTENTIONAL_SLEEP_COMMENT_PREFIX) | ||
| ) { | ||
| const reason = getIntentionalSleepReason(trimmedComment); | ||
| if (reason === null) { | ||
| return { | ||
| description, | ||
| isStandalone, | ||
| 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, | ||
| isStandalone, | ||
| 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, isStandalone }; | ||
| } | ||
|
|
||
| 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The magic number 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 getIntentionalSleepReason(trimmedComment: string): string | null { | ||
| const reason = trimmedComment | ||
| .slice(INTENTIONAL_SLEEP_COMMENT_PREFIX.length) | ||
| .trim(); | ||
| return reason.length >= MIN_INTENTIONAL_SLEEP_REASON_LENGTH ? reason : null; | ||
| } | ||
|
|
||
| function parseSleepDurationToSeconds(token: string): number | null { | ||
|
|
@@ -1130,7 +1186,13 @@ function getSleepSequentialSeparator(suffix: string): { rest: string } | null { | |
| return null; | ||
| } | ||
|
|
||
| function trimTrailingShellComment(command: string): string { | ||
| function splitTrailingShellComment( | ||
| command: string, | ||
| keepCommandsAfterCommentNewline = true, | ||
| ): { | ||
| command: string; | ||
| comment: string | null; | ||
| } { | ||
| let inSingleQuote = false; | ||
| let inDoubleQuote = false; | ||
| let inBacktick = false; | ||
|
|
@@ -1216,11 +1278,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 || !keepCommandsAfterCommentNewline | ||
| ? 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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The refactoring of This is arguably more correct, but it is undocumented and has no test. Consider adding a test for — qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in dcece9f. I preserved the pre-existing |
||
| return splitTrailingShellComment(command, false).command; | ||
| } | ||
|
|
||
| function hasTopLevelTrailingBackgroundOperator(command: string): boolean { | ||
|
|
@@ -4285,15 +4361,21 @@ 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 ?? | ||
| (sleepPattern.isStandalone | ||
| ? '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).' | ||
| : 'The intentional-sleep escape hatch only applies to standalone sleep commands; split follow-up commands into a separate invocation.'); | ||
| return ( | ||
| `Blocked: ${sleepPattern}. ` + | ||
| `Blocked: ${sleepPattern.description}. ` + | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| ); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The
validateToolParamstests 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 returnsnullfromvalidateToolParams. 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.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
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 resetreturnsnullfromvalidateToolParams.