Skip to content

fix(core): halt repeated shell inspection variants #5944

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.

Sign up for GitHub

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

Merged
yiliang114 merged 9 commits into from
Jun 28, 2026
+461 −16
Merged

fix(core): halt repeated shell inspection variants #5944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
91ec374
fix(core): halt repeated shell inspection variants
yiliang114 Jun 27, 2026
5aaec99
test(core): cover shell-stagnation streak reset, fix guard-count doc
yiliang114 Jun 28, 2026
2fd9ad0
fix(core): exclude write-bearing git chains from stagnation bucket
yiliang114 Jun 28, 2026
08bd62d
fix(core): halt repeated shell inspection variants
yiliang114 Jun 27, 2026
380782e
Merge remote review refinements
yiliang114 Jun 28, 2026
c2c73a7
fix(core): treat path-specific git diffs as progress
yiliang114 Jun 28, 2026
d8ab43f
test(core): cover shell stagnation guard boundaries
yiliang114 Jun 28, 2026
9f50530
test(core): address shell stagnation review feedback
yiliang114 Jun 28, 2026
f86b74e
docs(core): sync loop guard comments
yiliang114 Jun 28, 2026
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
3 changes: 3 additions & 0 deletions packages/cli/src/nonInteractiveCli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ const LOOP_TYPE_LABELS: Record<LoopType, string> = {
'the model spent too many consecutive calls reading files without making progress',
[LoopType.ACTION_STAGNATION]:
'the model kept calling the same tool without making progress',
[LoopType.SHELL_COMMAND_STAGNATION]:
'the model repeated similar shell inspection commands without making progress',
[LoopType.GLOBAL_TOOL_CALL_DUPLICATE]:
'the model repeated the same tool call across the turn, even when not back-to-back',
[LoopType.ALTERNATING_TOOL_CALL_PATTERN]:
Expand All @@ -129,6 +131,7 @@ function formatLoopDetectedMessage(loopType: LoopType | undefined): string {
const isAlwaysOn =
loopType === LoopType.TURN_TOOL_CALL_CAP ||
loopType === LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS ||
loopType === LoopType.SHELL_COMMAND_STAGNATION ||

@qwen-code-ci-bot qwen-code-ci-bot Jun 28, 2026

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.

[Critical] SHELL_COMMAND_STAGNATION is correctly added to isAlwaysOn here, but nonInteractiveCli.test.ts has no test verifying the user-facing message for this new type. Existing always-on types (CONSECUTIVE_IDENTICAL_TOOL_CALLS, TURN_TOOL_CALL_CAP) have analogous tests asserting that the "always-on guard" hint appears and the skipLoopDetection escape hatch does NOT. Without a parallel test, a future refactor could accidentally remove the new type from isAlwaysOn and no test would catch the regression.

Consider adding a test analogous to the existing always-on assertions:

it('shows the always-on hint for shell command stagnation loop type', async () => {
  // ... setup ...
  expect(processStderrSpy).toHaveBeenCalledWith(
    expect.stringContaining('always-on guard and cannot be disabled via `model.skipLoopDetection`'),
  );
  expect(processStderrSpy).not.toHaveBeenCalledWith(
    expect.stringContaining('Set the `model.skipLoopDetection` setting to true'),
  );
});

— qwen3.7-max via Qwen Code /review

@yiliang114 yiliang114 Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I do not think this is a critical correctness issue for the current PR. The runtime behavior is already wired correctly: SHELL_COMMAND_STAGNATION is included in the always-on branch, so the headless message does not suggest model.skipLoopDetection as an escape hatch for this loop type.

This is a reasonable regression-test hardening suggestion, but it is not blocking for the fix. We can follow up with a small dedicated nonInteractiveCli.test.ts assertion that locks the user-facing message for SHELL_COMMAND_STAGNATION.

loopType === LoopType.GLOBAL_TOOL_CALL_DUPLICATE;
const hint = isAlwaysOn
? ' This is an always-on guard and cannot be disabled via `model.skipLoopDetection`.'
Expand Down
14 changes: 8 additions & 6 deletions packages/core/src/core/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2143,9 +2143,10 @@ export class GeminiClient {
didUpdateIdeContextState = true;
}

// Always-on safety checks (consecutive-identical tool-call guard +
// per-turn tool-call cap). These fire before the skipLoopDetection
// gate so they cannot be bypassed by configuration.
// Always-on safety checks (consecutive-identical tool-call guard,
// shell inspection stagnation, and per-turn tool-call cap). These fire
// before the skipLoopDetection gate so they cannot be bypassed by
// configuration.
const alwaysOnLoop = this.loopDetector.checkAlwaysOnSafeties(event);
if (alwaysOnLoop) {
// Drop every tool call collected before the guard fired so the run
Expand Down Expand Up @@ -2175,9 +2176,10 @@ export class GeminiClient {
// interruptions. Only the historically false-positive-prone heuristics
// (content/thought repetition, read-file and action stagnation,
// global-duplicate and alternating tool-call patterns) sit behind this
// flag. The precise consecutive-identical guard and the per-turn cap
// run unconditionally in checkAlwaysOnSafeties above, so the documented
// escape hatch only relaxes the heuristics (see nonInteractiveCli.ts).
// flag. The precise consecutive-identical guard, shell inspection
// stagnation guard, and per-turn cap run unconditionally in
// checkAlwaysOnSafeties above, so the documented escape hatch only
// relaxes the heuristics (see nonInteractiveCli.ts).
const skipLoopDetection = this.config.getSkipLoopDetection();
const heuristicLoop =
!skipLoopDetection &&
Expand Down
308 changes: 308 additions & 0 deletions packages/core/src/services/loopDetectionService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const CONTENT_CHUNK_SIZE = 50;
// self-describing and failures point to the constant that changed.
const FILE_READ_WINDOW = 15;
const GLOBAL_DUPLICATE_THRESHOLD = 6;
const SHELL_COMMAND_STAGNATION_THRESHOLD = 8;
const ALTERNATING_PATTERN_CYCLES = 3;
const TURN_TOOL_CALL_CAP = 100;

Expand Down Expand Up @@ -221,6 +222,313 @@ describe('LoopDetectionService', () => {
});
});

describe('Shell Command Stagnation (Always-On Circuit Breaker)', () => {
Comment thread
yiliang114 marked this conversation as resolved.
it('halts repeated git inspection command variants via the always-on guard', () => {
const commands = [
'git status --short',
'git status --short && git diff --stat',
'git diff --name-only HEAD',
'git status --porcelain=v1',
'git diff --stat HEAD',
'git -C . status --short',
'git --no-pager diff --stat',
'git ls-files --modified',
];

for (const command of commands.slice(0, -1)) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}

expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: commands.at(-1),
description: 'Inspect repository changes',
}),
),
).toBe(true);
expect(service.getLastLoopType()).toBe(LoopType.SHELL_COMMAND_STAGNATION);
expect(loggers.logLoopDetected).toHaveBeenCalledWith(
mockConfig,
expect.objectContaining({
loop_type: 'shell_command_stagnation',
}),
);
});

