Skip to content

Commit 98abc5e

Browse files
chenanran555chenanran555
committed
fix(sdk): address memory and YAML review findings
1 parent f424d00 commit 98abc5e

11 files changed

Lines changed: 213 additions & 12 deletions

‎.changeset/fix-memory-store-conflict-name.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@openagentpack/sdk": patch
3+
---
4+
5+
Use the actual creation name when looking up a memory store after a creation conflict, preventing adoption of a different store named after the configuration key.

‎.changeset/preserve-environment-yaml-anchors.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@openagentpack/sdk": patch
3+
---
4+
5+
Preserve YAML anchors when redacting environment variable values so valid aliases remain usable in version previews and version snapshot checks. Stored and restored configuration retains its original values.

‎packages/sdk/src/internal/core/project-source-security.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { basename, dirname } from "node:path";
2-
import { isMap, isPair, isScalar, parseDocument, visit } from "yaml";
2+
import { isMap, isPair, isScalar, parseDocument, Scalar, visit } from "yaml";
33
import { resolveProjectConfigFromObject } from "../parser/resolve-project-config.ts";
44
import type { Diagnostic } from "../types/dto.ts";
55
import { validateProjectConfig } from "./validate-config.ts";
@@ -151,7 +151,7 @@ function redactEnvironmentVariableNodes(document: ReturnType<typeof parseDocumen
151151
references.add(entry.value);
152152
continue;
153153
}
154-
entry.value = document.createNode(REDACTED);
154+
entry.value = redactEnvironmentValue(entry.value);
155155
changed = true;
156156
}
157157
return;
@@ -160,12 +160,19 @@ function redactEnvironmentVariableNodes(document: ReturnType<typeof parseDocumen
160160
references.add(node.value);
161161
return;
162162
}
163-
node.value = document.createNode(REDACTED);
163+
node.value = redactEnvironmentValue(node.value);
164164
changed = true;
165165
});
166166
return { changed, references };
167167
}
168168

