smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): restrict auto-learn tools to top-level sessions

Browse Source 
The `manage_skill`/`learn` force-include and `isToolAllowed` gates only
checked `autolearn.enabled`, not session depth. A subagent created with an
explicit `tools:[...]` whitelist (which runs `approvalMode: "yolo"`) would
silently gain write-capable tools that can mutate `~/.omp/agent/managed-skills`.
The auto-learn controller only runs for top-level sessions, so gate both the
force-include and `isToolAllowed` for `manage_skill`/`learn` to `taskDepth === 0`.

Also tidies the gating test file: drop a module-level `Bun.env` mutation that
was never restored (the test runner skips the Python preflight already) and a
no-op `afterEach` homedir spy in a block that never mocks homedir. Adds a
subagent-exclusion test.

Addresses review threads on PR #2542 (threads 2, 3, 8, 16).
This commit is contained in:
metaphorics
2026-06-14 12:36:31 +09:00
parent 9662844853
commit 34c7f106d9
2 changed files with 26 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/tools/index.ts
+7 -4
View File
@@ -527,9 +527,11 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P
}
// Auto-learn tools are gated by `autolearn.enabled` but, like the memory
// tools above, must also be force-included into an explicit requestedTools
// list so a restricted session whose controller/guidance is active still
// exposes the tools the nudge points at.
if (session.settings.get("autolearn.enabled")) {
// list so a restricted top-level session whose controller/guidance is
// active still exposes the tools the nudge points at. Gated to top-level
// (taskDepth 0): the controller only runs there, so a subagent's explicit
// tool whitelist must never be silently widened with write-capable tools.
if (session.settings.get("autolearn.enabled") && (session.taskDepth ?? 0) === 0) {
if (!requestedTools.includes("manage_skill")) requestedTools.push("manage_skill");
if (
["hindsight", "mnemopi", "local"].includes(session.settings.get("memory.backend") ?? "") &&
@@ -571,10 +573,11 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P
if (name === "retain" || name === "recall" || name === "reflect") {
return ["hindsight", "mnemopi"].includes(session.settings.get("memory.backend") ?? "");
}
if (name === "manage_skill") return session.settings.get("autolearn.enabled");
if (name === "manage_skill") return session.settings.get("autolearn.enabled") && (session.taskDepth ?? 0) === 0;
if (name === "learn") {
return (
session.settings.get("autolearn.enabled") &&
(session.taskDepth ?? 0) === 0 &&
["hindsight", "mnemopi", "local"].includes(session.settings.get("memory.backend") ?? "")
);
}
packages/coding-agent/test/autolearn-tools-gating.test.ts
+19 -6
View File
@@ -9,8 +9,6 @@ import { createTools, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
import { LearnTool } from "@oh-my-pi/pi-coding-agent/tools/learn";
import { ManageSkillTool } from "@oh-my-pi/pi-coding-agent/tools/manage-skill";
Bun.env.PI_PYTHON_SKIP_CHECK = "1";
function makeSession(
settingsOverrides: Partial<Record<SettingPath, unknown>> = {},
extra: Partial<ToolSession> = {},
@@ -27,10 +25,6 @@ function makeSession(
}
describe("autolearn tool gating", () => {
afterEach(() => {
spyOn(os, "homedir").mockRestore();
});
it("offers neither tool by default (autolearn disabled)", async () => {
const names = (await createTools(makeSession())).map(t => t.name);
expect(names).not.toContain("learn");
@@ -72,6 +66,25 @@ describe("autolearn tool gating", () => {
expect(noBackend).not.toContain("learn");
});
it("excludes the tools from a subagent even with an explicit list", async () => {
// taskDepth > 0: the controller never runs here, so a subagent's explicit
// whitelist must not be silently widened with write-capable tools.
const sub = (
await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" }, { taskDepth: 1 }), [
"read",
])
).map(t => t.name);
expect(sub).not.toContain("manage_skill");
expect(sub).not.toContain("learn");
// Nor via discovery (no explicit list) at depth.
const subDiscovered = (
await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "mnemopi" }, { taskDepth: 1 }))
).map(t => t.name);
expect(subDiscovered).not.toContain("manage_skill");
expect(subDiscovered).not.toContain("learn");
});
it("offers learn with the file-based local backend", async () => {
const names = (await createTools(makeSession({ "autolearn.enabled": true, "memory.backend": "local" }))).map(
t => t.name,