fix(knowledge): preserve retrieval identity across origins and expose historical search - #214
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — ff5dd09b
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-09-20T11:40:19Z
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff5dd09b0a
ℹ️ 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".
| const qualified = (entry: OriginatedPage) => | ||
| formatKnowledgeCitationReference({ pageId: entry.page.id, origin: entry.origin }) |
There was a problem hiding this comment.
Escape delimiters in origin-qualified ranking IDs
When a lineage contains an ancestor ID with ::—which assertRunId currently permits—this encoding is not injective: { origin: 'inherited:a::b', pageId: 'c' } and { origin: 'inherited:a', pageId: 'b::c' } both become inherited:a::b::c. The resulting clones are treated as duplicate IDs by rankByGraph, suppressing valid graph edges, and any emitted qualified handle for the first page is parsed back as the wrong origin/page pair. Either reject the citation delimiter in run IDs or use an escaped/structured identity for ranking and serialization.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 226.1s (2 bridge agents) |
| Total | 226.1s |
💰 Value — sound
Fixes a real cross-origin identity bug in retrieval (same-id pages fusing/substituting through string-keyed RRF) by fusing on document identity and minting resolver-qualified citation handles at the brief boundary, plus surfaces already-existing filter controls on the tool — coherent, well-tested, a
- What it does: Three behavior deltas. (1) src/search.ts:96-101,126-134 replaces string-id RRF fusion with a generic
fuseRanks<T>keyed on page objects, so two distinct pages sharing one id each keep their own fused score and the engine returns the exact page that ranked (the old code'sbyIdmap,new Map(matched.map((page) => [page.id, page]))at HEAD~1, was last-write-wins and could return the inherited Ba - Goals it achieves: From the change itself: make retrieval identity match the identity semantics the codebase already established for read/write. Citation resolution (src/citation-resolution.ts, built in #152 'resolve run-scoped knowledge without silent shadowing') deliberately retains duplicate ids per origin and offers qualified handles — but search still fused by bare id and the brief returned bare
page.idas `c - Assessment: Good on its merits. It repairs the source rather than patching callers, and it reuses the established machinery —
parseKnowledgeCitationReference/resolveKnowledgeCitation/formatKnowledgeCitationReference/assertKnowledgeCitationsResolved— instead of inventing a parallel qualification scheme (verified by grepping all usages: graph.ts, invalidation-propagation.ts, promotion.ts, write-intake. - Better / existing approach: none — this is the right approach. I searched for an existing scoped-search or qualification view to reuse:
searchKnowledgePages/buildKnowledgeBriefare consumed only by the tool layer, the index wrapper, and tests; no prior implementation ranked with origin-qualified ids. The obvious alternative — threadingPageOriginthroughsearchKnowledgePagesand returning qualified handles natively — - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1789905526147,"sessionID":"ses_f415123daffeSlSxit1J4P4fEf","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_17713d20"}}}
🎯 Usefulness — sound
Fixes a real cross-origin identity bug in the one shared ranking primitive and reuses the existing citation grammar end-to-end, so every existing consumer (public search, briefs, tools, receipts, write/audit) gets correct behavior with no call-site changes.
- Integration: The identity fix lands in searchKnowledgePages (src/search.ts:74), the single primitive both public searchKnowledge (src/search.ts:65) and buildKnowledgeBrief (src/knowledge-brief.ts:127) already call, so it reaches all existing consumers without wiring changes. The tool-level exposure (excludeInvalidated/tags/kinds at src/knowledge-tools.ts:60-69,107-115) sits on knowledge_search, the agent-facin
- Fit with existing patterns: Fits the grain precisely: instead of inventing a new origin-qualification scheme, the brief synthesizes qualified views using the resolver's existing persisted-citation grammar (here::/shared::/inherited:::, src/citation-resolution.ts:94-121), keeping unscoped search free of run-scoped concepts while both share one fuseRanks implementation (src/search.ts:125-134). The new tool options deleg
- Real-world viability: Error and edge paths are handled deliberately: same-origin id collisions fail closed with an ambiguous error instead of silently choosing (knowledge-brief.ts:142-144, tested in src/search-origins.test.ts 'refuses a genuinely ambiguous same-origin citation'); malformed outLinks are dropped as ranking edges but the page stays retrievable and auditable (knowledge-brief.ts:111-122); unscoped graph ran
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Run-scoped view is rebuilt per query and cannot reuse a lexical index [maintenance] ``
buildKnowledgeBrief (src/knowledge-brief.ts:107-126) clones every visible page per call, and the clones are fresh objects, so a caller searching one chain repeatedly can never supply options.lexicalIndex (assertLexicalIndexMatches in src/search.ts:156-167 checks object identity). This matches the pre-PR cost profile (the index was rebuilt per call anyway) and is not a regression, but exposing the transformed page view (or accepting a chain-keyed index cache) would be the natural follow-up if bri
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.
|
Rechecked against current main |
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 8 non-blocking findings — ff5dd09b
Full multi-shot audit completed 5/5 planned shots over 8 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-09-20T12:21:09Z · immutable trace
|
Addressed the delimiter-collision review (discussion_r4056859286) at the shared citation formatter/parser, rather than restricting legal run/page names. |
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 43cf11b1
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-09-20T15:33:47Z
Preserve #215 peer range, package/optimizer qualification, and 17.0.3 history. Keep candidate 17.1.0 and retrieval implementations unchanged. Remove temporary reconciliation workflow.
Origin-safe retrieval — reconciled with current main and green
Current head:
a89c128a6a5502cb9800abcd9764cbc3b9859a96Base:
f1072251361ef78aee7d985d32516524bec8cb9a(merged #215)Tree:
de968679afd5d1c009e6541a8694795b577e8316Prepared as 17.1.0, not published.
Repair the source, not agents' naming conventions
The same page ID may legitimately exist in current, inherited and shared knowledge. Search previously fused hits by bare
page.id, then reconstructed through a last-write-wins map: a query matching the current Quantum page could return an unrelated inherited Banana page. Origin assignment also collapsed when the same object appeared at several origins.excludeInvalidated,tags, andkindscontrols throughknowledge_search. Agents can investigate failed/refuted work without changing the default or bypassing access boundaries.Review follow-through: delimiter collisions
inherited:a::b+ pagecandinherited:a+ pageb::ccould produce the same legacy handle. The shared formatter/parser now uses a versioned URI-encoded[origin, pageId]tuple only when the legacy spelling cannot round-trip. This repairs the identity owner rather than banning legal names in Lab.Normal handles remain byte-compatible. Legacy percent sequences remain literal.
knowledge-ref:v1:is reserved for the encoded representation; literal names beginning with that prefix are addressable using the formatter or structured-reference API. Historical ambiguous strings are not guessed or silently rewritten. Search uses the same formatter and supports literal prefix-named pages.Five regression tests exercise 72 distinct origin/page combinations, search-to-resolution identity, legacy compatibility, reserved-prefix names and malformed encoded handles. The public
citationIdscontract is used rather than an untyped extra field on retrieval results.Current-main reconciliation
Main advanced through #215 during final verification. This branch now includes it with history preserved, no force-push:
>=0.182.0 <0.184.0; development stays 0.182.0.43cf11b.The temporary reconciliation workflow removed itself. The final diff against current main has 11 files, 525 additions and 52 deletions; no lockfile or production-workflow changes beyond current main.
Verification — current head
Normal CI run 35522955686 is fully green on current head
a89c128a6a5502cb9800abcd9764cbc3b9859a96(PR merge test commit1e216a2a662469d1092a9206aa003be10c94f9a4).106110104153: 924 tests passed, 7 existing optional skips, zero failures; source/contract typechecks, lint, build, version, skill, path-containment and 997-export API checks pass.106110104264: GEPA and SkillOpt packed integrations pass with both supported Eval minors.The test log records one existing external-source probe returning early because its host blocked the request; neither the passed suite nor these package integrations imply that all external services were exercised.
Earlier retained verification: the initial origin repair added 12 regressions and a seven-failure negative control against unmodified main; delimiter follow-through added five tests and an exact-source formatting negative control preserving all 72 pairs. The original final head passed 916 tests before main added eight compatibility tests. The current 924-test CI supersedes those earlier counts and includes the merged compatibility work.
Rollout boundary
This expands reliable retrieval and access to failed-work history without prescribing agent behavior. Existing consumers and frozen method bundles still require publication and compatible adoption. Source changes do not retroactively alter retained experiments. Discovery Lab #883 contains its own admission/campaign simplifications; it does not copy this fix or pretend its Knowledge 17.0.2 pin contains it.
No main merge, package publication, deployment, paid inference or interruption of running pursuits was performed. This remains the single Knowledge repair PR.