fix(storage): unify direct-search echo detection behind one serialized-text chokepoint - #90
Merged
Merged
Conversation
…d-text chokepoint The echo-exclusion bug class (#64, PRs #86/#87/#89) was caused by 5-6 independent recognizers of "is this row a direct backscroll search echo" across three representations, kept in sync only by prose comments: - isDirectBackscrollSearchEcho: hand-rolled bash/exec_command token checks plus a shell-only decode fallback - directBackscrollSearchEchoSQL (recallFrequency IDF): SQL GLOB with an ASCII-only separator alphabet, shell form not matched at all - unmarkedDirectSearchCallSQL (PendingSearchEchoPaths requeue): the same GLOB, plus a separate 'text LIKE shell %' prefilter for shell - excludeDirectBackscrollSearchEchoes: dead duplicate with no production caller strings.Fields accepts every unicode.IsSpace rune (NBSP, U+2028/2029, U+3000, ...), so any separator outside the SQL alphabet — or any shell row with leading whitespace under the anchored LIKE — made pages exclude a row that IDF counting and requeue detection kept. Point-patching the SQL whitespace alphabet cannot close this class. Replace all of it with the broad-SQL-prefilter + strict-Go-predicate split PRs #87/#89 proved for the shell form, generalized to every shape: - directsearch.IsSerializedDirectSearchCall is the single strict predicate owning all three serialized shapes (bash, exec_command, shell argv decode); the shell decode moved here unchanged. - recallFrequency and PendingSearchEchoPaths share one provable-superset prefilter (content_type='tool', search_echo zero, text LIKE '%backscroll%') and apply the chokepoint in Go; no SQL-side shape recognizer remains to drift. - isDirectBackscrollSearchEcho is now SearchEcho || chokepoint, deleting the token indexing and the guard-ordering hazard. - Dead excludeDirectBackscrollSearchEchoes removed; its boundary test rewired onto the production refill path. Tests: new internal/storage/echo_parity_test.go asserts three-way page/IDF/requeue agreement over shape x separator alphabet (ASCII controls, NBSP, U+2028/2029, U+3000, raw vs JSON-escaped) x leading/trailing runs (RED on the old code: 48 diverging subtests, GREEN now); the boundary corpus in relaxation_echo_idf_test.go lost its SQL-equivalence column and gained the requeue leg; the mislabeled "shell NBSP separator" fixture now carries a real U+00A0 via an explicit escape (verified bytewise). Spike note: docs/research/2026-09-12-echo-chokepoint-spike.md. No schema migration. Ordinary search behavior/output unchanged; all prior echo-exclusion regressions (#85-#89) still green.
…oll substring Independent review of the chokepoint PR found the broad SQL prefilter (text LIKE '%backscroll%') was a provable superset of the strict Go predicate for the bash/exec_command shapes but not for the Codex shell shape: the argv is stored as raw, un-decoded JSON bytes, so a JSON \u-escaped letter of "backscroll" (e.g. \u0062ackscroll) would be accepted by IsSerializedDirectSearchCall after decoding yet missed by the prefilter — a one-row-wide three-way divergence. The guard makes the documented provable-superset claim true for every shape; it folds ASCII case to match LIKE semantics. Regression coverage: unit case in directsearch plus a three-way parity corpus entry asserting page, --relax IDF, and requeue agree (on not excluding the row).
Round-2 review found the round-1 guard folded case the wrong way for a superset guarantee: Go's strings.ToLower folds U+212A KELVIN SIGN to 'k', but SQLite LIKE folds only ASCII A-Z, so a row with an escaped-letter shell argv plus a BAC<KELVIN>SCROLL near-miss elsewhere in the text passed the guard while remaining invisible to the prefilter — the same three-way divergence one code point over. Case-sensitive strings.Contains(text, "backscroll") needs no folding reasoning at all: literal-lowercase-substring present implies LIKE matches, full stop, and every real-encoder row accepted by the predicate contains the literal by construction (verified non-narrowing by the reviewer's 391,300-input differential). Regression fixture added to the parity negatives; verified RED against the round-1 guard, GREEN here.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Structural hardening of the echo-exclusion bug class (#64, PRs #86/#87/#89). The class was caused by 5–6 independent recognizers of "is this row a direct Backscroll search echo" across three representations, kept in sync only by prose "keep in lockstep" comments. This PR ships the approved fix: generalize the broad-SQL-prefilter + strict-Go-predicate split (proven for the Codex
shellform in #87/#89) to all three serialized shapes, and give the boundary exactly one owner.What changed
directsearch.IsSerializedDirectSearchCallowns all three stored shapes —bash,exec_command, and the Codexshellwrapper (JSON argv decode moved here unchanged). No token-count floor before the shell decode (an all-escaped argv serializes to two tokens).isDirectBackscrollSearchEchois nowSearchEcho || IsSerializedDirectSearchCall(text)behind the existingcontent_typeguard — hand-rolled token indexing and the PR-fix(storage): exclude zero-valued Codex shell echoes from unfiltered search and relax IDF #89 guard-ordering hazard deleted.directBackscrollSearchEchoSQLandunmarkedDirectSearchCallSQL(GLOB patterns with an ASCII-only separator alphabet) replaced by one shared, provable-superset prefilter (content_type='tool' AND COALESCE(search_echo,0)=0 AND text LIKE '%backscroll%'— every accepted shape contains the literal substring) + the Go chokepoint as the strict pass, for both call sites (recallFrequencyIDF counting andPendingSearchEchoPaths/stalePathsrequeue detection). This closes the NBSP/U+2028/U+2029/U+3000 page/IDF divergence as a class — no separator-alphabet point patch.excludeDirectBackscrollSearchEchoes(no production caller); its boundary test rewired onto the productionrefillCandidatesWithoutDirectEchoespath.Tests
internal/storage/echo_parity_test.gogenerates shape {bash, exec_command, shell} × separator alphabet {ASCII controls, NBSP, U+2028, U+2029, U+3000, repeated/mixed runs} × leading/trailing runs and asserts three-way agreement (unfiltered page exclusion ==--relaxIDF exclusion == requeue detection). 48 subtests diverged on the old code; all pass now. Negative controls (status/searcher subcommands, absolute paths, env wrappers, folded case, nestedbash -lc, prose mentions) unchanged. Spike note:docs/research/2026-09-12-echo-chokepoint-spike.md.TestRecallFrequencySQLEchoPredicatePreservesBoundaries→TestEchoBoundaryCasesThreeWayParity: the SQL-equivalence column is obsolete by design (no SQL shape predicate remains); gained the requeue leg instead."shell NBSP separator"carried plain ASCII spaces; now uses an explicitjson.Marshal+SerializeToolInputround-trip).internal/directsearch/directsearch_test.go.just cigreen (aggregate coverage 86.6% ≥ 85%); all prior echo-exclusion regressions (fix(search): requeue search_echo=0 Codex and OpenCode calls #85–fix(storage): exclude zero-valued Codex shell echoes from unfiltered search and relax IDF #89) still pass. No schema migration; ordinary search behavior/output unchanged.Docs
docs/search.md#query-echo-handling, the--relaxsection, and the regression-owner list now describe the single-chokepoint architecture; AGENTS.md's stale "keep in lockstep" paragraphs were rewritten to the shared-code-path description.Out of scope (per captain decision)
Pi's unpaired-result-rows gap stays unbuilt — tracked separately as
bs-echo-pi-result-unmarked-r1.Note for local runs
cmd/backscroll/search_relaxation_echo_idf_e2e_test.goasserts!strings.Contains(out, "echo-")on full output paths; if yourGOTMPDIR/temp root contains the substringecho-, the test fails on the temp path itself (pre-existing fragility, reproduces on unmodifiedmain). Not caused by this PR.🤖 Generated with a firstmate crewmate worker