169+
function redactEnvironmentValue(value: unknown): Scalar<string> {
170+
const redacted = new Scalar(REDACTED);
171+
// References elsewhere in the preview must still resolve to the masked value.
172+
if (isScalar(value)) redacted.anchor = value.anchor;
173+
return redacted;
174+
}
175+
169176
function redactText(source: string, secrets: Set<string>): string {
170177
let redacted = source;
171178
for (const secret of [...secrets].sort((left, right) => right.length - left.length)) {

‎packages/sdk/src/internal/executor/executor.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,9 +592,11 @@ async function executeActionInner(
592592
// Explicit sync manages declared paths while preserving additional runtime memories.
593593
const current = new Map<string, { id: string; content_sha256: string }>();
594594
const seenCursors = new Set<string>();
595+
// Bailian's Memory-file listing has a lower limit than the other providers.
596+
const pageSize = address.provider === "bailian" ? 20 : 100;
595597
let cursor: string | undefined;
596598
do {
597-
const page = await provider.listMemories!(storeId, { limit: 100, cursor, view: "basic" }, apiMode);
599+
const page = await provider.listMemories!(storeId, { limit: pageSize, cursor, view: "basic" }, apiMode);
598600
for (const memory of page.data) {
599601
if (memory.type === "memory") current.set(memory.path, memory);
600602
}
@@ -639,6 +641,7 @@ async function executeActionInner(
639641
result = await createMemoryStore(remoteName, decl, apiMode);
640642
} catch (err) {
641643
result = await adoptOnConflict(err, address, provider, ctx.onFeedback, {
644+
searchNames: [remoteName],
642645
mode: apiMode,
643646
createOnly: ctx.createOnly,
644647
onExisting: async (existing) => reconcile(existing.id!),

‎packages/sdk/src/internal/executor/memory-initialization.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ async function existingMemoryPaths(provider: MemoryInitializerAdapter, storeId:
3232
const seenCursors = new Set<string>();
3333
let cursor: string | undefined;
3434
do {
35-
const page = await provider.listMemories(storeId, { limit: 100, cursor, view: "basic" }, mode);
35+
// Bailian's Memory-file listing accepts at most 20 entries per page.
36+
const page = await provider.listMemories(storeId, { limit: 20, cursor, view: "basic" }, mode);
3637
for (const memory of page.data) {
3738
if (memory.type === "memory") paths.add(memory.path);
3839
}

‎packages/sdk/src/internal/providers/bailian/adapter.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,8 @@ export class BailianAdapter implements ProviderAdapter {
309309
const seenCursors = new Set<string>();
310310
let cursor: string | undefined;
311311
do {
312-
const page = await this.listMemories(storeId, { limit: 100, page: cursor, view: "basic" });
312+
// Memory-file listing accepts at most 20 entries per page.
313+
const page = await this.listMemories(storeId, { limit: 20, page: cursor, view: "basic" });
313314
for (const memory of page.data) {
314315
if (memory.type === "memory" && hashes.has(memory.path)) {
315316
hashes.set(memory.path, memory.content_sha256);

‎packages/sdk/tests/unit/bailian-memory-iac.test.ts‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,13 @@ async function fixture(
3333
directories.push(directory);
3434
const statePath = join(directory, "state.json");
3535
const state = StateManager.initialize(statePath);
36-
const calls: Array<{ method: string; path: string; body?: Record<string, unknown> }> = [];
36+
const calls: Array<{
37+
method: string;
38+
path: string;
39+
body?: Record<string, unknown>;
40+
limit: string | null;
41+
page: string | null;
42+
}> = [];
3743
const memories = new Map<string, { id: string; content: string }>();
3844
let storeCount = 0;
3945
const remoteStore = {
@@ -61,7 +67,13 @@ async function fixture(
6167
const path = url.pathname.replace("/api/v1/agentstudio", "");
6268
const rawBody = await request.text();
6369
const body = rawBody ? (JSON.parse(rawBody) as Record<string, unknown>) : undefined;
64-
calls.push({ method: request.method, path, body });
70+
calls.push({
71+
method: request.method,
72+
path,
73+
body,
74+
limit: url.searchParams.get("limit"),
75+
page: url.searchParams.get("page"),
76+
});
6577
if (path === "/memory_stores" && request.method === "POST") {
6678
storeCount++;
6779
Object.assign(remoteStore, body, { id: `memstore_${storeCount}`, archived_at: null });
@@ -93,6 +105,10 @@ async function fixture(
93105
return Response.json({ id: memories.get(memoryPath)!.id, type: "memory", path: memoryPath });
94106
}
95107
if (path.endsWith("/memories")) {
108+
const limit = Number(url.searchParams.get("limit"));
109+
if (!Number.isInteger(limit) || limit < 1 || limit > 20) {
110+
return Response.json({ message: "limit must be between 1 and 20" }, { status: 400 });
111+
}
96112
const all = [...memories.entries()].map(([memoryPath, memory]) => ({
97113
id: memory.id,
98114
type: "memory",
@@ -157,6 +173,7 @@ describe("Bailian Memory Store IaC", () => {
157173
current.control.failPath = "/notes/second.md";
158174
const first = await current.apply();
159175
expect(first.partial).toBe(true);
176+
expect(first.results[0]?.error?.message).toContain("seed unavailable");
160177
const checkpoint = await StateManager.load(current.statePath);
161178
expect(checkpoint.getResource(address)?.remote_id).toBe("memstore_1");
162179
expect(checkpoint.getResource(address)?.memory_initialization?.pending_entries).toEqual([
@@ -179,6 +196,9 @@ describe("Bailian Memory Store IaC", () => {
179196
expect(current.memories.get("/notes/first.md")?.content).toBe("Agent's newer content");
180197
expect(current.memories.get("/notes/second.md")?.content).toBe("Agent wrote the pending path");
181198
expect(current.runtime.state.getResource(address)?.memory_initialization).toBeUndefined();
199+
const memoryReads = current.calls.filter((call) => call.method === "GET" && call.path.endsWith("/memories"));
200+
expect(memoryReads.every((call) => call.limit === "20")).toBe(true);
201+
expect(memoryReads.some((call) => call.page === "second-page")).toBe(true);
182202
current.memories.delete("/notes/first.md");
183203
current.runtime.config.memory_stores!.notes!.entries!.push({ key: "new.md", content: "Later initializer" });
184204
await current.apply();

‎packages/sdk/tests/unit/bailian-memory-sync-drift.test.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ const entries = [
1919
afterEach(() => setDefaultFetch(undefined));
2020

2121
function fixture(entriesMode: MemoryStoreDecl["entries_mode"] = "sync") {
22-
const calls: Array<{ method: string; path: string; page: string | null; view: string | null }> = [];
22+
const calls: Array<{ method: string; path: string; page: string | null; view: string | null; limit: string | null }> =
23+
[];
2324
const memories = new Map<string, { id: string; content: string }>();
2425
const control = { repeatCursor: false };
2526
const remoteStore = {
@@ -42,6 +43,7 @@ function fixture(entriesMode: MemoryStoreDecl["entries_mode"] = "sync") {
4243
path,
4344
page: url.searchParams.get("page"),
4445
view: url.searchParams.get("view"),
46+
limit: url.searchParams.get("limit"),
4547
});
4648
if (path === "/memory_stores" && request.method === "POST") return Response.json(remoteStore);
4749
if (path === "/memory_stores/memstore_notes") {
@@ -54,6 +56,10 @@ function fixture(entriesMode: MemoryStoreDecl["entries_mode"] = "sync") {
5456
memories.set(memoryPath, { id: `mem_${++memorySequence}`, content: body!.content as string });
5557
return Response.json({ id: memories.get(memoryPath)!.id, type: "memory", path: memoryPath });
5658
}
59+
const limit = Number(url.searchParams.get("limit"));
60+
if (!Number.isInteger(limit) || limit < 1 || limit > 20) {
61+
return Response.json({ message: "limit must be between 1 and 20" }, { status: 400 });
62+
}
5763
const all = [...memories.entries()].map(([memoryPath, memory]) => ({
5864
id: memory.id,
5965
type: "memory",
@@ -131,6 +137,7 @@ describe("Bailian explicit Memory sync drift", () => {
131137
expect((await current.refresh()).actions[0]?.action).toBe("no-op");
132138
const memoryReads = current.calls.filter((call) => call.method === "GET" && call.path.endsWith("/memories"));
133139
expect(memoryReads.every((call) => call.view === "basic")).toBe(true);
140+
expect(memoryReads.every((call) => call.limit === "20")).toBe(true);
134141
expect(memoryReads.some((call) => call.page === "opaque page +/1")).toBe(true);
135142
const serialized = JSON.stringify(current.runtime.state.getStateFile());
136143
for (const entry of entries) expect(serialized).not.toContain(entry.content);

‎packages/sdk/tests/unit/executor-conflict-adopt.test.ts‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ describe("executor conflict-adopt", () => {
310310

311311
expect(result.partial).toBe(false);
312312
expect(memoryProvider.findResource).toHaveBeenCalledTimes(1);
313+
expect(memoryProvider.findResource).toHaveBeenCalledWith("memory_store", "notes", undefined, "managed");
313314
expect(memoryProvider.updateMemoryStore).toHaveBeenCalledWith(
314315
"memstore_same_name",
315316
{ name: "notes", description: "Updated notes", metadata: {} },
@@ -323,6 +324,65 @@ describe("executor conflict-adopt", () => {
323324
expect(state.getResource(address)?.remote_id).toBe("memstore_same_name");
324325
});
325326

327+
test.each([
328+
true,
329+
false,
330+
])("memory store conflict only searches the actual creation name (matching store exists: %p)", async (matchingStoreExists) => {
331+
const memoryProvider = makeBailianMemoryProvider();
332+
memoryProvider.createMemoryStore.mockRejectedValue(new ConflictError(409, "exists", "Qoder API"));
333+
const findResource = mock(async (_resourceType: string, resourceName: string): Promise<RemoteResource | null> => {
334+
if (resourceName === "Shared notes" && matchingStoreExists) {
335+
return { id: "memstore_shared", type: "memory_store" };
336+
}
337+
if (resourceName === "notes") {
338+
return { id: "memstore_unrelated", type: "memory_store" };
339+
}
340+
return null;
341+
});
342+
const provider = {
343+
...memoryProvider.provider,
344+
name: "qoder",
345+
supportsResourceNameLookup: () => true,
346+
findResource,
347+
} as unknown as ProviderAdapter;
348+
const declaration: MemoryStoreDecl = { name: "Shared notes", description: "Updated notes" };
349+
const memoryConfig: ProjectConfig = {
350+
version: "1",
351+
providers: { qoder: {} },
352+
defaults: { provider: "qoder" },
353+
memory_stores: { notes: declaration },
354+
};
355+
const address = { ...memoryStoreAddress, provider: "qoder" };
356+
const plan: ExecutionPlan = {
357+
actions: [{ ...memoryStorePlan("create").actions[0]!, address }],
358+
diagnostics: [],
359+
};
360+
const state = StateManager.initialize(tmpPath());
361+
const result = await executePlan(plan, {
362+
config: memoryConfig,
363+
configPath: "/tmp/agents.yaml",
364+
providers: new Map([["qoder", provider]]),
365+
state,
366+
});
367+
368+
expect(memoryProvider.createMemoryStore).toHaveBeenCalledWith("Shared notes", declaration, "managed");
369+
expect(findResource).toHaveBeenCalledTimes(1);
370+
expect(findResource).toHaveBeenCalledWith("memory_store", "Shared notes", undefined, "managed");
371+
expect(result.partial).toBe(!matchingStoreExists);
372+
if (matchingStoreExists) {
373+
expect(memoryProvider.updateMemoryStore).toHaveBeenCalledWith(
374+
"memstore_shared",
375+
{ name: "Shared notes", description: "Updated notes", metadata: {} },
376+
"managed",
377+
);
378+
expect(state.getResource(address)?.remote_id).toBe("memstore_shared");
379+
} else {
380+
expect(memoryProvider.updateMemoryStore).not.toHaveBeenCalled();
381+
expect(state.getResource(address)).toBeUndefined();
382+
}
383+
expect(memoryProvider.deleteMemoryStore).not.toHaveBeenCalled();
384+
});
385+
326386
test("explicit sync memory store conflict reconciles seed entries without deleting learned memories", async () => {
327387
const calls: string[] = [];
328388
let storeMetadata: Record<string, string> | undefined;
@@ -348,7 +408,7 @@ describe("executor conflict-adopt", () => {
348408
updated_at: "",
349409
};
350410
},
351-
listMemories: async () => ({
411+
listMemories: mock(async () => ({
352412
data: [
353413
{
354414
id: "mem_seed",
@@ -374,7 +434,7 @@ describe("executor conflict-adopt", () => {
374434
},
375435
],
376436
has_more: false,
377-
}),
437+
})),
378438
updateMemory: async (_storeId: string, id: string) => {
379439
calls.push(`update-memory:${id}`);
380440
return { id, type: "memory" };
@@ -411,6 +471,11 @@ describe("executor conflict-adopt", () => {
411471
expect(result.partial).toBe(false);
412472
expect(calls).toEqual(["update-store:memstore_existing", "update-memory:mem_seed"]);
413473
expect(storeMetadata).toEqual({});
474+
expect(provider.listMemories).toHaveBeenCalledWith(
475+
"memstore_existing",
476+
expect.objectContaining({ limit: 100 }),
477+
"managed",
478+
);
414479
});
415480

416481
test("ConflictError → findResource finds existing → onExisting rebuilds via updateEnvironment", async () => {

‎packages/sdk/tests/unit/project-source-environment-redaction.test.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,52 @@ function previewDeployment(source: string): Record<string, unknown> {
3434
return parse(source).deployments.daily;
3535
}
3636

37-
describe("display-only Deployment environment variable redaction", () => {
37+
describe("display-only environment variable redaction", () => {
38+
test("keeps aliases valid when an Agent environment value defines an anchor", async () => {
39+
const source = `version: "1"
40+
providers:
41+
qoder: {}
42+
agents:
43+
assistant:
44+
model: synthetic-model
45+
instructions: Keep this configuration valid.
46+
environment_variables:
47+
MODE: &setting normal
48+
metadata:
49+
label: *setting
50+
`;
51+
const inspected = await inspectProjectSource(source, "/tmp/display-only-environment/agents.yaml");
52+
const preview = parse(inspected.redacted_source).agents.assistant;
53+
expect(inspected.diagnostics).toEqual([]);
54+
expect(preview.environment_variables.MODE).toBe("[redacted]");
55+
expect(preview.metadata.label).toBe("[redacted]");
56+
});
57+
58+
test("keeps aliases valid when a Qoder Deployment variable string defines an anchor", async () => {
59+
const source = `version: "1"
60+
providers:
61+
qoder: {}
62+
agents:
63+
assistant:
64+
model: synthetic-model
65+
instructions: Keep this configuration valid.
66+
deployments:
67+
daily:
68+
agent: assistant
69+
initial_events:
70+
- type: user.message
71+
content: Run the daily task.
72+
environment_variables: &settings MODE=normal
73+
metadata:
74+
label: *settings
75+
`;
76+
const inspected = await inspectProjectSource(source, "/tmp/display-only-environment/agents.yaml");
77+
const preview = previewDeployment(inspected.redacted_source);
78+
expect(inspected.diagnostics).toEqual([]);
79+
expect(preview.environment_variables).toBe("[redacted]");
80+
expect(preview.metadata).toEqual({ label: "[redacted]" });
81+
});
82+
3883
test("masks all map literals without rewriting names, references or unrelated short values", async () => {
3984
const variables = {
4085
LOG_LEVEL: "info",

0 commit comments

Comments
 (0)