it('resets the streak when a non-inspection tool call interrupts the run', () => {
// Vary the command text so the consecutive-identical guard (threshold 5)
// never fires and only the shell-stagnation bucket accumulates.
const variants = [
'git status --short',
'git diff --stat',
'git ls-files --modified',
'git status --porcelain=v1',
'git diff --name-only HEAD',
'git -C . status --short',
'git --no-pager diff --stat',
];
const gitInspect = (i: number) =>
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: variants[i % variants.length],
description: 'Inspect repository changes',
}),
);

// One short of the threshold, so the next inspection alone would trip.
for (let i = 0; i < SHELL_COMMAND_STAGNATION_THRESHOLD - 1; i++) {
expect(gitInspect(i)).toBe(false);
}

// A non-inspection tool call must reset the streak to zero.
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('read_file', {
absolute_path: '/repo/README.md',
}),
),
).toBe(false);

// Counting restarts from zero: a full threshold-minus-one run of git
// inspections still does not trip, proving the streak did not carry over.
for (let i = 0; i < SHELL_COMMAND_STAGNATION_THRESHOLD - 1; i++) {
expect(gitInspect(i)).toBe(false);
}
expect(service.getLastLoopType()).not.toBe(
LoopType.SHELL_COMMAND_STAGNATION,
);
});

it('resets the streak when a retry replays shell inspections', () => {
const variants = [
'git status --short',
'git diff --stat',
'git ls-files --modified',
'git status --porcelain=v1',
'git diff --name-only HEAD',
'git -C . status --short',
'git --no-pager diff --stat',
];

for (const command of variants) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}

expect(
service.checkAlwaysOnSafeties({
type: GeminiEventType.Retry,
} as ServerGeminiStreamEvent),
).toBe(false);

for (const command of variants) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}
expect(service.getLastLoopType()).not.toBe(
LoopType.SHELL_COMMAND_STAGNATION,
);
});

