Skip to content

Commit 3c5c885

Browse files
committed
tests
1 parent ae2626b commit 3c5c885

8 files changed

Lines changed: 307 additions & 11 deletions

File tree

TODO.md

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -664,7 +664,41 @@ storage, state, file-history) и умирает БЕЗ dispose (process.exit); r
664664
- navigation stack восстанавливается из persist-адаптера, prev после
665665
пересоздания сессии ведёт назад корректно.
666666

667-
## Найденные и исправленные баги (29 итого)
667+
## 29-й заход: логические edge cases (+4 теста, +4 бага, включая security)
668+
669+
Не броски, а вырожденные значения/пути, выведенные из чтения кода.
670+
671+
### Баг №30: maxToolCalls=0 выполнял ВСЕ тулы (slice(-0) ловушка)
672+
Файл: `src/client/ClientAgent.ts`
673+
- `toolCalls.slice(-maxToolCalls)` при 0 → slice(-0)===slice(0)===весь массив.
674+
«Отключить тулы нулём» вместо этого запускало их все. Исправление: явный
675+
клэмп (>0 ? slice : []) + при пустом наборе после клэмпа эмитить
676+
(tool-stripped) контент вместо зависания.
677+
678+
### Баг №31: keepMessages=0 отдавал модели ВСЮ историю (та же slice(-0))
679+
Файл: `src/client/ClientHistory.ts` — тот же slice(-0)===всё; исправлено явным
680+
клэмпом в пустое окно (0 = модель не видит ни одного common-сообщения).
681+
682+
### Баг №32: getState внутри setState-dispatchFn — дедлок
683+
Файл: `src/client/ClientState.ts`
684+
- read вставал в queued-очередь за незавершённым write (read-modify-write —
685+
типовой паттерн) → вечное зависание setState. Исправление: флаг _inDispatch;
686+
реентрантный read (внутри уже держащегося dispatch-лока) читает поле напрямую.
687+
Порядок fire-and-forget write→read сохранён (не обход очереди снаружи).
688+
689+
### Баг №33 (SECURITY): path traversal в persist через clientId/entityId
690+
Файл: `src/classes/Persist.ts`
691+
- `_getFilePath` = join(baseDir, entityName, `${entityId}.json`); entityId
692+
(clientId/stateName/...) — потенциально извне. clientId="../../x" через
693+
path.join вылезал из dump/ и мог перезаписать произвольный файл.
694+
- Исправление: санитизация entityId — `/`, `\`, `..``_`.
695+
696+
### Новые тесты (4), сьют вырос до 282/282 — test/spec/edgecases.test.mjs
697+
maxToolCalls=0 (0 тулов, без зависания); keepMessages=0 (пустое окно);
698+
реентрантный getState-в-setState (+сохранение порядка); path-traversal
699+
(двухпроцессный тест: запись НЕ вылезает за пределы cwd).
700+
701+
## Найденные и исправленные баги (33 итого)
668702

669703
### 1. Дедлок waitForOutput при functools-kit v4 (причина 39 упавших тестов)
670704
Файл: `src/client/ClientSwarm.ts`

src/classes/Persist.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,11 @@ class PersistBase<EntityName extends string = string> implements IPersistBase {
424424
* @private
425425
*/
426426
_getFilePath(entityId: EntityId): string {
427-
return join(this.baseDir, this.entityName, `${entityId}.json`);
427+
// entityId comes from clientId/stateName/etc. which may be attacker-supplied:
428+
// strip path separators and traversal so a value like "../../x" cannot escape
429+
// the entity directory and read/overwrite arbitrary files.
430+
const safeId = String(entityId).replace(/[/\\]/g, "_").replace(/\.\./g, "_");
431+
return join(this.baseDir, this.entityName, `${safeId}.json`);
428432
}
429433

430434
/**

src/client/ClientAgent.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,30 @@ const EXECUTE_FN = async (
412412
return;
413413
}
414414
}
415-
toolCalls = toolCalls.slice(-self.params.maxToolCalls);
415+
// slice(-0) === slice(0) returns the WHOLE array, so maxToolCalls=0 would
416+
// run every call instead of none — clamp explicitly.
417+
toolCalls =
418+
self.params.maxToolCalls > 0
419+
? toolCalls.slice(-self.params.maxToolCalls)
420+
: [];
421+
422+
if (!toolCalls.length) {
423+
// maxToolCalls=0 dropped every call: the model wanted tools but none may
424+
// run. Emit the (tool-stripped) content as the answer instead of leaving
425+
// the pending waitForOutput hanging forever.
426+
const result = await self.params.transform(
427+
message.content,
428+
self.params.clientId,
429+
self.params.agentName
430+
);
431+
await self.params.history.push({
432+
...message,
433+
tool_calls: [],
434+
agentName: self.params.agentName,
435+
});
436+
await self._emitOutput(mode, result, outputEpoch);
437+
return;
438+
}
416439

417440
{
418441
const priorityTool = toolCalls.find((call) =>

src/client/ClientHistory.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ export class ClientHistory implements IHistory {
198198
const systemMessages = systemMessagesRaw.filter(
199199
({ agentName }) => agentName === this.params.agentName
200200
);
201-
const commonMessages = commonMessagesRaw
201+
const filteredCommonMessages = commonMessagesRaw
202202
.map(({ content, tool_calls, ...other }) => ({
203203
...other,
204204
tool_calls,
@@ -216,8 +216,13 @@ export class ClientHistory implements IHistory {
216216
);
217217
return true;
218218
}
219-
})
220-
.slice(-this.params.keepMessages);
219+
});
220+
// slice(-0) === slice(0) keeps the WHOLE array, so keepMessages=0 would keep
221+
// all history instead of none — clamp explicitly to an empty window.
222+
const commonMessages =
223+
this.params.keepMessages > 0
224+
? filteredCommonMessages.slice(-this.params.keepMessages)
225+
: [];
221226
const assistantToolOutputCallSet = new Set<string>(
222227
commonMessages
223228
.filter(({ tool_call_id }) => !!tool_call_id)

src/client/ClientState.ts

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,13 @@ export class ClientState<State extends IStateData = IStateData>
100100
{
101101
public readonly stateChanged = new Subject<StateName>();
102102

103+
/**
104+
* True while a queued dispatch (read/write) is executing for this instance.
105+
* getState checks it to serve reentrant reads (getState inside a setState
106+
* dispatchFn) without re-entering the queue, which would deadlock.
107+
*/
108+
_inDispatch = false;
109+
103110
/**
104111
* The current state data, initialized as null and set during waitForInit.
105112
* Updated by setState and clearState, persisted via params.setState if provided.
@@ -110,10 +117,14 @@ export class ClientState<State extends IStateData = IStateData>
110117
* Queued dispatch function to read or write the state, delegating to DISPATCH_FN.
111118
* Ensures thread-safe state operations, supporting concurrent access from ClientAgent or tools.
112119
*/
113-
dispatch = queued(
114-
async (action: Action, payload) =>
115-
await DISPATCH_FN<State>(action, this, payload)
116-
) as (action: Action, payload?: DispatchFn<State>) => Promise<State>;
120+
dispatch = queued(async (action: Action, payload) => {
121+
this._inDispatch = true;
122+
try {
123+
return await DISPATCH_FN<State>(action, this, payload);
124+
} finally {
125+
this._inDispatch = false;
126+
}
127+
}) as (action: Action, payload?: DispatchFn<State>) => Promise<State>;
117128

118129
/**
119130
* Constructs a ClientState instance with the provided parameters.
@@ -256,7 +267,15 @@ export class ClientState<State extends IStateData = IStateData>
256267
this.params.logger.debug(
257268
`ClientState stateName=${this.params.stateName} clientId=${this.params.clientId} shared=${this.params.shared} getState`
258269
);
259-
await this.dispatch("read");
270+
// Reads normally go through the dispatch queue to observe writes in order.
271+
// A read issued from INSIDE a running write (getState within a setState
272+
// dispatchFn) would deadlock behind that write, so _inDispatch lets such a
273+
// reentrant read return the current field directly.
274+
if (this._inDispatch) {
275+
// already holding the dispatch lock: read the field synchronously
276+
} else {
277+
await this.dispatch("read");
278+
}
260279
if (this.params.callbacks?.onRead) {
261280
this.params.callbacks.onRead(
262281
this._state,

test/index.mjs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import "./spec/sweepguard.test.mjs";
4141
import "./spec/crashrecovery.test.mjs";
4242
import "./spec/finalguard.test.mjs";
4343
import "./spec/navprev.test.mjs";
44+
import "./spec/edgecases.test.mjs";
4445

4546
run(import.meta.url, () => {
4647
console.log("All tests are finished");

test/spec/edgecases.test.mjs

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
import { test } from "worker-testbed";
2+
import { execFileSync } from "node:child_process";
3+
import { mkdtempSync, writeFileSync, existsSync, rmSync } from "node:fs";
4+
import { tmpdir } from "node:os";
5+
import { join, dirname } from "node:path";
6+
import { fileURLToPath } from "node:url";
7+
8+
import {
9+
addAgent,
10+
addCompletion,
11+
addState,
12+
addSwarm,
13+
addTool,
14+
commitToolOutput,
15+
executeForce,
16+
session,
17+
setConfig,
18+
State,
19+
} from "../../build/index.mjs";
20+
import { randomString, sleep } from "functools-kit";
21+
22+
const HANG = Symbol("hang");
23+
const raceHang = (p, ms = 8000) => Promise.race([p, sleep(ms).then(() => HANG)]);
24+
25+
test("Will run no tools when maxToolCalls is zero", async ({ pass, fail }) => {
26+
setConfig({ CC_PERSIST_ENABLED_BY_DEFAULT: false });
27+
const executed = [];
28+
29+
addCompletion({
30+
completionName: "mock-completion",
31+
getCompletion: async ({ agentName, messages }) => {
32+
const [last] = messages.slice(-1);
33+
if (last.content === "start") {
34+
return {
35+
agentName,
36+
content: "no-tools-answer",
37+
role: "assistant",
38+
tool_calls: [
39+
{ function: { name: "e_tool", arguments: { t: "a" } } },
40+
{ function: { name: "e_tool", arguments: { t: "b" } } },
41+
{ function: { name: "e_tool", arguments: { t: "c" } } },
42+
],
43+
};
44+
}
45+
return { agentName, content: `echo:${last.content}`, role: "assistant" };
46+
},
47+
});
48+
addTool({
49+
toolName: "e_tool",
50+
validate: () => true,
51+
type: "function",
52+
function: { name: "e_tool", description: "", parameters: { type: "object", properties: {}, required: [] } },
53+
call: async ({ toolId, clientId, agentName, params, isLast }) => {
54+
executed.push(params.t);
55+
await commitToolOutput(toolId, "x", clientId, agentName);
56+
if (isLast) await executeForce("finish", clientId);
57+
},
58+
});
59+
const TEST_AGENT = addAgent({
60+
agentName: "test-agent",
61+
completion: "mock-completion",
62+
prompt: "",
63+
tools: ["e_tool"],
64+
maxToolCalls: 0,
65+
});
66+
const TEST_SWARM = addSwarm({ swarmName: "test-swarm", agentList: [TEST_AGENT], defaultAgent: TEST_AGENT });
67+
68+
const chatSession = session(randomString(), TEST_SWARM);
69+
const result = await raceHang(chatSession.complete("start"));
70+
await raceHang(chatSession.dispose());
71+
72+
if (executed.length === 0 && result === "no-tools-answer") {
73+
pass();
74+
return;
75+
}
76+
fail(`executed=${JSON.stringify(executed)} result=${String(result)}`);
77+
});
78+
79+
test("Will keep zero prior messages when keepMessages is zero", async ({ pass, fail }) => {
80+
setConfig({ CC_PERSIST_ENABLED_BY_DEFAULT: false });
81+
let windowSnapshot = null;
82+
83+
let turn = 0;
84+
addCompletion({
85+
completionName: "mock-completion",
86+
getCompletion: async ({ agentName, messages }) => {
87+
turn += 1;
88+
if (turn === 3) {
89+
windowSnapshot = messages.filter((m) => m.role !== "system").map((m) => m.content);
90+
}
91+
const [last] = messages.slice(-1) ?? [{ content: "none" }];
92+
return { agentName, content: `echo:${last?.content ?? "none"}`, role: "assistant" };
93+
},
94+
});
95+
const TEST_AGENT = addAgent({
96+
agentName: "test-agent",
97+
completion: "mock-completion",
98+
prompt: "",
99+
keepMessages: 0,
100+
});
101+
const TEST_SWARM = addSwarm({ swarmName: "test-swarm", agentList: [TEST_AGENT], defaultAgent: TEST_AGENT });
102+
103+
const chatSession = session(randomString(), TEST_SWARM);
104+
await raceHang(chatSession.complete("turn1"));
105+
await raceHang(chatSession.complete("turn2"));
106+
await raceHang(chatSession.complete("turn3"));
107+
await raceHang(chatSession.dispose());
108+
109+
// keepMessages=0 is a zero-width window: the model sees no common (user/
110+
// assistant) messages at all, not even the current turn's own message.
111+
if (Array.isArray(windowSnapshot) && windowSnapshot.length === 0) {
112+
pass();
113+
return;
114+
}
115+
fail(`window=${JSON.stringify(windowSnapshot)}`);
116+
});
117+
118+
test("Will serve reentrant getState inside setState without deadlock", async ({ pass, fail }) => {
119+
setConfig({ CC_PERSIST_ENABLED_BY_DEFAULT: false });
120+
121+
addCompletion({
122+
completionName: "mock-completion",
123+
getCompletion: async ({ agentName, messages }) => {
124+
const [last] = messages.slice(-1);
125+
return { agentName, content: `echo:${last.content}`, role: "assistant" };
126+
},
127+
});
128+
addState({ stateName: "test-state", getDefaultState: () => ({ v: 10 }) });
129+
const TEST_AGENT = addAgent({
130+
agentName: "test-agent",
131+
completion: "mock-completion",
132+
prompt: "",
133+
states: ["test-state"],
134+
});
135+
const TEST_SWARM = addSwarm({ swarmName: "test-swarm", agentList: [TEST_AGENT], defaultAgent: TEST_AGENT });
136+
137+
const CLIENT_ID = randomString();
138+
const chatSession = session(CLIENT_ID, TEST_SWARM);
139+
const context = { clientId: CLIENT_ID, agentName: TEST_AGENT, stateName: "test-state" };
140+
141+
const nested = await raceHang(
142+
State.setState(async (prev) => {
143+
const inner = await State.getState(context);
144+
return { v: prev.v + inner.v };
145+
}, context)
146+
);
147+
148+
// Fire-and-forget writes must still be observed in order by a following read.
149+
State.setState(() => ({ v: 100 }), context);
150+
State.setState((p) => ({ v: p.v + 1 }), context);
151+
State.setState((p) => ({ v: p.v + 1 }), context);
152+
const ordered = await raceHang(State.getState(context));
153+
await raceHang(chatSession.dispose());
154+
155+
if (nested !== HANG && nested.v === 20 && ordered !== HANG && ordered.v === 102) {
156+
pass();
157+
return;
158+
}
159+
fail(`nested=${JSON.stringify(nested)} ordered=${JSON.stringify(ordered)}`);
160+
});
161+
162+
test("Will contain persistence writes inside dump dir for traversal client ids", async ({ pass, fail }) => {
163+
const buildPath = join(
164+
dirname(fileURLToPath(import.meta.url)),
165+
"..",
166+
"..",
167+
"build",
168+
"index.mjs"
169+
);
170+
const workDir = mkdtempSync(join(tmpdir(), "swarm-traversal-"));
171+
const escapeTarget = join(workDir, "ESCAPED.json");
172+
const scriptPath = join(workDir, "scenario.mjs");
173+
const script = `
174+
import { addAgent, addCompletion, addState, addSwarm, session, setConfig, State } from ${JSON.stringify(buildPath)};
175+
setConfig({ CC_PERSIST_ENABLED_BY_DEFAULT: true });
176+
addState({ stateName: "st", getDefaultState: () => ({ v: 0 }), persist: true });
177+
addCompletion({ completionName: "c", getCompletion: async ({ agentName, messages }) => {
178+
const [l] = messages.slice(-1); return { agentName, content: "echo:" + l.content, role: "assistant" }; }});
179+
const A = addAgent({ agentName: "a", completion: "c", prompt: "", states: ["st"] });
180+
const S = addSwarm({ swarmName: "s", agentList: [A], defaultAgent: A, persist: true });
181+
const evil = "../../ESCAPED";
182+
const cs = session(evil, S);
183+
await State.setState(() => ({ v: 99 }), { clientId: evil, agentName: A, stateName: "st" });
184+
await cs.dispose();
185+
console.log("DONE");
186+
process.exit(0);
187+
`;
188+
writeFileSync(scriptPath, script);
189+
try {
190+
const out = execFileSync(process.execPath, [scriptPath], {
191+
cwd: workDir,
192+
encoding: "utf-8",
193+
timeout: 30_000,
194+
});
195+
const escaped = existsSync(escapeTarget);
196+
if (out.includes("DONE") && !escaped) {
197+
pass();
198+
return;
199+
}
200+
fail(`escaped=${escaped} out=${out}`);
201+
} finally {
202+
rmSync(workDir, { recursive: true, force: true });
203+
}
204+
});

types.d.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6482,6 +6482,12 @@ type Action = "read" | "write";
64826482
declare class ClientState<State extends IStateData = IStateData> implements IState<State>, IStateChangeEvent {
64836483
readonly params: IStateParams<State>;
64846484
readonly stateChanged: Subject<string>;
6485+
/**
6486+
* True while a queued dispatch (read/write) is executing for this instance.
6487+
* getState checks it to serve reentrant reads (getState inside a setState
6488+
* dispatchFn) without re-entering the queue, which would deadlock.
6489+
*/
6490+
_inDispatch: boolean;
64856491
/**
64866492
* The current state data, initialized as null and set during waitForInit.
64876493
* Updated by setState and clearState, persisted via params.setState if provided.

0 commit comments

Comments
 (0)