Uh oh!
There was an error while loading. Please reload this page.
context-graph-enrich: scan signal not plumbing (row-selection filter) - #119
Merged
Conversation
T1 propose scanned every part of ai_gateway_messages with no content filter, concatenating them into the per-session text handed to Haiku, and T2 re-deref'd the same source by message id for the curator. On a real ~634k-part corpus that meant ~60% of the fed characters were tool_result (raw file/command output) and ~33% of scanned rows carried no content_text at all (every tool_call part, plus the signature-only thinking parts a proxy never persists) — bulk that costs row budget and crowds the per-session char cap without being durable knowledge worth extracting. Add a shared content filter (contentFilterClauses) applied in BOTH the T1 scan and the T2 deref, behind two source-config knobs: - require_text (default true): drop rows whose text column is null/empty. This already excludes "thinking tokens" — they're empty in the proxy. - exclude_part_types (default ['tool_result']): drop whole part kinds; an explicit [] disables it. part_type_column (default 'part_type') names the column it reads. Filtering the T2 deref matters: it reads source by message id, so a kept text part could otherwise re-admit its message's tool_result siblings into the expensive Opus call. Watermark stays safe — filtered-out rows carry nothing to process and the cursor advances over rows the tick did see. Sub-agent (is_sidechain) exclusion was considered and deferred: only ~12% of text (~80% already tool_result), a leaky signal (NULL on 37% of rows), and a recall policy call rather than a pure cost lever. Documented in LLP 0028#row-selection along with the corpus composition that motivated the defaults. Tests green (68 in the enrich suite); tsc + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Jun 16, 2026
ContributorAuthor
Dual-agent review — |
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Codex | Contract & Interface Fidelity — default part-type filter applied to custom sources (major, config.js:85/92/97, sql.js:106) | Config field chain; Risks #1 |
| Claude | safeDeref filtered WHERE untested (minor, curate.js:267) | Direct callers (safeDeref); Risks #2 |
Codex review
Fix Validations
T1 propose scanned empty/tool-result rows
- Status: correct
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:113, hypaware-core/plugins-workspace/context-graph-enrich/src/sql.js:103
- Assessment:
buildProposeQuerynow ANDs the shared content predicates into the source scan beforeLIMIT. ExistinggroupSourceRowsalready skipped empty text after fetch at propose.js:145, but that did not solve row-budget crowding.
T2 source deref re-admitted excluded sibling rows
- Status: correct
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:267, hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:270
- Assessment:
safeDerefapplies the samecontentFilterClauses(cfg)while dereferencing byid_column, which closes the message-id sibling leak described in the PR.
Findings
2) Contract & Interface Fidelity
- Severity: major
- Confidence: high
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/config.js:85, hypaware-core/plugins-workspace/context-graph-enrich/src/config.js:92, hypaware-core/plugins-workspace/context-graph-enrich/src/config.js:97, hypaware-core/plugins-workspace/context-graph-enrich/src/sql.js:106
- Why it matters: Existing custom
source_datasetconfigs that only satisfy the previous source contract now get a defaultpart_type NOT IN (...)predicate against a column they may not have, turning valid configs into failing propose queries. - Suggested fix: Either make the default part-type exclusion conditional on the default
ai_gateway_messagessource, or validate custom sources/configs so users must explicitly providepart_type_columnorexclude_part_types: []before activation.
No Finding
- Behavioral Correctness
- Change Impact / Blast Radius
- Concurrency, Ordering & State Safety
- Error Handling & Resilience
- Security Surface
- Resource Lifecycle & Cleanup
- Release Safety
- Test Evidence Quality
- Architectural Consistency
- Debuggability & Operability
Evidence Bundle
- Changed hot paths:
validateEnrichConfig,contentFilterClauses,buildProposeQuery,runProposeTicksource scan,safeDerefT2 source excerpt. - Impacted callers:
runProposeTickcallsbuildProposeQueryat hypaware-core/plugins-workspace/context-graph-enrich/src/propose.js:41;runCurateTickcallssafeDerefat hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:97; plugin validation usesvalidateEnrichConfigat hypaware-core/plugins-workspace/context-graph-enrich/src/index.js:36 and :40. - Impacted tests: config defaults/overrides at test/plugins/context-graph-enrich-config.test.js:16 and :78; propose SQL filter coverage at test/plugins/context-graph-enrich-propose.test.js:43 and :67; SQL helper coverage at test/plugins/context-graph-enrich-sql.test.js:27 and :34.
- Unresolved uncertainty: I did not run the suite; this is a targeted diff review. No direct execution test for the private T2
safeDerefquery was visible in the changed test set.
Claude review
Claude review
safeDeref's new content-filter WHERE clause has no test assertion
- Severity: minor
- Confidence: 88
- Evidence: hypaware-core/plugins-workspace/context-graph-enrich/src/curate.js:267
- Why it matters: The single behavior-changing line in curate.js — ANDing
contentFilterClauses(cfg)into the derefWHEREso an excluded part sharing amessage_idwith a kept text part is not re-admitted into the curator excerpt — is exercised but never asserted (the curate test'sfakeQuerymatches only onFROM <table>and ignores the WHERE), so a regression in the deref filter assembly would not be caught. - Suggested fix: Add a curate test that captures the deref query string (or seeds the source dataset with a kept text part and an excluded
tool_resultpart sharing onemessage_id) and asserts the excluded part is absent from the curator excerpt, mirroring thebuildProposeQueryfilter assertions.
Four of five review passes (guidance compliance, shallow bug scan, historical/git-history, contract & callers) found no issues at or above the confidence threshold. Notable confirmations from those passes:
- No-semicolon / JSDoc-type /
@importstyle rules all satisfied; the@ref LLP 0028#row-selectionannotations resolve to the new## Row selectionheading. cfginsafeDerefisruntime.config(the full validatedEnrichConfig); allcontentFilterClausescall sites pass a fully-populated cfg, so.length/.mapnever hitundefined.- The watermark-safety claim holds: the cursor advances only over rows the query returned (built into
rowMeta), and the content filter is deterministic, so filtered higher-ts rows are re-filtered each tick and never cause an unfiltered row to be skipped. - Schema confirmed:
part_type(non-null) andcontent_text(nullable) exist at schema v6;mapPartTypeemits'tool_result', matching the defaultexclude_part_types. - No EnrichConfig is constructed outside
validateEnrichConfigin any runtime path; full suite (1189 tests) passes.
Reports: .git/dual-review/pr-119
Dual-review of PR #119 surfaced two issues in the row-selection filter: - Codex (major, contract fidelity): the default `exclude_part_types: ['tool_result']` was applied to *any* configured `source_dataset`. A custom source that lacks a `part_type` column would get `part_type NOT IN (...)` against a non-existent column, breaking every propose/deref scan. Gate the default to the default `ai_gateway_messages` schema: a custom `source_dataset` now defaults to no part-type filter ([]) and opts in explicitly. `require_text` is not gated — it only reads `text_column`, which every source already configures. - Claude (minor, coverage): safeDeref's new filtered WHERE was exercised but never asserted. Add curate tests that capture the deref SQL and assert the shared content filter is ANDed in (default source) and absent (custom source with the filter disabled). LLP 0028#row-selection documents the schema-bound default; the validation site carries the @ref. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Jun 16, 2026
ContributorAuthor
Addressed dual-review findings — |
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
T1 propose scanned every part of
ai_gateway_messageswith no content filter, concatenating them into the per-session text handed to Haiku; T2 then re-deref'd the same source by message id for the curator. Measured on a real ~634k-part corpus:tool_result(raw file/command output)is_sidechain) rowstool_resultpart_type='reasoning')On top of that, ~33% of all scanned rows carry zero
content_text(everytool_callpart + the signature-only thinking parts). They were already dropped from the model text but still consumed the 200-row/tick budget and crowded the per-session char cap.What
A shared content filter (
contentFilterClauses) applied in both the T1 scan (buildProposeQuery) and the T2 source deref (safeDeref), behind two source-config knobs:require_text(defaulttrue) — drop rows whose text column is null/empty. This already excludes "thinking tokens" (empty in the proxy), so they need no dedicated filter.exclude_part_types(default['tool_result']) — drop whole part kinds; an explicit[]disables it.part_type_column(default'part_type') names the column it reads.Filtering the T2 deref is the non-obvious bit: it reads source by
message_id, so a kept text part could otherwise re-admit its message'stool_resultsiblings into the expensive Opus call. The filter keeps T2 consistent with T1.Watermark stays safe: filtered-out rows carry nothing to process, and the cursor advances over the rows the tick did see, so the next
ts >= cursorscan naturally starts past them. The filter is applied beforeLIMIT, so each tick's row budget is spent on signal.Deferred (documented, not dropped)
Sub-agent (
is_sidechain) exclusion — the original ask — is left out and written up inLLP 0028#row-selection: only ~2.3% incremental oncetool_resultis gone, a leaky signal (NULLon 37% of rows; only 150/3940 conversations flagged), and a recall policy call rather than a pure cost lever.exclude_part_typescan't express it (row flag, not part type); a sidechain predicate is noted as a future knob.Notes / caveat
The per-session 12k-char cap bounds the absolute token bill, so the biggest win is signal density inside that window + freed row budget; pure $ savings land on under-cap ticks.
Test plan
contentFilterClauses, the config knobs incl. explicit-[]and validation errors, andbuildProposeQuery's WHERE)tsc -p tsconfig.json --noEmitcleannode scripts/check-syntax.jsclean🤖 Generated with Claude Code