feat(doc-linker): belief↔document anchor v2.0 (#435) - #494
Conversation
Reviewer's GuideImplements the v2.0 belief↔document linker: a new belief_documents table and DocAnchor abstraction, ingest- and CLI-based anchor writers, and an opt-in retrieve_v2(with_doc_anchors=True) projection, plus corpus/bench-gate wiring and tests. Sequence diagram for ingest-time document anchor creationsequenceDiagram
actor Operator
participant IngestClient
participant DerivationWorker
participant MemoryStore
participant DB as belief_documents
Operator->>IngestClient: submit document with source_path
IngestClient->>DerivationWorker: DerivationInput(source_path)
DerivationWorker->>MemoryStore: insert_or_corroborate()
MemoryStore-->>DerivationWorker: actual_id
alt source_path is set
DerivationWorker->>DerivationWorker: file_uri_from_path(source_path)
DerivationWorker->>MemoryStore: link_belief_to_document(belief_id=actual_id, doc_uri=file_uri, anchor_type=ANCHOR_INGEST)
MemoryStore->>DB: INSERT OR IGNORE belief_id, doc_uri, anchor_type, position_hint, created_at
DB-->>MemoryStore: canonical row
MemoryStore-->>DerivationWorker: DocAnchor
else source_path is None
DerivationWorker-->>DerivationWorker: skip anchor write
end
Sequence diagram for manual anchor via aelf lock --docsequenceDiagram
actor User
participant CLI as aelf_lock_CLI
participant MemoryStore
participant DocLinker as doc_linker_module
participant DB as belief_documents
User->>CLI: aelf lock --doc DOC_URI
CLI->>MemoryStore: lock_belief()
MemoryStore-->>CLI: belief_id
CLI->>DocLinker: link_belief_to_document(store, belief_id, doc_uri=DOC_URI, anchor_type=ANCHOR_MANUAL)
DocLinker->>MemoryStore: link_belief_to_document(belief_id, DOC_URI, ANCHOR_MANUAL)
MemoryStore->>DB: INSERT OR IGNORE belief_id, DOC_URI, anchor_type, position_hint, created_at
DB-->>MemoryStore: canonical row
MemoryStore-->>DocLinker: DocAnchor
DocLinker-->>CLI: DocAnchor
CLI-->>User: print locked belief_id
Sequence diagram for retrieve_v2 with with_doc_anchors=TruesequenceDiagram
actor Adapter
participant Retrieval as retrieve_v2
participant MemoryStore
participant DB as belief_documents
Adapter->>Retrieval: retrieve_v2(..., with_doc_anchors=True)
Retrieval->>MemoryStore: core retrieval queries
MemoryStore-->>Retrieval: beliefs list
alt with_doc_anchors and beliefs not empty
Retrieval->>MemoryStore: get_doc_anchors_batch([belief_ids])
MemoryStore->>DB: SELECT * FROM belief_documents WHERE belief_id IN (...)
DB-->>MemoryStore: rows ordered by belief_id, created_at
MemoryStore-->>Retrieval: dict belief_id -> list DocAnchor
Retrieval->>Retrieval: build doc_anchors list aligned with beliefs
else
Retrieval-->>Retrieval: doc_anchors = []
end
Retrieval-->>Adapter: RetrievalResult(beliefs, doc_anchors, ...)
ER diagram for new belief_documents tableerDiagram
beliefs {
TEXT id PK
TEXT content
TEXT other_columns
}
belief_documents {
TEXT belief_id FK
TEXT doc_uri
TEXT anchor_type
TEXT position_hint
REAL created_at
TEXT belief_id_doc_uri PK
}
beliefs ||--o{ belief_documents : has
Class diagram for DocAnchor, MemoryStore, RetrievalResult, and linker helpersclassDiagram
class DocAnchor {
+str belief_id
+str doc_uri
+str anchor_type
+str position_hint
+float created_at
}
class MemoryStore {
+link_belief_to_document(belief_id: str, doc_uri: str, anchor_type: str, position_hint: str) DocAnchor
+get_doc_anchors(belief_id: str) list~DocAnchor~
+get_doc_anchors_batch(belief_ids: list~str~) dict~str, list~DocAnchor~~
}
class RetrievalResult {
+list~Belief~ beliefs
+list~str~ entity_hits
+list~str~ locked_ids
+list~str~ l1_ids
+list~list~DocAnchor~~ doc_anchors
+list~CompressedBelief~ compressed_beliefs
}
class DocLinkerModule {
+file_uri_from_path(source_path: str, project_root: Path, position_hint: str) str
+link_belief_to_document(store: MemoryStore, belief_id: str, doc_uri: str, anchor_type: str, position_hint: str) DocAnchor
+get_doc_anchors(store: MemoryStore, belief_id: str) list~DocAnchor~
}
class RetrievalModule {
+retrieve_v2(store: MemoryStore, with_doc_anchors: bool) RetrievalResult
}
MemoryStore --> DocAnchor : returns
RetrievalResult --> DocAnchor : contains
DocLinkerModule ..> MemoryStore : uses
DocLinkerModule ..> DocAnchor : creates
RetrievalModule ..> MemoryStore : uses
RetrievalModule ..> RetrievalResult : returns
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 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 |
robotrocketscience
added
author-Toug
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- In
MemoryStore.get_doc_anchors_batch, you build the query directly from thebelief_idslist, which means duplicate ids inflate the placeholder list and cause redundant rows to be materialized; consider deduplicating or asserting uniqueness on the input to keep the query smaller and behavior easier to reason about. - The
MemoryStore.link_belief_to_documentmethod performs its ownanchor_typevalidation and also importsANCHOR_TYPES/DocAnchor, while the publicdoc_linker.link_belief_to_documentalready enforces the same constraints; ifMemoryStore.link_belief_to_documentis intended to be internal-only, you could either drop the duplicated validation and keep a clearer layering (validation in the doc_linker module) or document why both layers are needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MemoryStore.get_doc_anchors_batch`, you build the query directly from the `belief_ids` list, which means duplicate ids inflate the placeholder list and cause redundant rows to be materialized; consider deduplicating or asserting uniqueness on the input to keep the query smaller and behavior easier to reason about.
- The `MemoryStore.link_belief_to_document` method performs its own `anchor_type` validation and also imports `ANCHOR_TYPES`/`DocAnchor`, while the public `doc_linker.link_belief_to_document` already enforces the same constraints; if `MemoryStore.link_belief_to_document` is intended to be internal-only, you could either drop the duplicated validation and keep a clearer layering (validation in the doc_linker module) or document why both layers are needed.
## Individual Comments
### Comment 1
<location path="src/aelfrice/doc_linker.py" line_range="83-92" />
<code_context>
+ stores the same value separately so consumers can read it without
+ parsing the URI.
+ """
+ p = Path(source_path)
+ rel: str
+ if project_root is not None:
+ try:
+ rel = str(p.resolve().relative_to(project_root.resolve()))
+ except ValueError:
+ rel = str(p)
+ else:
+ rel = str(p)
+ uri = f"file:{rel}" if not rel.startswith("/") else f"file://{rel}"
+ if position_hint:
+ uri = f"{uri}#{position_hint}"
</code_context>
<issue_to_address>
**issue (bug_risk):** The `file://` URI construction may be incorrect / non-portable on Windows paths.
On Windows, `Path(source_path)` will yield paths like `C:\foo\bar.py`, so `rel.startswith('/')` will be false and you’ll generate `file:C:\foo\bar.py`, which is not a valid file URI and may not round-trip. Consider using `Path.as_uri()` (and then adapting it for the project-root-relative case) so both absolute and project-root-relative values are valid `file://` URIs on all platforms, including correct handling of drive letters.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/store.py" line_range="2039-2047" />
<code_context>
cur = self._conn.execute(
f"""
SELECT belief_id, doc_uri, anchor_type, position_hint, created_at
FROM belief_documents
WHERE belief_id IN ({ph})
ORDER BY belief_id ASC, created_at ASC
""",
tuple(belief_ids),
)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| p = Path(source_path) | ||
| rel: str | ||
| if project_root is not None: | ||
| try: | ||
| rel = str(p.resolve().relative_to(project_root.resolve())) | ||
| except ValueError: | ||
| rel = str(p) | ||
| else: | ||
| rel = str(p) | ||
| uri = f"file:{rel}" if not rel.startswith("/") else f"file://{rel}" |
There was a problem hiding this comment.
issue (bug_risk): The file:// URI construction may be incorrect / non-portable on Windows paths.
On Windows, Path(source_path) will yield paths like C:\foo\bar.py, so rel.startswith('/') will be false and you’ll generate file:C:\foo\bar.py, which is not a valid file URI and may not round-trip. Consider using Path.as_uri() (and then adapting it for the project-root-relative case) so both absolute and project-root-relative values are valid file:// URIs on all platforms, including correct handling of drive letters.
| cur = self._conn.execute( | ||
| f""" | ||
| SELECT belief_id, doc_uri, anchor_type, position_hint, created_at | ||
| FROM belief_documents | ||
| WHERE belief_id IN ({ph}) | ||
| ORDER BY belief_id ASC, created_at ASC | ||
| """, | ||
| tuple(belief_ids), | ||
| ) |
There was a problem hiding this comment.
security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
Source: opengrep
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from typing import Final | ||
|
|
||
| from aelfrice.derivation import DerivationInput, RouteOverrides, derive | ||
| from aelfrice.doc_linker import ANCHOR_INGEST, file_uri_from_path |
| from typing import TYPE_CHECKING, Final | ||
|
|
||
| if TYPE_CHECKING: | ||
| from aelfrice.store import MemoryStore |
| """ | ||
| # Imported here to avoid a module-level cycle: doc_linker imports | ||
| # MemoryStore via TYPE_CHECKING-only. | ||
| from aelfrice.doc_linker import ANCHOR_TYPES, DocAnchor |
|
|
||
| def get_doc_anchors(self, belief_id: str) -> list["DocAnchor"]: # noqa: F821 | ||
| """Return every anchor for one belief, ordered by `created_at` ASC.""" | ||
| from aelfrice.doc_linker import DocAnchor |
| costs one indexed read per call rather than one per surfaced | ||
| belief. Result dict has an entry for every requested id. | ||
| """ | ||
| from aelfrice.doc_linker import DocAnchor |
|
[claim:review:Setr:2026-05-08T19:26:09Z] |
ReviewDiff: 14 files, +1178 / −2. 26 new tests (storage/worker/retrieve/CLI). Spec § A2 bench-gate scaffolded but skips on public CI per directory-of-origin rule. Discretion grep against Cannot merge today — CI failures dispositioned
Code-side review
DispositionCode-side LGTM. Holding |
|
[release:review:Setr:2026-05-08T19:29:16Z] |
|
[claim:review:kulili:2026-05-08T19:31:44Z] |
|
[claim:review:Gylf:2026-05-08T19:31:50Z] |
|
[release:review:Gylf:2026-05-08T19:31:55Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
Concur with Setr's review (code LGTM, blocked on replay-soak streak). Verified |
|
[release:review:kulili:2026-05-08T19:33:42Z] |
|
[claim:review:Gylf:2026-05-08T19:45:05Z] |
|
Reviewed at Required change 1 — rebase + content conflict on Required change 2 — CodeQL runner_mod = pytest.importorskip(
"tests.retrieve_uplift_runner",
reason=(
"doc-linker uplift runner not yet wired "
"(operator gate; spec § A2 — pending lab-side corpus + scorer)"
),
)
results = runner_mod.run_doc_linker_uplift(rows)CodeQL flow analysis can't see that Non-blocking observations:
Releasing review claim. Re-flag |
|
[release:review:Gylf:2026-05-08T19:47:43Z] |
robotrocketscience
removed
attn:review
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
2d6036b to
8fa3618
Compare
robotrocketscience
added
attn:review
|
Rebased onto |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
[claim:review:Kulili:2026-05-08T20:05:21Z] |
State at 2026-05-08T20:06Z (review claim → release)Branch: What landed on
The conflict is concentrated where Other gates (unchanged from prior review pass):
Action: author rebase onto |
|
[release:review:Kulili:2026-05-08T20:06:40Z] |
Adds the storage layer for the document/semantic linker per docs/feature-doc-linker.md. New `belief_documents` table is sibling to `belief_corroborations` and `belief_versions` (same conventions: composite PK, ON DELETE CASCADE on `belief_id`, dedicated index for the reverse- direction query). `INSERT OR IGNORE` gives idempotency on `(belief_id, doc_uri)` per spec A3. `src/aelfrice/doc_linker.py` exposes the public DocAnchor dataclass plus `link_belief_to_document()` / `get_doc_anchors()` wrappers, matching the spec's import contract. `MemoryStore.get_doc_anchors_batch()` is the batched fetch used by the retrieve()-side projection in a follow-up commit. No writer hooks yet — derivation worker, retrieve() kwarg, and CLI flag land in subsequent commits. Schema migration is forward-only and additive (CREATE TABLE IF NOT EXISTS), so this commit is safe to land independently: empty table == no impact.
…er (#435) After insert_or_corroborate, when the log row carried source_path, the worker writes a `belief_documents` anchor pointing at that path with anchor_type='ingest'. Idempotent on (belief_id, doc_uri) so re-derive of the same row produces no extra anchors. Skips silently when source_path is None — transcript ingest, lock / remember without --doc, and any future entry point that doesn't carry a canonical doc URI just don't get anchors. Two ingests of the same content (corroboration) from different files stack two anchors on the one belief. URI normalisation (project-root relative vs absolute) is left to file_uri_from_path() callers; the worker passes raw source_path through for v2.0.0.
Per spec § "Where the linker sits in retrieval", the linker is a post-rank, pre-pack projection. New kwarg `with_doc_anchors` (default False) on `retrieve_v2()` opt-ins to one batched SELECT that joins `belief_documents` rows onto the result. `RetrievalResult.doc_anchors` is a parallel list to `beliefs`: same length, same order, one `list[DocAnchor]` per belief (empty when the belief has no anchors). Default-off keeps the adapter wire shape byte-identical — `doc_anchors=[]` is the new field's default. Anchors are metadata for the consumer; they do NOT count against the token-budget pack. A future revision that wants inline rendering can add `with_doc_anchors_inline=True` separately (deferred per spec § "Where the linker sits"). The public `retrieve()` keeps its `list[Belief]` return type — only the v2 wrapper exposes the projection, matching the precedent set by `compressed_beliefs` (#434).
The lock entry point passes source_path=None to the derivation worker (cli_remember has no canonical document), so the ingest-time hook does not fire on this path. The new --doc flag is the manual surface — when set, the CLI handler writes one anchor_type='manual' row after the worker stamps the belief. Idempotent on re-lock with the same URI (storage layer's INSERT OR IGNORE on (belief_id, doc_uri)). Empty / unset URI is a no-op write — no validation beyond the storage layer's non-empty check. Spec § "Linker invocation point" lists this as the manual surface; URI scheme is opaque (file:// or https:// recommended).
Spec § A1: corpus mount point at tests/corpus/v2_0/doc_linker/ with the .gitkeep scaffold; per-row schema documented in tests/corpus/v2_0/README.md matches the precedent set by retrieve_uplift, dedup, and the BFS edge modules. Spec § A2 bench-gate test at tests/bench_gate/test_doc_linker.py uses the autouse `bench_gated` marker — public CI skips when AELFRICE_CORPUS_ROOT is unset, and a corpus-mounted run currently surfaces SKIPPED until the uplift runner (`tests.retrieve_uplift_runner.run_doc_linker_uplift`) is written. The runner is the operator-side gate for flipping `with_doc_anchors` to default-on; the schema migration + writer are already shipped (additive, no rows == no impact) so a fail here is a revert of the writer hook only. Spec status updated to "implementation shipped, bench-gate pending lab-side corpus" — matches the pattern set by #433 / #434. Composition tracker A5 row deferred — the tracker doc itself is not yet on main (referenced from #154 only), so per the precedent set by #433 /
8fa3618 to
c9589af
Compare
|
Re-rebased onto |
|
[claim:review:Gylf:2026-05-08T20:09:51Z] |
CodeQL flags 'run_doc_linker_uplift may be uninitialized' on the try/import + pytest.skip pattern — flow analysis can't see that pytest.skip raises. pytest.importorskip is the idiomatic equivalent that returns the module on success and skips otherwise, with no post-import name-binding ambiguity. Same fix as 5465b3c applied to the clustering bench-gate at #496.
|
Re-reviewed at the rebased + fix-up tip. Reviewer-side commit State:
CodeQL "Cyclic import" notices ×5 between
Will FF-merge once CI clears on the fix commit. |
|
[release:review:Gylf:2026-05-08T20:16:04Z] |
The three function-local imports in MemoryStore.link_belief_to_document, get_doc_anchors, and get_doc_anchors_batch now read from the leaf module aelfrice.doc_linker_types instead of aelfrice.doc_linker. Closes the residual edge that #494's merge introduced; the static cycle-finder reports 0 cycles ≤ length 4 across src/aelfrice now, matching the post-#500 invariant. Drops the inline comment about TYPE_CHECKING-only avoidance — the leaf module makes that workaround unnecessary; the comment's premise no longer holds. No behavior change. 2948 tests pass.
2 participants
Summary
Implements the document/semantic linker per
docs/feature-doc-linker.md.Belief↔document anchors stored in a new
belief_documentstable, writtenat ingest-time when
source_pathis known and viaaelf lock --doc=URImanually. Retrieval consumers opt in via
retrieve_v2(..., with_doc_anchors=True).Closes 435.
Commits
feat(doc-linker): belief_documents schema + DocAnchor module— new SQLite table (sibling shape tobelief_corroborations/belief_versions), newsrc/aelfrice/doc_linker.pymodule exposingDocAnchor+link_belief_to_document+get_doc_anchors, plusMemoryStore.{link_belief_to_document,get_doc_anchors,get_doc_anchors_batch}. 14 storage tests covering A3 idempotency + A4 schema migration + FK cascade + multi-URI ordering.feat(doc-linker): hook ingest-time anchor writer into derivation worker— when aningest_logrow carriessource_path, the worker writes abelief_documentsrow withanchor_type='ingest'afterinsert_or_corroborate. Idempotent on re-derive. Skips whensource_path is None(transcripts, lock without--doc).feat(doc-linker): wire with_doc_anchors kwarg into retrieve_v2— new opt-in kwarg, default False (adapter wire shape stays byte-identical when off). On, one batchedbelief_id IN (...)SELECT joins anchors onto the result viaRetrievalResult.doc_anchors(parallel list tobeliefs). Anchors do NOT count against the token-budget pack.feat(doc-linker): aelf lock --doc=URI writes manual anchor— new--docflag onaelf lock. Writesanchor_type='manual'after the worker stamps the belief; idempotent on re-lock.feat(doc-linker): bench-gate harness scaffold + spec status— corpus mount point attests/corpus/v2_0/doc_linker/+ bench-gate test using the autousebench_gatedmarker (skips on public CI). Spec status flipped from "spec, no implementation" to "implementation shipped, bench-gate pending lab-side corpus".Composition tracker A5 row deferred — the tracker doc itself is not yet on
main, matches the precedent set by other v2.0 placeholder issues.Out of scope (per spec)
anchor_type='derived'writers (retrieval-time inference). Enum value is reserved; no writer in this PR.aelf doctor --normalize-doc-uriscleanup pass.with_doc_anchors_inline=Truerendering.[retrieval] with_doc_anchors_default = trueknob.retrieve()(list[Belief]return) — only the v2 wrapper exposes the projection, matching the prior placeholder precedent.Test plan
aelf lock --helpsurfaces--doc DOC_URIblock.belief_documents),MemoryStorecreates the table on first open, anchor write/read round-trip succeeds.tests/test_corpus_schema.pyaccepts the newdoc_linkermodule schema.tests/bench_gate/test_doc_linker.pyskips cleanly whenAELFRICE_CORPUS_ROOTis unset.tests.retrieve_uplift_runner.run_doc_linker_uplift) is the next gate for flippingwith_doc_anchorsto default-on. The schema migration is forward-only and additive — empty table == no impact, so a negative bench is a writer-revert, not a schema rollback.