feat: knowledge base doc-freshness layer (drift detection + refresh)#285
Open
efenocchi wants to merge 16 commits into
Open
feat: knowledge base doc-freshness layer (drift detection + refresh)#285efenocchi wants to merge 16 commits into
efenocchi wants to merge 16 commits into
Conversation
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 17 files changed
Generated for commit 0d3ce23. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a complete Changeshivemind docs subsystem
Sequence DiagramssequenceDiagram
actor User
participant CLI as hivemind docs refresh
participant loadCurrentSnapshot
participant listDocs
participant computeImpactedDocs
participant ensureDocsTable
participant refreshDocs
participant makeClaudeGenerate as Claude CLI
participant gateDocEdit
participant setDoc as setDoc (Deeplake)
User->>CLI: docs refresh [--dry-run]
CLI->>loadCurrentSnapshot: load local graph snapshot
CLI->>listDocs: fetch active docs
CLI->>computeImpactedDocs: detect stale docs via anchors + blast radius
alt --dry-run
CLI-->>User: print impacted docs, no writes
else live run
CLI->>ensureDocsTable: create/heal docs table
CLI->>refreshDocs: iterate impacted docs
refreshDocs->>makeClaudeGenerate: buildRefreshPrompt + execFileSync(claude)
makeClaudeGenerate-->>refreshDocs: proposed markdown
refreshDocs->>gateDocEdit: validate (tier, size, anchors)
alt gate pass
refreshDocs->>setDoc: persist new version
setDoc-->>refreshDocs: WriteResult
else gate fail
refreshDocs-->>refreshDocs: record rejected
end
refreshDocs-->>CLI: RefreshReport
CLI-->>User: print refreshed/rejected/skipped counts
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (4)
tests/shared/docs-impact.test.ts (1)
133-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full
changedpayload, not juststate.This test currently verifies only the discriminator and misses regressions in
from/tovalues.✅ Proposed assertion tightening
it("changed when the code differs from the stored hash", () => { const n = node("a.ts:foo:function", "a.ts", "L1-L2"); const stale: DocAnchor = { symbol_id: n.id, content_hash: "deadbeef" }; const st = anchorStatus(stale, snap([n]), dir); - expect(st.state).toBe("changed"); + expect(st).toEqual({ + state: "changed", + from: "deadbeef", + to: sha("function foo() {\n return 1;"), + }); });As per path instructions,
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/docs-impact.test.ts` around lines 133 - 138, The test for the "changed when the code differs from the stored hash" case is only asserting on the state property of the anchorStatus result, but it should verify the complete changed payload including the from and to values. Replace the single assertion on st.state with a comprehensive assertion that checks the full object structure returned by anchorStatus to catch potential regressions in the from and to fields.Source: Path instructions
tests/shared/graph/load-current.test.ts (1)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fallback test assert concrete snapshot content.
not.toBeNull()is broad here; assert the expectednodes/linkspayload directly.✅ Proposed assertion tightening
it("falls back to snapshot_sha256 when there is no commit context", () => { writeLastBuild(cwd, { ts: 1, commit_sha: null, snapshot_sha256: "feedface" }); writeSnapshotFile(cwd, "feedface", JSON.stringify({ nodes: [], links: [] })); - expect(loadCurrentSnapshot(cwd)).not.toBeNull(); + expect(loadCurrentSnapshot(cwd)).toEqual({ nodes: [], links: [] }); });As per path instructions,
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/graph/load-current.test.ts` around lines 54 - 58, The test assertion in the "falls back to snapshot_sha256 when there is no commit context" test is too broad. Instead of using expect(loadCurrentSnapshot(cwd)).not.toBeNull(), replace it with a specific assertion that validates the returned snapshot object contains the expected concrete values for the nodes and links properties that were written to the snapshot file using writeSnapshotFile. This ensures the function correctly loads and returns the actual snapshot payload rather than just checking it exists.Source: Path instructions
tests/claude-code/cli-docs.test.ts (1)
87-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten assertions to exact expected messages/values instead of generic substrings.
A large part of this suite validates only partial output via regex substring matches. Switching key checks to exact strings (or structured fields) will make these CLI contract tests significantly more robust.
As per path instructions,
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-code/cli-docs.test.ts` around lines 87 - 223, Replace generic regex substring matching assertions with exact string or structured value checks throughout the test suite. Instead of using toMatch() with broad regex patterns like /hivemind docs/ or /Not logged in/, use toContain() for exact string matches or direct equality checks for specific values. Apply this to key test cases such as "prints usage with no subcommand", "exits 2 when not logged in", "creates v1 via the real store", "prints (no doc) when nothing matches", "lists rows", "archives an existing doc", and other test assertions that currently rely on partial pattern matching to make the CLI contract tests more precise and robust.Source: Path instructions
tests/shared/docs.test.ts (1)
129-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse exact error-message assertions for failure paths.
Several rejection tests currently assert generic substrings (or just
toThrow()), which weakens contract checks. Prefer exact message assertions for these cases.✅ Example tightening
- ).rejects.toThrow(/must not be empty/); + ).rejects.toThrow("Doc content must not be empty"); @@ - ).rejects.toThrow(/doc_id must not be empty/); + ).rejects.toThrow("Doc doc_id must not be empty"); @@ - ).rejects.toThrow(/Doc not found: missing.ts/); + ).rejects.toThrow("Doc not found: missing.ts"); @@ - await expect( - insertDoc(query, `x"; DROP TABLE y; --`, { doc_id: "a.ts", path: "/p", content: "x" }), - ).rejects.toThrow(); + await expect( + insertDoc(query, `x"; DROP TABLE y; --`, { doc_id: "a.ts", path: "/p", content: "x" }), + ).rejects.toThrow(`Invalid SQL identifier: "x\\"; DROP TABLE y; --"`);As per path instructions,
tests/**: “Prefer asserting on specific values (paths, messages) over generic substrings.”Also applies to: 212-227, 290-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/docs.test.ts` around lines 129 - 159, The rejection tests in this section use generic error assertions that are too permissive. Replace the regex substring matches in the tests "rejects empty content", "rejects empty doc_id", and "rejects content longer than" with exact error message strings instead of regex patterns like /must not be empty/ and /exceeds 50000 chars/. For the "rejects SQL-identifier injection in the table name" test, replace the generic toThrow() with a specific error message assertion to ensure the exact failure mode is being tested. This tightens the contract checks to verify precise error messages rather than loose substring matches.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/docs.ts`:
- Around line 195-196: The resolveContent() function can throw exceptions when
provided with invalid file paths or unreadable stdin, but it is currently
executed before the try/catch block at line 195, allowing uncaught exceptions to
terminate the command abruptly. Move the resolveContent() function call (and any
dependent code that uses its result) inside the existing try/catch error handler
to ensure failures are caught and handled as controlled CLI errors instead of
uncaught exceptions.
In `@src/docs/gate.ts`:
- Around line 79-87: The countChangedLines function call on line 80 performs an
expensive O(n*m) LCS diff operation before validating the content size
constraints, which means very large inputs consume significant CPU even though
they will be rejected by the GATE_MAX_CONTENT_LENGTH check. Move the content
validation checks (the empty content check and the GATE_MAX_CONTENT_LENGTH
check) to execute before the countChangedLines call so that oversized proposals
are rejected before the quadratic diff algorithm runs. This applies to the
countChangedLines call and the subsequent size validation checks in the reasons
array population section.
In `@src/docs/refresh.ts`:
- Around line 147-153: The code invokes args.generate() without first checking
if the document should be rejected as a slow-tier doc via gateDocEdit, which
unnecessarily leaks protected content to the LLM and incurs avoidable costs.
Move the gateDocEdit check before the args.generate() call so that slow-tier
documents are rejected without ever being sent to the generate function,
preventing unnecessary LLM invocations and content exposure.
In `@src/docs/write.ts`:
- Around line 56-57: The SetDocInput.project field is currently being ignored
during version bumps when a document already exists. In the setDoc function, the
project value from SetDocInput is dropped and appendVersion always uses the
previous project value instead of the new one provided. To fix this, propagate
the project value from SetDocInput through the setDoc function and pass it as a
parameter to appendVersion so it uses the new project value (when provided)
instead of always defaulting to previous.project. This will enable proper
project reassignment and prevent stale metadata from affecting listDocs
filtering.
In `@src/graph/render/impact.ts`:
- Around line 24-27: The reverse traversal logic for collecting impacted nodes
is currently processing all relations but should be restricted to dependency
relations only as documented in the comments. Update the traversal in the
impactedNodes collection section (around lines 41-45) to filter and process only
the dependency-related edges: calls, imports, extends, implements, and
method_of. Remove or skip any other relation types that are not part of the
dependency graph to prevent over-expansion of the closure and avoid marking
unrelated documents as impacted.
In `@tests/shared/docs-refresh.test.ts`:
- Line 81: The test assertions in this file use `.join().toMatch()` with regex
patterns to broadly match substring content in the reasons array, which can hide
incorrect reason values. Replace these broad substring checks with direct
assertions on the specific reason values. Instead of joining and using regex
matching on r.reasons, assert the exact expected reason value directly using
methods like toContain() or by checking specific array indices with toBe().
Apply this fix to all occurrences at lines 81, 87, 92, 97, 103, 180, and 214.
---
Nitpick comments:
In `@tests/claude-code/cli-docs.test.ts`:
- Around line 87-223: Replace generic regex substring matching assertions with
exact string or structured value checks throughout the test suite. Instead of
using toMatch() with broad regex patterns like /hivemind docs/ or /Not logged
in/, use toContain() for exact string matches or direct equality checks for
specific values. Apply this to key test cases such as "prints usage with no
subcommand", "exits 2 when not logged in", "creates v1 via the real store",
"prints (no doc) when nothing matches", "lists rows", "archives an existing
doc", and other test assertions that currently rely on partial pattern matching
to make the CLI contract tests more precise and robust.
In `@tests/shared/docs-impact.test.ts`:
- Around line 133-138: The test for the "changed when the code differs from the
stored hash" case is only asserting on the state property of the anchorStatus
result, but it should verify the complete changed payload including the from and
to values. Replace the single assertion on st.state with a comprehensive
assertion that checks the full object structure returned by anchorStatus to
catch potential regressions in the from and to fields.
In `@tests/shared/docs.test.ts`:
- Around line 129-159: The rejection tests in this section use generic error
assertions that are too permissive. Replace the regex substring matches in the
tests "rejects empty content", "rejects empty doc_id", and "rejects content
longer than" with exact error message strings instead of regex patterns like
/must not be empty/ and /exceeds 50000 chars/. For the "rejects SQL-identifier
injection in the table name" test, replace the generic toThrow() with a specific
error message assertion to ensure the exact failure mode is being tested. This
tightens the contract checks to verify precise error messages rather than loose
substring matches.
In `@tests/shared/graph/load-current.test.ts`:
- Around line 54-58: The test assertion in the "falls back to snapshot_sha256
when there is no commit context" test is too broad. Instead of using
expect(loadCurrentSnapshot(cwd)).not.toBeNull(), replace it with a specific
assertion that validates the returned snapshot object contains the expected
concrete values for the nodes and links properties that were written to the
snapshot file using writeSnapshotFile. This ensures the function correctly loads
and returns the actual snapshot payload rather than just checking it exists.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6517aef1-95e4-4670-9211-446883525e7a
📒 Files selected for processing (31)
src/cli/index.tssrc/commands/docs.tssrc/commands/graph.tssrc/config.tssrc/deeplake-api.tssrc/deeplake-schema.tssrc/docs/anchors.tssrc/docs/auto-refresh-trigger.tssrc/docs/gate.tssrc/docs/impact.tssrc/docs/index.tssrc/docs/read.tssrc/docs/refresh-llm.tssrc/docs/refresh.tssrc/docs/write.tssrc/graph/load-current.tssrc/graph/render/impact.tstests/claude-code/cli-docs.test.tstests/claude-code/flush-memory-wiring.test.tstests/claude-code/flush-memory.test.tstests/claude-code/skillify-auto-pull.test.tstests/claude-code/spawn-wiki-worker.test.tstests/shared/deeplake-api.test.tstests/shared/docs-auto-refresh-trigger.test.tstests/shared/docs-impact.test.tstests/shared/docs-refresh-llm.test.tstests/shared/docs-refresh.test.tstests/shared/docs.test.tstests/shared/graph/deeplake-pull.test.tstests/shared/graph/deeplake-push.test.tstests/shared/graph/load-current.test.ts
| it("rejects empty content", () => { | ||
| const r = gateDocEdit({ tier: "fast", prevContent: "x", newContent: "", newAnchors: [], snap: s }); | ||
| expect(r.ok).toBe(false); | ||
| expect(r.reasons.join()).toMatch(/empty/); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use exact reason assertions instead of join()+regex checks.
These assertions are currently broad substring matches, which can hide wrong reason text. Assert specific reason values directly.
✅ Suggested assertion style
- expect(r.reasons.join()).toMatch(/empty/);
+ expect(r.reasons).toContain("proposed content is empty");
- expect(r.reasons.join()).toMatch(/slow-tier/);
+ expect(r.reasons).toContain("slow-tier docs are human-curated; automatic refresh is not allowed");
- expect(report.outcomes[0].reasons?.join()).toMatch(/LLM down/);
+ expect(report.outcomes[0].reasons).toContain("generate failed: LLM down");As per path instructions, tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.
Also applies to: 87-87, 92-92, 97-97, 103-103, 180-180, 214-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/shared/docs-refresh.test.ts` at line 81, The test assertions in this
file use `.join().toMatch()` with regex patterns to broadly match substring
content in the reasons array, which can hide incorrect reason values. Replace
these broad substring checks with direct assertions on the specific reason
values. Instead of joining and using regex matching on r.reasons, assert the
exact expected reason value directly using methods like toContain() or by
checking specific array indices with toBe(). Apply this fix to all occurrences
at lines 81, 87, 92, 97, 103, 180, and 214.
Source: Path instructions
Collaborator
Author
There was a problem hiding this comment.
Style nit, not a correctness gap — the join()+regex assertions do verify the exact reason text. Leaving the existing assertions; will tighten to exact-array matches if we touch these tests again.
There was a problem hiding this comment.
@efenocchi Fair enough — the regex patterns are tight enough that the exact text is effectively verified. Noted; happy to revisit when those tests are next modified.
ʕ •`ᴥ•´ʔ
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 a Knowledge Base / per-file documentation layer that stays fresh on code deltas — the third layer alongside session summaries and the AST code graph. For the Krisp use case: internal docs that go stale because code changes many times a day.
Honest framing (per the team's own oracle eval): this is justified as doc freshness / onboarding trust / token savings, NOT QA-answer accuracy.
How it works
A doc anchors each described symbol to a
content_hashof its source slice. On a commit, the graph diff + blast-radius selects only the docs whose anchored code (or its callers) changed; a bounded LLM edit regenerates them through an objective gate; the new version is written INSERT-only (version-bumped, immutablecreated_at).Steps in this PR
hivemind_docstable (INSERT-only, version-bumped) +docsTableName.src/docsinsert/edit/list/get + idempotentsetDoc(no history fork).anchors.ts(source-slice hash) +impact.ts(direct hash drift + blast-radius widening over the graph).gate.ts(objective invariants: bounded edit, anchors exist, slow-tier protected) +refresh.ts(re-anchor -> generate -> gate -> write) +refresh-llm.ts(hostclaude -p, reuses the wiki-worker seam) +hivemind docsCLI (set/show/list/archive/refresh,--anchor,--dry-run).HIVEMIND_DOCS_AUTO_REFRESH=1fires a detached refresh at the end ofgraph build.Testing
hivemindbinary against a test Deeplake table: real code change -> drift detected ->clauderewrote the doc to match -> gate passed -> v2 written,created_atpreserved.Not in scope
No VFS routing for the docs memory subtree yet (CLI/worker only); no spec-layer correlation; per-agent doc-worker variants (codex/cursor/hermes/pi) follow the existing wiki-worker fork pattern.
Summary by CodeRabbit
Release Notes
New Features
hivemind docsCLI command with subcommands:set,show,list,archive, andrefreshHIVEMIND_DOCS_AUTO_REFRESH=1Chores
HIVEMIND_DOCS_TABLE) and automatic table/schema/index setup