feat(memory): ranked search + Compiled Truth fact-superseding#16
Open
smixs wants to merge 3 commits into
Open
Conversation
Ports Iva's memory-v2 onto the Obsidian vault: - sync autograph search.py (BM25 + graph rerank) and supersede.py (same-entity contradiction detector) from upstream - brain-system.md: recall protocol — search.py FIRST for 'what do I know about X' before manual grep; read superseded/inferred hits carefully; MEMORY.md size discipline - dbrain-processor/classification.md: ADD/SUPERSEDE/NOOP contract — rewrite the current value (Compiled Truth), move the old to an append-only ## History line, tag confidence EXTRACTED/INFERRED/AMBIGUOUS - process.sh: run supersede.py before the daily turn so candidates are ready - processor.py: daily turn resolves .graph/supersede-candidates.json Verified in-vault: search ranks real cards; discover→schema→supersede detects a company conflict and writes supersede-candidates.json.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds supersede-based conflict handling for vault cards, wires it into daily processing, and introduces a ranked vault search CLI with updated recall instructions and confidence guidance. ChangesMemory Conflict Handling and Ranked Recall
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant search.py
participant SQLite
participant vault-graph.json
CLI->>search.py: query, scope, limit
search.py->>SQLite: BM25 search or naive fallback
SQLite-->>search.py: ranked hits
search.py->>vault-graph.json: load graph and compute BFS distances
search.py->>search.py: apply coverage, proximity, and stale demotion
search.py-->>CLI: ranked hits with snippets
sequenceDiagram
participant scripts/process.sh
participant supersede.py
participant Vault cards
participant .graph/supersede-candidates.json
scripts/process.sh->>supersede.py: run pre-processing supersede scan
supersede.py->>Vault cards: collect duplicate groups and parse frontmatter
supersede.py->>.graph/supersede-candidates.json: write conflict candidates
supersede.py->>Vault cards: rewrite older card as superseded with --apply
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eddb9a2f1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| vault_dir = Path(args[0]) | ||
| apply = "--apply" in args | ||
| verbose = "--verbose" in args | ||
| schema = load_schema(vault_dir / ".claude" / "skills" / "autograph" / "schema.json") |
There was a problem hiding this comment.
In this repo the vault ships vault/.claude/skills/autograph/schema.example.json but no schema.json, so invoking the new scan as wired by scripts/process.sh (python3 vault/.claude/skills/autograph/scripts/supersede.py vault) raises FileNotFoundError before writing .graph/supersede-candidates.json. Because the shell step suppresses that failure, the daily processor will read a stale or missing candidates file and the new Compiled Truth conflict-resolution path never runs for this vault unless a schema file has been generated out of band; use the existing default schema discovery/fallback instead of hard-coding schema.json here.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
deploy/brain-system.md (1)
64-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language hint to the fenced code block.
Markdownlint (MD040) flags this block for missing a language annotation.
📝 Suggested fix
-``` +```bash uv run vault/.claude/skills/autograph/scripts/search.py "<query>" --vault vault</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@deploy/brain-system.mdaround lines 64 - 66, The fenced command example in
the markdown block is missing a language annotation, which triggers MD040.
Update the code fence that contains the search.py command to use the appropriate
language hint so the block is explicitly marked as a bash shell snippet.</details> <!-- cr-comment:v1:e9368af52115e53124896d13 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>vault/.claude/skills/autograph/scripts/search.py (2)</summary><blockquote> `216-229`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _⚡ Quick win_ **IDF/coverage use substring containment, not token membership.** `df`/`coverage` check `t in h` against the raw concatenated haystack string, so a short token like `"cat"` will count as present in any doc containing `"category"`, `"education"`, etc. This inflates document frequency (lowering IDF) and coverage for unrelated docs, undermining the "language-agnostic IDF" design the docstring describes (Lines 215, 219-229). Building a tokenized set per doc (e.g. via `content_tokens`) for membership tests would fix this without much extra cost. <details> <summary>♻️ Suggested approach</summary> ```diff - hay_by_path = { - x["path"]: f"{x['title']} {x['meta']} {x['tags']} {x['body']}".lower() for x in docs - } - idf = {} - for t in tokens: - df = sum(1 for h in hay_by_path.values() if t in h) - idf[t] = math.log((len(docs) + 1) / (df + 1)) + 1 - idf_total = sum(idf.values()) or 1 - - def coverage(path): - if not tokens: - return 1.0 - hay = hay_by_path.get(path, "") - return sum(idf[t] for t in tokens if t in hay) / idf_total + tokset_by_path = { + x["path"]: set(content_tokens(f"{x['title']} {x['meta']} {x['tags']} {x['body']}")) + for x in docs + } + idf = {} + for t in tokens: + df = sum(1 for s in tokset_by_path.values() if t in s) + idf[t] = math.log((len(docs) + 1) / (df + 1)) + 1 + idf_total = sum(idf.values()) or 1 + + def coverage(path): + if not tokens: + return 1.0 + toks = tokset_by_path.get(path, set()) + return sum(idf[t] for t in tokens if t in toks) / idf_total🤖 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 `@vault/.claude/skills/autograph/scripts/search.py` around lines 216 - 229, The IDF and coverage logic in search.py is using raw substring checks against the concatenated haystack, which causes false matches for short tokens. Update the document-frequency calculation and the coverage() helper to test membership against per-document token sets (for example the existing content_tokens-style tokenization) instead of using t in h on the lowercased string. Keep the behavior in the hay_by_path/idf/coverage flow consistent so only real tokens contribute to df, IDF, and coverage.
273-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVacuous selftest assertion.
assert search_memory(v, "zzzznomatch", limit=5)["count"] == 0 or Truealways evaluates toTrueregardless of the actual count, so it never verifies the "no matches" case — it only confirmssearch_memorydoesn't raise. That's a weaker guarantee than the comment ("graceful even with no matches") implies, and means the PR's claim that--selftestvalidates ranking/demotion behavior is not fully backed by this assertion.🐛 Suggested fix
- assert search_memory(v, "zzzznomatch", limit=5)["count"] == 0 or True + assert search_memory(v, "zzzznomatch", limit=5)["count"] == 0🤖 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 `@vault/.claude/skills/autograph/scripts/search.py` around lines 273 - 278, The selftest in search.py has a vacuous assertion for the no-matches case, so it never actually verifies behavior. Update the `search_memory` selftest assertion near the ranking check to assert the returned result for a query like “zzzznomatch” has zero hits/count, instead of using an expression that always passes. Keep the existing `files` ranking check intact, and make the no-match check in `search_memory`/`--selftest` fail if the search returns any results.
🤖 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 `@scripts/process.sh`:
- Around line 83-89: The supersede scan in process.sh can leave a stale
.graph/supersede-candidates.json behind when supersede.py fails before writing a
fresh file. Update the scan step around the supersede.py invocation so that a
failure also clears or removes the existing candidates artifact before
continuing, not just logs the error. Use the supersede.py call and the
.graph/supersede-candidates.json artifact as the key touchpoints, and keep the
daily turn from consuming stale conflict data.
In `@vault/.claude/skills/autograph/scripts/supersede.py`:
- Around line 77-97: apply_supersede currently trusts cand["current"] and
cand["superseded_candidates"] from scan() without verifying that the chosen
current card is strictly newer, so add an explicit date comparison before
writing superseded metadata. In apply_supersede, inspect the parsed frontmatter
for both cards and only proceed when the current card’s date is present and
strictly greater than the old card’s date; otherwise skip the candidate. Keep
the safety check aligned with the docstring and ensure the status/superseded_by
update only happens after that validation.
- Line 31: The conflict scan is still counting cards already marked as
superseded because `scan()`/`collect_duplicate_groups()` continue to treat
`status` as an active conflict field. Update the supersede flow in
`supersede.py` so cards with `status == "superseded"` are filtered out before
duplicate/conflict grouping, or otherwise exclude `status` from live
contradiction detection while preserving the `apply_supersede()` write. Keep the
fix aligned with `CONFLICT_FIELDS`, `scan()`, and `collect_duplicate_groups()`.
---
Nitpick comments:
In `@deploy/brain-system.md`:
- Around line 64-66: The fenced command example in the markdown block is missing
a language annotation, which triggers MD040. Update the code fence that contains
the search.py command to use the appropriate language hint so the block is
explicitly marked as a bash shell snippet.
In `@vault/.claude/skills/autograph/scripts/search.py`:
- Around line 216-229: The IDF and coverage logic in search.py is using raw
substring checks against the concatenated haystack, which causes false matches
for short tokens. Update the document-frequency calculation and the coverage()
helper to test membership against per-document token sets (for example the
existing content_tokens-style tokenization) instead of using t in h on the
lowercased string. Keep the behavior in the hay_by_path/idf/coverage flow
consistent so only real tokens contribute to df, IDF, and coverage.
- Around line 273-278: The selftest in search.py has a vacuous assertion for the
no-matches case, so it never actually verifies behavior. Update the
`search_memory` selftest assertion near the ranking check to assert the returned
result for a query like “zzzznomatch” has zero hits/count, instead of using an
expression that always passes. Keep the existing `files` ranking check intact,
and make the no-match check in `search_memory`/`--selftest` fail if the search
returns any results.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aee42c2b-5818-4672-b251-2a7b9eb6f188
📒 Files selected for processing (6)
deploy/brain-system.mdscripts/process.shsrc/d_brain/services/processor.pyvault/.claude/skills/autograph/scripts/search.pyvault/.claude/skills/autograph/scripts/supersede.pyvault/.claude/skills/dbrain-processor/references/classification.md
- processor.py: reflow daily prompt lines under ruff line-length (E501) - process.sh: clear stale .graph/supersede-candidates.json when the scan fails, so the daily turn never resolves against a previous run's conflicts - sync search.py/supersede.py with autograph fixes: bm25 column-weight alignment (UNINDEXED path slot), schema fallback so the scan runs on a fresh vault, tolerate frontmatter-less notes, safe --apply
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vault/.claude/skills/autograph/scripts/search.py (1)
281-282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTautological assertion never validates the no-match case.
assert ... == 0 or Trueis always true regardless of the returned count, so this only checks thatsearch_memorydoesn't raise. Dropor Trueto actually assert the empty-result contract.💚 Proposed fix
- # graceful even with no matches - assert search_memory(v, "zzzznomatch", limit=5)["count"] == 0 or True + # graceful even with no matches + assert search_memory(v, "zzzznomatch", limit=5)["count"] == 0🤖 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 `@vault/.claude/skills/autograph/scripts/search.py` around lines 281 - 282, The no-match test in search_memory is tautological because the assertion always passes with “or True”. Update the assertion in the search.py test to check the returned count directly, using the search_memory call with the “zzzznomatch” query, so it genuinely verifies that the empty-result contract holds instead of only ensuring no exception is raised.
🤖 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.
Outside diff comments:
In `@vault/.claude/skills/autograph/scripts/search.py`:
- Around line 281-282: The no-match test in search_memory is tautological
because the assertion always passes with “or True”. Update the assertion in the
search.py test to check the returned count directly, using the search_memory
call with the “zzzznomatch” query, so it genuinely verifies that the
empty-result contract holds instead of only ensuring no exception is raised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 300b33db-48da-4b9e-8e47-98ff038aa30c
📒 Files selected for processing (4)
scripts/process.shsrc/d_brain/services/processor.pyvault/.claude/skills/autograph/scripts/search.pyvault/.claude/skills/autograph/scripts/supersede.py
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/process.sh
- src/d_brain/services/processor.py
- vault/.claude/skills/autograph/scripts/supersede.py
…conflicts, selftest
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.
Ports Iva's memory-v2 onto the vault. Today recall is grep/glob + graph walking with no ranking, and there's no model for contradicting facts (a job change just stacks two cards). This adds both.
Ranked search (autograph)
search.py— BM25 via stdlib sqlite3 FTS5 + link-graph rerank over the nightlyvault-graph.json, IDF coverage, stale-status demotion, graceful naive fallback. Schema/language-agnostic. Synced from autograph upstream (smixs/autograph#3).brain-system.mdrecall protocol: on "что я знаю про X / как звали / когда решили", runsearch.pyFIRST, open the top hits, readsuperseded/inferredcarefully. Plus MEMORY.md size discipline (it's the always-loaded core).Compiled Truth + History + confidence
supersede.py(synced) detects same-entity cards with conflicting scalar fields and writes.graph/supersede-candidates.json. Wired in:process.shruns the scan before the daily turn so candidates are ready.processor.pydaily prompt gains a step: resolve each conflict by rewriting the current value (Compiled Truth), moving the old to an append-only## Historyline, taggingconfidence:.dbrain-processor/classification.mddocuments the ADD/SUPERSEDE/NOOP + confidence contract (taxonomy-neutral, appended to the existing file).Verified in-vault
search.py --selftestgreen; the exact recall command ranks a real card in this vault.discover.py → generate_schema.py → supersede.pydetects a company conflict (current=newer card, stale=older) and writes the candidates file.Depends on autograph upstream PR (smixs/autograph#3) for the canonical scripts; synced copies are included here so the vault is self-contained.
Summary by CodeRabbit
supersededwithsuperseded_by.MEMORY.md, including history pruning and confidence handling rules.