Skip to content

fix(knowledge): preserve retrieval identity across origins and expose historical search - #214

Merged
drewstone merged 10 commits into
mainfrom
fix/origin-aware-retrieval
Sep 20, 2026
Merged

drewstone merged 10 commits into
mainfrom
fix/origin-aware-retrieval

Conversation

@drewstone

@drewstone drewstone commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Origin-safe retrieval — reconciled with current main and green

Current head: a89c128a6a5502cb9800abcd9764cbc3b9859a96
Base: f1072251361ef78aee7d985d32516524bec8cb9a (merged #215)
Tree: de968679afd5d1c009e6541a8694795b577e8316
Prepared 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.

  • Preserve exact document identity through the existing ranking implementation; the public string-key reciprocal-rank-fusion helper stays API-compatible and delegates to the same primitive.
  • Build run-scoped ranking views using the existing citation parser/resolver. Qualify ambiguous handles against the entire visible chain, including filtered-out duplicates. Unique ordinary IDs keep their old handle. Returned pages and raw evidence remain unchanged.
  • Resolve graph links through the same citation semantics; ambiguous bare IDs cannot manufacture a relationship. Malformed outgoing links do not prevent retrieving the page and remain available for citation audit.
  • Expose existing optional excludeInvalidated, tags, and kinds controls through knowledge_search. Agents can investigate failed/refuted work without changing the default or bypassing access boundaries.
  • Preserve existing record/intake fixes. No page/store migration, Runtime dependency, new search engine, ontology or role policy.

Review follow-through: delimiter collisions

inherited:a::b + page c and inherited:a + page b::c could 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 citationIds contract 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:

  • Retain Eval peer compatibility >=0.182.0 <0.184.0; development stays 0.182.0.
  • Preserve main's packed-package and official GEPA/SkillOpt verification against both Eval 0.182 and 0.183.
  • Keep the 17.0.3 changelog entry below the 17.1.0 candidate and align the README installation version.
  • Assert all retrieval source and test files plus citation documentation remain byte-identical to previously verified head 43cf11b.
  • Assert all six incoming qualification scripts/workflows remain byte-identical to main.

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 commit 1e216a2a662469d1092a9206aa003be10c94f9a4).

  • Core job 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.
  • Packed ESM consumers pass separately with Eval 0.182.0 and 0.183.0, including clean installation, portable declarations, imports, CLI version and re-pack.
  • Official-optimizers job 106110104264: GEPA and SkillOpt packed integrations pass with both supported Eval minors.
  • Node 22.23.2, pnpm 10.34.5, frozen installation.

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-20T11:44:06.286663Z ff5dd09 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/knowledge-brief.ts
Comment on lines +104 to +105
const qualified = (entry: OriginatedPage) =>
formatKnowledgeCitationReference({ pageId: entry.page.id, origin: entry.origin })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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's byId map, 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.id as `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/buildKnowledgeBrief are consumed only by the tool layer, the index wrapper, and tests; no prior implementation ranked with origin-qualified ids. The obvious alternative — threading PageOrigin through searchKnowledgePages and 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.

value-audit · 20260920T120231Z

Copy link
Copy Markdown
Contributor Author

Rechecked against current main ee172b1ed30d507652f1132eda382f46aadc0f95; this remains the single Knowledge repair PR and is conflict-free. Normal CI run 35508480316 is now successful on head ff5dd09b0a1e98c6b2fe6004c5f8d63c3995af87: core lint/typecheck/tests/build/packed lifecycle and the separate official GEPA/SkillOpt packed-consumer integration both passed. This closes the earlier unverified CI/packed-consumer portion of the PR description; it does not claim publication or consumer adoption.

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — ff5dd09b

Review health 100/100 · Reviewer score 71/100 · Confidence 85/100 · 8 findings (8 low)

glm: Correctness 71 · Security 71 · Testing 71 · Architecture 71

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 8 changed files. Global verifier still owns final merge decision.

🟡 LOW Comment 'retain the page unchanged' is easy to misread as retaining the link — src/knowledge-brief.ts

The comment says a malformed link means 'retain the page unchanged', but the flatMap returns [] for that link on the synthetic ranking page, dropping it as a graph edge; what is retained is the original page object (verified: brief results carry outLinks ['inherited:::state'] unchanged) so citation audit can still diagnose it. Behavior is correct; rewording to 'retain the page, drop only the ranking edge' would prevent a future reader from 'fixing' this into retaining a bogus edge or restoring the raw link on the synthetic page.

🟡 LOW One same-origin id collision makes the entire brief throw, dropping retrieval for all pages — src/knowledge-brief.ts

assertKnowledgeCitationsResolved(candidates, [reference]) throws KnowledgeCitationResolutionError when a ranked page's raw id is duplicated within a single origin, so no brief is produced at all — unambiguous pages in the same chain also lose retrieval. This is deliberate fail-closed design (test 'refuses a genuinely ambiguous same-origin citation', search-origins.test.ts:117) and the duplicate-page-id lint plus write-intake rules own preventing that state, so it is defense-in-depth rather than a bug. If callers need degraded-mode retrieval, a future option could skip only the ambiguous page, but shipping as-is is safe: no citation handle in the output can be misresolved.

