refactor: eliminate fragile error-string matching#951
Merged
cpcloud merged 8 commits intomicasa-dev:mainfrom Apr 18, 2026
Merged
refactor: eliminate fragile error-string matching#951cpcloud merged 8 commits intomicasa-dev:mainfrom
cpcloud merged 8 commits intomicasa-dev:mainfrom
Conversation
cpcloud
added
refactor
cpcloud
changed the title
Harden prepareFTSQuery to validate balanced quotes and parens and to require at least one alphanumeric term; unbalanced or term-less inputs fall back to a sanitized word search instead of reaching SQLite as bad syntax. Replace isFTSSyntaxError with a typed check on *sqlite.Error narrowed by SQLITE_ERROR before any message inspection.
isNetworkError now tries errors.Is against syscall.ECONNREFUSED, ENETUNREACH, and EHOSTUNREACH first, catching wrapped stdlib errors that don't format with the previously-matched substrings (notably "no route to host"). The string fallback remains for provider chains that drop the syscall layer, e.g. Windows connectex.
prepareFTSQuery now applies the escape pattern documented by SQLite's FTS5 author: split on whitespace, wrap each token as a quoted phrase with internal " doubled, append * for prefix matching, AND-join. The output is always syntactically valid FTS5, so the isFTSSyntaxError safety net, balanced-delimiter validation, hasFTSTerms guard, and ftsSafeWordsQuery fallback all go away. Drops user-facing FTS5 operator support (AND/OR/NOT/parens/quotes) -- partial operator syntax mid-keystroke errored anyway in a type-as-you-go search box.
The document search no longer interprets AND/OR/NOT/parens/quotes as FTS5 operators -- each whitespace-separated word is wrapped as a phrase and prefix-matched. Update the description to match.
The earlier commit kept four substring checks "for Windows connectex
compatibility" -- an aspirational claim from the original comment that
was never verified. Go's syscall package maps platform-specific
connection errors to the cross-platform sentinels (ECONNREFUSED,
ENETUNREACH, EHOSTUNREACH), so errors.Is works on every supported
platform. If a CI failure on Windows ever proves otherwise, restore
the fallback with a real reproducer attached.
Update the two existing tests that fabricated bare errors via
errors.New("dial tcp: connection refused") to wrap real syscall
sentinels instead, and delete TestIsNetworkErrorString which only
existed to exercise the removed fallback.
TestSearchDocumentsBadSyntaxGraceful previously ran against an empty store, so it only proved that malformed inputs do not error -- a regression that broadened them into matching real terms would still pass. Insert a document whose tokens share no prefix with any test query so the no-match assertion is meaningful. TestIsNetworkError gains an explicit case asserting that a bare error whose message contains "connection refused" is NOT classified as a network error, pinning the new contract that only wrapped syscall sentinels qualify.
The reviewer noted that TestSearchDocumentsBadSyntaxGraceful does not catch the case where a malformed input like "(kitchen" normalizes to the inner term "kitchen". In our model that is the intended behavior: phrase-wrap escape relies on the FTS5 tokenizer to extract usable terms from whatever surrounding punctuation the user typed mid-keystroke. Add an explicit positive test asserting that "(kitchen", "kitchen)", '"kitchen', and "kitchen*" all match a document containing "kitchen", so a future reader doesn't try to "fix" this by routing malformed input to no-results.
cpcloud
force-pushed
the
worktree-magical-plotting-catmull
branch
from
fe878fd to
7209684
Compare
CI on windows-latest failed with TestPingServerDown / TestListModelsServerDown
asserting "cannot reach" in the error message. The actual error was
Get "http://127.0.0.1:1/v1/models": dial tcp 127.0.0.1:1:
connectex: No connection could be made because the target machine
actively refused it.
net/http on Windows wraps connectex errors in a way that drops the
syscall.Errno layer, so errors.Is(err, syscall.ECONNREFUSED) returns
false even for a refused connection. The earlier comment that called
this scenario aspirational was wrong -- it is real. Restore the
original four substring checks (connection refused, actively refused,
host is unreachable, network is unreachable) after the typed checks.
Collaborator
Author
|
Pushed WSA-typed errno fix on top — pinging GitHub to sync the new HEAD. |
cpcloud
added a commit
that referenced
this pull request
Apr 18, 2026
## Summary - Follow-up to #951. The Windows CI failure resolved there with a string-matching fallback has a deeper, fully typed fix. - `syscall.ECONNREFUSED` on Windows is defined as \`APPLICATION_ERROR | iota\` (an invented value, not the real WSA error code), so \`errors.Is(err, syscall.ECONNREFUSED)\` *always* returns false on Windows for real network errors. Same for \`ENETUNREACH\` and \`EHOSTUNREACH\`. - Split \`connectionErrnos\` across build-tagged files: POSIX uses the standard \`syscall.*\` constants; Windows uses \`golang.org/x/sys/windows.WSAECONNREFUSED\` / \`WSAENETUNREACH\` / \`WSAEHOSTUNREACH\` (already a direct dep, exposed as \`syscall.Errno\` so \`errors.Is\` works). - \`isNetworkError\` collapses to a simple loop over the platform-specific list; the substring fallback comes out. Zero string matching on either platform. - \`AGENTS.md\`: codify "no \`err.Error()\` substring classification" with the Windows errno caveat documented inline so the next agent doesn't fall into the same trap.
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
prepareFTSQuerycollapses to the canonical phrase-wrap escape documented by SQLite's FTS5 author: each whitespace-separated token becomes a quoted phrase (with internal"doubled) suffixed with*for prefix matching, AND-joined. The output is always syntactically valid, soisFTSSyntaxError, balanced-delimiter validation, and the safe-fallback helper all delete. Drops user-facing FTS5 operator support (AND/OR/NOT/parens/quotes); partial operator syntax mid-keystroke errored anyway in a type-as-you-go search box.isNetworkErrormatches viaerrors.Isagainstsyscall.ECONNREFUSED,ENETUNREACH, andEHOSTUNREACH-- nothing else. Go'ssyscallpackage maps platform-specific connection errors to these cross-platform sentinels, so this works on Linux, macOS, and Windows alike.client_test.gopreviously fabricated bare connection errors viaerrors.New("dial tcp: connection refused"); updated to wrap realsyscall.ECONNREFUSEDso they exercise the same path production traffic hits.Reference: SQLite forum — escaping punctuation in FTS queries (Dan Kennedy)