feat(coding-agent): add manual fact store MVP#74
Open
durch wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR introduces a new fact store system comprising schema definitions, SQLite persistence, fact resolution logic, prompt formatting, CLI commands, and interactive mode integration. The implementation adds fact management operations (add, view, search, explain, retract, erase), a Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI Parser
participant FactsCLI as Facts CLI Module
participant Store as FactStore
participant DB as SQLite Database
User->>CLI: facts add --scope repo --sensitivity sensitive ...
CLI->>FactsCLI: parseFactsArgv() → FactsCommandArgs
FactsCLI->>FactsCLI: validateFactKind(), normalizeInputs()
FactsCLI->>Store: FactStore.open()
Store->>DB: CREATE TABLE fact_assertions, fact_events
FactsCLI->>Store: add(NewFactAssertionInput)
Store->>Store: createFactAssertion() → FactAssertion with UUID
Store->>DB: INSERT INTO fact_assertions
Store->>DB: INSERT INTO fact_events (event: "add")
Store-->>FactsCLI: FactAssertion
FactsCLI->>Store: close()
FactsCLI-->>CLI: output text
CLI-->>User: Display results
sequenceDiagram
actor User
participant InteractiveMode as Interactive Mode
participant CmdCtrl as Command Controller
participant FactsCLI as Facts CLI Module
participant Resolver as Fact Resolver
participant Store as FactStore
User->>InteractiveMode: /facts search subject:file kind:implementation
InteractiveMode->>CmdCtrl: handleFactsCommand(text)
CmdCtrl->>FactsCLI: parseFactsArgv() + runFactsCommand()
FactsCLI->>Store: FactStore.open()
FactsCLI->>Store: search(FactSearchOptions)
Store->>Store: query raw facts from SQLite
FactsCLI->>Resolver: resolveActiveFacts(rawFacts, options)
Resolver->>Resolver: filterByOmissionReason() [expire, dispute, sensitivity]
Resolver->>Resolver: groupByConflictKey()
Resolver->>Resolver: selectByAuthority() [source, confidence, updatedAt]
Resolver-->>FactsCLI: FactResolutionResult {active, omitted}
FactsCLI->>FactsCLI: formatResults() → markdown/json
FactsCLI->>Store: close()
FactsCLI-->>CmdCtrl: formatted output string
CmdCtrl->>InteractiveMode: displayPanel(title, text)
InteractiveMode-->>User: Render facts panel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/coding-agent/src/facts/prompt-format.ts`:
- Around line 31-33: The prompt string built inline in prompt-format.ts (the
const lines entry using escapeXml(now.toISOString()) and the precedence text)
must be moved into a static Markdown template (e.g., facts.md) using Handlebars
placeholders for dynamic fields like as_of and precedence; update the module to
load and compile that .md template at runtime (using Handlebars.compile or the
project's template loader) and pass in { as_of: escapeXml(now.toISOString()),
precedence: "current user instructions, repo state, runtime output, and explicit
corrections outrank stored facts" } instead of constructing the string inline;
remove the inline template literal in prompt-format.ts (and the similar
occurrence around the later reference at line ~75) so no prompt text is created
via template literals/string concatenation in code.
- Around line 42-49: The projected size calculation undercounts the newline
separators from lines.join("\n"), so update the projection in the selection loop
to include the number of separators that will exist after adding the candidate
fact: compute projected = charCount + rendered.length + lines.length /* newline
separators after join */ + "\n</known-facts>".length, and use that to decide
omitted.push({ id: fact.id, reason: "max_chars" })/continue; this ensures the
separators introduced by lines.join("\n") are accounted for before
lines.push(rendered) and selected.push(fact).
In `@packages/coding-agent/src/facts/storage.ts`:
- Around line 249-252: The retract(id: string, reason?: string, nowMs: number =
Date.now()) method currently returns the existing FactAssertion when the record
is missing or already has status "erased", which makes callers treat that as a
successful retract; change the early-return behavior so that when there is no
record (existing is falsy) or when existing.status === "erased" the function
returns null to signal a no-op, and only return a FactAssertion on an actual
state change; update the function signature's return handling accordingly and
ensure callers of retract handle null as "nothing to retract".
In `@packages/coding-agent/src/modes/controllers/command-controller.ts`:
- Around line 731-733: The facts markdown panel can be broken if the
user-controlled output (sanitized/displayed from replaceTabs) contains
triple-backtick fences; before calling showMarkdownPanel, escape any ```
sequences in displayed (e.g., create safeDisplayed = displayed.replace(/```/g,
"\\`\\`\\`")) and pass safeDisplayed to showMarkdownPanel instead of displayed
so the fenced code block cannot be prematurely closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e01fb41-7d9d-40d8-9de7-a9c4bcf70b5c
📒 Files selected for processing (16)
docs/adr/0005-fact-store-belief-layer.mdpackages/coding-agent/package.jsonpackages/coding-agent/src/cli.tspackages/coding-agent/src/cli/facts-cli.tspackages/coding-agent/src/commands/facts.tspackages/coding-agent/src/facts/index.tspackages/coding-agent/src/facts/prompt-format.tspackages/coding-agent/src/facts/resolver.tspackages/coding-agent/src/facts/schema.tspackages/coding-agent/src/facts/storage.tspackages/coding-agent/src/index.tspackages/coding-agent/src/modes/controllers/command-controller.tspackages/coding-agent/src/modes/interactive-mode.tspackages/coding-agent/src/modes/types.tspackages/coding-agent/src/slash-commands/builtin-registry.tspackages/coding-agent/test/facts.test.ts
Comment on lines
+31
to
+33
| const lines = [ | ||
| `<known-facts as_of="${escapeXml(now.toISOString())}" precedence="current user instructions, repo state, runtime output, and explicit corrections outrank stored facts">`, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move prompt template text to a static .md file.
This module currently constructs prompt text inline with template literals/string concatenation. Please move the prompt body to a static markdown template and inject only dynamic fields.
As per coding guidelines, "Never build prompts in code. Prompts must not use inline strings, template literals, or string concatenation. Prompts live in static .md files with Handlebars for dynamic content."
Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/coding-agent/src/facts/prompt-format.ts` around lines 31 - 33, The
prompt string built inline in prompt-format.ts (the const lines entry using
escapeXml(now.toISOString()) and the precedence text) must be moved into a
static Markdown template (e.g., facts.md) using Handlebars placeholders for
dynamic fields like as_of and precedence; update the module to load and compile
that .md template at runtime (using Handlebars.compile or the project's template
loader) and pass in { as_of: escapeXml(now.toISOString()), precedence: "current
user instructions, repo state, runtime output, and explicit corrections outrank
stored facts" } instead of constructing the string inline; remove the inline
template literal in prompt-format.ts (and the similar occurrence around the
later reference at line ~75) so no prompt text is created via template
literals/string concatenation in code.
Comment on lines
+42
to
+49
| const projected = charCount + rendered.length + "\n</known-facts>".length; | ||
| if (projected > maxChars) { | ||
| omitted.push({ id: fact.id, reason: "max_chars" }); | ||
| continue; | ||
| } | ||
| lines.push(rendered); | ||
| charCount += rendered.length; | ||
| selected.push(fact); |
There was a problem hiding this comment.
maxChars enforcement undercounts separators and can overshoot budget.
The projection omits some newline bytes inserted by lines.join("\n"), so facts can be accepted even when final text.length > maxChars.
Proposed fix
- const rendered = renderFact(fact);
- const projected = charCount + rendered.length + "\n</known-facts>".length;
- if (projected > maxChars) {
+ const rendered = renderFact(fact);
+ const projectedText = [...lines, rendered, "</known-facts>"].join("\n");
+ if (projectedText.length > maxChars) {
omitted.push({ id: fact.id, reason: "max_chars" });
continue;
}
lines.push(rendered);
- charCount += rendered.length;
+ charCount = projectedText.length - "</known-facts>".length;
selected.push(fact);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/coding-agent/src/facts/prompt-format.ts` around lines 42 - 49, The
projected size calculation undercounts the newline separators from
lines.join("\n"), so update the projection in the selection loop to include the
number of separators that will exist after adding the candidate fact: compute
projected = charCount + rendered.length + lines.length /* newline separators
after join */ + "\n</known-facts>".length, and use that to decide omitted.push({
id: fact.id, reason: "max_chars" })/continue; this ensures the separators
introduced by lines.join("\n") are accounted for before lines.push(rendered) and
selected.push(fact).
Comment on lines
+249
to
+252
| retract(id: string, reason?: string, nowMs: number = Date.now()): FactAssertion | null { | ||
| const existing = this.get(id); | ||
| if (!existing || existing.status === "erased") return existing; | ||
| const nowSec = Math.floor(nowMs / 1000); |
There was a problem hiding this comment.
retract() conflates “not retractable” with success for erased facts.
When a fact is already erased, retract() returns that record instead of signaling a no-op/error. Downstream, CLI can report a successful retract even though nothing changed.
Proposed fix
retract(id: string, reason?: string, nowMs: number = Date.now()): FactAssertion | null {
const existing = this.get(id);
- if (!existing || existing.status === "erased") return existing;
+ if (!existing) return null;
+ if (existing.status === "erased") return null;
const nowSec = Math.floor(nowMs / 1000);
const transaction = this.#db.transaction(() => {
this.#updateStatusStmt.run("retracted", nowSec, id);
this.#recordEvent(id, "retract", reason, nowMs);
});
transaction();
return this.get(id);
}// packages/coding-agent/src/cli/facts-cli.ts
const fact = store.retract(id, reasonParts.join(" ") || undefined);
-if (!fact) return `Fact not found: ${id}`;
+if (!fact) return `Fact not found or not retractable: ${id}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| retract(id: string, reason?: string, nowMs: number = Date.now()): FactAssertion | null { | |
| const existing = this.get(id); | |
| if (!existing || existing.status === "erased") return existing; | |
| const nowSec = Math.floor(nowMs / 1000); | |
| retract(id: string, reason?: string, nowMs: number = Date.now()): FactAssertion | null { | |
| const existing = this.get(id); | |
| if (!existing) return null; | |
| if (existing.status === "erased") return null; | |
| const nowSec = Math.floor(nowMs / 1000); | |
| const transaction = this.#db.transaction(() => { | |
| this.#updateStatusStmt.run("retracted", nowSec, id); | |
| this.#recordEvent(id, "retract", reason, nowMs); | |
| }); | |
| transaction(); | |
| return this.get(id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/coding-agent/src/facts/storage.ts` around lines 249 - 252, The
retract(id: string, reason?: string, nowMs: number = Date.now()) method
currently returns the existing FactAssertion when the record is missing or
already has status "erased", which makes callers treat that as a successful
retract; change the early-return behavior so that when there is no record
(existing is falsy) or when existing.status === "erased" the function returns
null to signal a no-op, and only return a FactAssertion on an actual state
change; update the function signature's return handling accordingly and ensure
callers of retract handle null as "nothing to retract".
Comment on lines
+731
to
+733
| const sanitized = replaceTabs(output); | ||
| const displayed = sanitized.length > 20_000 ? `${sanitized.slice(0, 20_000)}\n... truncated ...` : sanitized; | ||
| showMarkdownPanel(this.ctx, "Facts", `\`\`\`text\n${displayed}\n\`\`\``); |
There was a problem hiding this comment.
Escape fenced-block delimiters in facts output rendering.
Line 733 wraps raw displayed text in fences; if facts data contains (user-controlled), the panel markdown can break and render malformed content.
Suggested hardening
- const displayed = sanitized.length > 20_000 ? `${sanitized.slice(0, 20_000)}\n... truncated ...` : sanitized;
- showMarkdownPanel(this.ctx, "Facts", `\`\`\`text\n${displayed}\n\`\`\``);
+ const displayed = sanitized.length > 20_000 ? `${sanitized.slice(0, 20_000)}\n... truncated ...` : sanitized;
+ const safeDisplayed = displayed.replace(/```/g, "\\`\\`\\`");
+ showMarkdownPanel(this.ctx, "Facts", `\`\`\`text\n${safeDisplayed}\n\`\`\``);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sanitized = replaceTabs(output); | |
| const displayed = sanitized.length > 20_000 ? `${sanitized.slice(0, 20_000)}\n... truncated ...` : sanitized; | |
| showMarkdownPanel(this.ctx, "Facts", `\`\`\`text\n${displayed}\n\`\`\``); | |
| const sanitized = replaceTabs(output); | |
| const displayed = sanitized.length > 20_000 ? `${sanitized.slice(0, 20_000)}\n... truncated ...` : sanitized; | |
| const safeDisplayed = displayed.replace(/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/coding-agent/src/modes/controllers/command-controller.ts` around
lines 731 - 733, The facts markdown panel can be broken if the user-controlled
output (sanitized/displayed from replaceTabs) contains triple-backtick fences;
before calling showMarkdownPanel, escape any ``` sequences in displayed (e.g.,
create safeDisplayed = displayed.replace(/```/g, "\\`\\`\\`")) and pass
safeDisplayed to showMarkdownPanel instead of displayed so the fenced code block
cannot be prematurely closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the first, conservative slice of a dedicated Fact Store for
packages/coding-agent:packages/coding-agent/src/facts/with:retractvserasesemantics<known-facts>prompt formattingoh-omp facts add|view|search|explain|retract|erase|path|prompt/facts ...with the same core command behaviorWhy
Recall stores evidence-shaped context and memory stores synthesized guidance, but neither is a good authority model for current scoped beliefs like project decisions, constraints, owners, dates, or explicit user preferences.
This PR introduces a small, inspectable, correctable Fact Store without changing the existing recall/memory contracts or replacing context assembly behavior.
Scope and Safety
This is intentionally manual-only:
.oh/*.md, assistant plans, or session summaries.fact_currentmaterialized table; active facts resolve on read while rules are still stabilizing.Privacy/safety guardrails included here:
secretfacts are rejected by the schema.personalandsensitivefacts are omitted from known-facts formatting unless explicitly allowed.eraseredacts payload-bearing fields, including subject, predicate, object, canonical text, evidence, supersession, and tags.Testing
bun test packages/coding-agent/test/facts.test.tsbun check:tstsgo -p tsconfig.jsonpassedoh-omp facts add/searchwith a temporary SQLite database.--strictcan be stored as fact values when command flags precede fact fieldsbun check:tspasses