Skip to content

Commit e4cd738

Browse files
committed
chore: address copilot reviews
1 parent 89898b8 commit e4cd738

9 files changed

Lines changed: 174 additions & 19 deletions

File tree

README.md

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,11 @@ values. Canonical nested values take precedence when both forms are supplied.
186186

187187
### Retained Compatibility
188188

189-
The canonical hot-tier config shape is now `redis.*`. For backward
190-
compatibility, the loader still accepts legacy nested `falkordb.*` values. Only
191-
the original Graphiti top-level aliases remain supported. Precedence is:
189+
The canonical hot-tier config shape is `redis.*`. Only the original Graphiti
190+
top-level aliases remain supported for backward compatibility. Precedence is:
192191

193192
1. `redis.*` (canonical)
194-
2. `falkordb.*` (legacy nested compatibility)
195-
3. top-level Graphiti aliases such as `endpoint` and `groupIdPrefix`
193+
2. top-level Graphiti aliases such as `endpoint` and `groupIdPrefix`
196194

197195
### Legacy Top-Level Keys
198196

@@ -205,9 +203,7 @@ still accepted and map to their nested equivalents:
205203
| `groupIdPrefix` | `graphiti.groupIdPrefix` |
206204
| `driftThreshold` | `graphiti.driftThreshold` |
207205

208-
Legacy nested `falkordb.*` keys remain accepted as compatibility aliases for the
209-
same `redis.*` settings. Removed top-level Redis aliases are no longer
210-
supported.
206+
Removed top-level Redis aliases are no longer supported.
211207

212208
## How It Works
213209

src/config.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,4 +253,22 @@ describe("config", () => {
253253
assertEquals(config.graphiti.endpoint, "http://localhost:8000/mcp");
254254
assertEquals(config.redis.endpoint, "redis://localhost:6379");
255255
});
256+
257+
it("fails open based on stable discovery error code instead of message text", () => {
258+
setConfigExplorerAdapterForTesting(() => ({
259+
search() {
260+
throw new ConfigLoadError("different discovery wording", {
261+
code: "config-discovery-search",
262+
});
263+
},
264+
load() {
265+
return null;
266+
},
267+
}));
268+
269+
const config = loadConfig();
270+
271+
assertEquals(config.graphiti.endpoint, "http://localhost:8000/mcp");
272+
assertEquals(config.redis.endpoint, "redis://localhost:6379");
273+
});
256274
});

src/config.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,24 @@ const DEFAULT_CONFIG = {
2222

2323
type ConfigLoadResult = { config: unknown } | null;
2424

25+
type ConfigLoadErrorCode =
26+
| "config-discovery-init"
27+
| "config-discovery-search"
28+
| "config-file-load"
29+
| "config-invalid";
30+
2531
export class ConfigLoadError extends Error {
2632
override cause: unknown;
33+
readonly code: ConfigLoadErrorCode;
2734

28-
constructor(message: string, options?: { cause?: unknown }) {
35+
constructor(
36+
message: string,
37+
options: { cause?: unknown; code: ConfigLoadErrorCode },
38+
) {
2939
super(message);
3040
this.name = "ConfigLoadError";
3141
this.cause = options?.cause;
42+
this.code = options.code;
3243
}
3344
}
3445

@@ -117,6 +128,7 @@ const assertExplicitUrl = (
117128
if (isValidUrlString(value)) return;
118129
throw new ConfigLoadError(
119130
`Invalid Graphiti config value for ${fieldName}: expected a valid URL`,
131+
{ code: "config-invalid" },
120132
);
121133
};
122134

@@ -226,7 +238,7 @@ const getConfigExplorerAdapter = (): ConfigExplorerAdapter => {
226238
} catch (err) {
227239
throw new ConfigLoadError(
228240
"Unable to initialize Graphiti config discovery",
229-
{ cause: err },
241+
{ cause: err, code: "config-discovery-init" },
230242
);
231243
}
232244
};
@@ -244,7 +256,7 @@ const loadConfigFile = (
244256
if (err instanceof ConfigLoadError) throw err;
245257
throw new ConfigLoadError(
246258
`Unable to load Graphiti config file: ${filePath}`,
247-
{ cause: err },
259+
{ cause: err, code: "config-file-load" },
248260
);
249261
}
250262
};
@@ -270,6 +282,7 @@ const searchConfig = (
270282
if (err instanceof ConfigLoadError) throw err;
271283
throw new ConfigLoadError("Unable to discover Graphiti config", {
272284
cause: err,
285+
code: "config-discovery-search",
273286
});
274287
}
275288
};
@@ -287,10 +300,9 @@ const loadLegacyConfig = (
287300
};
288301

289302
const isDiscoveryInfrastructureFailure = (error: unknown): boolean =>
290-
error instanceof ConfigLoadError && (
291-
error.message === "Unable to initialize Graphiti config discovery" ||
292-
error.message === "Unable to discover Graphiti config"
293-
);
303+
error instanceof ConfigLoadError &&
304+
(error.code === "config-discovery-init" ||
305+
error.code === "config-discovery-search");
294306

295307
export function loadConfig(directory?: string): GraphitiConfig {
296308
try {

src/handlers/event.test.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1118,7 +1118,7 @@ describe("event handler", () => {
11181118
);
11191119
});
11201120

1121-
it("does not cache the default context-limit after a transient provider failure", async () => {
1121+
it("caches the fallback context-limit after a transient provider failure", async () => {
11221122
const sessionManager = new MockSessionManager();
11231123
const state = sessionManager.createDefaultState("group-1", "user-1");
11241124
sessionManager.setState("session-1", state);
@@ -1179,8 +1179,8 @@ describe("event handler", () => {
11791179
});
11801180
await flushPromises();
11811181

1182-
assertEquals(providerCalls, 2);
1183-
assertEquals(state.contextLimit, 123_456);
1182+
assertEquals(providerCalls, 1);
1183+
assertEquals(state.contextLimit, 200_000);
11841184
});
11851185

11861186
it("caches unknown provider/model misses to avoid repeated lookups", async () => {
@@ -1933,6 +1933,11 @@ describe("event handler", () => {
19331933
assertEquals(redisEvents.calls.length, 1);
19341934
assertEquals(redisEvents.calls[0].sessionId, "session-1");
19351935
assertEquals(redisEvents.calls[0].category, "file.read");
1936+
assertEquals(
1937+
sessionManager.activeMarks.includes("child-session"),
1938+
true,
1939+
);
1940+
assertEquals(sessionManager.activeMarks.includes("session-1"), true);
19361941
});
19371942

19381943
it("routes child assistant buffering and completion through the canonical parent session", async () => {

src/handlers/event.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,7 @@ export function createEventHandler(deps: EventHandlerDeps): EventHook {
400400
const { state, resolved, canonicalSessionId } = await sessionManager
401401
.resolveSessionState(sessionId);
402402
if (!resolved || !state?.isMain || !canonicalSessionId) return;
403+
sessionManager.markResolvedSessionActive(sessionId, canonicalSessionId);
403404

404405
for (
405406
const structured of extractStructuredEvents({

src/services/batch-drain.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,42 @@ describe("batch drain", () => {
465465

466466
const second = await drain.drainGroup("group-1", failingGraphiti as never);
467467
assertEquals(second.status, "dead-letter");
468+
assertEquals(second.drained, 0);
469+
assertEquals(await redis.getListLength(drainPendingKey("group-1")), 0);
470+
assertEquals(await redis.getListLength(drainDeadKey("group-1")), 1);
471+
});
472+
473+
it("reports only successfully ingested events when a batch dead-letters mid-batch", async () => {
474+
const { redis, events, drain } = await createDeps({
475+
drain: { batchSize: 2 },
476+
});
477+
const first = createSessionEvent("message", "user", {
478+
summary: "first",
479+
body: "first",
480+
});
481+
const second = createSessionEvent("message", "user", {
482+
summary: "second",
483+
body: "second",
484+
});
485+
await events.recordEvent("session-1", "group-1", first);
486+
await events.recordEvent("session-1", "group-1", second);
487+
await redis.setString(
488+
drainRetryKey("group-1", `${first.id}:${second.id}`),
489+
JSON.stringify({ attempts: 1, nextAttemptAt: 0 }),
490+
60,
491+
);
492+
493+
let calls = 0;
494+
const result = await drain.drainGroup("group-1", {
495+
addMemory() {
496+
calls += 1;
497+
if (calls === 2) {
498+
throw new Error("boom");
499+
}
500+
},
501+
} as never);
502+
503+
assertEquals(result, { status: "dead-letter", drained: 1 });
468504
assertEquals(await redis.getListLength(drainPendingKey("group-1")), 0);
469505
assertEquals(await redis.getListLength(drainDeadKey("group-1")), 1);
470506
});

src/services/batch-drain.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,10 @@ export class BatchDrainService {
243243

244244
if (attempts >= this.options.drainRetryMax) {
245245
const remainingEntries = batch.slice(checkpointedCount);
246+
let drainedCount = 0;
247+
for (const entry of batch.slice(0, checkpointedCount)) {
248+
if (drainableEntryIds.has(entry.event.id)) drainedCount += 1;
249+
}
246250
logger.warn("Moving drain batch to dead-letter", {
247251
groupId,
248252
eventIds: remainingEntries.map((entry) => entry.event.id),
@@ -254,7 +258,7 @@ export class BatchDrainService {
254258
batch,
255259
);
256260
await this.redis.deleteKey(drainRetryKey(groupId, batchKey));
257-
return { status: "dead-letter", drained: remainingEntries.length };
261+
return { status: "dead-letter", drained: drainedCount };
258262
}
259263

260264
await this.events.releaseClaim(groupId, claimed.claimToken);

src/services/context-limit.test.ts

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { assertEquals } from "jsr:@std/assert@^1.0.0";
2+
import { resolveContextLimit } from "./context-limit.ts";
3+
4+
Deno.test("resolveContextLimit caches fallback defaults after provider lookup failure", async () => {
5+
const cache = new Map<string, number>();
6+
let calls = 0;
7+
const client = {
8+
provider: {
9+
list: () => {
10+
calls += 1;
11+
return Promise.reject(new Error("provider unavailable"));
12+
},
13+
},
14+
};
15+
16+
assertEquals(
17+
await resolveContextLimit(
18+
"openai",
19+
"gpt-5",
20+
client as never,
21+
undefined,
22+
cache,
23+
),
24+
200_000,
25+
);
26+
assertEquals(
27+
await resolveContextLimit(
28+
"openai",
29+
"gpt-5",
30+
client as never,
31+
undefined,
32+
cache,
33+
),
34+
200_000,
35+
);
36+
37+
assertEquals(calls, 1);
38+
});
39+
40+
Deno.test("resolveContextLimit keeps fallback caches scoped per normalized directory", async () => {
41+
const cache = new Map<string, number>();
42+
const calls: string[] = [];
43+
const client = {
44+
provider: {
45+
list: ({ query }: { query?: { directory?: string } }) => {
46+
calls.push(query?.directory ?? "");
47+
return Promise.reject(new Error("provider unavailable"));
48+
},
49+
},
50+
};
51+
52+
assertEquals(
53+
await resolveContextLimit(
54+
"openai",
55+
"gpt-5",
56+
client as never,
57+
"/tmp/project-a",
58+
cache,
59+
),
60+
200_000,
61+
);
62+
assertEquals(
63+
await resolveContextLimit(
64+
"openai",
65+
"gpt-5",
66+
client as never,
67+
"/tmp/project-a",
68+
cache,
69+
),
70+
200_000,
71+
);
72+
assertEquals(
73+
await resolveContextLimit("openai", "gpt-5", client as never, " ", cache),
74+
200_000,
75+
);
76+
assertEquals(
77+
await resolveContextLimit("openai", "gpt-5", client as never, "", cache),
78+
200_000,
79+
);
80+
81+
assertEquals(calls, ["/tmp/project-a", ""]);
82+
});

src/services/context-limit.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ export async function resolveContextLimit(
5353
}
5454
} catch (err) {
5555
logger.warn("Failed to fetch provider context limit", err);
56+
cache.set(modelKey, UNKNOWN_CONTEXT_LIMIT);
5657
return DEFAULT_CONTEXT_LIMIT;
5758
}
5859

0 commit comments

Comments
 (0)