From 9b3c31473441b2d850effa4b6a4ba251bd3feafd Mon Sep 17 00:00:00 2001 From: Xialie Zhuang <62231346+Lieisyourlie@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:14:44 +0800 Subject: [PATCH] fix(agents): a model-supplied id must not be a LIKE wildcard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `memory pin`, `memory delete` and `skills delete` interpolate their positional argument straight into a LIKE pattern: path LIKE `memory/%/${id}.md` path LIKE `skills/${name}/%` `%` and `_` are wildcards there, and the argument comes from the model. `cumora memory delete %` becomes `path LIKE 'memory/%/%.md'` and removes every memory the agent has, then reports "deleted %". Verified against Postgres 16: three memories in, DELETE 3, none left. `_` is the quieter version — one character wide, so `mem-aaaaaaaa-11_` hits a real id the caller never named. `skills delete %` is the same shape one command over. Escape rather than reject: skill names are free text, and Postgres's default LIKE escape is a backslash reaching it as a bound parameter, so no ESCAPE clause is needed. Confirmed both directions — an escaped `%` stops matching arbitrary content, and a skill genuinely named "100%" still deletes itself and nothing else. Second defect in the same family, fixed here because it is the other half of the documented workflow: `memory list` printed `m.id.slice(0, 10)` while ids are minted 16 wide (`mem-` + 12 characters of a UUID). So the token the listing showed could never resolve — `memory/%/mem-c2ad15.md` does not match `memory/observation/mem-c2ad155a-56a.md`. `cumora help` documents list → pin/delete as the workflow, and it could not round-trip for any memory ever written. Print the full id. Four of the six integration cases go red against the shipped code. The two that pass either way are the guards: an exact id still resolves, and a literal `%` in a name still matches itself. --- .../agent-cli-like-wildcards.test.ts | 139 ++++++++++++++++++ server/src/agents/cli.ts | 24 ++- 2 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 server/src/__integration__/agent-cli-like-wildcards.test.ts diff --git a/server/src/__integration__/agent-cli-like-wildcards.test.ts b/server/src/__integration__/agent-cli-like-wildcards.test.ts new file mode 100644 index 00000000..3df54d69 --- /dev/null +++ b/server/src/__integration__/agent-cli-like-wildcards.test.ts @@ -0,0 +1,139 @@ +/** + * A memory id the model supplies must match literally, and the id the tool + * prints must be the id the tool accepts. + * + * Two defects in the same command family: + * + * 1. `memory pin` / `memory delete` interpolate the positional argument into a + * LIKE pattern — `memory/%/${id}.md`. `%` and `_` are wildcards there, so + * `cumora memory delete %` became `path LIKE 'memory/%/%.md'` and removed + * every memory the agent had, reporting "deleted %". Verified against + * Postgres 16 before the fix: three memories in, `DELETE 3`, none left. + * `skills delete` is the same shape one command over. + * + * 2. `memory list` printed `m.id.slice(0, 10)` while ids are minted 16 wide + * (`mem-` + 12 characters of a UUID). So the token the listing showed could + * never resolve: `path LIKE 'memory/%/mem-c2ad15.md'` does not match + * `memory/observation/mem-c2ad155a-56a.md`. `cumora help` documents + * list → pin/delete as the workflow, and it could not round-trip for any + * memory ever written. + * + * Run: INTEGRATION_DATABASE_URL=… npm run test:integration + */ +import { test, before, beforeEach, after } from 'node:test' +import assert from 'node:assert/strict' +import { pool } from '../db/pool.js' +import { ensureSchemaOnce, resetAllTables, seedCompanyWithAgent, teardownAll } from './_helpers.js' +import { runCli } from '../agents/cli.js' + +before(async () => { await ensureSchemaOnce() }) +beforeEach(async () => { await resetAllTables() }) +after(async () => { await teardownAll() }) + +async function seedMemories(agentId: string, companyId: string, ids: string[]): Promise { + for (const id of ids) { + await pool.query( + `INSERT INTO agent_workspace (agent_id, path, body, meta, company_id, updated_at) + VALUES ($1, $2, $3, '{}'::jsonb, $4, NOW())`, + [agentId, `memory/observation/${id}.md`, `body of ${id}`, companyId], + ) + } +} + +async function memoryCount(agentId: string): Promise { + const { rows } = await pool.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM agent_workspace + WHERE agent_id = $1 AND path LIKE 'memory/%'`, + [agentId], + ) + return Number(rows[0].count) +} + +// ── the wildcard ─────────────────────────────────────────────────────────── + +test('[integration] `memory delete %` deletes nothing', async () => { + const { agentId, companyId } = await seedCompanyWithAgent() + await seedMemories(agentId, companyId, ['mem-aaaaaaaa-111', 'mem-bbbbbbbb-222', 'mem-cccccccc-333']) + assert.equal(await memoryCount(agentId), 3) + + const res = await runCli(['--as', agentId, 'memory', 'delete', '%']) + assert.equal(res.ok, false, `expected a refusal, got: ${res.text}`) + assert.equal(await memoryCount(agentId), 3, 'a wildcard wiped the agent memory') +}) + +test('[integration] an underscore is a literal too', async () => { + // `_` matches exactly one character, so `mem-aaaaaaaa-11_` would have hit a + // real id — a subtler wipe than `%` and just as unintended. + const { agentId, companyId } = await seedCompanyWithAgent() + await seedMemories(agentId, companyId, ['mem-aaaaaaaa-111']) + + const res = await runCli(['--as', agentId, 'memory', 'delete', 'mem-aaaaaaaa-11_']) + assert.equal(res.ok, false, `expected a refusal, got: ${res.text}`) + assert.equal(await memoryCount(agentId), 1) +}) + +test('[integration] `skills delete %` deletes nothing', async () => { + const { agentId, companyId } = await seedCompanyWithAgent() + for (const path of ['skills/alpha/SKILL.md', 'skills/alpha/run.md', 'skills/beta/SKILL.md']) { + await pool.query( + `INSERT INTO agent_workspace (agent_id, path, body, company_id, updated_at) + VALUES ($1, $2, 'x', $3, NOW())`, + [agentId, path, companyId], + ) + } + const res = await runCli(['--as', agentId, 'skills', 'delete', '%']) + assert.equal(res.ok, false, `expected a refusal, got: ${res.text}`) + const { rows } = await pool.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM agent_workspace WHERE agent_id = $1 AND path LIKE 'skills/%'`, + [agentId], + ) + assert.equal(Number(rows[0].count), 3, 'a wildcard wiped the agent skills') +}) + +// ── and the ordinary path still works ────────────────────────────────────── + +test('[integration] an exact id still pins and deletes', async () => { + const { agentId, companyId } = await seedCompanyWithAgent() + await seedMemories(agentId, companyId, ['mem-aaaaaaaa-111', 'mem-bbbbbbbb-222']) + + const pin = await runCli(['--as', agentId, 'memory', 'pin', 'mem-aaaaaaaa-111']) + assert.equal(pin.ok, true, pin.text) + + const del = await runCli(['--as', agentId, 'memory', 'delete', 'mem-bbbbbbbb-222']) + assert.equal(del.ok, true, del.text) + assert.equal(await memoryCount(agentId), 1) +}) + +test('[integration] a name containing a percent matches only itself', async () => { + // Escaping must not make a legitimate literal unreachable. + const { agentId, companyId } = await seedCompanyWithAgent() + for (const path of ['skills/100%/SKILL.md', 'skills/other/SKILL.md']) { + await pool.query( + `INSERT INTO agent_workspace (agent_id, path, body, company_id, updated_at) + VALUES ($1, $2, 'x', $3, NOW())`, + [agentId, path, companyId], + ) + } + const res = await runCli(['--as', agentId, 'skills', 'delete', '100%']) + assert.equal(res.ok, true, res.text) + const { rows } = await pool.query<{ path: string }>( + `SELECT path FROM agent_workspace WHERE agent_id = $1 AND path LIKE 'skills/%' ORDER BY path`, + [agentId], + ) + assert.deepEqual(rows.map((r) => r.path), ['skills/other/SKILL.md']) +}) + +// ── the id the listing prints is the id the commands take ────────────────── + +test('[integration] the id shown by `memory list` round-trips into `memory pin`', async () => { + const { agentId, companyId } = await seedCompanyWithAgent() + await seedMemories(agentId, companyId, ['mem-c2ad155a-56a']) + + const list = await runCli(['--as', agentId, 'memory', 'list']) + assert.equal(list.ok, true, list.text) + const printed = list.text.match(/\[(mem-[^\]]+)\]/)?.[1] + assert.ok(printed, `no memory id in the listing:\n${list.text}`) + + const pin = await runCli(['--as', agentId, 'memory', 'pin', printed]) + assert.equal(pin.ok, true, `the id the listing printed does not resolve: ${pin.text}`) +}) diff --git a/server/src/agents/cli.ts b/server/src/agents/cli.ts index e3a5fffb..c76ddeae 100644 --- a/server/src/agents/cli.ts +++ b/server/src/agents/cli.ts @@ -2735,7 +2735,7 @@ on demand via \`cumora skills read ${name} references/\`._ const r = await pool.query( `DELETE FROM agent_workspace WHERE agent_id = $1 AND (path = $2 OR path LIKE $3)`, - [me, `skills/${name}/SKILL.md`, `skills/${name}/%`], + [me, `skills/${name}/SKILL.md`, `skills/${likeLiteral(name)}/%`], ) if ((r.rowCount ?? 0) === 0) return err(`no such skill: ${name}`) return ok(`deleted skill "${name}" (${r.rowCount} files removed)`, [{ @@ -4020,6 +4020,22 @@ function normalizeMemoryKind(raw: unknown): MemoryKind { * in the `meta` JSONB column. New writes stamp `source.conversationId` / * `source.projectId` (issue #45); existing `source: null` rows stay GLOBAL * — we never guess-migrate them into a project. See memory-scope.ts. */ +/** Escape LIKE metacharacters in a value that must match LITERALLY. + * + * These patterns are built from a positional argument the MODEL supplies, and + * `%` and `_` are wildcards to LIKE. `cumora memory delete %` became + * `path LIKE 'memory/%/%.md'` and removed every memory the agent had, then + * reported "deleted %" — verified against Postgres 16: three rows in, DELETE 3, + * none left. `cumora skills delete %` is the same shape one command over. + * + * Postgres's default LIKE escape is a backslash, and the pattern reaches it as + * a bound parameter, so escaping here is sufficient — no ESCAPE clause needed. + * Confirmed: `'…mem-aaa.md' LIKE 'memory/%/\%.md'` is false once escaped, and + * a literal `%` in a name still matches itself. */ +export function likeLiteral(value: string): string { + return value.replace(/[\\%_]/g, (ch) => `\\${ch}`) +} + async function cmdMemory(parsed: ParsedArgs): Promise { const op = parsed.positional[0] const me = resolveAs(parsed) @@ -4089,7 +4105,7 @@ async function cmdMemory(parsed: ParsedArgs): Promise { const t = new Date(m.created_at).toLocaleDateString() const pin = m.pinned ? '★ ' : ' ' const proj = m.projectId ? ` proj:${m.projectId}` : ' global' - return ` ${pin}[${m.id.slice(0, 10)}] ${m.kind.padEnd(11)} ${(m.about ?? '-').padEnd(10)} ${t}${proj}\n ${m.body.slice(0, 280).replace(/\n/g, ' \\n ')}` + return ` ${pin}[${m.id}] ${m.kind.padEnd(11)} ${(m.about ?? '-').padEnd(10)} ${t}${proj}\n ${m.body.slice(0, 280).replace(/\n/g, ' \\n ')}` }), ].join('\n')) } @@ -4147,7 +4163,7 @@ async function cmdMemory(parsed: ParsedArgs): Promise { || jsonb_build_object('pinned', NOT COALESCE((meta->>'pinned')::boolean, false)) WHERE agent_id = $1 AND path LIKE $2 RETURNING meta`, - [me, `memory/%/${id}.md`], + [me, `memory/%/${likeLiteral(id)}.md`], ) if ((r.rowCount ?? 0) === 0) return err(`no memory ${id} for ${me}`) return ok(`pinned: ${r.rows[0].meta?.pinned}`, [{ @@ -4163,7 +4179,7 @@ async function cmdMemory(parsed: ParsedArgs): Promise { if (!id) return err('usage: memory delete ') const r = await pool.query( `DELETE FROM agent_workspace WHERE agent_id = $1 AND path LIKE $2`, - [me, `memory/%/${id}.md`], + [me, `memory/%/${likeLiteral(id)}.md`], ) if ((r.rowCount ?? 0) === 0) return err(`no memory ${id} for ${me}`) return ok(`deleted ${id}`, [{