mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-07-27 01:54:53 +00:00
fix(agent-core-v2): surface provider auth errors and align agent copy with v1 (#1631)
* fix(agent-core-v2): surface provider rejection on post-refresh 401 - map a 401 that survives forced token refresh to PROVIDER_AUTH_ERROR carrying the provider's sanitized message, instead of a misleading AUTH_LOGIN_REQUIRED re-login prompt - propagate provider auth errors through full compaction instead of wrapping them as compaction failure - export sanitizeStatusErrorMessage for reuse * fix(agent-core-v2): align repeat-reminder and AskUserQuestion copy with v1 - rewrite repeated tool call reminders to redirect instead of prohibit (port of v1 #1518) - simplify makeReminderText2 to (repeatCount), no longer echoing tool name and args - treat dismissed AskUserQuestion as no answer, not the recommended pick (port of v1 #1550) - update toolDedupe test assertions and regenerate affected wire snapshots * chore: add changeset for v2 reminder and question copy alignment * fix(kap-server): drain in-flight heartbeat writes before unlinking the instance file - count writes that passed the released check and await them in release() so their atomic rename cannot recreate the file after unlink - harden the heartbeat test: 1ms cadence plus a post-release re-check catches the recreate-after-unlink race (verified failing on old code) * docs(agent-core-v2): move toProviderAuthError rationale to the file header Address Codex review: agent-core-v2 keeps comments in the top-of-file header only, never beside functions
This commit is contained in:
parent
96b83281b2
commit
2d874fbd73
15 changed files with 116 additions and 67 deletions
5
.changeset/fix-kap-server-heartbeat-release-race.md
Normal file
5
.changeset/fix-kap-server-heartbeat-release-race.md
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
"@moonshot-ai/kimi-code": patch
|
||||
---
|
||||
|
||||
Fix a race where a heartbeat write in flight during server shutdown could recreate the instance file right after it was removed.
|
||||
5
.changeset/fix-v2-oauth-provider-auth-error.md
Normal file
5
.changeset/fix-v2-oauth-provider-auth-error.md
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
"@moonshot-ai/kimi-code": patch
|
||||
---
|
||||
|
||||
Surface the provider's actual rejection message instead of a misleading re-login prompt when an OAuth-managed model keeps returning 401 after a token refresh.
|
||||
5
.changeset/fix-v2-reminder-and-question-copy.md
Normal file
5
.changeset/fix-v2-reminder-and-question-copy.md
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
"@moonshot-ai/kimi-code": patch
|
||||
---
|
||||
|
||||
Rewrite repeated-tool-call reminders to redirect the agent toward a different action instead of prohibiting the call, and treat a dismissed question prompt as no answer rather than the recommended option.
|
||||
|
|
@ -670,7 +670,13 @@ export class AgentFullCompactionService extends Disposable implements IAgentFull
|
|||
thinking_effort: this.profile.data().thinkingLevel,
|
||||
error_type: error instanceof Error ? error.name : 'Unknown',
|
||||
});
|
||||
if (isError2(error) && error.code === ErrorCodes.AUTH_LOGIN_REQUIRED) throw error;
|
||||
if (
|
||||
isError2(error) &&
|
||||
(error.code === ErrorCodes.AUTH_LOGIN_REQUIRED ||
|
||||
error.code === ErrorCodes.PROVIDER_AUTH_ERROR)
|
||||
) {
|
||||
throw error;
|
||||
}
|
||||
throw new Error2(ErrorCodes.COMPACTION_FAILED, String(error), { cause: error });
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -18,4 +18,4 @@ Overusing this tool interrupts the user's flow. Only use it when the user's inpu
|
|||
- Question texts must be unique across the call, and option labels must be unique within each question
|
||||
- You can ask 1-4 questions at a time; group related questions to minimize interruptions
|
||||
- If you recommend a specific option, list it first and append "(Recommended)" to its label
|
||||
- The result is JSON with an `answers` object keyed by question text; each value is the chosen option's label (comma-separated labels for multi_select, or the user's own words if they picked "Other"); if `answers` is empty and a `note` says the user dismissed it, they declined to answer — proceed with your best judgment and do not re-ask the same question
|
||||
- The result is JSON with an `answers` object keyed by question text; each value is the chosen option's label (comma-separated labels for multi_select, or the user's own words if they picked "Other"); if `answers` is empty and a `note` says the user dismissed it, they chose not to answer — do not treat this as selecting the recommended option; decide based on context and do not re-ask the same question
|
||||
|
|
|
|||
|
|
@ -22,32 +22,28 @@ import { IAgentToolDedupeService, type ToolDedupeResult } from './toolDedupe';
|
|||
|
||||
const REMINDER_TEXT_1 =
|
||||
'\n\n<system-reminder>\n' +
|
||||
'You are repeating the exact same tool call with identical parameters.' +
|
||||
' Please carefully analyze the previous result. If the task is not yet complete,' +
|
||||
' try a different method or parameters instead of repeating the same call.' +
|
||||
'The same tool call has been repeated several times in a row. ' +
|
||||
'Before making your next call, write one sentence stating what new information you expect it to produce. ' +
|
||||
'Then act on that sentence: if it names something this result does not already give you, choose the action that best provides it; otherwise, continue with the evidence you already have.' +
|
||||
'\n</system-reminder>';
|
||||
|
||||
function makeReminderText2(toolName: string, repeatCount: number, args: unknown): string {
|
||||
const argsStr = canonicalTelemetryArgs(args);
|
||||
function makeReminderText2(repeatCount: number): string {
|
||||
return (
|
||||
'\n\n<system-reminder>\n' +
|
||||
'You have repeatedly called the same tool with identical parameters many times.\n' +
|
||||
'Repeated tool call detected:\n' +
|
||||
`- tool: ${toolName}\n` +
|
||||
`- repeated_times: ${String(repeatCount)}\n` +
|
||||
`- arguments: ${argsStr}\n` +
|
||||
'The previous repeated calls did not make progress. Do not call this exact same tool with the exact same arguments again.\n' +
|
||||
'Carefully inspect the latest tool result and choose a different next action, different parameters, or finish the task if enough evidence has been gathered.' +
|
||||
`The same tool call has now been issued ${String(repeatCount)} times in a row. ` +
|
||||
'Choose exactly one of the following and state your choice before acting:\n' +
|
||||
'(1) Falsification check: run the cheapest test that could conclusively disprove your current approach, if such a test exists.\n' +
|
||||
'(2) Missing input: tell the user precisely what information or decision you need to proceed, and ask for it.\n' +
|
||||
'(3) Conclude: deliver your best result based on the evidence already gathered, listing anything that remains uncertain.' +
|
||||
'\n</system-reminder>'
|
||||
);
|
||||
}
|
||||
|
||||
const REMINDER_TEXT_3 =
|
||||
'\n\n<system-reminder>\n' +
|
||||
'You are stuck in a dead end and have repeatedly made the same function call without progress.\n' +
|
||||
'Stop all function calls immediately. Do not call any tool in your next response.\n' +
|
||||
'In analysis, review the current execution state and identify why progress is blocked.\n' +
|
||||
'Then return a text-only summary to the user that reports the current problem, what has already been tried, and what information or decision is needed next.' +
|
||||
'Write your final response now, without any further tool calls. ' +
|
||||
'Cover: the current blocker, each approach you have tried and what it established, and the specific information or decision you need from the user to unblock progress. ' +
|
||||
'Text only.' +
|
||||
'\n</system-reminder>';
|
||||
|
||||
const REPEAT_REMINDER_1_START = 3;
|
||||
|
|
@ -269,7 +265,7 @@ export class AgentToolDedupeService extends Disposable implements IAgentToolDedu
|
|||
finalResult = appendReminder(result, REMINDER_TEXT_3);
|
||||
action = 'r3';
|
||||
} else if (streak >= REPEAT_REMINDER_2_START) {
|
||||
finalResult = appendReminder(result, makeReminderText2(toolName, streak, args));
|
||||
finalResult = appendReminder(result, makeReminderText2(streak));
|
||||
action = 'r2';
|
||||
} else if (streak >= REPEAT_REMINDER_1_START) {
|
||||
finalResult = appendReminder(result, REMINDER_TEXT_1);
|
||||
|
|
|
|||
|
|
@ -15,6 +15,11 @@
|
|||
* wire I/O to `IProtocolAdapterRegistry.createChatProvider(...)` + kosong's
|
||||
* `generate(...)`. Phase 8 replaces the wire with native adapters; only this
|
||||
* file changes.
|
||||
*
|
||||
* Provider error translation also lives here: a 401 that survives a forced
|
||||
* token refresh means the provider rejected the account itself, so it is
|
||||
* surfaced as `PROVIDER_AUTH_ERROR` carrying the provider's message instead
|
||||
* of a misleading re-login prompt.
|
||||
*/
|
||||
|
||||
import { AsyncEventQueue } from '#/_base/asyncEventQueue';
|
||||
|
|
@ -28,7 +33,7 @@ import { type ThinkingEffort } from '#/app/llmProtocol/thinkingEffort';
|
|||
import type { ChatProvider } from '#/app/llmProtocol/provider';
|
||||
import type { Protocol, ProtocolProviderOptions } from '#/app/protocol/protocol';
|
||||
import { generate, type GenerateResult } from '#/app/llmProtocol/generate';
|
||||
import { translateProviderError } from '#/app/protocol/errors';
|
||||
import { sanitizeStatusErrorMessage, translateProviderError } from '#/app/protocol/errors';
|
||||
import { type ProtocolAdapterRegistry } from '#/app/protocol/protocolAdapterRegistry';
|
||||
import { ErrorCodes, Error2 } from '#/errors';
|
||||
|
||||
|
|
@ -333,7 +338,7 @@ export class ModelImpl implements Model {
|
|||
try {
|
||||
return await run(refreshedAuth);
|
||||
} catch (error) {
|
||||
if (isUnauthorizedStatusError(error)) throw toLoginRequiredError(error);
|
||||
if (isUnauthorizedStatusError(error)) throw toProviderAuthError(error);
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
|
@ -347,11 +352,13 @@ function isUnauthorizedStatusError(error: unknown): error is APIStatusError {
|
|||
return error instanceof APIStatusError && error.statusCode === 401;
|
||||
}
|
||||
|
||||
function toLoginRequiredError(error: APIStatusError): Error2 {
|
||||
function toProviderAuthError(error: APIStatusError): Error2 {
|
||||
const reason = sanitizeStatusErrorMessage(error.message);
|
||||
return new Error2(
|
||||
ErrorCodes.AUTH_LOGIN_REQUIRED,
|
||||
'OAuth provider credentials were rejected. Send /login to login.',
|
||||
ErrorCodes.PROVIDER_AUTH_ERROR,
|
||||
reason.length > 0 ? reason : 'OAuth provider credentials were rejected.',
|
||||
{
|
||||
name: error.name,
|
||||
cause: error,
|
||||
details: {
|
||||
statusCode: error.statusCode,
|
||||
|
|
|
|||
|
|
@ -139,7 +139,7 @@ export function translateProviderError(error: unknown): Error2 {
|
|||
return new Error2(CoreErrors.codes.INTERNAL, String(error), { cause: error });
|
||||
}
|
||||
|
||||
function sanitizeStatusErrorMessage(message: string): string {
|
||||
export function sanitizeStatusErrorMessage(message: string): string {
|
||||
const titleMatch = /<title[^>]*>([\s\S]*?)<\/title>/i.exec(message);
|
||||
const extracted = titleMatch?.[1]?.trim();
|
||||
const normalized = extracted !== undefined && extracted.length > 0 ? extracted : message;
|
||||
|
|
|
|||
|
|
@ -428,7 +428,7 @@ describe('FullCompaction', () => {
|
|||
).toBe(false);
|
||||
});
|
||||
|
||||
it('force-refreshes OAuth credentials on compaction 401 and falls back to login_required when replay 401', async () => {
|
||||
it('force-refreshes OAuth credentials on compaction 401 and treats replay 401 as provider auth error', async () => {
|
||||
const tokenCalls: Array<boolean | undefined> = [];
|
||||
const authKeys: string[] = [];
|
||||
const oauthOptions = oauthTestAgentOptions(async (options) => {
|
||||
|
|
@ -467,7 +467,7 @@ describe('FullCompaction', () => {
|
|||
expect.objectContaining({
|
||||
event: 'error',
|
||||
args: expect.objectContaining({
|
||||
code: 'auth.login_required',
|
||||
code: 'provider.auth_error',
|
||||
details: expect.objectContaining({
|
||||
statusCode: 401,
|
||||
requestId: 'req-compact-401',
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -366,8 +366,8 @@ describe('AgentToolDedupeService', () => {
|
|||
registerRead(h);
|
||||
const last = await runStreak(h, 3);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain('repeating the exact same tool call');
|
||||
expect(last.output as string).not.toContain('repeated_times');
|
||||
expect(last.output as string).toContain('what new information you expect');
|
||||
expect(last.output as string).not.toContain('Choose exactly one');
|
||||
});
|
||||
|
||||
it('keeps injecting reminder1 at 4 consecutive', async () => {
|
||||
|
|
@ -375,7 +375,7 @@ describe('AgentToolDedupeService', () => {
|
|||
registerRead(h);
|
||||
const last = await runStreak(h, 4);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain('repeating the exact same tool call');
|
||||
expect(last.output as string).toContain('what new information you expect');
|
||||
});
|
||||
|
||||
it('injects reminder2 at exactly 5 consecutive', async () => {
|
||||
|
|
@ -383,9 +383,9 @@ describe('AgentToolDedupeService', () => {
|
|||
registerRead(h);
|
||||
const last = await runStreak(h, 5);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain('repeated_times: 5');
|
||||
expect(last.output as string).toContain('tool: Read');
|
||||
expect(last.output as string).toContain('arguments:');
|
||||
expect(last.output as string).toContain('issued 5 times in a row');
|
||||
expect(last.output as string).toContain('Choose exactly one of the following');
|
||||
expect(last.output as string).toContain('Falsification check');
|
||||
});
|
||||
|
||||
it.each([6, 7])('keeps injecting reminder2 at %i consecutive', async (streak) => {
|
||||
|
|
@ -393,8 +393,8 @@ describe('AgentToolDedupeService', () => {
|
|||
registerRead(h);
|
||||
const last = await runStreak(h, streak);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain(`repeated_times: ${String(streak)}`);
|
||||
expect(last.output as string).toContain('tool: Read');
|
||||
expect(last.output as string).toContain(`issued ${String(streak)} times in a row`);
|
||||
expect(last.output as string).toContain('Choose exactly one of the following');
|
||||
});
|
||||
|
||||
it('injects the dead-end reminder at exactly 8 consecutive', async () => {
|
||||
|
|
@ -402,7 +402,7 @@ describe('AgentToolDedupeService', () => {
|
|||
registerRead(h);
|
||||
const last = await runStreak(h, 8);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain('stuck in a dead end');
|
||||
expect(last.output as string).toContain('without any further tool calls');
|
||||
});
|
||||
|
||||
it('resets streak when a different call is interleaved', async () => {
|
||||
|
|
@ -438,9 +438,9 @@ describe('AgentToolDedupeService', () => {
|
|||
expect(tool.calls.length).toBe(callsBefore + 1);
|
||||
const byId = new Map(results.map((result) => [result.toolCallId, result.result]));
|
||||
expect(byId.get('orig')!.output as string).toContain('<system-reminder>');
|
||||
expect(byId.get('orig')!.output as string).toContain('repeating the exact same tool call');
|
||||
expect(byId.get('orig')!.output as string).toContain('what new information you expect');
|
||||
expect(byId.get('dup')!.output as string).toContain('<system-reminder>');
|
||||
expect(byId.get('dup')!.output as string).toContain('repeating the exact same tool call');
|
||||
expect(byId.get('dup')!.output as string).toContain('what new information you expect');
|
||||
});
|
||||
|
||||
it('same-step spam alone does not trigger reminder', async () => {
|
||||
|
|
@ -483,7 +483,7 @@ describe('AgentToolDedupeService', () => {
|
|||
}
|
||||
const [final] = await runStep(h, 1, 5, [toolCall('final', 'X', { a: 1 })]);
|
||||
// Text-only array is normalized to a joined string by the executor.
|
||||
expect(final!.result.output).toBe('hello' + makeReminderText2('X', 5, { a: 1 }));
|
||||
expect(final!.result.output).toBe('hello' + makeReminderText2(5));
|
||||
});
|
||||
|
||||
it('pushes a new text part when trailing part is non-text', async () => {
|
||||
|
|
@ -626,8 +626,8 @@ describe('AgentToolDedupeService', () => {
|
|||
h.registry.register(new EchoTool('Read'));
|
||||
const last = await runStreak(h, 8);
|
||||
expect(last.output as string).toContain('<system-reminder>');
|
||||
expect(last.output as string).toContain('stuck in a dead end');
|
||||
expect(last.output as string).toContain('Stop all function calls immediately');
|
||||
expect(last.output as string).toContain('Write your final response now');
|
||||
expect(last.output as string).toContain('without any further tool calls');
|
||||
// 8 is the reminder threshold, not yet force-stop. The executor always
|
||||
// materializes `stopTurn` as a boolean, so a non-stopped result is `false`.
|
||||
expect(last.isError).toBeUndefined();
|
||||
|
|
@ -640,7 +640,7 @@ describe('AgentToolDedupeService', () => {
|
|||
const h = createHarness();
|
||||
h.registry.register(new EchoTool('Read'));
|
||||
const last = await runStreak(h, streak);
|
||||
expect(last.output as string).toContain('stuck in a dead end');
|
||||
expect(last.output as string).toContain('Write your final response now');
|
||||
expect(last.isError).toBeUndefined();
|
||||
expect(stopTurnOf(last)).toBeFalsy();
|
||||
},
|
||||
|
|
@ -650,7 +650,7 @@ describe('AgentToolDedupeService', () => {
|
|||
const h = createHarness();
|
||||
h.registry.register(new EchoTool('Read'));
|
||||
const last = await runStreak(h, 12);
|
||||
expect(last.output as string).toContain('stuck in a dead end');
|
||||
expect(last.output as string).toContain('Write your final response now');
|
||||
// The underlying tool succeeded — force-stop must not flip it to error.
|
||||
expect(last.isError).toBeUndefined();
|
||||
expect(stopTurnOf(last)).toBe(true);
|
||||
|
|
@ -682,7 +682,7 @@ describe('AgentToolDedupeService', () => {
|
|||
// The underlying tool was an error — that must survive force-stop.
|
||||
expect(last!.isError).toBe(true);
|
||||
expect(stopTurnOf(last!)).toBe(true);
|
||||
expect(last!.output as string).toContain('stuck in a dead end');
|
||||
expect(last!.output as string).toContain('Write your final response now');
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -356,7 +356,7 @@ describe('ModelResolverService', () => {
|
|||
expect(events).toContainEqual({ type: 'part', part: { type: 'text', text: 'recovered' } });
|
||||
});
|
||||
|
||||
it('throws login_required when force-refresh and replay both 401', async () => {
|
||||
it('throws provider auth error when force-refresh and replay both 401', async () => {
|
||||
configureOAuthModel();
|
||||
const authKeys: string[] = [];
|
||||
resolveTokenProvider.mockReturnValue({
|
||||
|
|
@ -378,7 +378,9 @@ describe('ModelResolverService', () => {
|
|||
void _event;
|
||||
}
|
||||
}).rejects.toMatchObject({
|
||||
code: 'auth.login_required',
|
||||
code: 'provider.auth_error',
|
||||
name: 'APIStatusError',
|
||||
message: 'Unauthorized',
|
||||
details: {
|
||||
statusCode: 401,
|
||||
requestId: 'req-401',
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -246,20 +246,31 @@ export function createInstanceRegistry(options: InstanceRegistryOptions = {}): I
|
|||
// the latest port without re-reading the file.
|
||||
const state: { port: number; released: boolean } = { port: info.port, released: false };
|
||||
|
||||
// Count of writes that passed the `released` check but have not finished
|
||||
// their atomic rename yet. `release()` must drain them before unlinking:
|
||||
// a rename that lands after the unlink would recreate the file.
|
||||
let inflightWrites = 0;
|
||||
let onWritesDrained: (() => void) | null = null;
|
||||
|
||||
const write = async (): Promise<void> => {
|
||||
// Bail out once released so a heartbeat tick that was already in flight
|
||||
// when `release()` ran cannot recreate the just-unlinked file.
|
||||
// Bail out once released so no new write starts after `release()`.
|
||||
if (state.released) return;
|
||||
const full: ServerInstanceInfo = {
|
||||
serverId,
|
||||
pid: info.pid,
|
||||
host: info.host,
|
||||
port: state.port,
|
||||
startedAt: info.startedAt,
|
||||
heartbeatAt: now(),
|
||||
...(info.hostVersion !== undefined ? { hostVersion: info.hostVersion } : {}),
|
||||
};
|
||||
await writeFileAtomic(filePath, encode(full));
|
||||
inflightWrites += 1;
|
||||
try {
|
||||
const full: ServerInstanceInfo = {
|
||||
serverId,
|
||||
pid: info.pid,
|
||||
host: info.host,
|
||||
port: state.port,
|
||||
startedAt: info.startedAt,
|
||||
heartbeatAt: now(),
|
||||
...(info.hostVersion !== undefined ? { hostVersion: info.hostVersion } : {}),
|
||||
};
|
||||
await writeFileAtomic(filePath, encode(full));
|
||||
} finally {
|
||||
inflightWrites -= 1;
|
||||
if (inflightWrites === 0) onWritesDrained?.();
|
||||
}
|
||||
};
|
||||
|
||||
await write();
|
||||
|
|
@ -282,6 +293,13 @@ export function createInstanceRegistry(options: InstanceRegistryOptions = {}): I
|
|||
if (state.released) return;
|
||||
state.released = true;
|
||||
clearInterval(timer);
|
||||
// Wait for writes already in flight so their atomic rename cannot
|
||||
// recreate the file after we unlink it.
|
||||
if (inflightWrites > 0) {
|
||||
await new Promise<void>((resolve) => {
|
||||
onWritesDrained = resolve;
|
||||
});
|
||||
}
|
||||
try {
|
||||
await unlink(filePath);
|
||||
} catch (err) {
|
||||
|
|
|
|||
|
|
@ -210,7 +210,9 @@ describe('createInstanceRegistry — heartbeat', () => {
|
|||
let tick = 0;
|
||||
const registry = createInstanceRegistry({
|
||||
instancesDir,
|
||||
heartbeatIntervalMs: 20,
|
||||
// 1ms cadence keeps a write in flight at almost every moment, so
|
||||
// `release()` is exercised against the recreate-after-unlink race.
|
||||
heartbeatIntervalMs: 1,
|
||||
now: () => ++tick,
|
||||
});
|
||||
const reg = await registry.register(baseInfo);
|
||||
|
|
@ -221,7 +223,10 @@ describe('createInstanceRegistry — heartbeat', () => {
|
|||
expect(later).toBeGreaterThan(first);
|
||||
|
||||
await reg.release();
|
||||
// File is gone after release.
|
||||
// File is gone after release, and a heartbeat write that was in flight
|
||||
// when release() ran must not recreate it.
|
||||
expect(existsSync(join(instancesDir, `${reg.serverId}.json`))).toBe(false);
|
||||
await sleep(30);
|
||||
expect(existsSync(join(instancesDir, `${reg.serverId}.json`))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue