feat: Knowledge graph read loop — auto-prime agents with persistent memory#115
feat: Knowledge graph read loop — auto-prime agents with persistent memory#115glassBead-tc wants to merge 17 commits intomainfrom
Conversation
- server-factory.ts: preserve title and annotations fields on resource blocks in gateway content transformation. Previously stripped, breaking profile priming audience targeting. - tool-descriptions.ts: add knowledge, read_thoughts, and get_structure to GATEWAY_DESCRIPTION. Previously only listed in GATEWAY_TOOL.description but not in the registered tool description agents actually see. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- New knowledge_prime action returns curated markdown summary of recent entities, suitable for injection into agent context - Accepts optional limit, types filter, and since date - Returns compact markdown with entity name, type, and label - Includes graph stats header (total entities, relations) - Add knowledge_prime to operations catalog for discoverability - Fix listEntities sort: add created_at DESC tiebreaker when importance_score is equal (all scores are currently 0.5) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Agents now receive a curated knowledge graph summary when they call the cipher operation. Follows the profile priming pattern: - One-shot gating via sessionsPrimed set (scoped key) - Resource block with audience: ['assistant'] annotation - Graceful degradation when handler unavailable or graph empty - Skips priming text when no entities exist Also fixes server-factory annotation passthrough to use proper type assertion instead of Record<string, unknown>. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CLAUDE.md: add optional knowledge query step after cipher, note that knowledge context is auto-injected with cipher response - Team prompt template: annotate cipher step with knowledge priming note Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Greptile OverviewGreptile SummaryThis PR closes the knowledge-graph “read loop” by introducing a Key integration points are:
Merge blockers in the current implementation:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant McpServer as McpServer(registerTool: thoughtbox_gateway)
participant Gateway as GatewayHandler.handle()
participant ToolReg as ToolRegistry
participant Knowledge as KnowledgeHandler.processOperation()
participant Storage as FileSystemKnowledgeStorage
Client->>McpServer: CallTool thoughtbox_gateway {operation:"cipher"}
McpServer->>Gateway: handle(toolArgs, extra.sessionId)
Gateway->>ToolReg: getSessionStage(sessionId)
ToolReg-->>Gateway: STAGE_1_INIT_COMPLETE (or higher)
Gateway->>Gateway: handleCipher() => cipher text
Gateway-->>Client: content[0]=text(cipher)
alt knowledgeHandler enabled AND session not primed
Gateway->>Knowledge: processOperation({action:"knowledge_prime", limit:15})
Knowledge->>Storage: listEntities({created_after: since?, types?, limit})
Storage-->>Knowledge: Entity[]
Knowledge-->>Gateway: content=[{type:"text", text: markdown}, {type:"resource", ...opDef}]
Gateway->>Gateway: append resource block thoughtbox://knowledge/priming (audience=assistant)
Gateway-->>Client: content += resource(knowledge priming)
else already primed / disabled
Gateway-->>Client: no knowledge priming appended
end
|
Additional Comments (6)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 247:250
Comment:
**Knowledge priming not cleared**
`clearSession()` removes `sessionsPrimed` using the raw `mcpSessionId`, but knowledge priming stores keys like ```${mcpSessionId}:knowledge``` (`gateway-handler.ts:425-451`). This leaves knowledge priming keys behind, so `sessionsPrimed` can grow without bound and a reused sessionId would be treated as already primed. You likely want to delete the same key format you add (and/or separate sets for profile vs knowledge priming).
How can I resolve this? If you propose a fix, please make it concise.
The gateway-to-MCP response mapping for Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 407:416
Comment:
**Resource title still dropped**
The gateway-to-MCP response mapping for `resource` blocks still drops `resource.title` (it only forwards `uri`, `mimeType`, `text`, plus `annotations`). Since the PR adds a priming block with `title: 'Knowledge Graph Context'` (`gateway-handler.ts:437-446`), that title won’t reach clients. If profile priming relies on `title` for targeting as noted in the PR description, that’s still broken unless `title` is forwarded here too.
How can I resolve this? If you propose a fix, please make it concise.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 1051:1065
Comment:
**Missing knowledge_prime in help**
When `knowledge` is called without `args.action`, the `available_actions` list omits the newly added `knowledge_prime` action. That makes the new operation undiscoverable through the gateway’s own error guidance.
How can I resolve this? If you propose a fix, please make it concise.
Also appears at src/gateway/gateway-handler.ts:423-456. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 247:252
Comment:
**Knowledge priming never cleared**
`clearSession()` deletes `this.sessionsPrimed.delete(mcpSessionId)`, but knowledge priming stores keys as ```${mcpSessionId ?? '__default__'}:knowledge``` (added in the new cipher priming block). That means the `:knowledge` entries will remain in the Set forever, and a reused sessionId could be treated as already primed. Consider deleting both the base key and the suffixed key (and/or splitting profile vs knowledge priming into separate Sets).
Also appears at src/gateway/gateway-handler.ts:423-456.
How can I resolve this? If you propose a fix, please make it concise.
The gateway-to-MCP content transform for Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 403:416
Comment:
**Resource title still dropped**
The gateway-to-MCP content transform for `resource` blocks preserves `uri/mimeType/text` (and now `annotations`), but it still does not pass through `block.resource.title`. Since the new knowledge priming block sets `title: 'Knowledge Graph Context'` (and the PR description mentions title preservation), the title will be lost in the actual tool response.
How can I resolve this? If you propose a fix, please make it concise.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 1050:1065
Comment:
**Missing knowledge_prime in help**
When `args.action` is missing, the error payload’s `available_actions` list omits the newly added `knowledge_prime` action. This makes the new action undiscoverable via the gateway’s own guidance/error output.
How can I resolve this? If you propose a fix, please make it concise. |
- Clear scoped knowledge priming key in clearSession() to prevent Set growth and stale priming state on session reuse - Preserve resource.title in gateway-to-MCP content transform - Add knowledge_prime to available_actions error guidance Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Captures root causes, fixes, prevention strategies, and related documentation for the write-only knowledge graph issue (PR #115). Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
🤖 Augment PR SummarySummary: Closes the knowledge-graph “read loop” by letting agents read back curated, persistent memory. Changes:
Technical Notes: Priming is gated via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
src/server-factory.ts
Outdated
| mimeType: block.resource.mimeType, | ||
| text: block.resource.text, | ||
| }, | ||
| ...(block.resource.annotations ? { annotations: block.resource.annotations as { audience?: ("assistant" | "user")[]; priority?: number } } : {}), |
There was a problem hiding this comment.
This mapping now forwards annotations, but it still drops block.resource.title, so priming resources lose their titles in the MCP response. Consider passing title through as well to preserve the metadata produced by profile/knowledge priming.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in round 2 (commit c2a2225) — server-factory.ts line 414 now spreads block.resource.title when present. The annotations are also forwarded from both top-level and nested positions (lines 416-418).
src/gateway/gateway-handler.ts
Outdated
| } as ContentBlock); | ||
| } | ||
| } | ||
| this.sessionsPrimed.add(knowledgePrimingKey); |
There was a problem hiding this comment.
this.sessionsPrimed.add(knowledgePrimingKey) runs even when primeResult.isError is true, which can permanently suppress priming for the session after a soft-error result. Consider only marking the session as primed after a successful prime (or an explicit “no entities” response) so transient failures can retry.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — moved sessionsPrimed.add() outside the inner content-check block but still inside the non-error check. Now marks as primed on ANY non-error response (success or empty graph), allowing retry only on transient failures caught by the catch block.
src/knowledge/handler.ts
Outdated
| } | ||
|
|
||
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = args.limit ?? 15; |
There was a problem hiding this comment.
limit is accepted as-is; if a caller passes 0, storage.listEntities() currently treats it as falsy and omits the SQL LIMIT, potentially returning the entire graph. Consider clamping/validating limit to avoid unbounded output.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — handlePrime limit clamped to [1, 100], handleListEntities limit clamped to [1, 500] when provided. Different caps: priming is context-injected (100 max keeps token cost sane), entity listing is an explicit user query (500 max is generous but bounded).
src/knowledge/handler.ts
Outdated
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = args.limit ?? 15; | ||
| const types = args.types as string[] | undefined; | ||
| const since = args.since ? new Date(args.since) : undefined; |
There was a problem hiding this comment.
new Date(args.since) can produce an Invalid Date for bad input, and downstream getTime() becomes NaN, which may yield confusing/empty results. Consider validating since (and rejecting/ignoring invalid values) before passing it into storage filters.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
| } | ||
|
|
||
| // Knowledge priming: append curated knowledge summary on first cipher call per session |
There was a problem hiding this comment.
Since this adds a new one-shot injection path, consider adding a unit test to assert knowledge priming is included on the first cipher call per session and not re-injected on subsequent calls. That would help prevent regressions around sessionsPrimed gating.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Good suggestion — filed as follow-up. The profile priming path already has tests (gateway-profile-priming.test.ts, T-GP-1 through T-GP-6). Knowledge priming tests should follow the same pattern.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Knowledge priming: append curated knowledge summary on first cipher call per session | ||
| if (operation === 'cipher' && this.knowledgeHandler) { | ||
| const knowledgePrimingKey = `${mcpSessionId ?? '__default__'}:knowledge`; | ||
| if (!this.sessionsPrimed.has(knowledgePrimingKey)) { |
There was a problem hiding this comment.
Knowledge priming never gated
knowledgePrimingKey is ${mcpSessionId ?? '__default__'}:knowledge, but clearSession() only deletes ${mcpSessionId}:knowledge and never clears the __default__:knowledge key. If handle() is invoked without a session id (or if a transport doesn't provide one), the first cipher call will permanently disable knowledge priming for all subsequent such sessions on that server instance. You likely want clearSession() (or equivalent) to also clear __default__:knowledge, or avoid using a global __default__ key for per-session gating.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 424:427
Comment:
**Knowledge priming never gated**
`knowledgePrimingKey` is `${mcpSessionId ?? '__default__'}:knowledge`, but `clearSession()` only deletes `${mcpSessionId}:knowledge` and never clears the `__default__:knowledge` key. If `handle()` is invoked without a session id (or if a transport doesn't provide one), the first `cipher` call will permanently disable knowledge priming for all subsequent such sessions on that server instance. You likely want `clearSession()` (or equivalent) to also clear `__default__:knowledge`, or avoid using a global `__default__` key for per-session gating.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in round 3 (commit 2c18ebf) — added documentation to clearSession() explaining the key namespace scheme (${id} for profile, ${id}:knowledge for knowledge) and the __default__ lifecycle. The __default__ keys are self-cleaning by process exit in stdio mode and shared-by-design in HTTP mode. Wiring clearSession() to the transport close handler is tracked separately by thoughtbox-32q.
src/gateway/gateway-handler.ts
Outdated
| this.sessionsPrimed.add(knowledgePrimingKey); | ||
| } catch (err) { | ||
| // Knowledge priming is optional — don't fail the cipher operation | ||
| console.warn(`[Knowledge] Priming failed: ${(err as Error).message}`); | ||
| } |
There was a problem hiding this comment.
Priming set on failure
sessionsPrimed.add(knowledgePrimingKey) happens inside the try block and will run even if processOperation() returns an error payload or returns No knowledge graph...; this permanently suppresses priming retries for that session even though the priming didn't actually happen (e.g., transient storage/index error). Consider only adding the key after you successfully append the priming resource block.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 451:455
Comment:
**Priming set on failure**
`sessionsPrimed.add(knowledgePrimingKey)` happens inside the `try` block and will run even if `processOperation()` returns an error payload or returns `No knowledge graph...`; this permanently suppresses priming retries for that session even though the priming didn't actually happen (e.g., transient storage/index error). Consider only adding the key after you successfully append the priming resource block.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — sessionsPrimed.add() now runs after any non-error response from knowledge_prime, not just after successful content injection. Transient errors (caught) still allow retry.
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = args.limit ?? 15; | ||
| const types = args.types as string[] | undefined; | ||
| const since = args.since ? new Date(args.since) : undefined; | ||
|
|
||
| const entities = await this.storage.listEntities({ | ||
| types: types as any, | ||
| created_after: since, | ||
| limit, | ||
| }); |
There was a problem hiding this comment.
Invalid date not handled
since is parsed with new Date(args.since) and passed through to created_after. If the caller provides a non-ISO string, new Date(...) becomes an invalid date object, and listEntities() will end up binding NaN (getTime()) into the SQL filter (created_at >= ?), which in SQLite will make the predicate fail and return an empty set. This will look like “no entities” rather than a clear input error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 279:288
Comment:
**Invalid date not handled**
`since` is parsed with `new Date(args.since)` and passed through to `created_after`. If the caller provides a non-ISO string, `new Date(...)` becomes an invalid date object, and `listEntities()` will end up binding `NaN` (`getTime()`) into the SQL filter (`created_at >= ?`), which in SQLite will make the predicate fail and return an empty set. This will look like “no entities” rather than a clear input error.
How can I resolve this? If you propose a fix, please make it concise.
src/knowledge/handler.ts
Outdated
| const stats = await this.storage.getStats(); | ||
| const totalEntities = Object.values(stats.entity_counts).reduce((a, b) => a + b, 0); | ||
| const totalRelations = Object.values(stats.relation_counts).reduce((a, b) => a + b, 0); | ||
|
|
||
| const header = `## Prior Knowledge (${entities.length} of ${totalEntities} entities, ${totalRelations} relations)`; | ||
| const text = [header, '', ...lines].join('\n'); |
There was a problem hiding this comment.
Stats shape can throw
handlePrime() assumes stats.entity_counts and stats.relation_counts are always present and objects. If getStats() ever changes shape (or returns null-ish fields on partial init), Object.values(...).reduce(...) will throw and break knowledge priming (and thus cipher auto-injection). If this is intended to be resilient, consider defensive defaults before reducing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 305:310
Comment:
**Stats shape can throw**
`handlePrime()` assumes `stats.entity_counts` and `stats.relation_counts` are always present and objects. If `getStats()` ever changes shape (or returns null-ish fields on partial init), `Object.values(...).reduce(...)` will throw and break knowledge priming (and thus cipher auto-injection). If this is intended to be resilient, consider defensive defaults before reducing.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — added defensive default: (await this.storage.getStats()) ?? { entity_counts: {}, relation_counts: {} }. The existing ?? {} on the nested properties is preserved as a second layer of defense.
- Only mark knowledge priming as done after successful injection, not on error or empty graph (prevents permanent suppression) - Validate 'since' date parameter in knowledge_prime to surface invalid dates as errors instead of silent empty results - Add defensive defaults for stats shape in handlePrime to prevent throws on partial init Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Adds fixes 6-8 (date validation, defensive stats, priming-on-success-only) and prevention strategies 6-8 from Greptile review round 2. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
src/gateway/gateway-handler.ts
Outdated
| annotations: { | ||
| audience: ['assistant'], | ||
| priority: 0.6, | ||
| }, |
There was a problem hiding this comment.
Wrong resource field placement
In the knowledge priming injection block, annotations is nested under resource (lines 443-446). In MCP content blocks, annotations is a top-level field on the content block, not inside resource (your server-factory transform also expects block.resource.annotations, which won’t exist if annotations are placed correctly). As written, downstream consumers won’t see the audience/priority annotations, and the new preservation code in server-factory.ts won’t help because it only copies block.resource.annotations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 443:446
Comment:
**Wrong resource field placement**
In the knowledge priming injection block, `annotations` is nested under `resource` (lines 443-446). In MCP content blocks, `annotations` is a top-level field on the content block, not inside `resource` (your server-factory transform also expects `block.resource.annotations`, which won’t exist if annotations are placed correctly). As written, downstream consumers won’t see the audience/priority annotations, and the new preservation code in `server-factory.ts` won’t help because it only copies `block.resource.annotations`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Investigated: the current code is actually correct per MCP SDK EmbeddedResourceSchema — annotations IS at the top level of the content block (line 448), not nested inside resource. The inconsistency is that other handlers (profile-primer.ts, thought-handler.ts, sessions/index.ts) place annotations inside resource, but that's out of scope for this PR. The server-factory.ts transform already handles both placements as a belt-and-suspenders measure (lines 416-418).
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 1320:1324
Comment:
**Outdated resource description**
`thoughtbox://knowledge/operations` now includes the new `knowledge_prime` operation (see `getKnowledgeOperationsCatalog()` / `KNOWLEDGE_OPERATIONS`), but the resource description here still lists operations ending in `stats` only. Agents relying on resource descriptions for discovery will miss `knowledge_prime`. Update the description string to include `knowledge_prime`.
How can I resolve this? If you propose a fix, please make it concise. |
|
@roomote let's address these two issues raised by Greptile:
|
Fixed both issues: updated knowledge operations resource description to include knowledge_prime, and moved annotations to top-level in knowledge priming content block per MCP spec. |
…top-level in priming block
Additional Comments (1)
In the gateway tool response transform, annotations are only forwarded from This will happen whenever Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 403:417
Comment:
**Annotations never forwarded**
In the gateway tool response transform, annotations are only forwarded from `block.resource.annotations`, but the gateway now correctly emits annotations at the *top level* of the content block (`block.annotations`). As a result, the `audience/priority` annotations on the injected knowledge priming resource will still be dropped on the wire.
This will happen whenever `GatewayHandler` emits `{ type: 'resource', ..., annotations: {...} }` (e.g. knowledge priming), because `server-factory.ts` never copies `block.annotations` into the MCP content block.
How can I resolve this? If you propose a fix, please make it concise. |
|
@roomote let's fix this issue: In the gateway tool response transform, annotations are only forwarded from block.resource.annotations, but the gateway now correctly emits annotations at the top level of the content block (block.annotations). As a result, the audience/priority annotations on the injected knowledge priming resource will still be dropped on the wire. This will happen whenever GatewayHandler emits { type: 'resource', ..., annotations: {...} } (e.g. knowledge priming), because server-factory.ts never copies block.annotations into the MCP content block. |
Fixed the annotation passthrough issue. The gateway response transform now forwards |
Fixes annotation passthrough in server-factory.ts to check for and forward block.annotations (top-level) in addition to block.resource.annotations. This ensures audience/priority annotations on knowledge priming resources are preserved on the wire.
| const entityCounts = stats.entity_counts ?? {}; | ||
| const relationCounts = stats.relation_counts ?? {}; | ||
| const totalEntities = Object.values(entityCounts).reduce((a: number, b: number) => a + b, 0); |
There was a problem hiding this comment.
Priming can crash
handlePrime() assumes this.storage.getStats() always returns a non-null object; if storage returns null/undefined (e.g., partial init), stats.entity_counts will throw before the ?? {} kicks in. Consider defaulting stats itself (e.g., const stats = (await ... ) ?? {}) or guarding before reading entity_counts/relation_counts to keep cipher priming resilient.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 313:315
Comment:
**Priming can crash**
`handlePrime()` assumes `this.storage.getStats()` always returns a non-null object; if storage returns `null`/`undefined` (e.g., partial init), `stats.entity_counts` will throw before the `?? {}` kicks in. Consider defaulting `stats` itself (e.g., `const stats = (await ... ) ?? {}`) or guarding before reading `entity_counts`/`relation_counts` to keep cipher priming resilient.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — getStats() result now has a defensive nullish coalescing default before accessing nested properties.
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = args.limit ?? 15; | ||
| const types = args.types as string[] | undefined; | ||
| let since: Date | undefined; | ||
| if (args.since) { |
There was a problem hiding this comment.
Unbounded limit accepted
handlePrime() passes args.limit through without validation; callers can request extremely large limit values, which will generate a huge markdown string and then get auto-injected into the cipher response, creating a real risk of oversized responses/timeouts. Clamp limit to a reasonable maximum (or enforce it in storage) for priming/injection paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 279:283
Comment:
**Unbounded limit accepted**
`handlePrime()` passes `args.limit` through without validation; callers can request extremely large `limit` values, which will generate a huge markdown string and then get auto-injected into the `cipher` response, creating a real risk of oversized responses/timeouts. Clamp `limit` to a reasonable maximum (or enforce it in storage) for priming/injection paths.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — limit clamped to Math.min(Math.max(args.limit ?? 15, 1), 100) in handlePrime.
| if (operation === 'cipher' && this.knowledgeHandler) { | ||
| const knowledgePrimingKey = `${mcpSessionId ?? '__default__'}:knowledge`; | ||
| if (!this.sessionsPrimed.has(knowledgePrimingKey)) { | ||
| try { | ||
| const primeResult = await this.knowledgeHandler.processOperation({ |
There was a problem hiding this comment.
Priming not retried
Knowledge priming is skipped permanently for a session when knowledge_prime returns a non-error result but no usable text (e.g., content[0] is a resource, or the first text block is empty). Since sessionsPrimed.add() only happens on successful injection, this will cause priming to be attempted on every cipher call for that session, repeatedly doing work and spamming logs on persistent "empty" results. Consider making knowledge_prime return a consistent text block shape, or tightening the check to avoid repeated attempts when the handler returns a stable "no-op" response.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 429:433
Comment:
**Priming not retried**
Knowledge priming is skipped permanently for a session when `knowledge_prime` returns a non-error result but no usable text (e.g., `content[0]` is a resource, or the first text block is empty). Since `sessionsPrimed.add()` only happens on successful injection, this will cause priming to be attempted on every `cipher` call for that session, repeatedly doing work and spamming logs on persistent "empty" results. Consider making `knowledge_prime` return a consistent text block shape, or tightening the check to avoid repeated attempts when the handler returns a stable "no-op" response.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in round 3 (commit 2c18ebf) — sessionsPrimed.add() now runs after any non-error response, including the "No knowledge graph entities" case. This prevents repeated attempts on every cipher call while still allowing retry on transient failures.
- Fix priming retry spam: mark session as primed on any non-error response (success OR empty graph), only allow retry on transient failures - Add stats null safety in handlePrime with defensive default - Clamp limit in handlePrime [1,100] and handleListEntities [1,500] - Add date validation for created_after/created_before in handleListEntities - Document __default__ key lifecycle in clearSession Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| // Knowledge priming: append curated knowledge summary on first cipher call per session | ||
| if (operation === 'cipher' && this.knowledgeHandler) { | ||
| const knowledgePrimingKey = `${mcpSessionId ?? '__default__'}:knowledge`; | ||
| if (!this.sessionsPrimed.has(knowledgePrimingKey)) { | ||
| try { | ||
| const primeResult = await this.knowledgeHandler.processOperation({ | ||
| action: 'knowledge_prime', | ||
| limit: 15, | ||
| }); | ||
| if (!primeResult.isError && primeResult.content.length > 0) { | ||
| const primeText = primeResult.content[0]?.text; | ||
| if (primeText && !primeText.startsWith('No knowledge graph entities')) { | ||
| result.content.push({ | ||
| type: 'resource', | ||
| resource: { | ||
| uri: 'thoughtbox://knowledge/priming', | ||
| title: 'Knowledge Graph Context', | ||
| mimeType: 'text/markdown', | ||
| text: primeText, | ||
| }, | ||
| annotations: { | ||
| audience: ['assistant'], | ||
| priority: 0.6, | ||
| }, | ||
| } as ContentBlock); | ||
| } | ||
| // Mark as primed on ANY non-error response (success OR empty graph). | ||
| // Only transient failures (caught below) should allow retry. | ||
| this.sessionsPrimed.add(knowledgePrimingKey); | ||
| } |
There was a problem hiding this comment.
Priming suppressed on no-inject
this.sessionsPrimed.add(knowledgePrimingKey) runs for any non-error knowledge_prime response, even when you don't append the priming resource block (e.g., primeText is empty / only a resource block / starts with "No knowledge graph entities"). In those cases, knowledge priming is permanently suppressed for that session despite no priming actually occurring, which breaks the PR’s stated “auto-inject knowledge priming on first cipher call” behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 437:466
Comment:
**Priming suppressed on no-inject**
`this.sessionsPrimed.add(knowledgePrimingKey)` runs for *any* non-error `knowledge_prime` response, even when you don't append the priming resource block (e.g., `primeText` is empty / only a resource block / starts with "No knowledge graph entities"). In those cases, knowledge priming is permanently suppressed for that session despite no priming actually occurring, which breaks the PR’s stated “auto-inject knowledge priming on first cipher call” behavior.
How can I resolve this? If you propose a fix, please make it concise.| // Build compact markdown summary | ||
| const lines = entities.map(e => { | ||
| const typeTag = e.type; | ||
| return `- **${e.name}** [${typeTag}]: ${e.label}`; | ||
| }); | ||
|
|
||
| const stats = (await this.storage.getStats()) ?? { entity_counts: {}, relation_counts: {} } as any; | ||
| const entityCounts = stats.entity_counts ?? {}; | ||
| const relationCounts = stats.relation_counts ?? {}; | ||
| const totalEntities = Object.values(entityCounts).reduce((a: number, b: number) => a + b, 0); | ||
| const totalRelations = Object.values(relationCounts).reduce((a: number, b: number) => a + b, 0); | ||
|
|
||
| const header = `## Prior Knowledge (${entities.length} of ${totalEntities} entities, ${totalRelations} relations)`; | ||
| const text = [header, '', ...lines].join('\n'); | ||
|
|
There was a problem hiding this comment.
Markdown injection via entity fields
handlePrime() interpolates e.name and e.label directly into markdown (- **${e.name}** ... ${e.label}). Since entity creation is tool-exposed, a caller can include markdown/control sequences (e.g. **, backticks, headings) that will be auto-injected into the cipher response. If the goal is “curated context”, these fields need escaping/sanitization (or render as plain text) to prevent prompt/control injection through stored knowledge.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 323:337
Comment:
**Markdown injection via entity fields**
`handlePrime()` interpolates `e.name` and `e.label` directly into markdown (`- **${e.name}** ... ${e.label}`). Since entity creation is tool-exposed, a caller can include markdown/control sequences (e.g. `**`, backticks, headings) that will be auto-injected into the cipher response. If the goal is “curated context”, these fields need escaping/sanitization (or render as plain text) to prevent prompt/control injection through stored knowledge.
How can I resolve this? If you propose a fix, please make it concise.FileSystemKnowledgeStorage was not receiving basePath (args.dataDir), causing it to default to os.homedir() (~/.thoughtbox) inside the container — ephemeral filesystem, not the mounted Docker volume. Knowledge graph data was lost on every container rebuild while sessions persisted correctly. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| // Build compact markdown summary | ||
| const lines = entities.map(e => { | ||
| const typeTag = e.type; | ||
| return `- **${e.name}** [${typeTag}]: ${e.label}`; | ||
| }); |
There was a problem hiding this comment.
Persisted markdown injection risk
handlePrime() builds markdown lines with unescaped e.name and e.label (e.g. - **${e.name}** ... ${e.label}). Since entities are tool-writable (create_entity accepts arbitrary name/label), a caller can store prompt/control sequences in the knowledge graph and have them auto-injected into the first cipher response for future sessions (via GatewayHandler’s priming append). This breaks the “curated context” guarantee and enables prompt injection through persisted memory; the priming output should escape/strip markdown control characters or otherwise render these fields as plain text before injection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 323:327
Comment:
**Persisted markdown injection risk**
`handlePrime()` builds markdown lines with unescaped `e.name` and `e.label` (e.g. ``- **${e.name}** ... ${e.label}``). Since entities are tool-writable (`create_entity` accepts arbitrary `name`/`label`), a caller can store prompt/control sequences in the knowledge graph and have them auto-injected into the first `cipher` response for future sessions (via `GatewayHandler`’s priming append). This breaks the “curated context” guarantee and enables prompt injection through persisted memory; the priming output should escape/strip markdown control characters or otherwise render these fields as plain text before injection.
How can I resolve this? If you propose a fix, please make it concise.Strip newlines and markdown heading markers from entity names and labels before building the priming digest. Prevents multi-line injection that could break out of the list-item structure in auto-injected cipher context. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
src/knowledge/handler.ts
Outdated
| const sanitize = (s: string) => s.replace(/[\r\n]+/g, ' ').replace(/#{1,6}\s/g, '').trim(); | ||
| const lines = entities.map(e => { | ||
| const typeTag = e.type; | ||
| return `- **${sanitize(e.name)}** [${typeTag}]: ${sanitize(e.label)}`; | ||
| }); |
There was a problem hiding this comment.
Markdown not fully escaped
handlePrime() still interpolates entity name/label into markdown list items after only stripping newlines and #{1,6}\s headers (src/knowledge/handler.ts:325-329). This still allows stored entities to inject markdown control (e.g., backticks, **, links) into the auto-injected cipher priming block, which can change how the priming renders and can carry prompt-control text across sessions. If this output is meant to be “curated context”, these fields should be escaped/normalized to plain text more comprehensively before injection (at least backticks, asterisks, brackets/parentheses, and leading list markers).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 325:329
Comment:
**Markdown not fully escaped**
`handlePrime()` still interpolates entity `name`/`label` into markdown list items after only stripping newlines and `#{1,6}\s` headers (`src/knowledge/handler.ts:325-329`). This still allows stored entities to inject markdown control (e.g., backticks, `**`, links) into the auto-injected cipher priming block, which can change how the priming renders and can carry prompt-control text across sessions. If this output is meant to be “curated context”, these fields should be escaped/normalized to plain text more comprehensively before injection (at least backticks, asterisks, brackets/parentheses, and leading list markers).
How can I resolve this? If you propose a fix, please make it concise.| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = Math.min(Math.max(args.limit ?? 15, 1), 100); | ||
| const types = args.types as string[] | undefined; | ||
| let since: Date | undefined; | ||
| if (args.since) { | ||
| const parsed = new Date(args.since); | ||
| if (isNaN(parsed.getTime())) { | ||
| throw new Error(`Invalid date for 'since': ${args.since}`); | ||
| } | ||
| since = parsed; | ||
| } | ||
|
|
||
| const entities = await this.storage.listEntities({ | ||
| types: types as any, | ||
| created_after: since, | ||
| limit, | ||
| }); |
There was a problem hiding this comment.
types accepts invalid values
handlePrime() casts args.types to string[] and then passes it to listEntities as types as any (src/knowledge/handler.ts:298-310). Since EntityFilter.types is EntityType[], this bypasses validation and lets callers send arbitrary strings. At runtime this becomes a type IN (?,...) filter (src/knowledge/storage.ts:399-402), which will silently return empty results for invalid values, making priming look like “no entities” instead of an input error. Either validate types against the EntityType union before querying or reject unknown types with a clear error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 296:312
Comment:
**`types` accepts invalid values**
`handlePrime()` casts `args.types` to `string[]` and then passes it to `listEntities` as `types as any` (`src/knowledge/handler.ts:298-310`). Since `EntityFilter.types` is `EntityType[]`, this bypasses validation and lets callers send arbitrary strings. At runtime this becomes a `type IN (?,...)` filter (`src/knowledge/storage.ts:399-402`), which will silently return empty results for invalid values, making priming look like “no entities” instead of an input error. Either validate `types` against the `EntityType` union before querying or reject unknown types with a clear error.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| sql += ` ORDER BY importance_score DESC`; | ||
| sql += ` ORDER BY importance_score DESC, created_at DESC`; |
There was a problem hiding this comment.
Sort tiebreaker missing
listEntities() orders by importance_score DESC, created_at DESC (src/knowledge/storage.ts:424), but entities created in the same millisecond will still be returned in an arbitrary order. Since priming relies on recency and importance_score is currently constant, adding a stable tiebreaker (e.g., id DESC) would make ordering deterministic for identical timestamps.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/storage.ts
Line: 424:424
Comment:
**Sort tiebreaker missing**
`listEntities()` orders by `importance_score DESC, created_at DESC` (`src/knowledge/storage.ts:424`), but entities created in the same millisecond will still be returned in an arbitrary order. Since priming relies on recency and `importance_score` is currently constant, adding a stable tiebreaker (e.g., `id DESC`) would make ordering deterministic for identical timestamps.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/knowledge/storage.ts
Line: 683:699
Comment:
**Null `last_accessed_at` crash**
`rowToEntity()` unconditionally does `new Date(row.last_accessed_at)` (`src/knowledge/storage.ts:698`), but `last_accessed_at` is inserted as `null` for JSONL-rebuilt rows (`insertEntityFromJsonl` sets it to `null` at `src/knowledge/storage.ts:246`). `new Date(null)` yields the epoch date, which is incorrect; and if `better-sqlite3` returns `undefined` here, it becomes an invalid date. This affects entities returned via `listEntities()`/`getEntity()` and will leak misleading timestamps into priming output. Consider mapping nullish to `new Date(0)` only if intended, or to `created_at`/`updated_at`, or making `last_accessed_at?: Date` in the type and handling `null` explicitly.
How can I resolve this? If you propose a fix, please make it concise. |
Create comprehensive relation network for 15 new entities: - constraint-relaxation-for-insight (3 relations) - gitignore-causes-knowledge-loss (3 relations) - protocol-level-enforcement-over-rules (2 relations) - structural-vs-functional-verification-bias (3 relations) - root-cause-via-five-whys-channel (3 relations) - decision-via-trade-off-matrix (3 relations) - cipher-as-thinking-scaffold (3 relations) - session-as-artifact (3 relations) - notebooks-vs-thought-chains (3 relations) - coordinator-behavior-workflow (3 relations) - multi-agent-contributor-loop (3 relations) - mental-models-as-processes (3 relations) - parallel-progressive-disclosure (3 relations) - single-user-startup-sequence (3 relations) - minimal-response-token-efficiency (3 relations) All relations target high-value hub entities (progressive-disclosure, hub-orchestration, multi-agent-collab-reasoning, etc). Relation types: RELATES_TO (43), DEPENDS_ON (1), BUILDS_ON (0). Graph now contains 99 entities with 45+ new relations connecting emerging concepts to core architectural patterns. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = Math.min(Math.max(args.limit ?? 15, 1), 100); | ||
| const types = args.types as string[] | undefined; | ||
| let since: Date | undefined; |
There was a problem hiding this comment.
Unvalidated types bypass
handlePrime() casts args.types to string[] and then passes it through as types as any (src/knowledge/handler.ts:298-310). This allows arbitrary strings to reach FileSystemKnowledgeStorage.listEntities() where they become a type IN (?,...) filter (src/knowledge/storage.ts:399-402), leading to silent empty results for invalid values. Since knowledge_prime output is auto-injected on first cipher, this will present as “no knowledge” rather than a clear input error when callers pass a typo/unknown type. Please validate types against the supported entity type enum/union before querying and return an explicit error for unknown values.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 296:299
Comment:
**Unvalidated `types` bypass**
`handlePrime()` casts `args.types` to `string[]` and then passes it through as `types as any` (`src/knowledge/handler.ts:298-310`). This allows arbitrary strings to reach `FileSystemKnowledgeStorage.listEntities()` where they become a `type IN (?,...)` filter (`src/knowledge/storage.ts:399-402`), leading to silent empty results for invalid values. Since `knowledge_prime` output is auto-injected on first `cipher`, this will present as “no knowledge” rather than a clear input error when callers pass a typo/unknown type. Please validate `types` against the supported entity type enum/union before querying and return an explicit error for unknown values.
How can I resolve this? If you propose a fix, please make it concise.| // Build compact markdown summary | ||
| // Sanitize name/label: collapse to single line, strip markdown heading markers | ||
| const sanitize = (s: string) => s.replace(/[\r\n]+/g, ' ').replace(/#{1,6}\s/g, '').trim(); | ||
| const lines = entities.map(e => { | ||
| const typeTag = e.type; | ||
| return `- **${sanitize(e.name)}** [${typeTag}]: ${sanitize(e.label)}`; | ||
| }); |
There was a problem hiding this comment.
Markdown injection persists
The sanitize() helper only removes newlines and markdown heading markers, but handlePrime() still interpolates e.name/e.label into markdown with **...** and [...] (src/knowledge/handler.ts:325-329). Because entities are tool-writable, stored values containing backticks/asterisks/link syntax can alter the rendered priming block and carry prompt/control text across sessions via auto-injection on cipher. If this output is intended to be “curated context”, name/label should be escaped/normalized more comprehensively (or rendered as plain text) before inclusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 323:329
Comment:
**Markdown injection persists**
The `sanitize()` helper only removes newlines and markdown heading markers, but `handlePrime()` still interpolates `e.name`/`e.label` into markdown with `**...**` and `[...]` (`src/knowledge/handler.ts:325-329`). Because entities are tool-writable, stored values containing backticks/asterisks/link syntax can alter the rendered priming block and carry prompt/control text across sessions via auto-injection on `cipher`. If this output is intended to be “curated context”, `name`/`label` should be escaped/normalized more comprehensively (or rendered as plain text) before inclusion.
How can I resolve this? If you propose a fix, please make it concise.| const primeText = primeResult.content[0]?.text; | ||
| if (primeText && !primeText.startsWith('No knowledge graph entities')) { | ||
| result.content.push({ | ||
| type: 'resource', | ||
| resource: { | ||
| uri: 'thoughtbox://knowledge/priming', |
There was a problem hiding this comment.
Priming reads wrong block
The injection path assumes the priming text is always in primeResult.content[0].text (src/gateway/gateway-handler.ts:447-448), but KnowledgeHandler.processOperation() appends a per-operation resource block to the end of content (src/knowledge/handler.ts:80-91). If any future change causes the first content block from knowledge_prime not to be the text block (or returns multiple blocks), this will skip injection even though a text summary is present. Consider selecting the first type === 'text' block (or enforcing knowledge_prime to return a single text block) before deciding whether to inject.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 447:452
Comment:
**Priming reads wrong block**
The injection path assumes the priming text is always in `primeResult.content[0].text` (`src/gateway/gateway-handler.ts:447-448`), but `KnowledgeHandler.processOperation()` appends a per-operation `resource` block to the end of `content` (`src/knowledge/handler.ts:80-91`). If any future change causes the first content block from `knowledge_prime` not to be the text block (or returns multiple blocks), this will skip injection even though a text summary is present. Consider selecting the first `type === 'text'` block (or enforcing `knowledge_prime` to return a single text block) before deciding whether to inject.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| sql += ` ORDER BY importance_score DESC`; | ||
| sql += ` ORDER BY importance_score DESC, created_at DESC`; | ||
|
|
There was a problem hiding this comment.
Nondeterministic entity ordering
listEntities() orders by importance_score DESC, created_at DESC (src/knowledge/storage.ts:424), but entities created in the same millisecond can still return in arbitrary order. Since knowledge_prime relies on recency and importance_score is currently constant, this can make priming unstable across runs. Add a stable tiebreaker (e.g., id DESC) to make ordering deterministic when timestamps tie.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/storage.ts
Line: 423:425
Comment:
**Nondeterministic entity ordering**
`listEntities()` orders by `importance_score DESC, created_at DESC` (`src/knowledge/storage.ts:424`), but entities created in the same millisecond can still return in arbitrary order. Since `knowledge_prime` relies on recency and `importance_score` is currently constant, this can make priming unstable across runs. Add a stable tiebreaker (e.g., `id DESC`) to make ordering deterministic when timestamps tie.
How can I resolve this? If you propose a fix, please make it concise.
glassBead-tc
force-pushed
the
feat/knowledge-read-loop
branch
from
7ab8988 to
91c8959
Compare
Comprehensive guides for extracting knowledge graph entities from reasoning sessions: For humans: - docs-for-humans/knowledge-graph-extraction.md: Complete workflow guide with phases, naming conventions, validation checklist For LLMs: - docs-for-llms/KNOWLEDGE-GRAPH-EXTRACTION.md: Quick start, API patterns, cipher mapping - docs-for-llms/KNOWLEDGE-GRAPH-WORKFLOW.md: Parallel extraction patterns, performance optimization Methodology validated via Feb 2026 extraction: 40 sessions → 99 entities, 321 relations in ~25 min using parallel sub-agents. Contains NO user data - only generalized extraction patterns. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| private async handlePrime(args: any): Promise<{ content: Array<any> }> { | ||
| const limit = Math.min(Math.max(args.limit ?? 15, 1), 100); | ||
| const types = args.types as string[] | undefined; | ||
| let since: Date | undefined; |
There was a problem hiding this comment.
Invalid types bypass validation
handlePrime() casts args.types to string[] and then passes it through as types as any into listEntities(). Any typo/unknown value will silently become a type IN (...) filter that returns an empty set, making priming look like “no knowledge” instead of a clear input error. This breaks the contract implied by the knowledge_prime schema enum and makes debugging harder; validate types against the allowed entity types and reject unknown values.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 296:299
Comment:
**Invalid types bypass validation**
`handlePrime()` casts `args.types` to `string[]` and then passes it through as `types as any` into `listEntities()`. Any typo/unknown value will silently become a `type IN (...)` filter that returns an empty set, making priming look like “no knowledge” instead of a clear input error. This breaks the contract implied by the `knowledge_prime` schema enum and makes debugging harder; validate `types` against the allowed entity types and reject unknown values.
How can I resolve this? If you propose a fix, please make it concise.| // Build compact markdown summary | ||
| // Sanitize name/label: collapse to single line, strip markdown heading markers | ||
| const sanitize = (s: string) => s.replace(/[\r\n]+/g, ' ').replace(/#{1,6}\s/g, '').trim(); | ||
| const lines = entities.map(e => { |
There was a problem hiding this comment.
Persisted markdown injection
knowledge_prime builds markdown using entity name/label, but sanitize() only removes newlines and heading markers. Since entities are tool-writable and this output is auto-injected into the first cipher response, stored values containing markdown control (e.g., backticks, **, link syntax) can change rendering and carry prompt/control text across sessions. If this is meant to be “curated context”, escape/normalize these fields more comprehensively or render them as plain text before injection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/handler.ts
Line: 323:326
Comment:
**Persisted markdown injection**
`knowledge_prime` builds markdown using entity `name`/`label`, but `sanitize()` only removes newlines and heading markers. Since entities are tool-writable and this output is auto-injected into the first `cipher` response, stored values containing markdown control (e.g., backticks, `**`, link syntax) can change rendering and carry prompt/control text across sessions. If this is meant to be “curated context”, escape/normalize these fields more comprehensively or render them as plain text before injection.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
The Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 1324:1327
Comment:
**Catalog description out of sync**
The `knowledge-operations` resource description still lists operations ending in `stats` and omits `knowledge_prime`, while the ListResources handler’s description includes it. This inconsistency makes discovery/documentation depend on which path the client uses; update the `registerResource("knowledge-operations")` description to include `knowledge_prime` as well.
How can I resolve this? If you propose a fix, please make it concise. |
|
@roomote create fixes for these issues:
handlePrime() casts args.types to string[] and then passes it through as types as any into listEntities(). Any typo/unknown value will silently become a type IN (...) filter that returns an empty set, making priming look like “no knowledge” instead of a clear input error. This breaks the contract implied by the knowledge_prime schema enum and makes debugging harder; validate types against the allowed entity types and reject unknown values.
knowledge_prime builds markdown using entity name/label, but sanitize() only removes newlines and heading markers. Since entities are tool-writable and this output is auto-injected into the first cipher response, stored values containing markdown control (e.g., backticks, **, link syntax) can change rendering and carry prompt/control text across sessions. If this is meant to be “curated context”, escape/normalize these fields more comprehensively or render them as plain text before injection.
The knowledge-operations resource description still lists operations ending in stats and omits knowledge_prime, while the ListResources handler’s description includes it. This inconsistency makes discovery/documentation depend on which path the client uses; update the registerResource("knowledge-operations") description to include knowledge_prime as well. |
Fixed all three reported issues. TypeScript compiles clean; unit tests pass (330/330, same 9 pre-existing branch-retrieval failures). |
… description
- Validate types param against EntityType enum in handlePrime(), reject
unknown values with clear error listing allowed types
- Harden sanitize() to strip markdown control characters (backticks, bold,
italic, strikethrough, link syntax, HTML tags) from entity names/labels
before injection into cipher context
- Update registerResource("knowledge-operations") description to include
knowledge_prime, matching the ListResources handler description
| try { | ||
| const primeResult = await this.knowledgeHandler.processOperation({ | ||
| action: 'knowledge_prime', | ||
| limit: 15, | ||
| }); | ||
| if (!primeResult.isError && primeResult.content.length > 0) { | ||
| const primeText = primeResult.content[0]?.text; | ||
| if (primeText && !primeText.startsWith('No knowledge graph entities')) { |
There was a problem hiding this comment.
Priming reads wrong block
The injection path assumes the knowledge summary is always at primeResult.content[0].text (src/gateway/gateway-handler.ts:446-448). But KnowledgeHandler.processOperation() appends a resource block to the end of content for discoverability (src/knowledge/handler.ts:86-97), and future edits could also reorder blocks. If the first block isn’t the text block, priming will silently skip injection even when a text summary exists; this breaks the “auto-prime on first cipher call” behavior. Select the first type === 'text' block (or enforce a single-block response for knowledge_prime).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/gateway-handler.ts
Line: 441:448
Comment:
**Priming reads wrong block**
The injection path assumes the knowledge summary is always at `primeResult.content[0].text` (src/gateway/gateway-handler.ts:446-448). But `KnowledgeHandler.processOperation()` appends a `resource` block to the end of `content` for discoverability (src/knowledge/handler.ts:86-97), and future edits could also reorder blocks. If the first block isn’t the text block, priming will silently skip injection even when a text summary exists; this breaks the “auto-prime on first cipher call” behavior. Select the first `type === 'text'` block (or enforce a single-block response for `knowledge_prime`).
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| sql += ` ORDER BY importance_score DESC`; | ||
| sql += ` ORDER BY importance_score DESC, created_at DESC`; | ||
|
|
||
| if (filter?.limit) { |
There was a problem hiding this comment.
Nondeterministic entity ordering
listEntities() now orders by importance_score DESC, created_at DESC (src/knowledge/storage.ts:424), but rows with identical created_at (ms resolution) and identical importance_score (currently constant) can still be returned in arbitrary order. Since knowledge_prime relies on recency to pick “top N”, this can make priming unstable across runs. Add a stable tiebreaker (e.g., id DESC) to make ordering deterministic when timestamps tie.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/knowledge/storage.ts
Line: 423:426
Comment:
**Nondeterministic entity ordering**
`listEntities()` now orders by `importance_score DESC, created_at DESC` (src/knowledge/storage.ts:424), but rows with identical `created_at` (ms resolution) and identical `importance_score` (currently constant) can still be returned in arbitrary order. Since `knowledge_prime` relies on recency to pick “top N”, this can make priming unstable across runs. Add a stable tiebreaker (e.g., `id DESC`) to make ordering deterministic when timestamps tie.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server-factory.ts
Line: 251:257
Comment:
**Knowledge storage path regression**
`FileSystemKnowledgeStorage` now receives `basePath: args.dataDir` (src/server-factory.ts:254-257). When `dataDir` is undefined (which is valid per `CreateMcpServerArgs`), this will fall back to the storage default `~/.thoughtbox` (because storage uses `options.basePath || ...`). That changes behavior from the previous default base path used elsewhere in the server (and may unintentionally mix data across runs/projects). If knowledge storage is meant to live under the same dataDir as other persisted components, this needs an explicit default/consistency decision (either require `dataDir` for knowledge persistence or don’t pass it when undefined).
How can I resolve this? If you propose a fix, please make it concise. |
|
@roomote fix this issue: FileSystemKnowledgeStorage now receives basePath: args.dataDir (src/server-factory.ts:254-257). When dataDir is undefined (which is valid per CreateMcpServerArgs), this will fall back to the storage default ~/.thoughtbox (because storage uses options.basePath || ...). That changes behavior from the previous default base path used elsewhere in the server (and may unintentionally mix data across runs/projects). If knowledge storage is meant to live under the same dataDir as other persisted components, this needs an explicit default/consistency decision (either require dataDir for knowledge persistence or don’t pass it when undefined). |
2 participants
Summary
Closes the knowledge graph read loop. Agents now receive a curated knowledge summary when they bootstrap (on cipher call), and can explicitly query for more context. The graph becomes memory that informs future work, not a write-only log.
What changed:
knowledge_primeaction on KnowledgeHandler — returns compact markdown summary of recent entities (top 15 by recency, filterable by type and date)audience: ['assistant'])titleandannotationsfields were being dropped in content transformation, breaking existing profile priming targetingknowledge,read_thoughts, andget_structureoperations (agents couldn't discover them)listEntitiesnow usescreated_at DESCas tiebreaker whenimportance_scoreis equal (all scores are currently 0.5)4 commits:
fix: preserve resource annotations and align gateway descriptionfeat(knowledge): add knowledge_prime action for context primingfeat(knowledge): auto-inject knowledge priming into cipher responsedocs: update bootstrap sequence with knowledge graph primingTest plan
npm run build)knowledge_primeaction returns compact markdown with entity summariesknowledge_primeacceptslimit,types, andsinceparameters🤖 Generated with Claude Code