-
Notifications
You must be signed in to change notification settings - Fork 956
fix(agent-core): honor [tools].disabled config in v1 engine #2537
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
base: main
Are you sure you want to change the base?
Changes from all commits
4212655
e232a3b
82958df
84112ee
35e3f23
a4e1651
42eb65f
7f6a47a
0d8505c
bf82349
dd3e923
0cbcda7
ef02f39
08fd35a
950c5ef
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 |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| --- | ||
| "@moonshot-ai/kimi-code": patch | ||
| "@moonshot-ai/kimi-code-sdk": patch | ||
| --- | ||
|
|
||
| fix(agent-core): honor [tools].disabled config in v1 engine | ||
|
|
||
| The `[tools].disabled` array in config.toml was silently ignored by the | ||
| v1 engine (v2 has a dedicated toolPolicy service for this). Read the | ||
| section from config.raw in bootstrapAgentProfile and merge it into the | ||
| profile's disallowedTools so disabled tools are filtered from both the | ||
| top-level tool list and the subagent Agent tool description. | ||
|
|
||
| agent-core is bundled into the SDK artifact (node-sdk `alwaysBundle`), so | ||
| in-process SDK consumers (createKimiHarness/SDKRpcClient v1 sessions) | ||
| that set `[tools].disabled` need the SDK version bumped to receive the | ||
| fix. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -450,7 +450,33 @@ export class Agent { | |
| ): void { | ||
| this.setActiveProfile(profile, brandHome); | ||
| this.updateSystemPromptFromProfile(profile, context, subagentNames); | ||
| // Persist only the profile's own denylist via setActiveTools (it is | ||
| // replayed on resume). The global [tools].disabled denylist is applied | ||
| // separately via addDisallowedTools, which mutates the deny set without | ||
| // logging a set_active_tools record - otherwise a config deny added at | ||
| // startup would be written into the wire record and survive a later | ||
| // config removal on cold resume. #2534. | ||
| this.tools.setActiveTools(profile.tools, profile.disallowedTools); | ||
| const toolsDisabled = this.configDisabledTools(); | ||
| if (toolsDisabled.length > 0) { | ||
| this.tools.addDisallowedTools(toolsDisabled); | ||
|
Comment on lines
+460
to
+462
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.
Applying AGENTS.md reference: AGENTS.md:L60-L60 Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
|
|
||
| /** Global [tools].disabled from options.config.raw, if provided. #2534. */ | ||
| private configDisabledTools(): string[] { | ||
| const tools = this.kimiConfig?.raw?.['tools']; | ||
| if ( | ||
| tools === undefined || | ||
| typeof tools !== 'object' || | ||
| tools === null || | ||
| !Array.isArray((tools as Record<string, unknown>)['disabled']) | ||
| ) { | ||
| return []; | ||
| } | ||
| return ((tools as Record<string, unknown>)['disabled'] as string[]).filter( | ||
| (v): v is string => typeof v === 'string', | ||
| ); | ||
| } | ||
|
|
||
| /** Push a refreshed session config snapshot and rebuild config-dependent builtin tools. */ | ||
|
|
@@ -532,6 +558,15 @@ export class Agent { | |
|
|
||
| async resume(options?: AgentRecordsReplayOptions): Promise<{ warning?: string }> { | ||
| const result = await this.records.replay(options); | ||
| // A standalone Agent (no Session) replays persisted tool state here but | ||
| // never routes through Session.restoreAgentProfileHandle, so re-apply the | ||
| // global [tools].disabled denylist non-persistently (mirrors useProfile). | ||
| // Without this a config deny added before resuming an existing Agent has | ||
| // no effect until the next useProfile / setActiveTools RPC. #2534. | ||
| const toolsDisabled = this.configDisabledTools(); | ||
| if (toolsDisabled.length > 0) { | ||
| this.tools.addDisallowedTools(toolsDisabled); | ||
| } | ||
| this.flushPendingAnthropicThinkingEffortWarnings(); | ||
| try { | ||
| this.replayBuilder.postRestoring = true; | ||
|
|
@@ -648,7 +683,16 @@ export class Agent { | |
| this.tools.unregisterUserTool(payload.name); | ||
| }, | ||
| setActiveTools: (payload) => { | ||
| // Persist the runtime selection only. The global [tools].disabled | ||
| // denylist is re-applied via addDisallowedTools (no record) so a | ||
| // runtime selection cannot reactivate a disabled tool, while keeping | ||
| // config-only denies out of the persisted set_active_tools record. | ||
| // #2534. | ||
| this.tools.setActiveTools(payload.names); | ||
| const toolsDisabled = this.configDisabledTools(); | ||
| if (toolsDisabled.length > 0) { | ||
| this.tools.addDisallowedTools(toolsDisabled); | ||
| } | ||
| }, | ||
| stopBackground: (payload) => { | ||
| void this.background.stop(payload.taskId, payload.reason); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -530,10 +530,15 @@ export class ToolManager { | |
| } | ||
|
|
||
| setActiveTools(names: readonly string[], disallowedNames?: readonly string[]): void { | ||
| // Callers compose [tools].disabled into an array, but an empty denylist | ||
| // carries no information; normalize to undefined so the serialized record | ||
| // omits it (matching the pre-config behavior) instead of changing every | ||
| // set_active_tools record with an empty disallowedNames. #2534. | ||
| this.agent.records.logRecord({ | ||
| type: 'tools.set_active_tools', | ||
| names, | ||
| disallowedNames, | ||
| disallowedNames: | ||
| disallowedNames && disallowedNames.length > 0 ? disallowedNames : undefined, | ||
| }); | ||
| // MCP entries are glob patterns gated separately; the rest are exact | ||
| // builtin/user tool names. The split keeps every caller on one string[]. | ||
|
|
@@ -552,6 +557,33 @@ export class ToolManager { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Add to the denied set without replacing the replayed enabled set. Used by | ||
| * the resume path so applying [tools].disabled to a restored agent does not | ||
| * drop unrelated replayed host/user tools or a previous runtime selection. | ||
| * #2534. | ||
| */ | ||
| addDisallowedTools(names: readonly string[]): void { | ||
| if (names.length === 0) return; | ||
| const denials = names.filter((name) => !isMcpToolName(name)); | ||
| const mcpDenies = names.filter((name) => isMcpToolName(name)); | ||
| if (denials.length > 0) { | ||
| this.disabledTools = new Set([...this.disabledTools, ...denials]); | ||
|
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.
When Useful? React with 👍 / 👎. |
||
| } | ||
| if (mcpDenies.length > 0) { | ||
| this.mcpDenyPatterns = [...this.mcpDenyPatterns, ...mcpDenies]; | ||
| } | ||
| // Builtin construction bakes `allowBackground` from the Task* trio into | ||
| // Bash/Agent (see setActiveTools). The resume path already rebuilt the | ||
| // builtins during wire replay, before these denies were applied, so a | ||
| // newly-denied Task* tool would leave allowBackground stuck on until the | ||
| // next setActiveTools. Rebuild here so the denylist takes effect on the | ||
| // already-constructed builtins. #2534. | ||
| if (this.agent.config.hasProvider) { | ||
| this.initializeBuiltinTools(); | ||
| } | ||
| } | ||
|
|
||
| copyLoopToolsFrom(source: ToolManager): void { | ||
| this.loopToolsOverride = source.loopTools; | ||
| } | ||
|
|
||
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.
When this core fix is consumed through the published SDK (for example SDKRpcClient/KimiHarness v1 sessions honoring
[tools].disabled), it will not reach npm users because@moonshot-ai/agent-coreis private andpackages/node-sdk/tsdown.config.tsbundles it into@moonshot-ai/kimi-code-sdk. With only@moonshot-ai/kimi-codelisted here, the release PR will version/publish the CLI but leave SDK consumers on the old behavior, so please add the SDK package to the changeset as well.Useful? React with 👍 / 👎.