mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-20 01:01:53 +00:00
fix(serve): apply round-1 review fold-in 2a (HIGH + CodeQL) on PR 16
Round-1 inline review (#4249) flagged ~28 items across Copilot, wenshao, and CodeQL. This commit lands the HIGH-severity correctness fixes plus the two CodeQL polynomial-regex warnings. Validation tighten — `parseAgentConfig` + `parseAgentUpdates`: - Trim leading/trailing whitespace on `name` before passing to SubagentManager. `" tester "` would otherwise create a frontmatter name with spaces that case-insensitive lookups can never find. - Fail-closed (422 invalid_config) on present-but-wrong-type optional scalars: `model`, `color`, `approvalMode`, `background`. Previously malformed values silently dropped through validation, masking client-serialization bugs. - Validate `approvalMode` against the `APPROVAL_MODES` enum on both create and update; an unknown value used to 201 with the field silently omitted from the saved file. - `runConfig` is now whitelist-sanitized to `{ max_time_minutes, max_turns }` only; unknown keys are dropped, malformed values return 422. Previously the whole input object was persisted verbatim into YAML frontmatter. - `?scope=` query is fail-closed for repeated values (`?scope=workspace&scope=global`) — Express parses these as arrays which the previous `typeof === 'string'` check silently treated as absent, broadening DELETE/UPDATE semantics from one level to both. - Empty update body returns 400 invalid_config (previously rewrote the file + emitted a misleading `agent_changed` event). - No-op updates (every supplied field already matches `existing`) return 200 + `changed: false` and SKIP the file rewrite + event fan-out. Memory write helper — `writeContextFile.ts`: - Move whitespace-only no-op detection BEFORE `fs.mkdir`. Without this, an empty POST still created the parent directory and bumped its mtime even though `changed: false` was reported. - Replace two polynomial regex patterns flagged by CodeQL (`/^\s+|\s+$/g` and `/^\n+|\n+$/g`) with hand-rolled `while` loops. Same pattern auth.ts:120-125 already uses for the same CodeQL rule. SDK — `DaemonClient.ts` + `types.ts`: - `DaemonWriteMemoryResult` gains optional `changed?: boolean` so typed callers can suppress redundant cache invalidation on no-op appends. Optional for forward-compat with daemons that predate the field — undefined treats as "changed: true" (legacy contract). - `deleteWorkspaceAgent` only swallows 404 when the body's `code` is `agent_not_found`. A bare 404 (older daemon, misrouted proxy, generic gateway page) now throws — previously the SDK silently reported success even when the request never reached a route that understands workspace agents. - `updateWorkspaceAgent` adds an optional `scope` parameter mirroring `deleteWorkspaceAgent`, so callers can target the user- level definition when a project-level agent shadows it. Validation: - typecheck: cli + sdk-typescript clean - vitest: serve 357/357 + writeContextFile 12/12 = 369/369 passing (was 362; +7 new) - eslint: clean Explicitly NOT applying (out of scope per issue #4175 PR 16 review-resolution policy): - Copilot's "strict gate after body parser" finding — already documented as PR 15 review-resolved tradeoff at auth.ts:256-269.
This commit is contained in:
parent
3d5e605094
commit
134c43c821
6 changed files with 550 additions and 61 deletions
|
|
@ -500,4 +500,136 @@ describe('workspace agents routes', () => {
|
|||
expect(res.status).toBe(400);
|
||||
expect(res.body.code).toBe('invalid_client_id');
|
||||
});
|
||||
|
||||
it('trims leading/trailing whitespace on the agent name', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
const res = await request(app).post('/workspace/agents').send({
|
||||
name: ' trimmed-name ',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a trimmed name agent',
|
||||
scope: 'workspace',
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.agent.name).toBe('trimmed-name');
|
||||
// File on disk uses the trimmed name; the original-with-spaces
|
||||
// version must NOT exist (would otherwise be unfindable via
|
||||
// case-insensitive lookup).
|
||||
const onDisk = await fs.readFile(
|
||||
path.join(workspace, QWEN_DIR, 'agents', 'trimmed-name.md'),
|
||||
'utf8',
|
||||
);
|
||||
expect(onDisk).toContain('name: trimmed-name');
|
||||
});
|
||||
|
||||
it('returns 422 invalid_config when scalar field has wrong type on create', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
const res = await request(app).post('/workspace/agents').send({
|
||||
name: 'wrong-type',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a wrong-type test agent',
|
||||
scope: 'workspace',
|
||||
model: 123,
|
||||
});
|
||||
expect(res.status).toBe(422);
|
||||
expect(res.body.code).toBe('invalid_config');
|
||||
expect(res.body.error).toMatch(/model.*string/);
|
||||
});
|
||||
|
||||
it('returns 422 invalid_config for unknown approvalMode', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
const res = await request(app).post('/workspace/agents').send({
|
||||
name: 'bad-mode',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a bad-mode test agent',
|
||||
scope: 'workspace',
|
||||
approvalMode: 'rampage',
|
||||
});
|
||||
expect(res.status).toBe(422);
|
||||
expect(res.body.code).toBe('invalid_config');
|
||||
expect(res.body.error).toMatch(/approvalMode/);
|
||||
});
|
||||
|
||||
it('strips unknown runConfig keys and rejects malformed values', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
// Unknown keys are silently dropped, valid known keys preserved.
|
||||
const res = await request(app)
|
||||
.post('/workspace/agents')
|
||||
.send({
|
||||
name: 'run-config',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a run-config test agent',
|
||||
scope: 'workspace',
|
||||
runConfig: { max_turns: 5, mystery_field: 'oops' },
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.agent.runConfig).toEqual({ max_turns: 5 });
|
||||
|
||||
// Malformed known field fails closed.
|
||||
const res2 = await request(app)
|
||||
.post('/workspace/agents')
|
||||
.send({
|
||||
name: 'run-config-bad',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a run-config bad agent',
|
||||
scope: 'workspace',
|
||||
runConfig: { max_turns: -1 },
|
||||
});
|
||||
expect(res2.status).toBe(422);
|
||||
expect(res2.body.code).toBe('invalid_config');
|
||||
});
|
||||
|
||||
it('rejects 400 invalid_scope on repeated ?scope= query', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
// Express parses repeated query params as an array; we should
|
||||
// fail-closed rather than treating it as absent.
|
||||
const res = await request(app).delete(
|
||||
'/workspace/agents/some-name?scope=workspace&scope=global',
|
||||
);
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.code).toBe('invalid_scope');
|
||||
});
|
||||
|
||||
it('rejects 400 invalid_config for empty update body', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
await request(app).post('/workspace/agents').send({
|
||||
name: 'has-fields',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a has-fields test agent',
|
||||
scope: 'workspace',
|
||||
});
|
||||
const res = await request(app)
|
||||
.post('/workspace/agents/has-fields')
|
||||
.send({});
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.code).toBe('invalid_config');
|
||||
});
|
||||
|
||||
it('short-circuits no-op updates with changed: false and no event', async () => {
|
||||
const bridge = buildBridgeStub();
|
||||
const app = buildApp({ bridge, boundWorkspace: workspace });
|
||||
await request(app).post('/workspace/agents').send({
|
||||
name: 'noop-target',
|
||||
description: 'a description longer than ten chars',
|
||||
systemPrompt: 'you are a noop-target agent',
|
||||
scope: 'workspace',
|
||||
});
|
||||
const eventsBefore = (bridge as unknown as { events: RecordedEvent[] })
|
||||
.events.length;
|
||||
|
||||
const res = await request(app)
|
||||
.post('/workspace/agents/noop-target')
|
||||
.send({ description: 'a description longer than ten chars' });
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.changed).toBe(false);
|
||||
|
||||
// No new agent_changed event for the no-op update.
|
||||
const events = (bridge as unknown as { events: RecordedEvent[] }).events;
|
||||
expect(events.length).toBe(eventsBefore);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@
|
|||
|
||||
import type { Application, Request, RequestHandler, Response } from 'express';
|
||||
import {
|
||||
APPROVAL_MODES,
|
||||
BuiltinAgentRegistry,
|
||||
SubagentError,
|
||||
SubagentErrorCode,
|
||||
|
|
@ -245,8 +246,27 @@ export function mountWorkspaceAgentsRoutes(
|
|||
const updates = parseAgentUpdates(body, res);
|
||||
if (!updates) return;
|
||||
|
||||
const scopeQuery =
|
||||
typeof req.query['scope'] === 'string' ? req.query['scope'] : undefined;
|
||||
// Fail-closed on `?scope=` malformations. `req.query['scope']`
|
||||
// is `undefined` when absent, a `string` when supplied once, an
|
||||
// array when repeated (`?scope=workspace&scope=global`), or a
|
||||
// ParsedQs object on a nested form. We only accept a single
|
||||
// string; anything else returns `invalid_scope` rather than
|
||||
// silently treating "?scope=...&scope=..." as if no scope were
|
||||
// provided. Matches the fail-closed posture of
|
||||
// `parseMaxQueuedQuery` for the SSE route.
|
||||
const rawScope = req.query['scope'];
|
||||
let scopeQuery: string | undefined;
|
||||
if (rawScope === undefined) {
|
||||
scopeQuery = undefined;
|
||||
} else if (typeof rawScope === 'string') {
|
||||
scopeQuery = rawScope;
|
||||
} else {
|
||||
res.status(400).json({
|
||||
error: '`scope` query must be a single "workspace" or "global" value',
|
||||
code: 'invalid_scope',
|
||||
});
|
||||
return;
|
||||
}
|
||||
let preferredLevel: SubagentLevel | undefined;
|
||||
if (scopeQuery !== undefined) {
|
||||
if (scopeQuery !== 'workspace' && scopeQuery !== 'global') {
|
||||
|
|
@ -283,6 +303,32 @@ export function mountWorkspaceAgentsRoutes(
|
|||
return;
|
||||
}
|
||||
|
||||
// Empty / no-op update detection. An empty body or a body whose
|
||||
// recognized fields all match `existing` would otherwise rewrite
|
||||
// the file (mtime bump) AND fan out an `agent_changed` event for
|
||||
// a request that didn't change anything — the same misleading
|
||||
// signal the memory route avoids for whitespace-only appends.
|
||||
// Reject empty payloads with 400; short-circuit no-op updates
|
||||
// with 200 + `changed: false` so adapters can suppress redundant
|
||||
// toasts without re-fetching.
|
||||
if (Object.keys(updates).length === 0) {
|
||||
res.status(400).json({
|
||||
error:
|
||||
'`POST /workspace/agents/:agentType` requires at least one updatable field in the body',
|
||||
code: 'invalid_config',
|
||||
name: agentType,
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (isNoOpUpdate(existing, updates)) {
|
||||
res.status(200).json({
|
||||
ok: true,
|
||||
agent: toDetail(existing),
|
||||
changed: false,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
await manager.updateSubagent(agentType, updates, existing.level);
|
||||
} catch (err) {
|
||||
|
|
@ -363,7 +409,9 @@ export function mountWorkspaceAgentsRoutes(
|
|||
data: { change: 'updated', name: existing.name, level: eventLevel },
|
||||
...(originatorClientId ? { originatorClientId } : {}),
|
||||
});
|
||||
res.status(200).json({ ok: true, agent: toDetail(updated) });
|
||||
res
|
||||
.status(200)
|
||||
.json({ ok: true, agent: toDetail(updated), changed: true });
|
||||
},
|
||||
);
|
||||
|
||||
|
|
@ -383,8 +431,27 @@ export function mountWorkspaceAgentsRoutes(
|
|||
if (clientIdResult === null) return;
|
||||
const originatorClientId = clientIdResult;
|
||||
|
||||
const scopeQuery =
|
||||
typeof req.query['scope'] === 'string' ? req.query['scope'] : undefined;
|
||||
// Fail-closed on `?scope=` malformations. `req.query['scope']`
|
||||
// is `undefined` when absent, a `string` when supplied once, an
|
||||
// array when repeated (`?scope=workspace&scope=global`), or a
|
||||
// ParsedQs object on a nested form. We only accept a single
|
||||
// string; anything else returns `invalid_scope` rather than
|
||||
// silently treating "?scope=...&scope=..." as if no scope were
|
||||
// provided. Matches the fail-closed posture of
|
||||
// `parseMaxQueuedQuery` for the SSE route.
|
||||
const rawScope = req.query['scope'];
|
||||
let scopeQuery: string | undefined;
|
||||
if (rawScope === undefined) {
|
||||
scopeQuery = undefined;
|
||||
} else if (typeof rawScope === 'string') {
|
||||
scopeQuery = rawScope;
|
||||
} else {
|
||||
res.status(400).json({
|
||||
error: '`scope` query must be a single "workspace" or "global" value',
|
||||
code: 'invalid_scope',
|
||||
});
|
||||
return;
|
||||
}
|
||||
let scopedLevel: SubagentLevel | undefined;
|
||||
if (scopeQuery !== undefined) {
|
||||
if (scopeQuery !== 'workspace' && scopeQuery !== 'global') {
|
||||
|
|
@ -490,14 +557,22 @@ function parseAgentConfig(
|
|||
level: SubagentLevel,
|
||||
res: Response,
|
||||
): SubagentConfig | undefined {
|
||||
const name = body['name'];
|
||||
if (typeof name !== 'string' || name.trim().length === 0) {
|
||||
const rawName = body['name'];
|
||||
if (typeof rawName !== 'string' || rawName.trim().length === 0) {
|
||||
res.status(422).json({
|
||||
error: '`name` is required and must be a non-empty string',
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return undefined;
|
||||
}
|
||||
// Trim leading/trailing whitespace BEFORE storing. Without this, a
|
||||
// client posting `{ name: " tester " }` would land a file whose
|
||||
// frontmatter `name` field literally contains the spaces; the
|
||||
// resolver's case-insensitive cascade still wouldn't match `/agents/
|
||||
// tester` because the lookup name and the on-disk name differ.
|
||||
// Better to normalize at the boundary than carry untrimmed names
|
||||
// through validation + serialization.
|
||||
const name = rawName.trim();
|
||||
// Reject names that shadow a built-in subagent. Without this check a
|
||||
// client could `POST /workspace/agents { name: "general-purpose" }`
|
||||
// and write a project-level file at `<workspace>/.qwen/agents/
|
||||
|
|
@ -548,28 +623,45 @@ function parseAgentConfig(
|
|||
};
|
||||
if (tools !== undefined) config.tools = tools;
|
||||
if (disallowedTools !== undefined) config.disallowedTools = disallowedTools;
|
||||
|
||||
// Optional scalar fields. Present-but-wrong-type fails closed (422)
|
||||
// rather than silently dropping the field — `SubagentValidator`
|
||||
// doesn't reject these, and `serializeSubagent` only writes recognized
|
||||
// values, so without explicit validation a `model: 123` payload would
|
||||
// 201 with no `model` field on the file (masking client-serialization
|
||||
// bugs).
|
||||
if (rejectIfPresentWrongType(body, 'model', 'string', res)) return undefined;
|
||||
if (typeof body['model'] === 'string') config.model = body['model'];
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'color', 'string', res)) return undefined;
|
||||
if (typeof body['color'] === 'string') config.color = body['color'];
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'approvalMode', 'string', res)) {
|
||||
return undefined;
|
||||
}
|
||||
if (typeof body['approvalMode'] === 'string') {
|
||||
config.approvalMode = body['approvalMode'];
|
||||
}
|
||||
if (typeof body['background'] === 'boolean') {
|
||||
config.background = body['background'];
|
||||
}
|
||||
const runConfig = body['runConfig'];
|
||||
if (runConfig !== undefined) {
|
||||
if (
|
||||
typeof runConfig !== 'object' ||
|
||||
runConfig === null ||
|
||||
Array.isArray(runConfig)
|
||||
) {
|
||||
if (!APPROVAL_MODES.includes(body['approvalMode'] as never)) {
|
||||
res.status(422).json({
|
||||
error: '`runConfig` must be an object when provided',
|
||||
error: `\`approvalMode\` must be one of ${JSON.stringify(APPROVAL_MODES)}`,
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return undefined;
|
||||
}
|
||||
config.runConfig = runConfig as SubagentConfig['runConfig'];
|
||||
config.approvalMode = body['approvalMode'];
|
||||
}
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'background', 'boolean', res)) {
|
||||
return undefined;
|
||||
}
|
||||
if (typeof body['background'] === 'boolean') {
|
||||
config.background = body['background'];
|
||||
}
|
||||
|
||||
const runConfig = body['runConfig'];
|
||||
if (runConfig !== undefined) {
|
||||
const sanitized = sanitizeRunConfig(runConfig, res);
|
||||
if (sanitized === null) return undefined;
|
||||
config.runConfig = sanitized;
|
||||
}
|
||||
return config;
|
||||
}
|
||||
|
|
@ -615,32 +707,40 @@ function parseAgentUpdates(
|
|||
updates.disallowedTools = disallowedTools;
|
||||
}
|
||||
}
|
||||
if ('model' in body && typeof body['model'] === 'string') {
|
||||
updates.model = body['model'];
|
||||
// Optional scalar fields. Match the create-side fail-closed posture
|
||||
// so a typo like `model: 123` returns 422 instead of silently
|
||||
// succeeding with no model change.
|
||||
if (rejectIfPresentWrongType(body, 'model', 'string', res)) return undefined;
|
||||
if (typeof body['model'] === 'string') updates.model = body['model'];
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'color', 'string', res)) return undefined;
|
||||
if (typeof body['color'] === 'string') updates.color = body['color'];
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'approvalMode', 'string', res)) {
|
||||
return undefined;
|
||||
}
|
||||
if ('color' in body && typeof body['color'] === 'string') {
|
||||
updates.color = body['color'];
|
||||
}
|
||||
if ('approvalMode' in body && typeof body['approvalMode'] === 'string') {
|
||||
updates.approvalMode = body['approvalMode'];
|
||||
}
|
||||
if ('background' in body && typeof body['background'] === 'boolean') {
|
||||
updates.background = body['background'];
|
||||
}
|
||||
if ('runConfig' in body) {
|
||||
const runConfig = body['runConfig'];
|
||||
if (
|
||||
typeof runConfig !== 'object' ||
|
||||
runConfig === null ||
|
||||
Array.isArray(runConfig)
|
||||
) {
|
||||
if (typeof body['approvalMode'] === 'string') {
|
||||
if (!APPROVAL_MODES.includes(body['approvalMode'] as never)) {
|
||||
res.status(422).json({
|
||||
error: '`runConfig` must be an object when provided',
|
||||
error: `\`approvalMode\` must be one of ${JSON.stringify(APPROVAL_MODES)}`,
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return undefined;
|
||||
}
|
||||
updates.runConfig = runConfig as SubagentConfig['runConfig'];
|
||||
updates.approvalMode = body['approvalMode'];
|
||||
}
|
||||
|
||||
if (rejectIfPresentWrongType(body, 'background', 'boolean', res)) {
|
||||
return undefined;
|
||||
}
|
||||
if (typeof body['background'] === 'boolean') {
|
||||
updates.background = body['background'];
|
||||
}
|
||||
|
||||
if ('runConfig' in body) {
|
||||
const sanitized = sanitizeRunConfig(body['runConfig'], res);
|
||||
if (sanitized === null) return undefined;
|
||||
updates.runConfig = sanitized;
|
||||
}
|
||||
return updates;
|
||||
}
|
||||
|
|
@ -661,6 +761,165 @@ function parseStringArray(
|
|||
return value as string[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns `true` and sends a 422 when `body[key]` is present but the
|
||||
* wrong scalar type. The caller then returns `undefined` to short-
|
||||
* circuit the route. `false` covers both "absent" and "right type" so
|
||||
* the caller proceeds. Used to give scalar fields the same fail-closed
|
||||
* posture as `parseStringArray` / `sanitizeRunConfig`.
|
||||
*/
|
||||
function rejectIfPresentWrongType(
|
||||
body: Record<string, unknown>,
|
||||
key: string,
|
||||
expected: 'string' | 'boolean',
|
||||
res: Response,
|
||||
): boolean {
|
||||
if (!(key in body)) return false;
|
||||
if (typeof body[key] === expected) return false;
|
||||
res.status(422).json({
|
||||
error: `\`${key}\` must be a ${expected} when provided`,
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect a no-op update — every supplied field already matches the
|
||||
* existing agent's value. Without this check an empty (or
|
||||
* value-unchanged) PATCH still rewrites the file, bumps mtime, and
|
||||
* fans out a misleading `agent_changed` event. The recognized-field
|
||||
* comparison covers what `parseAgentUpdates` produces; unknown keys
|
||||
* are dropped upstream so we don't need to handle them here.
|
||||
*/
|
||||
function isNoOpUpdate(
|
||||
existing: SubagentConfig,
|
||||
updates: Partial<SubagentConfig>,
|
||||
): boolean {
|
||||
if (
|
||||
updates.description !== undefined &&
|
||||
updates.description !== existing.description
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
updates.systemPrompt !== undefined &&
|
||||
updates.systemPrompt !== existing.systemPrompt
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
updates.tools !== undefined &&
|
||||
!shallowArrayEqual(updates.tools, existing.tools)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
updates.disallowedTools !== undefined &&
|
||||
!shallowArrayEqual(updates.disallowedTools, existing.disallowedTools)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (updates.model !== undefined && updates.model !== existing.model) {
|
||||
return false;
|
||||
}
|
||||
if (updates.color !== undefined && updates.color !== existing.color) {
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
updates.approvalMode !== undefined &&
|
||||
updates.approvalMode !== existing.approvalMode
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
updates.background !== undefined &&
|
||||
updates.background !== existing.background
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (updates.runConfig !== undefined) {
|
||||
const e = existing.runConfig ?? {};
|
||||
const u = updates.runConfig;
|
||||
if (
|
||||
u['max_time_minutes'] !== e['max_time_minutes'] ||
|
||||
u['max_turns'] !== e['max_turns']
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
function shallowArrayEqual(
|
||||
a: readonly string[] | undefined,
|
||||
b: readonly string[] | undefined,
|
||||
): boolean {
|
||||
if (a === b) return true;
|
||||
if (!a || !b) return false;
|
||||
if (a.length !== b.length) return false;
|
||||
for (let i = 0; i < a.length; i++) if (a[i] !== b[i]) return false;
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sanitize `runConfig` to only the documented fields. Without this
|
||||
* filter `SubagentManager.serializeSubagent` writes whatever object the
|
||||
* client sent into the agent's frontmatter, including unknown or
|
||||
* YAML-sensitive keys that downstream parsers may choke on. Returning
|
||||
* a fresh whitelist-shaped object also makes the wire contract
|
||||
* self-documenting at the route boundary.
|
||||
*
|
||||
* - `undefined` is impossible here (caller checks `'runConfig' in body`).
|
||||
* - `null` (sent) → 422 invalid_config (the route handler converts
|
||||
* the null sentinel to a short-circuit).
|
||||
* - Right-shape object → returns a new object with only `max_time_minutes`
|
||||
* and `max_turns` if they validate as finite positive numbers.
|
||||
*/
|
||||
function sanitizeRunConfig(
|
||||
raw: unknown,
|
||||
res: Response,
|
||||
): SubagentConfig['runConfig'] | null {
|
||||
if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) {
|
||||
res.status(422).json({
|
||||
error: '`runConfig` must be an object when provided',
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return null;
|
||||
}
|
||||
const input = raw as Record<string, unknown>;
|
||||
const out: Record<string, unknown> = {};
|
||||
|
||||
if ('max_time_minutes' in input) {
|
||||
const v = input['max_time_minutes'];
|
||||
if (typeof v !== 'number' || !Number.isFinite(v) || v <= 0) {
|
||||
res.status(422).json({
|
||||
error:
|
||||
'`runConfig.max_time_minutes` must be a positive finite number when provided',
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return null;
|
||||
}
|
||||
out['max_time_minutes'] = v;
|
||||
}
|
||||
if ('max_turns' in input) {
|
||||
const v = input['max_turns'];
|
||||
if (
|
||||
typeof v !== 'number' ||
|
||||
!Number.isFinite(v) ||
|
||||
v <= 0 ||
|
||||
!Number.isInteger(v)
|
||||
) {
|
||||
res.status(422).json({
|
||||
error: '`runConfig.max_turns` must be a positive integer when provided',
|
||||
code: 'invalid_config',
|
||||
});
|
||||
return null;
|
||||
}
|
||||
out['max_turns'] = v;
|
||||
}
|
||||
return out as SubagentConfig['runConfig'];
|
||||
}
|
||||
|
||||
function toSummary(config: SubagentConfig): ServeWorkspaceAgentSummary {
|
||||
const summary: ServeWorkspaceAgentSummary = {
|
||||
kind: 'agent',
|
||||
|
|
|
|||
|
|
@ -217,4 +217,21 @@ describe('writeWorkspaceContextFile', () => {
|
|||
fs.access(path.join(workspace, DEFAULT_CONTEXT_FILENAME)),
|
||||
).rejects.toMatchObject({ code: 'ENOENT' });
|
||||
});
|
||||
|
||||
it('does not create the parent directory on a no-op append', async () => {
|
||||
// Whitespace-only append targeting a non-existent nested path
|
||||
// must NOT call fs.mkdir — the no-op detection short-circuits
|
||||
// BEFORE acquiring the lock or touching the filesystem. Without
|
||||
// this, an empty POST would still bump the parent directory's
|
||||
// mtime even though the helper reports `changed: false`.
|
||||
const nested = path.join(workspace, 'never-exists');
|
||||
const result = await writeWorkspaceContextFile({
|
||||
scope: 'workspace',
|
||||
mode: 'append',
|
||||
content: '\n\n',
|
||||
projectRoot: nested,
|
||||
});
|
||||
expect(result.changed).toBe(false);
|
||||
await expect(fs.access(nested)).rejects.toMatchObject({ code: 'ENOENT' });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -98,6 +98,22 @@ export async function writeWorkspaceContextFile(
|
|||
}
|
||||
const filePath = resolveContextFilePath(options.scope, options.projectRoot);
|
||||
|
||||
// Append-mode no-op detection BEFORE acquiring the lock or
|
||||
// creating directories. Re-writing the same bytes would bump
|
||||
// mtime + the parent dir mtime AND fan out a misleading
|
||||
// `memory_changed` event; whitespace-only POSTs from a flaky
|
||||
// pipeline shouldn't reach the filesystem at all.
|
||||
if (options.mode === 'append' && isWhitespaceOnly(options.content)) {
|
||||
let bytes = 0;
|
||||
try {
|
||||
const stat = await fs.stat(filePath);
|
||||
bytes = stat.size;
|
||||
} catch (err) {
|
||||
if (!isEnoent(err)) throw err;
|
||||
}
|
||||
return { filePath, bytesWritten: bytes, changed: false };
|
||||
}
|
||||
|
||||
// Hold the per-file mutex for the entire read-compose-write sequence.
|
||||
// Concurrent `POST /workspace/memory` appends from different SSE
|
||||
// clients would otherwise interleave reads and lose entries on the
|
||||
|
|
@ -119,23 +135,6 @@ export async function writeWorkspaceContextFile(
|
|||
};
|
||||
}
|
||||
|
||||
// Append mode. When the trimmed content is empty (whitespace-only
|
||||
// input from a flaky pipeline / accidental empty POST), short-circuit
|
||||
// BEFORE re-writing the file. Re-writing the same bytes would still
|
||||
// bump mtime AND trigger `memory_changed` SSE fan-out across every
|
||||
// subscribed client — a misleading "memory just changed" toast for a
|
||||
// request that changed nothing.
|
||||
if (options.content.replace(/^\s+|\s+$/g, '').length === 0) {
|
||||
let bytes = 0;
|
||||
try {
|
||||
const stat = await fs.stat(filePath);
|
||||
bytes = stat.size;
|
||||
} catch (err) {
|
||||
if (!isEnoent(err)) throw err;
|
||||
}
|
||||
return { filePath, bytesWritten: bytes, changed: false };
|
||||
}
|
||||
|
||||
const next = await composeAppendedContent(filePath, options.content);
|
||||
await fs.writeFile(filePath, next, { encoding: 'utf8', mode: 0o644 });
|
||||
return {
|
||||
|
|
@ -167,7 +166,7 @@ async function composeAppendedContent(
|
|||
if (!isEnoent(err)) throw err;
|
||||
}
|
||||
|
||||
const trimmed = newContent.replace(/^\n+|\n+$/g, '');
|
||||
const trimmed = trimNewlines(newContent);
|
||||
if (trimmed.length === 0) return existing;
|
||||
|
||||
if (existing.length === 0) {
|
||||
|
|
@ -191,3 +190,45 @@ function isEnoent(err: unknown): boolean {
|
|||
(err as { code?: string }).code === 'ENOENT'
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Hand-rolled `^\s+|\s+$` substitute. CodeQL's polynomial-regex
|
||||
* detector flags `\s+` with anchors as a ReDoS risk on
|
||||
* attacker-controlled input; the linear loop sidesteps the rule
|
||||
* without changing behavior. Mirrors the same pattern used by
|
||||
* `auth.ts:120-125` for header-credential parsing.
|
||||
*/
|
||||
function isWhitespaceOnly(s: string): boolean {
|
||||
for (let i = 0; i < s.length; i++) {
|
||||
const c = s.charCodeAt(i);
|
||||
// ASCII space, tab, line feed, carriage return, form feed,
|
||||
// vertical tab. All non-printable whitespace control chars the
|
||||
// route's "no-op append" check should treat as empty content.
|
||||
if (
|
||||
c !== 0x20 &&
|
||||
c !== 0x09 &&
|
||||
c !== 0x0a &&
|
||||
c !== 0x0d &&
|
||||
c !== 0x0c &&
|
||||
c !== 0x0b
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Hand-rolled `^\n+|\n+$` substitute. Same CodeQL rationale as
|
||||
* `isWhitespaceOnly`. Trims only `\n` so the section-header insert
|
||||
* path keeps its newline framing semantics — a leading `\t` in
|
||||
* `newContent` is preserved as part of the user's bullet, while
|
||||
* `\n\n- entry\n` collapses to `- entry`.
|
||||
*/
|
||||
function trimNewlines(s: string): string {
|
||||
let start = 0;
|
||||
let end = s.length;
|
||||
while (start < end && s.charCodeAt(start) === 0x0a) start++;
|
||||
while (end > start && s.charCodeAt(end - 1) === 0x0a) end--;
|
||||
return start === 0 && end === s.length ? s : s.slice(start, end);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -456,14 +456,24 @@ export class DaemonClient {
|
|||
* Update a project- or user-level subagent definition. Built-in /
|
||||
* extension / session-level agents are read-only and return 403
|
||||
* `agent_readonly`; missing agents return 404 `agent_not_found`.
|
||||
*
|
||||
* Optional `scope` mirrors the delete helper: when a project agent
|
||||
* shadows a user-level agent of the same name, pass
|
||||
* `{ scope: 'global' }` to update the user-level definition
|
||||
* specifically. Without the scope the daemon resolves through the
|
||||
* default precedence (project > user) and updates the project entry.
|
||||
*/
|
||||
async updateWorkspaceAgent(
|
||||
agentType: string,
|
||||
req: DaemonUpdateAgentRequest,
|
||||
opts: { scope?: 'workspace' | 'global' } = {},
|
||||
clientId?: string,
|
||||
): Promise<DaemonAgentMutationResult> {
|
||||
const url = opts.scope
|
||||
? `${this.baseUrl}/workspace/agents/${encodeURIComponent(agentType)}?scope=${encodeURIComponent(opts.scope)}`
|
||||
: `${this.baseUrl}/workspace/agents/${encodeURIComponent(agentType)}`;
|
||||
return await this.fetchWithTimeout(
|
||||
`${this.baseUrl}/workspace/agents/${encodeURIComponent(agentType)}`,
|
||||
url,
|
||||
{
|
||||
method: 'POST',
|
||||
headers: this.headers({ 'Content-Type': 'application/json' }, clientId),
|
||||
|
|
@ -502,7 +512,7 @@ export class DaemonClient {
|
|||
headers: this.headers({}, clientId),
|
||||
},
|
||||
async (res) => {
|
||||
if (res.status === 204 || res.status === 404) {
|
||||
if (res.status === 204) {
|
||||
try {
|
||||
await res.body?.cancel();
|
||||
} catch {
|
||||
|
|
@ -510,6 +520,23 @@ export class DaemonClient {
|
|||
}
|
||||
return;
|
||||
}
|
||||
// Treat as idempotent ONLY when the daemon explicitly says
|
||||
// `agent_not_found`. A bare 404 (e.g. an HTTP proxy returning
|
||||
// a generic page, an older daemon that doesn't know the
|
||||
// route, a misrouted load balancer) would otherwise be
|
||||
// silently swallowed and the SDK caller would believe the
|
||||
// agent was deleted when the request never reached a route
|
||||
// that understands workspace agents. Failing on non-
|
||||
// structured 404s makes routing errors visible.
|
||||
if (res.status === 404) {
|
||||
const err = await this.failOnError(
|
||||
res,
|
||||
'DELETE /workspace/agents/:agentType',
|
||||
);
|
||||
const body = err.body as { code?: unknown } | undefined;
|
||||
if (body && body.code === 'agent_not_found') return;
|
||||
throw err;
|
||||
}
|
||||
throw await this.failOnError(
|
||||
res,
|
||||
'DELETE /workspace/agents/:agentType',
|
||||
|
|
|
|||
|
|
@ -316,6 +316,19 @@ export interface DaemonWriteMemoryResult {
|
|||
filePath: string;
|
||||
bytesWritten: number;
|
||||
mode: 'append' | 'replace';
|
||||
/**
|
||||
* `true` when the daemon actually mutated the file on disk. `false`
|
||||
* for whitespace-only `append` requests that short-circuited
|
||||
* upstream — the route accepted the request as well-formed (200
|
||||
* OK) but the helper detected the trimmed content was empty and
|
||||
* skipped the write to avoid an mtime bump + a misleading
|
||||
* `memory_changed` event. SDK consumers can branch on this to
|
||||
* suppress redundant cache invalidation. Optional at the type
|
||||
* level for forward-compat with daemons that predate the field —
|
||||
* those return undefined and callers should treat that as
|
||||
* `changed: true` (the legacy contract).
|
||||
*/
|
||||
changed?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue