mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-08-23 07:37:18 +00:00
fix(flow): source-check auxiliary flow guards and prefer any valid definition in FlowStart
The TodoList veto now applies only when both TodoList and the batched FlowStart resolve to builtin registrations; worker-agent undo events no longer publish a session flow status of null over the main agent's active run; FlowStart mirrors discovery precedence by falling back to a valid user-level definition when the project file is readable but invalid; and spawnable profile descriptions no longer advertise the supervisor-only flow tools that a worker can never receive.
This commit is contained in:
parent
cd9c9cdce5
commit
1199188295
9 changed files with 169 additions and 48 deletions
|
|
@ -6,6 +6,7 @@ import {
|
|||
userCancellationReason,
|
||||
} from '#/_base/utils/abort';
|
||||
import { Error2, ErrorCodes, isError2 } from '#/errors';
|
||||
import { FLOW_TOOL_NAMES } from '#/features/flow/flow';
|
||||
import { toInputJsonSchema } from '#/tool/input-schema';
|
||||
import { matchesGlobRuleSubject } from '#/tool/rule-match';
|
||||
import {
|
||||
|
|
@ -514,20 +515,23 @@ function buildProfileDescriptions(
|
|||
source: ToolReference['source'],
|
||||
) => boolean,
|
||||
): string {
|
||||
const advertisableTools = tools.filter((tool) => !FLOW_TOOL_NAMES.has(tool.name));
|
||||
return profiles
|
||||
.map((profile) => {
|
||||
const details = [profile.description, profile.whenToUse].filter(
|
||||
(part): part is string => part !== undefined && part.length > 0,
|
||||
);
|
||||
const header = details.length === 0 ? `- ${profile.name}` : `- ${profile.name}: ${details.join(' ')}`;
|
||||
const activeTools = resolveActiveToolNames(profile);
|
||||
const externallyRestricted = tools.some(
|
||||
const activeTools = resolveActiveToolNames(profile)?.filter(
|
||||
(name) => !FLOW_TOOL_NAMES.has(name),
|
||||
);
|
||||
const externallyRestricted = advertisableTools.some(
|
||||
(tool) =>
|
||||
evaluateToolActive(profile, tool.name, tool.source) &&
|
||||
!isToolActive(profile, tool.name, tool.source),
|
||||
);
|
||||
if (externallyRestricted) {
|
||||
const effectiveTools = tools
|
||||
const effectiveTools = advertisableTools
|
||||
.filter((tool) => isToolActive(profile, tool.name, tool.source))
|
||||
.map((tool) => tool.name);
|
||||
if (effectiveTools.length === 0) {
|
||||
|
|
|
|||
|
|
@ -12,6 +12,18 @@ export const FLOW_ADVANCE_TOOL_NAME = 'FlowAdvance';
|
|||
export const FLOW_ABORT_TOOL_NAME = 'FlowAbort';
|
||||
export const FLOW_JUMP_TOOL_NAME = 'FlowJump';
|
||||
|
||||
/**
|
||||
* Builtin flow tool names. They are registered supervisor-only (main agent),
|
||||
* so a spawned worker never receives them regardless of its profile's
|
||||
* allowlist.
|
||||
*/
|
||||
export const FLOW_TOOL_NAMES: ReadonlySet<string> = new Set([
|
||||
FLOW_START_TOOL_NAME,
|
||||
FLOW_ADVANCE_TOOL_NAME,
|
||||
FLOW_ABORT_TOOL_NAME,
|
||||
FLOW_JUMP_TOOL_NAME,
|
||||
]);
|
||||
|
||||
export const FLOWS_PROJECT_DIR = '.kimi-code/flows';
|
||||
|
||||
export const FlowGateKindSchema = z.enum(['ai', 'human', 'ai-then-human']);
|
||||
|
|
|
|||
|
|
@ -28,6 +28,8 @@ import {
|
|||
FLOW_ADVANCE_TOOL_NAME,
|
||||
FLOW_FLAG_ID,
|
||||
FLOW_JUMP_TOOL_NAME,
|
||||
FLOW_START_TOOL_NAME,
|
||||
FLOW_TOOL_NAMES,
|
||||
FlowDefinitionSchema,
|
||||
IAgentFlowService,
|
||||
type FlowAdvanceOutcome,
|
||||
|
|
@ -50,13 +52,6 @@ import { ISessionWorkspaceContext } from '#/session/workspaceContext/workspaceCo
|
|||
|
||||
import { FlowJumped, FlowRunEnded, FlowRunStarted, FlowVerdict, flowGatesKey, flowKey } from './flowOps';
|
||||
|
||||
const FLOW_TOOL_NAMES: ReadonlySet<string> = new Set([
|
||||
'FlowStart',
|
||||
'FlowAdvance',
|
||||
'FlowAbort',
|
||||
'FlowJump',
|
||||
]);
|
||||
|
||||
export class AgentFlowService extends Disposable implements IAgentFlowService {
|
||||
declare readonly _serviceBrand: undefined;
|
||||
|
||||
|
|
@ -141,8 +136,10 @@ export class AgentFlowService extends Disposable implements IAgentFlowService {
|
|||
this._register(
|
||||
toolExecutor.onBeforeExecuteTool((event) => {
|
||||
if (!this.flags.enabled(FLOW_FLAG_ID)) return;
|
||||
if (event.toolCall.name !== 'TodoList') return;
|
||||
const startsInBatch = event.toolCalls.some((call) => call.name === 'FlowStart');
|
||||
if (event.toolCall.name !== 'TodoList' || !this.isBuiltinTool('TodoList')) return;
|
||||
const startsInBatch = event.toolCalls.some(
|
||||
(call) => call.name === FLOW_START_TOOL_NAME && this.isBuiltinFlowTool(call.name),
|
||||
);
|
||||
if (!this.run().active && !startsInBatch) return;
|
||||
event.veto(
|
||||
denyToolExecution(
|
||||
|
|
@ -157,6 +154,7 @@ export class AgentFlowService extends Disposable implements IAgentFlowService {
|
|||
eventBus.subscribe(ContextUndone, () => {
|
||||
this.epoch += 1;
|
||||
if (!this.flags.enabled(FLOW_FLAG_ID)) return;
|
||||
if (this.scopeContext.agentId !== 'main') return;
|
||||
void this.dispatcher.dispatch(new AgentStatusUpdated({ flowRun: this.summary() }));
|
||||
}),
|
||||
);
|
||||
|
|
@ -185,6 +183,10 @@ export class AgentFlowService extends Disposable implements IAgentFlowService {
|
|||
|
||||
private isBuiltinFlowTool(name: string): boolean {
|
||||
if (!FLOW_TOOL_NAMES.has(name)) return false;
|
||||
return this.isBuiltinTool(name);
|
||||
}
|
||||
|
||||
private isBuiltinTool(name: string): boolean {
|
||||
return this.toolRegistry
|
||||
.listReferences()
|
||||
.some((reference) => reference.name === name && reference.source === 'builtin');
|
||||
|
|
|
|||
|
|
@ -63,52 +63,71 @@ export class FlowStartTool implements IFlowStartTool {
|
|||
};
|
||||
}
|
||||
|
||||
let text: string | undefined;
|
||||
let projectText: string | undefined;
|
||||
const lease = this.runtime.acquire(['fs']);
|
||||
try {
|
||||
if (lease.runtime.identity.generation !== generation) {
|
||||
return { isError: true, output: 'Runtime changed before execution. Retry the tool call.' };
|
||||
}
|
||||
try {
|
||||
text = await lease.runtime.fs!.readText(path);
|
||||
projectText = await lease.runtime.fs!.readText(path);
|
||||
} catch {
|
||||
text = undefined;
|
||||
projectText = undefined;
|
||||
}
|
||||
} finally {
|
||||
lease.dispose();
|
||||
}
|
||||
|
||||
let sourcePath = path;
|
||||
if (text === undefined) {
|
||||
const userPath = userFlowDefinitionPath(this.bootstrap.homeDir, args.flow);
|
||||
try {
|
||||
text = await this.hostFs.readText(userPath);
|
||||
sourcePath = userPath;
|
||||
} catch {
|
||||
return {
|
||||
isError: true,
|
||||
output: `Could not read the flow definition at ${path} or ${userPath}. Check that the file exists under ${FLOWS_PROJECT_DIR}/ or ${userFlowsDir(this.bootstrap.homeDir)}/.`,
|
||||
};
|
||||
}
|
||||
const project =
|
||||
projectText === undefined ? undefined : this.validateDefinition(projectText, path, args.flow);
|
||||
if (project?.definition !== undefined) {
|
||||
return this.startRun(project.definition, args);
|
||||
}
|
||||
|
||||
const userPath = userFlowDefinitionPath(this.bootstrap.homeDir, args.flow);
|
||||
let userText: string | undefined;
|
||||
try {
|
||||
userText = await this.hostFs.readText(userPath);
|
||||
} catch {
|
||||
userText = undefined;
|
||||
}
|
||||
const user =
|
||||
userText === undefined ? undefined : this.validateDefinition(userText, userPath, args.flow);
|
||||
if (user?.definition !== undefined) {
|
||||
return this.startRun(user.definition, args);
|
||||
}
|
||||
|
||||
if (project !== undefined) return { isError: true, output: project.error! };
|
||||
if (user !== undefined) return { isError: true, output: user.error! };
|
||||
return {
|
||||
isError: true,
|
||||
output: `Could not read the flow definition at ${path} or ${userPath}. Check that the file exists under ${FLOWS_PROJECT_DIR}/ or ${userFlowsDir(this.bootstrap.homeDir)}/.`,
|
||||
};
|
||||
}
|
||||
|
||||
private validateDefinition(
|
||||
text: string,
|
||||
sourcePath: string,
|
||||
flowId: string,
|
||||
): { definition?: FlowDefinition; error?: string } {
|
||||
let definition: FlowDefinition;
|
||||
try {
|
||||
definition = parseFlowDefinition(text);
|
||||
} catch (error) {
|
||||
if (error instanceof FlowDefinitionParseError) {
|
||||
return { isError: true, output: `${error.message} (${sourcePath})` };
|
||||
return { error: `${error.message} (${sourcePath})` };
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
|
||||
if (definition.id !== args.flow) {
|
||||
if (definition.id !== flowId) {
|
||||
return {
|
||||
isError: true,
|
||||
output: `The definition at ${sourcePath} declares id \`${definition.id}\`, which does not match the requested flow \`${args.flow}\`. Fix the file's id or request the flow by its declared id.`,
|
||||
error: `The definition at ${sourcePath} declares id \`${definition.id}\`, which does not match the requested flow \`${flowId}\`. Fix the file's id or request the flow by its declared id.`,
|
||||
};
|
||||
}
|
||||
return { definition };
|
||||
}
|
||||
|
||||
private startRun(definition: FlowDefinition, args: FlowStartInput): ExecutableToolResult {
|
||||
if (this.flow.hasPendingActivation()) {
|
||||
return {
|
||||
isError: true,
|
||||
|
|
|
|||
|
|
@ -291,7 +291,7 @@ describe('FullCompaction', () => {
|
|||
properties: expect.objectContaining({
|
||||
agent_id: 'main',
|
||||
source: 'manual',
|
||||
tokens_before: 3_313,
|
||||
tokens_before: 3_302,
|
||||
tokens_after: expect.any(Number),
|
||||
duration_ms: expect.any(Number),
|
||||
compacted_count: 6,
|
||||
|
|
@ -570,7 +570,7 @@ describe('FullCompaction', () => {
|
|||
session_id: 'test-session',
|
||||
cwd: dir,
|
||||
trigger: 'auto',
|
||||
token_count: 3_313,
|
||||
token_count: 3_302,
|
||||
});
|
||||
expect(post).toMatchObject({
|
||||
hook_event_name: 'PostCompact',
|
||||
|
|
@ -656,7 +656,7 @@ describe('FullCompaction', () => {
|
|||
event: 'compaction_finished',
|
||||
properties: expect.objectContaining({
|
||||
source: 'manual',
|
||||
tokens_before: 14_991,
|
||||
tokens_before: 14_980,
|
||||
retry_count: 1,
|
||||
trace_id: 'trace-compact-1',
|
||||
}),
|
||||
|
|
@ -1039,7 +1039,7 @@ describe('FullCompaction', () => {
|
|||
properties: expect.objectContaining({
|
||||
agent_id: 'main',
|
||||
source: 'manual',
|
||||
tokens_before: 14_991,
|
||||
tokens_before: 14_980,
|
||||
duration_ms: expect.any(Number),
|
||||
round: 1,
|
||||
retry_count: 0,
|
||||
|
|
@ -1264,7 +1264,7 @@ describe('FullCompaction', () => {
|
|||
event: 'compaction_failed',
|
||||
properties: expect.objectContaining({
|
||||
source: 'manual',
|
||||
tokens_before: 14_991,
|
||||
tokens_before: 14_980,
|
||||
duration_ms: expect.any(Number),
|
||||
retry_count: 4,
|
||||
error_type: 'APIConnectionError',
|
||||
|
|
@ -1637,8 +1637,8 @@ describe('FullCompaction', () => {
|
|||
event: 'compaction_finished',
|
||||
properties: expect.objectContaining({
|
||||
source: 'auto',
|
||||
tokens_before: 3_320,
|
||||
tokens_after: 3_304,
|
||||
tokens_before: 3_309,
|
||||
tokens_after: 3_293,
|
||||
compacted_count: 7,
|
||||
retry_count: 0,
|
||||
}),
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -74,6 +74,7 @@ describe('AgentFlowService', () => {
|
|||
let contextMessages: ContextMessage[];
|
||||
let configHandlers: ((e: ConfigChangedEvent) => void)[];
|
||||
let flowToolSource: 'builtin' | 'user';
|
||||
let todoToolSource: 'builtin' | 'user';
|
||||
|
||||
beforeEach(() => {
|
||||
disposables = new DisposableStore();
|
||||
|
|
@ -96,13 +97,16 @@ describe('AgentFlowService', () => {
|
|||
homeDir: '/home/.kimi-code',
|
||||
} as unknown as IBootstrapService);
|
||||
ix.stub(IAgentToolRegistryService, {
|
||||
listReferences: () =>
|
||||
['FlowStart', 'FlowAdvance', 'FlowAbort', 'FlowJump'].map((name) => ({
|
||||
listReferences: () => [
|
||||
...['FlowStart', 'FlowAdvance', 'FlowAbort', 'FlowJump'].map((name) => ({
|
||||
name,
|
||||
source: flowToolSource,
|
||||
})),
|
||||
{ name: 'TodoList', source: todoToolSource },
|
||||
],
|
||||
} as unknown as IAgentToolRegistryService);
|
||||
flowToolSource = 'builtin';
|
||||
todoToolSource = 'builtin';
|
||||
activationDataStore = new Map();
|
||||
contextMessages = [];
|
||||
configHandlers = [];
|
||||
|
|
@ -310,6 +314,20 @@ describe('AgentFlowService', () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it('does not publish a flow status for a worker-agent undo', () => {
|
||||
agentId = 'worker-1';
|
||||
const seen: unknown[] = [];
|
||||
disposables.add(
|
||||
ix.get(IEventBus).subscribe((e) => {
|
||||
if (e.type === 'agent.status.updated') {
|
||||
seen.push((e as AgentStatusUpdated).flowRun);
|
||||
}
|
||||
}),
|
||||
);
|
||||
ix.get(IEventBus).publish(new ContextUndone({ turns: 1 }));
|
||||
expect(seen).toEqual([]);
|
||||
});
|
||||
|
||||
it('jump moves the pointer, records the audit entry, and bumps the epoch', () => {
|
||||
service.start(DEFINITION, 'task');
|
||||
const epochBefore = service.runEpoch();
|
||||
|
|
@ -785,6 +803,48 @@ describe('AgentFlowService', () => {
|
|||
expect(decision?.veto?.isError).toBe(true);
|
||||
expect(decision?.veto?.output).toContain('flow run');
|
||||
});
|
||||
|
||||
it('leaves a shadowing TodoList registration alone during a run', async () => {
|
||||
todoToolSource = 'user';
|
||||
service.start(DEFINITION, 'task');
|
||||
const todoCall: ToolCall = {
|
||||
type: 'function',
|
||||
id: 'call_todo_shadow',
|
||||
name: 'TodoList',
|
||||
arguments: '{}',
|
||||
};
|
||||
const context: ResolvedToolExecutionHookContext = {
|
||||
turnId: 0,
|
||||
signal,
|
||||
toolCall: todoCall,
|
||||
toolCalls: [todoCall],
|
||||
args: {},
|
||||
execution: { approvalRule: 'TodoList', execute: async () => ({ output: '' }) },
|
||||
};
|
||||
expect(await executorEvents.fireBeforeExecute(context)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('ignores a shadowing FlowStart when judging a batched TodoList', async () => {
|
||||
flowToolSource = 'user';
|
||||
const todoCall: ToolCall = {
|
||||
type: 'function',
|
||||
id: 'call_todo_batch',
|
||||
name: 'TodoList',
|
||||
arguments: '{}',
|
||||
};
|
||||
const context: ResolvedToolExecutionHookContext = {
|
||||
turnId: 0,
|
||||
signal,
|
||||
toolCall: todoCall,
|
||||
toolCalls: [
|
||||
todoCall,
|
||||
{ type: 'function', id: 'call_start_shadow', name: 'FlowStart', arguments: '{}' },
|
||||
],
|
||||
args: {},
|
||||
execution: { approvalRule: 'TodoList', execute: async () => ({ output: '' }) },
|
||||
};
|
||||
expect(await executorEvents.fireBeforeExecute(context)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('FlowAdvance human-gate guard', () => {
|
||||
|
|
|
|||
|
|
@ -147,4 +147,28 @@ describe('FlowStartTool', () => {
|
|||
expect(result.output).toContain('/home/.kimi-code/flows/issue-fix.md');
|
||||
expect(start).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('falls back to a valid user definition when the project file is invalid', async () => {
|
||||
fileText = 'not a flow definition';
|
||||
userFileText = VALID_DEFINITION;
|
||||
const result = await execute('issue-fix');
|
||||
expect(result.isError).not.toBe(true);
|
||||
expect(result.output).toContain('Flow run started: `issue-fix`');
|
||||
});
|
||||
|
||||
it('falls back to the user definition when the project id mismatches, and reports the project error when the user file is missing', async () => {
|
||||
fileText = VALID_DEFINITION.replace('id: issue-fix', 'id: other-flow');
|
||||
userFileText = VALID_DEFINITION;
|
||||
const fallback = await execute('issue-fix');
|
||||
expect(fallback.isError).not.toBe(true);
|
||||
expect(fallback.output).toContain('Flow run started: `issue-fix`');
|
||||
|
||||
start.mockClear();
|
||||
userFileText = undefined;
|
||||
active = false;
|
||||
const failed = await execute('issue-fix');
|
||||
expect(failed.isError).toBe(true);
|
||||
expect(failed.output).toContain('does not match');
|
||||
expect(start).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
Loading…
Add table
Add a link
Reference in a new issue