mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
fix(core): disambiguate hard-rescue from manual /compress orphan-strip
Self-review (dual reviewer / pr-triage round 1) caught a correctness
regression in the hard-rescue path:
`sendMessageStream` calls `tryCompress(force=true)` from inside the
pre-push window when `effectiveTokens >= hard`. The service's
orphan-strip predicate at `chatCompressionService.ts:426-429` gated on
`force` alone, which conflated two distinct call shapes:
- manual `/compress` (force=true, trigger='manual'): user-initiated
between turns; trailing model funcCall IS orphaned because no
funcResponse is coming
- hard-rescue (force=true, trigger='auto'): automatic mid-turn;
trailing model funcCall is ACTIVE because its matching funcResponse
is sitting in the pending `userContent` waiting to be pushed
The strip fired for both, so a hard-rescue triggered mid tool-use loop
would drop the active funcCall. After compression returned and
`userContent` (the funcResponse) was pushed, the next API request
carried tool_result with no matching tool_use → provider validation
error.
The in-code comment at L422-424 already documented this exact
constraint for the auto-compress case (`force=false`), but reusing
`force=true` for hard-rescue silently violated the same constraint.
Fix:
- Gate `hasOrphanedFuncCall` on `compactTrigger === 'manual'` instead
of `force`. The trigger field already disambiguates intent.
- `sendMessageStream` hard-rescue now passes `trigger: 'auto'`
explicitly (without it, `force=true` defaults to `trigger='manual'`
via the `?? (force ? 'manual' : 'auto')` resolver).
Sibling audit for "force=true non-manual callsites":
- `GeminiClient.tryCompressChat` (manual /compress): correct — manual
- `sendMessageStream` hard-rescue: fixed in this commit
- `sendMessageStream` reactive overflow catch: already passes
trigger='auto'; runs AFTER API call (userContent in history), so if
it observes a trailing funcCall it IS orphaned but findCompressSplitPoint
handles the case without needing the strip
RED-first regression test added:
`preserves trailing model+funcCall under hard-rescue (force=true + trigger=auto)`
in `chatCompressionService.test.ts`. Failed against pre-fix code (the
strip dropped the funcCall); passes against the fix.
Adjacent fixes from the same triage round:
- `docs/users/configuration/settings.md`: the
`chatCompression.contextPercentageThreshold` row still said "use 0
to disable compression entirely" — code has ignored the value since
the removal commit. Marked the row REMOVED with migration guidance
pointing at the design doc.
- `packages/core/src/config/config.ts`: the deprecation warning now
tells users how to silence it (remove the key) and where to read
current behavior, instead of just announcing the removal.
- `docs/design/auto-compaction-threshold-redesign.md`: closed Open
Question 2 (small-window hard/auto collapse) — decision is to NOT
annotate `/context`, with rationale on file.
Tests: 2395 core tests passing, typecheck clean.
This commit is contained in:
LaZzyMan
parent
cb3b09edb8
commit
50bac974b0
6 changed files with 111 additions and 11 deletions
|
|
@ -425,4 +425,10 @@ const { warn, auto, hard, effectiveWindow } =
|
|||
## 开放问题（等 review）
|
||||
|
||||
1. **breaking change 强度**：警告 + 忽略字段 vs 启动报错。当前选警告，需要确认对企业部署/团队配置是否够友好
|
||||
2. **小窗口（32K）下 hard 与 auto 退化为同一值**：用户视角是否需要在 `/context` 明示「该窗口下 hard 已退化」
|
||||
|
||||
## 已结案
|
||||
|
||||
2. **小窗口（≤ ~76.7K）下 hard 与 auto 退化为同一值** — 决定**不在 `/context` 明示**。理由：
|
||||
- 塌缩范围不只是 32K，所有 `effectiveWindow - HARD_BUFFER ≤ 0.7 × window` 的窗口都塌缩（包括 64K）
|
||||
- 用户行为不变：`currentTier` 跳过 `'hard'` 直接到 `'auto'`，`context-high` band 为空带来的"少一档提示"在小窗口上是合理的——窗口本身就小，用户大概率手动管理上下文
|
||||
- 如果未来有真实用户报告"小窗口看不到中间档提示"，再决定加 UI 标注或调整 `context-high` 触发条件（这是 UI 工作，不是 spec 工作）。当前选不增加 UI 复杂度
|
||||
|
|
|
|||
|
|
@ -144,7 +144,7 @@ Settings are organized into categories. Most settings should be placed within th
|
|||
| `model.name` | string | The Qwen model to use for conversations. | `undefined` |
|
||||
| `model.maxSessionTurns` | number | Maximum number of user/model/tool turns to keep in a session. -1 means unlimited. | `-1` |
|
||||
| `model.generationConfig` | object | Advanced overrides passed to the underlying content generator. Supports request controls such as `timeout`, `maxRetries`, `enableCacheControl`, `splitToolMedia` (set `true` for strict OpenAI-compatible servers like LM Studio that reject non-text content on `role: "tool"` messages — splits media into a follow-up user message), `contextWindowSize` (override model's context window size), `modalities` (override auto-detected input modalities), `customHeaders` (custom HTTP headers for API requests), and `extra_body` (additional body parameters for OpenAI-compatible API requests only), along with fine-tuning knobs under `samplingParams` (for example `temperature`, `top_p`, `max_tokens`). Leave unset to rely on provider defaults. | `undefined` |
|
||||
| `model.chatCompression.contextPercentageThreshold` | number | Sets the threshold for chat history compression as a percentage of the model's total token limit. This is a value between 0 and 1 that applies to both automatic compression and the manual `/compress` command. For example, a value of `0.6` will trigger compression when the chat history exceeds 60% of the token limit. Use `0` to disable compression entirely. | `0.7` |
|
||||
| `model.chatCompression.contextPercentageThreshold` | number | **REMOVED.** Auto-compaction now uses a three-tier threshold ladder (warn / auto / hard) computed internally from the model's context window via the `computeThresholds()` function — no longer user-configurable. Setting this field in `settings.json` is silently ignored, and a one-line deprecation warning is emitted to stderr at startup. There is currently no replacement for "disable compression entirely" — reactive overflow recovery remains the safety net at the API layer if compression itself fails. (See PR #4345 / `docs/design/auto-compaction-threshold-redesign.md` for the redesign rationale.) | `N/A` |
|
||||
| `model.skipNextSpeakerCheck` | boolean | Skip the next speaker check. | `false` |
|
||||
| `model.skipLoopDetection` | boolean | Disables loop detection checks. Loop detection prevents infinite loops in AI responses but can generate false positives that interrupt legitimate workflows. Enable this option if you experience frequent false positive loop detection interruptions. | `false` |
|
||||
| `model.skipStartupContext` | boolean | Skips sending the startup workspace context (environment summary and acknowledgement) at the beginning of each session. Enable this if you prefer to provide context manually or want to save tokens on startup. | `false` |
|
||||
|
|
|
|||
|
|
@ -1049,7 +1049,9 @@ export class Config {
|
|||
// eslint-disable-next-line no-console
|
||||
console.warn(
|
||||
'[qwen-code] chatCompression.contextPercentageThreshold has been removed ' +
|
||||
'and is now controlled by built-in thresholds. Setting will be ignored.',
|
||||
'and is now controlled by built-in thresholds. Setting will be ignored. ' +
|
||||
'Remove this key from your settings.json to silence this warning; ' +
|
||||
'see docs/users/configuration/settings.md for current compaction behavior.',
|
||||
);
|
||||
}
|
||||
this.chatCompression = params.chatCompression;
|
||||
|
|
|
|||
|
|
@ -777,6 +777,14 @@ export class GeminiChat {
|
|||
{
|
||||
pendingUserMessage: userContent,
|
||||
precomputedEffectiveTokens: effectiveTokens,
|
||||
// Hard-rescue is force=true to bypass the cheap-gate breaker
|
||||
// but it's an AUTOMATIC trigger. Explicit trigger='auto' tells
|
||||
// the service to skip the manual-only orphan-strip that would
|
||||
// otherwise drop the active funcCall whose matching
|
||||
// funcResponse is sitting in `pendingUserMessage` waiting to
|
||||
// be pushed. Without this, hard-rescue mid tool-use loop
|
||||
// corrupts the next API request's tool-call/response pairing.
|
||||
trigger: shouldForceFromHard ? 'auto' : undefined,
|
||||
},
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -1899,6 +1899,82 @@ describe('ChatCompressionService', () => {
|
|||
expect(newHistory[i].role).not.toBe(newHistory[i - 1].role);
|
||||
}
|
||||
});
|
||||
|
||||
it('preserves trailing model+funcCall under hard-rescue (force=true + trigger=auto)', async () => {
|
||||
// Hard-rescue fires from inside sendMessageStream() BEFORE the pending
|
||||
// userContent (a funcResponse) is pushed onto history. At that moment
|
||||
// the trailing model+funcCall is ACTIVE, not orphaned — its matching
|
||||
// funcResponse is sitting in the pending message about to be appended.
|
||||
//
|
||||
// Pre-fix, the service's orphan-strip predicate gated on `force` alone,
|
||||
// which meant hard-rescue (force=true, trigger='auto') was conflated
|
||||
// with manual /compress and stripped the active funcCall — corrupting
|
||||
// tool-call/response pairing on the next API send. Fix: gate the strip
|
||||
// on `trigger === 'manual'` so only the explicit user-initiated
|
||||
// /compress path performs the orphan cleanup.
|
||||
const history: Content[] = [
|
||||
{ role: 'user', parts: [{ text: 'Fix all TypeScript errors.' }] },
|
||||
{
|
||||
role: 'model',
|
||||
parts: [{ functionCall: { name: 'glob', args: {} } }],
|
||||
},
|
||||
{
|
||||
role: 'user',
|
||||
parts: [
|
||||
{
|
||||
functionResponse: {
|
||||
name: 'glob',
|
||||
response: { result: 'files...' },
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
// The hard-rescue moment: this funcCall is active (the matching
|
||||
// funcResponse is in the pending userContent, not in history yet).
|
||||
{
|
||||
role: 'model',
|
||||
parts: [{ functionCall: { name: 'readFile', args: {} } }],
|
||||
},
|
||||
];
|
||||
vi.mocked(mockChat.getHistory).mockReturnValue(history);
|
||||
vi.mocked(uiTelemetryService.getLastPromptTokenCount).mockReturnValue(
|
||||
800,
|
||||
);
|
||||
vi.mocked(mockConfig.getContentGeneratorConfig).mockReturnValue({
|
||||
model: 'gemini-pro',
|
||||
contextWindowSize: 1000,
|
||||
} as unknown as ReturnType<typeof mockConfig.getContentGeneratorConfig>);
|
||||
|
||||
const mockGenerateContent = vi.fn().mockResolvedValue({
|
||||
text: 'state snapshot summary',
|
||||
usage: {
|
||||
promptTokenCount: 2000,
|
||||
candidatesTokenCount: 50,
|
||||
totalTokenCount: 2050,
|
||||
},
|
||||
});
|
||||
vi.mocked(mockConfig.getBaseLlmClient).mockReturnValue({
|
||||
generateText: mockGenerateContent,
|
||||
} as unknown as BaseLlmClient);
|
||||
|
||||
const result = await service.compress(mockChat, {
|
||||
promptId: mockPromptId,
|
||||
force: true,
|
||||
trigger: 'auto', // hard-rescue explicitly signals automatic intent
|
||||
model: mockModel,
|
||||
config: mockConfig,
|
||||
consecutiveFailures: 0,
|
||||
originalTokenCount: uiTelemetryService.getLastPromptTokenCount(),
|
||||
});
|
||||
|
||||
expect(result.info.compressionStatus).toBe(CompressionStatus.COMPRESSED);
|
||||
// The active funcCall must survive in the post-compression history so
|
||||
// the about-to-be-pushed funcResponse has its matching tool_use.
|
||||
const newHistory = result.newHistory!;
|
||||
const last = newHistory[newHistory.length - 1];
|
||||
expect(last.role).toBe('model');
|
||||
expect(last.parts?.some((p) => p.functionCall)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('tool-loop subagent absorption', () => {
|
||||
|
|
|
|||
|
|
@ -413,18 +413,26 @@ export class ChatCompressionService {
|
|||
}
|
||||
}
|
||||
|
||||
// For manual /compress (force=true), if the last message is an orphaned model
|
||||
// funcCall (agent interrupted/crashed before the response arrived), strip it
|
||||
// before computing the split point. After stripping, the history ends cleanly
|
||||
// (typically with a user funcResponse) and findCompressSplitPoint handles it
|
||||
// through its normal logic — no special-casing needed.
|
||||
// Only manual `/compress` (trigger='manual') performs the orphan-strip:
|
||||
// if the chat was interrupted with a trailing model funcCall whose
|
||||
// funcResponse never arrived, the user-initiated /compress between
|
||||
// turns can safely drop it before computing the split point.
|
||||
//
|
||||
// auto-compress (force=false) must NOT strip: it fires inside
|
||||
// sendMessageStream() before the matching funcResponse is pushed onto the
|
||||
// Both automatic paths (trigger='auto') — cheap-gate (force=false) AND
|
||||
// hard-rescue (force=true) — must NOT strip. They fire inside
|
||||
// sendMessageStream() BEFORE the pending funcResponse is pushed onto
|
||||
// history, so the trailing funcCall is still active, not orphaned.
|
||||
//
|
||||
// Gating on `trigger === 'manual'` instead of `force` disambiguates
|
||||
// "user wants this compressed now, history can be mutated" from
|
||||
// "automatic compression mid-turn, history snapshot is live state and
|
||||
// must be preserved verbatim". Earlier the predicate used `force`,
|
||||
// which is correct for manual /compress (force=true, trigger='manual')
|
||||
// but conflated hard-rescue (force=true, trigger='auto') and silently
|
||||
// stripped active funcCalls there.
|
||||
const lastMessage = curatedHistory[curatedHistory.length - 1];
|
||||
const hasOrphanedFuncCall =
|
||||
force &&
|
||||
compactTrigger === 'manual' &&
|
||||
lastMessage?.role === 'model' &&
|
||||
lastMessage.parts?.some((p) => !!p.functionCall);
|
||||
const historyForSplit = hasOrphanedFuncCall
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue