fix(search): requeue search_echo=0 Codex shell wrapper calls - #87
Merged
Merged
Conversation
Extend `unmarkedDirectSearchCallSQL` to also requeue pre-#80 Codex `shell` tool calls whose serialized text is a direct Backscroll search, mirroring how `isCodexDirectSearchCall` recognizes the shell argv shape at ingest time (argv is exactly [<shell>, -c|-lc, "backscroll search ..."]). The new GLOB clause matches `shell command=[*sh","-c|-lc","backscroll search"]` (bare) or `shell command=[*sh","-c|-lc","backscroll search<ws>..."]` (with extra args), requiring the shell binary basename to end in `sh` via the [[]*sh char class escape. A paired NOT GLOB excludes 4+ argv-element shell calls (whose argv[2] starts with `backscroll search`) so the SQL does not over-match cases the reader would reject via len(Command)==3 — otherwise requeueing would loop forever (SQL matches every sync, reader never marks, no convergence). Regression test `TestPendingSearchEchoPathsRequeuesZeroValuedCodexShellCall` covers the 3 requeue shapes (bare -c, with-args -c, with-args -lc) and the 5 non-requeue shapes (different command, wrong flag, 4-element -c/-lc, and the trailing-whitespace 4-element over-match guard).
…and 'search' Reviewer finding on PR #87: the reader's `isCodexDirectSearchCommand` accepts ANY whitespace separator between the 'backscroll' and 'search' tokens (it uses `strings.Fields`, which splits on every unicode.IsSpace rune), but the serialized text carries whatever the JSON encoder produced for the original argv[2]. For ASCII whitespace that's a literal char (space), a JSON single-letter escape (\t, \n, \f, \r), or — for every other unicode.IsSpace rune — a JSON \uXXXX escape (\u000b for vertical tab, \u0085 for NEL, \u00a0 for NBSP, etc.). The previous SQL only matched the literal-space case, so any pre-#80 Codex `shell` row whose stored text was `shell command=[..., "backscroll\\tsearch orchard"]` (or any other escape form) stayed at search_echo=0 forever and leaked into unfiltered recall. Fix: emit a separate bare + with-args GLOB pair per separator form (whitespace class, then four single-letter escapes, then a \uXXXX char class), each with its own NOT GLOB guard for the 4-element over-match trap. SQLite (modernc.org/sqlite) does not process backslash escapes in string literals by default, so the JSON backslash is built via `char(92) || '<letter>'` rather than `'\\<letter>'`. Regression test `TestPendingSearchEchoPathsRequeuesZeroValuedCodexShellSerializedWhitespace` builds each fixture's stored text via `readers.SerializeToolInput` from a real accepted argv triple (no hand-written `\\t`/\\n strings), then verifies that every accepted separator form requeues and that 4-element calls with tab or NBSP separators still do NOT requeue (the over-match guard holds per form).
…e decode
Round 2 reviewer's finding: the round-2 SQL GLOB (enumerating \t/\n/\f/\r +
a \\uXXXX char class) was correct for control chars but fundamentally
unsound for every other unicode.IsSpace rune (NBSP U+00A0, en/em spaces
U+2002-2003, NEL U+0085, etc) — real JSON encoders (Go's encoding/json and
Rust serde_json) only escape control chars (<0x20) and emit everything else
as raw UTF-8 bytes, so a SQL GLOB has provably no way to keep up with the
open-ended whitespace encoding space.
This round stops trying to enumerate separator byte sequences in SQL and
instead decodes the JSON argv in Go, applying the EXACT predicate the reader
already uses at ingest time. The two call sites now share a single source of
truth so they cannot drift apart again:
internal/directsearch/directsearch.go (new)
IsDirectSearchCommand(command string) bool
IsCodexDirectSearchCall(tool, arguments string) bool
internal/readers/claude_reader.go
isDirectSearchInput now calls directsearch.IsDirectSearchCommand
internal/readers/codex_reader.go
isCodexDirectSearchCall now wraps directsearch.IsCodexDirectSearchCall
(so the in-tree tests + readers package still own the predicate call site)
internal/storage/search.go
unmarkedDirectSearchCallSQL drops the Codex-shell clauses (kept only
bash / exec_cmd, whose text is simple key=value tokens with no JSON
encoding). The shell broad-filter clause moved into
PendingSearchEchoPaths as an additional OR branch.
internal/storage/queries.go
PendingSearchEchoPaths (via filterShellEchoZeroPaths /
pendingSearchEchoShellMatches) now selects rows whose text starts with
'shell command=[' in SQL, then decodes the JSON array and calls
directsearch.IsCodexDirectSearchCall('shell', <args>). paths are kept
when ANY of their echo-zero shell rows passes the strict predicate.
Regression test fixes the round-1 fixture-bug the reviewer flagged:
shellText / shellTextN now build the stored text via a real json.Marshal
round-trip into readers.SerializeToolInput, not fmt.Sprintf+%q — so the
NBSP / en-space / em-space cases serialize as the raw UTF-8 bytes real
JSON produces, not the Go strconv.Quote '\\u00a0' escape that never
appears on disk. Added 3 new Unicode-whitespace fixtures (en space, em space,
NBSP+'/bin/bash' -lc) and rebalanced the existing set.
Round 3 reviewer identified two bugs in how the requeue path reconstructs
JSON from the stored toolfmt text, both in internal/storage/queries.go:
Bug A: pendingSearchEchoShellMatches built "{\"command\":" +
text[len(\"shell command=\"):] + \"}\" \u2014 it assumed everything after
'command=' was exactly the JSON array. But SerializeToolInput emits ALL of
a shell call's fields as sorted 'key=value' tokens. Real Codex shell calls
carry workdir and/or timeout_ms and/or additional_permissions; the
reader ignores them when deciding SearchEcho, but the requeue path's
string-concatenation turned those trailing tokens into invalid JSON like
'{\"command\":[...,"backscroll search\"] timeout_ms=10000 workdir=/tmp}'
which json.Unmarshal rejects, the path is silently dropped, and a
legitimate search_echo=0 echo call never requeues.
Fix: decode exactly ONE JSON value starting right after 'command=' via
json.NewDecoder(strings.NewReader(remainder)).Decode(&commandArray) \u2014
the decoder stops after reading one complete value and ignores whatever
non-JSON key=value text follows it. We only ever need the command array,
not the full original arguments object.
Bug B: the broad SQL prefilter in stalePaths / filterShellEchoZeroPaths
used LIKE 'shell command=[%', assuming 'command' is the first serialized
key. SerializeToolInput sorts keys alphabetically, so a shell call
carrying 'additional_permissions' serializes as
'shell additional_permissions={...} command=[...]' and the LIKE prefilter
misses that row entirely. Fix: broaden the admission check to
LIKE 'shell %' (any content_type='tool' search_echo=0 row starting with the
bare shell tool-name token reaches the Go-side check, which locates the
' command=' token boundary anywhere in the sorted key=value list).
Regression test TestPendingSearchEchoPathsRequeuesCodexShellRoundTrip
builds each fixture via a full CodexReader.Parse round-trip of an inline
rollout JSONL with extra fields (workdir, timeout_ms, additional_permissions
in either position), not via SerializeToolInput directly. Covers 6 cases:
control (command-only), workdir+timeout_ms after command,
additional_permissions BEFORE command (the worst case), workdir BEFORE
command (also sorts before), wrong command (must NOT requeue), and
four-element argv (must NOT requeue, the over-match guard still holds).
Also added internal/directsearch/directsearch_test.go with direct unit
coverage for IsDirectSearchCommand and IsCodexDirectSearchCall (the
shared predicates both readers ingest and storage replay rely on) so the
predicate behavior is pinned independently of the storage and reader
packages. AGENTS.md Module Layout and Package Layout updated for the new
internal/directsearch package.
pablontiv
added a commit
that referenced
this pull request
Sep 12, 2026
…search and relax IDF (#89) * fix(storage): exclude zero-valued Codex shell echoes from unfiltered search and relax IDF The requeue path learned the Codex shell wrapper form in PR #87, but the query-time exclusion paths never did: isDirectBackscrollSearchEcho (Go) and directBackscrollSearchEchoSQL (SQL, used by recallFrequency) recognized only the bash and exec_command three-token prefixes. A pre-#80 tool row stored as search_echo=0 with a serialized 'shell command=["<sh>","-c"|"-lc", "backscroll search ..."]' call therefore leaked into unfiltered result pages and inflated unfiltered --relax document-frequency counting until an owner sync replayed the source — the same gap class fixed for exec_command in PR #86 (bs-echo-exec-cmd-gap-r1). Following PR #87's split, the Go predicate now decodes the JSON argv via the shared pendingSearchEchoShellMatches / directsearch.IsCodexDirectSearchCall instead of token-shape matching, and recallFrequency subtracts shell echo rows with the same broad-SQL-prefilter plus strict-Go-predicate split rather than attempting an unbounded SQL GLOB enumeration. Regression coverage: E2E with a real CodexReader round-trip and forced search_echo=0 (cmd/backscroll/echo_shell_zero_query_gap_test.go), per-row Go/SQL/IDF boundary cases including wrong-command, wrong-flag, extra-argv and non-search guards, a dedicated zero-valued shell IDF test, and shell rows in the at-scale SQL-vs-Go-scan cross-check. * docs: record shell-form query-echo exclusion split in AGENTS.md * fix(storage): hoist shell echo check above the three-token guard Review round 1 on PR #89 found that the pre-existing len(fields) < 3 guard ran before the new shell branch: a Codex shell call whose argv separators are ALL JSON control escapes (\t, \n, …) serializes to only two whitespace-separated tokens, never reached the shell check, and leaked into unfiltered result pages — while recallFrequency's IDF path, which has no such floor, excluded the identical row. The fix made the two paths disagree where at base they had both been wrong. The shell check now runs on the first whitespace-delimited token before any token-count floor, case-insensitively to match the SQL LIKE prefilters. Regression coverage: boundary-table row and E2E files built through a real CodexReader round-trip with a tab-separated argv (asserted to serialize to exactly two tokens), both mutation-checked RED without the hoist; docs and the pendingSearchEchoShellMatches comment corrected to describe the page-side Go matching and the two shape gates accurately.
pablontiv
added a commit
that referenced
this pull request
Sep 12, 2026
…d-text chokepoint (#90) * fix(storage): unify direct-search echo detection behind one serialized-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. * fix(directsearch): guard serialized-call predicate on literal backscroll 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). * fix(directsearch): make the superset guard case-sensitive 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.
Follow-up from the PR #85 adversarial review (
data/bs-review-pr85-r1/report.md, Note 1). PR #85'sunmarkedDirectSearchCallSQLrequeued pre-#80 Codex/OpenCodesearch_echo=0rows for bash andexec_commanddirect search calls, but did NOT cover Codex'sshellwrapper form: argv serialized asshell command=["<shell>","-c|-lc","backscroll search ..."].This PR adds that coverage.
What changes
internal/storage/search.go— extendunmarkedDirectSearchCallSQLwith two new GLOB clauses matching the Codexshellwrapper, plus two paired NOT GLOB clauses to exclude 4+ argv-element cases the reader rejects vialen(Command)==3(otherwise requeueing would loop forever — SQL matches every sync, reader never marks, no convergence).internal/storage/search_echo_test.go— addTestPendingSearchEchoPathsRequeuesZeroValuedCodexShellCall, an 8-fixture test parallel toTestPendingSearchEchoPathsRequeuesZeroValuedDirectCalls, covering the 3 requeue shapes (bare -c, with-args -c, with-args -lc) and the 5 non-requeue shapes (different command, wrong flag, 4-element -c/-lc, trailing-whitespace 4-element over-match guard).Mirrors
isCodexDirectSearchCallexactlyThe reader predicate (
internal/readers/codex_reader.go:133) accepts:tool == "shell"len(Command) == 3path.Base(Command[0])ends inshCommand[1] in {"-c", "-lc"}isDirectSearchCommand(Command[2])The SQL reproduces this shape via:
shell command=[[]*sh","-c|-lc","backscroll search"](bare)shell command=[[]*sh","-c|-lc","backscroll search<ws>..."](with-args)shellcalls regardless of trailing whitespace.Test results
go test ./internal/storage -run TestPendingSearchEchoPaths -count=1 -v: PASS (bothTestPendingSearchEchoPathsRequeuesZeroValuedDirectCallsand the newTestPendingSearchEchoPathsRequeuesZeroValuedCodexShellCall).go test ./internal/... -count=1: all green.go test ./cmd/backscroll -count=1: one pre-existing failure (TestSearchRelaxationQueryEchoesDoNotInvertIDF, query-time echo relaxation) — unrelated to this change, confirmed by reverting this branch and re-running on the base.This is the same scope the brief called out: additive, low-risk, touches only
unmarkedDirectSearchCallSQL.