it('honors an in-session disable for shell inspection stagnation', () => {
service.disableForSession();

const variants = [
'git status --short',
'git diff --stat',
'git ls-files --modified',
'git status --porcelain=v1',
'git diff --name-only HEAD',
'git -C . status --short',
'git --no-pager diff --stat',
'git ls-files --others',
];

for (const command of variants) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}
expect(loggers.logLoopDetected).not.toHaveBeenCalledWith(
mockConfig,
expect.objectContaining({
loop_type: 'shell_command_stagnation',
}),
);
});

it('does not bucket compound commands that also write to the repository', () => {
// Each chain stages and commits real work; the embedded `git status` must
// not classify the whole command as stagnant read-only inspection. Vary
// the path so the consecutive-identical guard never fires, isolating the
// shell-stagnation guard under test.
for (let i = 0; i < SHELL_COMMAND_STAGNATION_THRESHOLD; i++) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: `git add file-${i}.txt && git status --short && git commit -m progress-${i}`,
description: 'Stage, inspect, and commit progress',
}),
),
).toBe(false);
}
expect(service.getLastLoopType()).not.toBe(
LoopType.SHELL_COMMAND_STAGNATION,
);
});

it('does not bucket shell chains that include non-git commands', () => {
for (let i = 0; i < SHELL_COMMAND_STAGNATION_THRESHOLD; i++) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: `git status --short && npm test -- --runInBand=${i}`,
description: 'Inspect repository changes and run tests',
}),
),
).toBe(false);
}
expect(service.getLastLoopType()).not.toBe(
LoopType.SHELL_COMMAND_STAGNATION,
);
});

it('does not halt repeated non-git shell commands', () => {
for (let i = 0; i < SHELL_COMMAND_STAGNATION_THRESHOLD + 2; i++) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: `npm test -- --runInBand=${i}`,
description: 'Run tests',
}),
),
).toBe(false);
}
expect(service.getLastLoopType()).not.toBe(
LoopType.SHELL_COMMAND_STAGNATION,
);
});

it('halts newline-separated git inspection command variants', () => {
const commands = [
'git diff --stat\ngit status --short',
'git diff --name-only HEAD\ngit ls-files --modified',
'git --no-pager diff --stat\ngit status --porcelain=v1',
'git diff --stat HEAD\ngit ls-files --others',
'git diff --name-only\ngit status --short',
'git diff --stat\ngit -C . status --short',
'git --no-pager diff --stat\ngit ls-files --modified',
'git diff --name-only HEAD\ngit status --short',
];

for (const command of commands.slice(0, -1)) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}

expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command: commands.at(-1),
description: 'Inspect repository changes',
}),
),
).toBe(true);
expect(service.getLastLoopType()).toBe(LoopType.SHELL_COMMAND_STAGNATION);
});

it('does not halt file-specific git diff review commands', () => {
const commands = [
'git status --short',
'git diff --stat',
'git diff -- src/a.ts',
'git diff -- src/b.ts',
'git diff -- src/c.ts',
'git diff -- src/d.ts',
'git diff -- src/e.ts',
'git diff -- src/f.ts',
];

for (const command of commands) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}
expect(loggers.logLoopDetected).not.toHaveBeenCalledWith(
mockConfig,
expect.objectContaining({
loop_type: 'shell_command_stagnation',
}),
);
});

it('does not halt file-specific git diff review commands without -- separator', () => {
const commands = [
'git status --short',
'git diff --stat',
'git diff src/a.ts',
'git diff src/b.ts',
'git diff src/c.ts',
'git diff src/d.ts',
'git diff src/e.ts',
'git diff src/f.ts',
];

for (const command of commands) {
expect(
service.checkAlwaysOnSafeties(
createToolCallRequestEvent('run_shell_command', {
command,
description: 'Inspect repository changes',
}),
),
).toBe(false);
}
expect(loggers.logLoopDetected).not.toHaveBeenCalledWith(
mockConfig,
expect.objectContaining({
loop_type: 'shell_command_stagnation',
}),
);
});
});

describe('Content Loop Detection', () => {
const generateRandomString = (length: number) => {
let result = '';
Expand Down
Loading
Loading