🟡 LOW Per-hit parseKnowledgeCitationReference on the raw id can throw mid-ranking — src/knowledge-brief.ts

parseKnowledgeCitationReference(entry.page.id) throws TypeError if a page id is an empty/whitespace string, aborting the whole brief after ranking has run. Page ids are validated at load/write time upstream, so this is unreachable in normal operation and fails closed when hit, but moving the uniqueness/parse decision into the byId construction pass would surface it before any ranking work. Cosmetic robustness note only.

🟡 LOW kinds passthrough lacks a negative control at the tool boundary — src/knowledge-tools.test.ts

The passing case at lines 253-262 sends tags:['history'] AND kinds:['finding'] together for a fixture whose kind is 'finding'. If the kinds field were dropped from searchInput or the option spread in src/knowledge-tools.ts:113-114, zod would strip the unknown key and the test would still pass, because tags alone carries the filtering proof. tags has an excluding case (line 263-266) but kinds does not. Add one case such as invoke({ question: 'quantum', excludeInvalidated: false, tags: ['history'], kinds: ['note'] }) expec

🟡 LOW Empty tags/kinds arrays silently return zero results — src/knowledge-tools.ts

searchInput accepts tags: [] and kinds: [] (no .min(1)). An empty array is not undefined, so it is forwarded to buildKnowledgeBrief, where filterPages (search.ts:141-151) builds an empty Set and 'page.tags.some(tag => tags.has(tag))' is false for every page: the search returns zero hits with no warning. A caller that passes [] meaning 'no filter' gets an empty brief and a receipt whose configDigest records tags: [] (distinct from null), so the behavior is deterministic and auditable but surprising. Fix: add .min(1) with a descriptive error, or document the any-of semantics via .describe() alongside the existing excludeInvalidated describe.

🟡 LOW Order assertion relies on sort stability when pages tie on path and id — src/search-origins.test.ts

Both pages have id 'state' and path 'knowledge/state.md', so comparePages (src/search.ts:237) returns 0; if their lexical scores also tie, the expected [a, b] order holds only via Array.prototype.sort stability preserving input order. The assertion passed on head, and ES2019+ mandates stable sorts, so this is not a defect — but asserting set membership (or documenting the input-order pin) would make the test robust to tie-break changes. Impact: possible future false failure, never a false pass.

🟡 LOW Rank-to-index alignment assumption in assertRoundTrips — src/search-origins.test.ts

brief.citationIds[hit.rank - 1] assumes citation ids are indexed by contiguous result rank. This holds today because the helper passes no maxChars and buildKnowledgeBrief only ever truncates a prefix of the ranked list (src/knowledge-brief.ts:156-163). If rank ever became non-contiguous (e.g., a mid-list filter), the helper would silently cross-check the wrong reference. Impact: latent coupling only; no current misbehavior. A defensive lookup by hit.citationId would decouple it.

🟡 LOW Ambiguous top-5 lexical hits consume graph seed slots without backfill — src/search.ts

lexicalRanked.slice(0, 5).filter((page) => counts.get(page.id) === 1) slices to the top 5 before filtering ambiguous ids, so an ambiguous hit permanently consumes a seed slot and ranks 6+ never backfill; in the worst case (all top 5 duplicated ids) outLink graph boosting is fully disabled for that query, leaving only shared-source edges. This is the documented conservative intent ('unscoped callers must not invent that edge') and is covered by the 'does not infer an unscoped graph edge from an ambiguous bare id' test, so it is a recall tradeoff, not a correctness bug. If maximum recall matters, filter to unambiguous pages first and then slice to 5; otherwise leave as is.


tangletools · 2026-09-20T12:21:09Z · trace

tangletools
tangletools previously approved these changes Sep 20, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

Copy link
Copy Markdown
Contributor Author

Addressed the delimiter-collision review (discussion_r4056859286) at the shared citation formatter/parser, rather than restricting legal run/page names. inherited:a::b + c and inherited:a + b::c now produce distinct round-trippable handles. Ordinary legacy handles and literal legacy percent sequences are preserved; encoded-prefix behavior is explicitly documented. Five regression tests cover 72 distinct origin/page pairs and actual search-to-resolution correspondence. Final head 43cf11b190c2f8ba60d206bc404808f89cecec46 is green in normal CI run 35519132714: 916 tests pass, 7 existing optional skips, types/lint/build/packed consumer and official optimizer integration pass. Existing original retrieval and historical-search fixes remain consolidated in this PR.

tangletools
tangletools previously approved these changes Sep 20, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

github-actions Bot and others added 2 commits September 20, 2026 16:29
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.
@drewstone
drewstone merged commit 2851717 into main Sep 20, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants