feat(citations): resolve run-scoped knowledge without silent shadowing - #152
Conversation
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 16f6d8ea
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-17T12:41:44Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 70aec577
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-17T13:00:59Z
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 190.9s (2 bridge agents) |
| Total | 190.9s |
💰 Value — sound-with-nits
Adds lineage-aware page citations (cites frontmatter, chain-scoped resolution with no silent fallback, audit/lint) plus pluggable run lineage to the package that already owns run-scoped stores — coherent, in-grain, and verified working; only a small single-index lint integration gap remains.
- What it does: Three things. (1) Makes run ancestry pluggable: createRunScopedStores() accepts a RunLineageAuthority (src/run-scoped.ts:37) so a product run manifest can own lineage; the default file-backed authority becomes idempotent and rejects reopening a run under a different parent (src/run-scoped.ts:184-201); chains beyond 64 hops now throw instead of silently truncating (src/run-scoped.ts:107, old code j
- Goals it achieves: Let applications retire a second local lineage/citation implementation by making agent-knowledge the canonical owner — run-scoped stores already live here since #115, so the citation semantics belong beside them. It also closes two real fail-loud gaps: the old init() silently overwrote lineage.json when a run was reopened with a different parent, and the old lineage() silently truncated at 64 hops
- Assessment: Good on its merits. It mirrors the existing contradicts pattern exactly (frontmatter id list -> optional schema field -> idListField in store -> lint findings with self-reference check, src/lint.ts:215-235), so it is squarely in the codebase's grain. The qualified-reference grammar is minimal and round-trips (parseKnowledgeCitationReference falls back to whole-string pageId for unknown prefixes).
- Better / existing approach: none — this is the right approach. Searched for existing equivalents: rg 'broken-citation|ambiguous-citation' src/ (only the new files emit them), rg 'contradicts' src/lint.ts (single-index exact-id check, structurally cannot express cross-store ambiguity), rg 'cites|citation' src/lint.ts src/wikilinks.ts src/search.ts (search.ts citationId is a retrieval answer handle, not page-reference resoluti
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A coherent, in-grain lineage-aware citation layer built on the existing run-scoped chain API, fully exported and verified working, with a named imminent consumer (Discovery Lab) and one small adoption gap: the standard flat-index lint gate ignores the new cites field it now loads and graphs.
- Integration: All new surface is exported from the package entrypoint (src/index.ts:7-8 adds citation-lint and citation-resolution) and
citesis wired end-to-end in-repo: frontmatter load (src/store.ts:124), schema (src/schemas.ts:57), type (src/types.ts:98), graph edges with reason 'citation' (src/graph.ts:26-30), and the shared lint finding vocabulary (src/types.ts:163-164) so findings flow into existing co - Fit with existing patterns: Fits established package patterns well:
citesmirrors the existingcontradictsfrontmatter field; typed fail-closed errors (KnowledgeCitationResolutionError, KnowledgeCitationAuditError) match the repo's no-fallbacks doctrine; the lineage file write goes throughwithKnowledgeMutation(src/run-scoped.ts:187), the same lock convention the store uses; no-silent-shadowing semantics align with ho - Real-world viability: Solid beyond the happy path: lineage cycles, a 64-hop bound, idempotent re-record with a loud conflict on parent change under the mutation lock (src/run-scoped.ts:187-199), read-only external authorities verified against the requested parent at init (src/run-scoped.ts:121-130), audits run against one immutable loadChain snapshot (src/citation-resolution.ts:262-267), references are validated before
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Standard agent-knowledge lint gate cannot see broken cites; only the run-scoped API can [integration] ``
citesis now loaded (src/store.ts:124), schema-validated (src/schemas.ts:57), and turned into graph edges (src/graph.ts:26-30) in every knowledge base, butlintKnowledgeIndex(src/lint.ts:15-151) never checks it — while the directly analogouscontradictsgets a broken-contradiction error at src/lint.ts:220-233. Consequence: in a flat single-root KB (the documented index+lint workflow), a page withcites: [missing-id]passes lint silently and the graph silently drops the edge; the new bro
💰 Value Audit
🟡 Plain-index lint never checks cites; broken citations pass agent-knowledge lint silently outside run-scoped chains [maintenance] ``
lintKnowledgeIndex (src/lint.ts:15, wired to the CLI lint command at src/cli.ts:196) has no cites check, so
cites: [missing-id]in a non-run-scoped knowledge base produces no finding — while the analogous contradicts field gets broken-contradiction (src/lint.ts:224). The new 'broken-citation'/'ambiguous-citation' finding types (src/types.ts:163-164) are only emitted via lintCurrentRunCitations. Within one index ambiguity is impossible (duplicate-page-id is already an error, src/lint.ts:133), s
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — e800adc5
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-17T13:22:29Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.1s |
| Interrogation | 240.1s (2 bridge agents) |
| Total | 240.2s |
💰 Value — sound-with-nits
Adds first-class, lineage-aware page citations (cites frontmatter, origin-qualified resolution, chain audit, blocking lint) plus a pluggable run-lineage authority to agent-knowledge, and it is built squarely in the package's grain — only a small default-lint integration gap remains.
- What it does: Three concrete deltas. (1) Run ancestry becomes pluggable: createRunScopedStores() accepts a RunLineageAuthority (src/run-scoped.ts:37-40) so a product run manifest can own parentOf()/record(); the default file-backed authority (src/run-scoped.ts:167-208) is now idempotent, refuses parent changes as lineage conflicts, and lineage cycles/over-long chains fail loudly (src/run-scoped.ts:91-108). (2)
- Goals it achieves: Make this package the canonical owner of run-scoped citation semantics so a consumer (Discovery Lab per the PR body) can retire a parallel local implementation instead of extending a second one. Concretely better after merge: page-level 'builds on' relations are persisted, resolvable, and ambiguity-safe across run chains — the previous loadChain already retained duplicate ids (base a31bca5 src/run
- Assessment: Good on its merits. It parallels the existing page-id relation (contradicts) exactly — schema field, idListField store parse, lint vocabulary entry, self-reference check — which is the established grain (compare src/lint.ts:215-235 and src/schemas.ts:58). The three-valued non-coercing resolution, Object.freeze'd rows, and assert-pairs mirror existing patterns (claim-evidence, validate). The lineag
- Better / existing approach: No existing equivalent found — I grepped for prior citation/visibility/qualifier machinery (rg 'shadow|ambigu|qualif' across src, wikilinks.ts, kb-store.ts, changes.ts, freshness.ts, discovery.ts) and the only page-id relation machinery is contradicts, which this change correctly parallels rather than duplicates; run-chain resolution did not exist anywhere in the repo (loadChain was the only chain
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A coherent, fail-loud consolidation of run-scoped citation lineage into the canonical package: the cites load/graph path is live for every existing index consumer today, and the chain-resolution/lint APIs are credible library surface for the named external caller (Discovery Lab) via `RunLineageAut
- Integration: Partially wired now, rest ahead of its caller. Immediately live:
citesfrontmatter is parsed on the default page-load path (src/store.ts:124,137), validated by KnowledgePageSchema (src/schemas.ts:57), and projected into graph edges with reasoncitation(src/graph.ts:28-31) — so every existing index consumer (src/indexer.ts:22, src/kb-store.ts:166,278) surfaces citation edges with zero changes. - Fit with existing patterns: Fits the codebase grain on every axis.
citesmirrors the establishedcontradictsfrontmatter pattern (optional page-id list,idListFieldparse at src/store.ts:124-125, graph/lint consumers); graph citation edges reuse the exact wikilink edge mechanism (src/graph.ts:49-72,addDirectedEdge); lint findings extend the existingKnowledgeLintFindingvocabulary with two new types (src/types.ts: - Real-world viability: Built for the non-happy path. Batch resolution reads one chain snapshot so rows can't disagree (src/citation-resolution.ts:244-250); errors carry full structured diagnostics (KnowledgeCitationResolutionError embeds unresolved rows, KnowledgeCitationAuditError embeds the report); malformed references throw early (empty ids, invalid origins, src/citation-resolution.ts:279-298); unknown
prefix::id - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🎯 Usefulness Audit
🟡 Single-index lint has no broken-cites check, so the stock CLI lint cannot catch a typo'd citation [problem-fit] ``
The established pattern for page-id frontmatter references is in-index lint:
contradictstargets get 'Page contradicts unknown page id' findings in lintKnowledgeIndex (src/lint.ts:220-229).citesgets no equivalent, so a page citing a nonexistent id in a plain (non-run-scoped) store passesagent-knowledge lintsilently — the citation only fails if an application separately invokes auditCurrentRunCitations with a chain. Adding the parallel broken-citation check for in-indexcites(ambiguo
🟡 Singular resolveRunScopedCitation reloads the entire chain per call [ergonomics] ``
resolveRunScopedCitation calls stores.loadChain(runId) on every invocation (src/citation-resolution.ts:235-241), which re-reads every page of the current store, all ancestor stores, and the shared store each time. An app resolving citations one at a time in a loop pays O(chain × pages) per citation. The batch variants (resolveRunScopedCitations, assertRunScopedCitations) take one snapshot and the docs (docs/run-scoped-citations.md:48-56) steer toward them, so this is a footgun note, not a blocke
💰 Value Audit
🟡 cites is invisible to the default lint/validate path; only the separate run-scoped lint emits the new findings [against-grain] ``
store.ts parses cites for every knowledge base (src/store.ts:124-137) and graph.ts builds citation edges for any index (src/graph.ts:31-34), and KnowledgeLintFinding gained 'broken-citation'/'ambiguous-citation' (src/types.ts:163-164) — but lintKnowledgeIndex never emits them, so the CLI lint/validate/inspect gates (src/cli.ts:198, src/validate.ts:19, src/inspect.ts:33) cannot see a broken cites id in a plain single store. The contradicts precedent lints exactly this shape in-index (lintPageCont
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 27 | 67 | 42 | 27 |
| Confidence | 95 | 95 | 95 | 95 |
| Correctness | 27 | 67 | 42 | 27 |
| Security | 27 | 67 | 42 | 27 |
| Testing | 27 | 67 | 42 | 27 |
| Architecture | 27 | 67 | 42 | 27 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Origin-qualifier grammar silently mis-parses page ids containing '::' — src/citation-resolution.ts
The parser splits on the first '::' and, if the prefix is a valid origin ('here', 'shared', or 'inherited:'), returns {pageId, origin}. A page whose literal stable id is 'shared::foo' or 'inherited:r1::bar' cannot be cited as itself: the persisted value is silently reinterpreted as origin-qualified and resolution targets a different page (confirmed: parseKnowledgeCitationReference('shared::foo') => {pageId:'foo',origin:'shared'}). Page ids are only validated as z.string().min(1) (schemas.ts:49), and graph.ts:byId looks the raw string up verbatim, so graph edges and resolution disagree on the same persisted value. No escape or round-trip guard exists. This is silent mis-resolution of persisted data — the exact corruption class this PR's ambiguity machinery exists to prevent. Fix: rejec
🟠 MEDIUM Qualified citations defined here are silently dropped by the graph builder — src/citation-resolution.ts
This module defines the canonical qualified form 'here::' / 'inherited:::' / 'shared::' and citation-lint.ts:37 tells users to qualify ambiguous citations this way. But buildKnowledgeGraph (src/graph.ts:28-31) does byId.get(citedId) on the raw persisted string, so a qualified citation never matches any page id and the edge is silently skipped ('continue'). Impact: the exact remedy this module recommends produces no graph edge, so inDegree/outDegree and the graph-ranking stage of searchKnowledge (rankByGraph via reciprocal rank fusion, src/search.ts:74-75) lose the citation signal with no error. docs/run-scoped-citations.md:93 ('remains unresolved until it is qualified') implies qualification restores the edge; it does not. Fix: have graph.ts parse each cites
🟠 MEDIUM No unit tests for the changed graph edge semantics — src/graph.ts
The only tests exercising buildKnowledgeGraph are citation-storage.test.ts (2 scenarios: citation edge weight=1 + degree counts, and ambiguous duplicate-id skip). Untested: reason dedup when a wikilink and a citation and shared-source overlap land on the same edge key (lines 61, 88), self-citation skip (line 30), cite-to-missing-id silent drop, weight accumulation across reasons, and the addSourceOverlapEdges reason-dedup fix. These are exactly the behaviors this PR introduces/changes. Fix: add a graph.test.ts covering the merge/dedup matrix and skip cases.
🟠 MEDIUM Off-by-one: valid 64-ancestor lineage chain throws 'exceeds 64 ancestors' — src/run-scoped.ts
The loop runs at most MAX_LINEAGE_HOPS (64) parentOf queries, but a TERMINATING chain of exactly 64 ancestors requires 65 queries (64 parents plus the final null). Verified empirically: a Map authority with runs run-0..run-64 (run-64's parent null) makes lineage('run-64') throw
run lineage for 'run-64' exceeds 64 ancestorseven though the chain is valid durable state and the doc at line 58 says only ancestry BEYOND the bound is invalid. Impact: lineage() and loadChain() fail loudly on legitimate deep chains (e.g. 64 sequential resume/fork hops); the error message is also wrong (chain equals, not exceeds, 64). Pre-change code returned the truncated chain instead of thro
🟠 MEDIUM runId path traversal not blocked by assertRunId — src/run-scoped.ts
assertRunId (line 261-265) only rejects non-string/empty/whitespace runId. storePath defaults to join(options.root, runId, 'knowledge-base') (line 86-87) and init() passes it to initKnowledgeBase (line 128), which mkdir(root,{recursive:true}) via openSafeDirectoryTree. A runId like '../x' therefore escapes root and writes a knowledge store outside the intended tree, contradicting the module's 'physically isolated' promise.
🟡 LOW Blocking citation findings not wired into lint/validate pipeline — src/citation-lint.ts
docs/run-scoped-citations.md:91 promises 'lintCurrentRunCitations() converts missing, ambiguous, and self-citations into blocking package lint findings', but nothing in the package consumes it: cli.ts lint/validate commands call only lintKnowledgeIndex/validateKnowledgeIndex, and lint.ts has no citation branch. The severity 'error' rows produced here therefore never surface in
agent-knowledge lint(which exits 2 on error severity) oragent-knowledge validate --strict. The adapter only fires for an external caller that composes it with RunScopedStores (e.g. agent-runtime). If the runtime is the intended sole consumer this is fine, but the 'blocking lint' claim in the package docs is not yet true for the package's own workflow. Consider a thin integration test proving the finding type fl
🟡 LOW Malformed persisted citation aborts the whole citation lint — src/citation-lint.ts
A persisted citation of the form 'here::', 'shared::', or 'inherited:x::' (valid origin, empty pageId) passes idListField (store.ts:192-195 trims but keeps non-empty strings), then parseKnowledgeCitationReference(citation-resolution.ts:101-113) calls normalizeReference which throws TypeError 'pageId must be a non-empty string'. That throw propagates unguarded through auditCurrentRunCitations -> lintCurrentRunCitations, so one typo'd citation anywhere in the run's pages makes the entire citation audit reject instead of returning a per-page 'broken-citation' finding. Contrast: the equally-malformed 'garbage::id' is silently treated as a literal pageId and reported as missing, so malformed input is handled two inconsistent ways. Fix: normalize parse so empty pageId after a valid origin yields
🟡 LOW New blocking findings are not composed into the package's own lint/validate pipeline — src/citation-lint.ts
Grep of src/ shows no consumer of lintCurrentRunCitations or knowledgeCitationAuditFindings outside citation-lint.test.ts; cli.ts lint/validate call only lintKnowledgeIndex/validateKnowledgeIndex, which have no citation-audit branch. docs/run-scoped-citations.md:91 says lintCurrentRunCitations 'converts missing, ambiguous, and self-citations into blocking package lint findings' — true in vocabulary (severity 'error' is blocking wherever findings flow, validate.ts:43), but the package's own gates never produce these rows; only an external caller that composes RunScopedStores (e.g. agent-runtime) will see them. The CLI has no run-scoped context so omission is arguably by design; flagging so the global verifier can weigh whether the doc claim needs softening or an integration test proving the
🟡 LOW Qualification hint omits which runId to use — src/citation-lint.ts
The ambiguous-citation message instructs the user to 'qualify it as ... inherited:::' but the runId is never in scope for the message; a user must open metadata.candidates to read the ancestor runId out of origin strings like 'inherited:abc123'. The hint reads as if the current run's own runId qualifies, which would resolve to missing. Consider emitting the actual candidate origins (e.g. 'inherited:abc123::') in the message or leaving an explicit pointer to metadata.candidates.
🟡 LOW Self-citation reported under 'broken-citation' type — src/citation-lint.ts
When issue.kind === 'self', the finding is emitted with type 'broken-citation' (severity error) and a distinct message ('cites itself through ...'). The KnowledgeLintFinding.type union has no 'self-citation' member, so this is a deliberate reuse. Consequence: a consumer filtering findings by type === 'broken-citation' will group self-citations with genuinely unresolvable citations; the only discriminator is the message string, which is not machine-stable. Impact is low and non-blocking (both are correctly error-severity and blocking). Suggested fix, if the union is ever extended in types.ts: add a 'self-citation' type and emit it here; otherwise document that self-citations surface as broken-citation.
🟡 LOW The only async export has no repo test coverage — src/citation-lint.ts
citation-lint.test.ts covers only the pure knowledgeCitationAuditFindings mapper (one test hitting missing/ambiguous/self, ordering, message, metadata). lintCurrentRunCitations — the function real callers invoke, which composes stores.loadChain(runId) with sourceOrigins ['here'] filtering — is never called by any test. I verified it end-to-end manually in this shot (correct broken-citation finding against a real on-disk store), but a regression in the store composition (e.g. wrong argument order, origin filter, shared-root handling) would not be caught by CI. Add one fs-backed test using createRunScopedStores covering findings for a here-authored page citing an inherited/shared-duplicated id.
🟡 LOW lintCurrentRunCitations crashes on a schema-valid malformed citation instead of returning a finding — src/citation-lint.ts
Empirically confirmed with a tsx probe against real stores: a page whose frontmatter cites contains 'here::' (passes KnowledgePageSchema's z.string().min(1) and store.ts idListField's trim-and-keep) reaches parseKnowledgeCitationReference, which splits origin 'here' with empty pageId, and normalizeReference throws TypeError 'knowledge citation pageId must be a non-empty string' (citation-resolution.ts:283-285). The throw propagates unguarded through auditKnowledgeCitations' loop -> auditCurrentRunCitations -> this function, so one typo'd citation anywhere in the run's pages rejects the entire lint call and hides every other finding. Inconsistent with the equally malformed '::' or 'garbage::id', which surface as normal broken-citation findings. A crash is defensible under fail-loud doctrine
🟡 LOW All five exported run-scoped async wrappers are untested — src/citation-resolution.ts
resolveRunScopedCitation, resolveRunScopedCitations, assertRunScopedCitationsResolved, auditCurrentRunCitations, and assertCurrentRunCitationsResolved (lines 235-277) have no direct test anywhere: citation-resolution.test.ts exercises only the pure core; citation-lint.test.ts calls auditKnowledgeCitations directly and never lintCurrentRunCitations; run-scoped.test.ts covers loadChain but none of these wrappers. These are the functions real callers (agent runtimes) will invoke, including the loadChain-argument-ordering (stores, runId) contract and the single-snapshot-per-batch guarantee stated in the doc comment. Fix: add one fs-backed test per wrapper (or
🟡 LOW Audit aborts on one malformed qualified citation instead of reporting it — src/citation-resolution.ts
parseKnowledgeCitationReference('here::') and ('inherited:r1::') throw TypeError ('pageId must be a non-empty string') because the empty pageId reaches normalizeReference. The call at line 192 is inside the audit loop with no try/catch, so a single malformed persisted citation aborts the entire audit — lintCurrentRunCitations then throws instead of returning findings, hiding every other issue across the run. Inconsistent with the equally malformed '::', which does not throw and surfaces as a 'missing' issue. All of these pass the KnowledgePageSchema 'cites' check (z.string().min(1)). Fix: catch parse errors in the loop and emit an issue kind (e.g. 'malformed'),
🟡 LOW Reserved origin prefixes make some page ids unrepresentable as unqualified citations — src/citation-resolution.ts
The parser splits on the first '::' and treats a leading 'here', 'shared', or 'inherited:' as an origin qualifier (lines 105-112). Consequently a page whose id literally begins with one of these reserved prefixes and contains '::' (e.g. id 'here::foo') can never be cited unqualified: 'here::foo' is parsed as origin 'here' + pageId 'foo'. In practice ids.ts produces slugified ids (no '::') and stableId 'prefix_hash', so this is unreachable with the current id generator, but the grammar silently reserves a namespace with no runtime guard or documentation of the restriction on KnowledgeId. Low severity; consider documenting the '::'-free id invariant or re
🟡 LOW Schema-valid malformed citation 'here::' crashes the whole audit batch — src/citation-resolution.ts
A persisted cites entry 'here::' passes the store schema (z.array(z.string().min(1)), schemas.ts:57) and idListField (store.ts:192-195). During audit, parseKnowledgeCitationReference routes it to normalizeReference with an empty pageId, which throws a bare TypeError ('pageId must be a non-empty string') from inside the audit loop (line 192), aborting the entire lint/audit run — including reporting of the other pages' genuine missing/ambiguous findings — rather than emitting a broken-citation finding for that one page. Fail-loud is repo doctrine, but in a lint context one malformed frontmatter string suppresses all other findings. Fix: catch the parse failure in
🟡 LOW parse/format round-trip is not injective for ids or runIds containing '::' — src/citation-resolution.ts
parseKnowledgeCitationReference splits on the FIRST '::' (indexOf, line 105). KnowledgeId is a bare string (types.ts:1), the frontmatter schema only enforces min length 1 (schemas.ts:57), and assertRunId (run-scoped.ts:261) only requires a non-empty string, so '::' is legal in both page ids and run ids. formatKnowledgeCitationReference({pageId:'p', origin:'inherited:x::y'}) emits 'inherited:x::y::p', which reparses as {origin:'inherited:x', pageId:'y::p'} — a different reference. Similarly a page whose literal id is 'shared::foo' cannot be cited unqualified: the string reparses as a qualified reference to page 'foo' in the shared origin. The test at citation-re
🟡 LOW Asymmetric ambiguity handling between wikilinks and citations — src/graph.ts
Citations to a duplicated page ID are refused (targets.length !== 1) while wikilinks to the same duplicated ID still resolve last-wins via bySlug. Wikilink behavior is pre-existing and unchanged by this diff, and refusing ambiguous citations is the deliberately conservative choice (the test at src/citation-storage.test.ts:66 names this invariant), so this is a semantic note, not a regression. Consider a follow-up that flags duplicate IDs at lint time so both paths converge.
🟡 LOW Duplicate cites entries inflate edge weight for programmatic callers — src/graph.ts
The filesystem loader dedupes cites (store.ts idListField uses a Set), but buildKnowledgeGraph accepts arbitrary KnowledgePage[] from programmatic callers (kb-store, research loops). A page with cites: ['b','b'] yields weight 2 for one logical citation. This is consistent with wikilink semantics where each repeated mention also increments weight, so it is defensible; if unintended, dedupe citedId within the loop or document mention-counting semantics. Verified current behavior directly: weight=2, reasons deduped to ['citation'].
🟡 LOW Duplicate-page-id resolution is inconsistent between wikilinks and citations — src/graph.ts
bySlug is last-wins (lines 9-11), so a wikilink to a duplicated id still creates an edge to the last page and increments degrees; byId returns an array and the citation loop refuses the cite when targets.length !== 1. Meanwhile nodes are emitted once per page (lines 37-45), so a duplicated id yields two nodes with merged per-id degrees and merged edge weights. The graph is internally inconsistent in this state; it is guarded by the duplicate-page-id lint error, but buildKnowledgeGraph itself has no guard. Acceptable as conservative-by-design; document it
🟡 LOW No repo test for wikilink+citation edge merge and degree counting — src/graph.ts
citation-storage.test.ts covers the happy path and the ambiguity refusal, but not the merge case where one page both wikilinks and cites another target: the edge should collapse to a single entry with reasons ['wikilink','citation'], weight 2, and outDegree 2. I verified this behavior passes against the compiled head code (17/17 assertions), but it guards addDirectedEdge's reason-dedupe branch which is otherwise untested in the repo suite. Add one test to lock it in.
🟡 LOW Qualified citation references (origin::pageId) silently dropped from the graph — src/graph.ts
Line 29 does
byId.get(citedId)wherebyIdis keyed by the unqualifiedpage.id(line 8).KnowledgePage.citesis an unconstrainedstring[](types.ts:99) and store.tsidListFieldpasses frontmatter values verbatim, so a qualified cite likehere::prior-result,inherited:<run>::x, orshared::x— a documented first-class form (run-scoped-citations.md:23-30, parseKnowledgeCitationReference in citation-resolution.ts:100) — never matches anybyIdkey, sotargets.length === 0and the citation edge is silently omitted. Meanwhile `auditKnowledgeCitation
🟡 LOW Shared-source weight still double-counts for colliding ids while reasons are deduped — src/graph.ts
With two pages sharing one id, pairs (A1,B) and (A2,B) both key to 'a->b'; the new code dedupes the 'shared-source' reason (line 88) but still executes edge.weight += overlap*0.5 twice (line 87), so weight reflects both pages' overlap while reasons show one entry. The old code duplicated the reason; this PR fixes reasons but leaves the weight disagreement. Error-state only (duplicate-page-id lint) but the weight/reason asymmetry is now more visible.
🟡 LOW Internal authority detects parent conflict after filesystem mutation — src/run-scoped.ts
The 'refuse lineage drift before filesystem mutation' guarantee is implemented only for the read-only authority branch (line 119-126). For the default internal authority (authority.record present), init() runs initKnowledgeBase (line 128) BEFORE authority.record detects the parent conflict (line 197-200). In the normal flow initKnowledgeBase is idempotent so a re-init creates no new files, but if lineage.json exists with
🟡 LOW Object.freeze on the init record is inert on the default authority — src/run-scoped.ts
init() Object.freeze's the record literal (line 131-136) before passing it to authority.record, but the default internal authority's record() calls validateLineageRecord (line 192), which returns a freshly constructed, non-frozen object; the frozen original is discarded and only the new object is persisted. The freeze only has effect for a user-supplied external authority. Harmless, but misleading about the mutability guarantee. Consider freezing in validateLineageRecord's return, or drop the call-site freeze.
🟡 LOW Off-by-one in MAX_LINEAGE_HOPS boundary — src/run-scoped.ts
The loop runs
for hop < 64, pushing a parent each iteration and throwing at line 107 once 64 iterations have found non-null parents. So a chain of exactly 64 ancestors is rejected with 'exceeds 64 ancestors', while the constant's stated intent ('Ancestry beyond this bound is invalid') allows exactly 64. Max accepted is 63. The test ('refuses an ancestry chain beyond the declared safety bound') only exercises 65 ancestors (66 runs), so the exact-64 boundary is untested and the error message is slightly misleading. Trivial in practice; either accept the strictness deliberately or document that <=63 is the actual limit.
🟡 LOW Off-by-one: exactly 64 ancestors throws instead of terminating — src/run-scoped.ts
resolveLineage runs MAX_LINEAGE_HOPS (64) iterations, each pushing one ancestor, and only returns when parentOf yields null inside the loop. A terminating chain of exactly 64 ancestors (the 64th has null parent) exhausts the loop at hop=64 and throws 'exceeds 64 ancestors' — the final null read never happens. The comment 'Ancestry beyond this bound is invalid' implies 64 should be allowed; only 65+ should throw. The test (run-scoped.test.ts:125) uses 66 entries (65 ancestors), so it never exercises the boundary. Fix: move the null check into the loop post-condition (e.g. loop MAX_LINEAGE_HOPS+1 times, or check after loop whether the last parent is non-null before throwing).
🟡 LOW Writable authority rejects lineage only after store scaffold is created — src/run-scoped.ts
For a writable authority (
authority.recordpresent, including the default file authority),initrunsinitKnowledgeBase(storePath(runId))at line 128 BEFOREauthority.record(...)at line 130. Ifrecordthrows (lineage conflict, lines 196-203), the store scaffold has already been created. With the default file authority this is harmless — a conflicting lineage.json implies the store already existed — but a custom exte
🟡 LOW Writable external authority: init scaffolds the store before record() can reject — src/run-scoped.ts
For an authority WITH a record() method, init calls initKnowledgeBase (creating knowledge/, raw/sources/, .agent-knowledge/ and scaffold files) before authority.record; if the product's record() throws (e.g. its manifest rejects the run), a partial knowledge store is left on disk. The no-partial-store guarantee in the comment at lines 116-118 is scoped to read-only authorities, so this is a documented asymmetry rather than a contract breach, but nothing warns writable-authority implementers. Also record() is invoked on every reopen with a fresh createdAt and the RunLineageAuthority interface does not state that record must be idempotent/conflict-checking (the inter
🟡 LOW assertRunId accepts path segments; default runStorePath escapes options.root — src/run-scoped.ts
assertRunId (line 261) only checks non-empty string, so runId='../escape' passes and the default storePath join(options.root, runId, 'knowledge-base') resolves outside options.root (verified: join('/root','../escape','knowledge-base') -> '/escape/knowledge-base'); initKnowledgeBase and readRecord then read/write at the escaped path. Not a regression (pre-change code had no runId validation at all) and runStorePath is caller-supplied anyway, but the new validation boundary reads as containment it does not provide. Fix: reject runIds containing '/', '\', '.', '..', or NUL in assertRunId (e.g. /^[A-Za-z0-9][A-Za-z0-9._-]*$/ minus dot-only segments), or resolve the default
🟡 LOW assertRunId does not prevent path escape via runId — src/run-scoped.ts
assertRunIdonly requires a non-empty string.runId(and each parent runId returned by the authority, line 99) is interpolated intojoin(options.root, runId, 'knowledge-base')at line 87, so a value like'../victim'anchors the store outside the configured root, and all subsequent durable-fs operations (init, lineage read, loadChain) then operate outside root. Pre-existing (the old code did the same join without validation), but this PR introducesassertRunIdas the new validation seam and is the right place to reject path separators and..
🟡 LOW Schema accepts whitespace-only cites entries that the resolver rejects with a TypeError — src/schemas.ts
z.string().min(1)passes' '(length >= 1), butparseKnowledgeCitationReference(citation-resolution.ts:101) throws TypeError('persisted knowledge citation must be a non-empty string') for any whitespace-only string. InauditKnowledgeCitations(citation-resolution.ts:190-192) the throw is uncaught, so a whitespace-only cite escalates to a hard error instead of a lint finding. The frontmatter path cannot produce this (idListFieldtrims and filters empties in store.ts:192-195), but a programmaticputPagecaller could persist one. Matches the pre-existingcontradictsfield exactly, so this is a consistency nit, not a regression. Fix: usez.string().trim().min(1)or a refine that mirrors the resolver's check.
🟡 LOW cites element schema accepts whitespace-only ids that the loader would never produce — src/schemas.ts
z.string().min(1) accepts a whitespace-only entry like " ". idListField (store.ts:194) trims and filters empties, so loader-produced pages cannot contain one, but a hand-edited or externally written index.json parsed via KnowledgeIndexSchema would persist the value instead of rejecting it; it then surfaces only later as an unresolved-citation lint row rather than a loud parse failure. KnowledgePageInvalidationSchema.reason (schemas.ts:41) already uses the stricter z.string().trim().min(1) idiom. Impact is minor and the line is consistent with the pre-existing contradicts field (schemas.ts:58). Fix: use z.string().trim().min(1) for both cites and contradicts in a follow-up, accepting the one-time re-validation of existing indexes.
🟡 LOW Non-string cites frontmatter entries are silently dropped — src/store.ts
idListField -> arrayField filters non-strings without diagnostics. Verified live: a page with
cites: [42, true]loads with the cites field entirely absent ('junk entries: undefined, field present: false'), so a typo'd YAML cite evaporates and neither lint nor any later stage can report it — tension with the repo's fail-loud doctrine. This is the pre-existing grain shared with contradicts/sources/tags, not a regression this diff introduces, and the normalized page.cites is derived while the raw value survives on page.frontmatter. Fix (optional, follow-up): a lint check on the raw frontmatter values rather than changing the loader.
🟡 LOW Qualified citation forms preserved by the store never project graph edges — src/store.ts
The store correctly passes raw strings through (verified live:
here::prior-result-> cites: ["here::prior-result"]), which is right because origin-qualified resolution is run-scoped and belongs to citation-resolution.ts. But buildKnowledgeGraph does a raw byId.get on the persisted string (graph.ts:28-31), so the qualified form this PR's lint messages recommend produces no citation edge and no search-ranking signal (verified: 'qualified edge projected: false'). The store-side behavior is correct; the gap is in graph.ts (covered by another shot). Noting it here because this diff is where the raw string enters the page object. No store change needed; if graph.ts later parses qualified refs, this passthrough already carries everything required.
🟡 LOW Scalar cites form untested through the frontmatter loader — src/store.ts
idListFieldintentionally supports a scalar string (cites: single-target→['single-target']), verified working by manual tsx harness, but citation-storage.test.ts only exercises the YAML list form. A regression in the scalar branch would ship silently since no test asserts it. Also no test covers qualified citations (here::x,inherited:run::x,shared::x) loaded from frontmatter and then resolved by auditKnowledgeCitations; qualified refs are only tested at the object level in citation-resolution.test.ts. Fix: add two cases to citation-storage.test.ts covering scalar cites and a qualified-ref cite.
🟡 LOW Extending KnowledgeLintFinding.type is a minor semver-visible break — src/types.ts
Adding 'broken-citation' and 'ambiguous-citation' to the literal union breaks downstream exhaustive switches on the finding type. Both new members are emitted at severity 'error' by src/citation-lint.ts, so any external fail-on-error policy (e.g., AGENTS.md treats missing-source as blocking) will also hard-block on the new findings. Intended behavior for this PR; just confirm the package version bump reflects the union widening.
🟡 LOW cites doc comment omits the origin-qualified citation grammar — src/types.ts
The comment says 'Stable page ids this page explicitly builds on', but parseKnowledgeCitationReference (src/citation-resolution.ts:95-113) and the lint message at src/citation-lint.ts:37 define persisted entries as a grammar: bare
pageIdORhere::<pageId>/shared::<pageId>/inherited:<runId>::<pageId>.KnowledgeIdis an alias ofstring, so the type accepts it, but a consumer that treats entries as raw page ids will silently miss every qualified citation — graph.ts:29 (byId.get(citedId)) already does exactly that, so qualified citations produce no graph edge. Fix in this file: state the qualified form in the comment (or type it as a dedicated citation-reference string). The graph-side behavior belongs to another shot.
tangletools · 2026-08-17T14:04:06Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 38 non-blocking findings — e800adc5
Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 13 changed files. Global verifier still owns final merge decision.
Summary comment for this run: full summary
tangletools · 2026-08-17T14:04:06Z
The loop ran one parentOf query per allowed ancestor, so a valid chain of exactly 64 ancestors could not reach its terminating null query and threw. One extra query proves the chain ends. Run ids that contain path separators or dot segments are now rejected before they can escape the store root.
buildKnowledgeGraph looked the persisted string up verbatim, so the qualified forms the ambiguity lint recommends produced no edge. The graph now resolves the parsed page id and keeps a malformed value as a literal so an index build never fails on stored data.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 3c642d5e
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-17T18:27:21Z
2 participants
Mission
Make
agent-knowledgethe canonical owner of lineage-aware page citations so Discovery Lab can retire its local lineage/search/citation machinery instead of extending a second implementation.What this PR adds
External lineage authority
createRunScopedStores()now accepts aRunLineageAuthority.A product whose run manifest already owns ancestry can supply
parentOf(runId)and optionallyrecord(record). The default file-backed authority remains available and is now idempotent: reopening the same run/parent succeeds, while changing the parent fails as a lineage conflict.Lineage cycles and chains beyond the declared 64-hop safety bound fail loudly.
First-class page citations
KnowledgePageandKnowledgePageSchemanow retaincitesfrontmatter. Exact in-index citations become graph edges with reasoncitation. Duplicate target ids do not create a guessed edge.Missing / resolved / ambiguous semantics
New public contracts include:
KnowledgeCitationReferenceKnowledgeCitationResolutionKnowledgeCitationResolutionErrorresolveKnowledgeCitation(s)resolveRunScopedCitation(s)assertRunScopedCitationsResolved()An unqualified page id resolves only when exactly one visible page owns it. Intentional reuse can be qualified as:
here::<pageId>inherited:<runId>::<pageId>shared::<pageId>No nearest/newest/shared fallback exists.
Persisted-chain audit and lint
auditKnowledgeCitations()auditCurrentRunCitations()assertCurrentRunCitationsResolved()knowledgeCitationAuditFindings()lintCurrentRunCitations()These inspect current-run writes against one immutable current/ancestor/shared visibility snapshot and retain missing, ambiguous, and self-citations as blocking findings.
Migration guidance
docs/run-scoped-citations.mddefines the dual-read and deletion procedure for applications with an existing local store implementation.Why this belongs here
Knowledge owns persistent sourced pages, run-scoped stores, lineage-chain reads, graph relations, and claim evidence. Eval owns readiness and experimental judgment; Runtime owns agent execution. Discovery Lab should provide its manifest adapter and experiments, not a second durable knowledge substrate.
Compatibility
citesretain their previous shape.Tests
The added tests cover:
Related: tangle-network/discovery#48, tangle-network/discovery#49