feat(index): harden project search provenance and FTS repair - #113
feat(index): harden project search provenance and FTS repair#113kunkunGames wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9029cf6b3
ℹ️ 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".
kunkunGames
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a9369b9e
ℹ️ 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".
|
Conflict/merge-order note for my six open PRs is in #104 (comment #104 (comment)). Short version: a pairwise trial merge shows these PRs conflict only in |
|
@codex review Both open comments are addressed in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70b856ac4a
ℹ️ 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".
|
@codex review Please re-review the latest head, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc533c7615
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please re-review the latest head, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31bf33d210
ℹ️ 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".
|
@codex review Latest validated head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8efbbe4068
ℹ️ 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".
|
@codex review Please re-review the latest head, |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Staging this rather than taking it whole. The idea is sound and the incremental-sentinel work in particular is careful, so I want to be straight about what is happening to it. Taking, in an upcoming release: search hardening (param validation, limit clamp, code-point-safe preview with truncation metadata, real error propagation), match provenance on the existing Not scheduling slices 4 through 8 — the schema v3 migration and the four structured FTS tables. That is honest accounting rather than a soft no: costed properly it is eight to ten days once the redesign below is included, and there is no demonstrated demand for it (no requests on the Ideas board, this PR is the only ask). I would rather spend that week on the updater path and the deprecation sweep. If demand shows up on the board I will revisit. The specific blockers, in case you want to take any of this further: It does not build on UE 5.7. The migration itself is fine — transactional, rolls back cleanly, cannot corrupt a database. The problem is the failure policy: on failure Related: one unloadable level or one DataTable with a null row struct fails the sentinel without advancing hashes, so the same failing delta retries on every editor start, forever. Broken assets are normal in real projects.
Leaving open while the first three slices land. You will be credited. |
c180619 to
01ffbdd
Compare
kunkunGames
changed the title
01ffbdd to
b880852
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b880852fce
ℹ️ 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".
b880852 to
c11f8d8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11f8d8a9f
ℹ️ 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".
c11f8d8 to
6008779
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60087799ca
ℹ️ 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".
6008779 to
e52c730
Compare
|
The staging call is fair and the branch is now exactly the three slices you said you'd take. Slices 4–8 are deleted, not flagged off. Current head That removal resolves three of your four blockers by deletion. The fourth I re-verified rather than asserting, because it's the one that would send this back on sight. "It does not build on UE 5.7." It does now. Every file this PR touches in "The failure policy takes the entire project index offline." Gone with the migration — there is no "One unloadable level or one DataTable with a null row struct fails the sentinel without advancing hashes." Also gone — no sentinel writer, no incremental ownership change here. Still worth keeping on your list for whatever ships the structured tables later; broken assets are normal and that retry loop is real. " constexpr int32 PreviewCodePoints = 240;
inline FString ProjectPreview(const FString& Value, int32& OutLength, bool& bOutTruncated)
{
OutLength = CountUnicodeCodePoints(Value);
bOutTruncated = OutLength > PreviewCodePoints;
return bOutTruncated ? LeftUnicodeCodePoints(Value, PreviewCodePoints) : Value;
}Surrogate pairs count as one code point, so the bound can't split an astral character. Two things you didn't comment on, so they aren't a surprise:
Error codes are split: Happy to rebase onto whatever you merge first — the only conflict against my other open PRs is the |
|
@codex review Latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e52c730513
ℹ️ 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".
| return Error.Contains(TEXT("fts5: syntax error"), ESearchCase::IgnoreCase) | ||
| || Error.Contains(TEXT("unterminated string"), ESearchCase::IgnoreCase) | ||
| || Error.Contains(TEXT("malformed MATCH"), ESearchCase::IgnoreCase) | ||
| || Error.Contains(TEXT("unknown special query"), ESearchCase::IgnoreCase); |
There was a problem hiding this comment.
When a caller submits a malformed query such as NEAR(foo, abc), the new projector accepts the whole NEAR group and SQLite reports expected integer, got "abc". Because that diagnostic matches none of these checks, RunSearch returns InternalError and the action emits -32603 instead of the documented invalid-parameter -32602. Validate the NEAR distance in the projector or recognize this MATCH-parser diagnostic as a syntax error.
Useful? React with 👍 / 👎.
kunkunGames
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a9beb5a0c
ℹ️ 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".
| if depth == 0: | ||
| self.position += 1 | ||
| return _trim_fts_space( | ||
| self.query[start:self.position] | ||
| ) |
There was a problem hiding this comment.
When the Python fallback receives NEAR(foo, abc) or NEAR(foo, 1, 2), this scanner returns the whole group as applicable without validating the optional distance, so the query reaches SQLite and is reported as the generic “Invalid or failed FTS5 query” error. The shared live/native projector now rejects these forms before execution with an explicit caller-syntax error, breaking the documented parity and preventing Python clients from distinguishing invalid input from index failure; mirror the distance/comma validation here.
Useful? React with 👍 / 👎.
| std::string near_text; | ||
| const near_parse_result near_result = | ||
| try_parse_near_group(near_text); | ||
| if (near_result == near_parse_result::parsed) | ||
| { | ||
| if (has_initial_anchor) | ||
| { | ||
| fail_at( | ||
| "FTS5 initial-token anchor is not valid on a NEAR group", | ||
| expression_start); | ||
| return nullptr; | ||
| } | ||
| result = node::make_leaf(std::move(near_text)); |
There was a problem hiding this comment.
Fresh evidence beyond the distance case is that malformed NEAR contents remain opaque here: for example, asset_name:foo AND node_name:NEAR(foo OR bar) is rejected by SQLite because OR is not valid inside a NEAR phrase list, but both table projections become inapplicable before either executes the NEAR expression, so search incorrectly succeeds with zero results. Parse and validate the NEAR phrase list before projection so malformed syntax cannot be hidden inside a cross-table conjunction.
Useful? React with 👍 / 👎.
| try: | ||
| rows = self.db.execute( | ||
| sql, | ||
| (search_query, limit), | ||
| ).fetchall() | ||
| except sqlite3.OperationalError as error: |
There was a problem hiding this comment.
When an FTS shadow table is corrupt, SQLite reports SQLITE_CORRUPT and Python raises sqlite3.DatabaseError rather than sqlite3.OperationalError from this fetchall(). The exception therefore escapes and the fallback exits with a traceback instead of the new fail-closed success: false JSON response, unlike the live and native implementations; catch the broader SQLite database error class while preserving the same failure envelope.
Useful? React with 👍 / 👎.
Validate complete FTS5 NEAR phrase grammar before table projection in native and Python paths. Preserve structured Python failures for SQLite corruption, make the native query build fail closed per translation unit, and add focused regression coverage plus synchronized docs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
kunkunGames
changed the title
…ading as zero results Three changes to discovery and project search, plus their offline mirrors. CROSS-NAMESPACE DISCOVER (#112) discover's filter predicate lived inside the namespace branch, and the no-namespace branch never parsed filter, offset, limit or detail at all. So discover(filter="set_property") with no namespace was SILENTLY IGNORED -- the caller got the full namespace inventory back and no error, and finding a capability by partial name meant iterating every namespace by hand. A non-empty filter with no namespace now searches the whole registry in registry order, tagging each row with its owning namespace and reporting matched_namespaces pre-pagination, so "which namespace owns this" stays answerable even when rows are capped. Unlike the per-namespace path this one is not naturally bounded -- ~150 actions worst case there versus ~1,400+ here -- so an absent limit caps at 50 while an explicit limit=0 still means ALL, matching the existing contract. Uncapped it would emit a row per match across the entire registry, from the tool whose terse redesign existed to cut payload. PAGINATION OVERFLOW (#112, independently shippable) SliceEnd = FMath::Clamp(SliceStart + Limit, ...) formed SliceStart + Limit as int32 before clamping. offset=1 with limit=2147483647 signed-overflows -- and that call is well-formed, because TryGetNumberField range-checks against TNumericLimits<int32> and so accepts INT32_MAX verbatim while rejecting anything larger. On MSVC it wrapped negative, Clamp collapsed the slice, and the caller got an EMPTY actions array plus a NEGATIVE next_offset. Min() is now taken first so every intermediate stays in range, and next_offset emits SliceEnd, which is value-identical and structurally overflow-free. SEARCH ERROR CLASSIFICATION AND VALIDATION (#113 slices) while (Step() == Row) treats a statement error as end-of-results, and Stmt.Create / SetBindingValueByIndex returns were unchecked, so a genuine FTS5 syntax error or a prepare failure surfaced as "no matches" -- the caller could not tell a bad query from an empty index. Caller errors now return -32602 and storage or schema failures -32603, with LIMIT bound as a parameter rather than formatted into the SQL. Adds query validation, a limit clamp and a 4096-character query cap. DELIBERATE DEVIATION FROM THE PLAN, on review of live SQLite behaviour: the plan's literal classification would have regressed column-qualified search. search runs two independent statements, and a query like node_name:Branch errors on the asset table before returning real hits from the node table -- so it works today only because the error is swallowed as end-of-results. Treating any per-table error as fatal would turn a working feature into a hard error. A table reporting "no such column" is therefore treated as not applicable and skipped; the query is only rejected when BOTH tables reject it, or when the error matches the table-independent syntax patterns. That behaviour is now documented rather than accidental, and has a regression test. PR #113's 1449-line FTS5 grammar parser is NOT taken. It introduced an unguarded recursive AST teardown with no query-length cap, which kills the editor process at roughly 32KB of chained terms -- reachable over MCP, on the game thread, with no recovery -- and a 15,935-query differential against real SQLite showed the safety property it claimed was actually delivered by an eight-line error classifier. Queries continue to bind straight into MATCH ?, where SQLite's own bounded grammar handles them. Reported and prototyped by @kunkunGames (#112, #113).
Folds the per-phase entries into the shared files in one pass. They were written as fragments during execution because five agents worked the tree concurrently and a shared file has no conflict marker -- a concurrent edit just loses the earlier entry silently. Also records the follow-ups the reviews surfaced but the fixes deliberately did not absorb, so they survive as tracked gaps rather than as review comments nobody reads again: - Pin-type strictness is still asymmetric: three variable actions keep the best-effort parser, so an unresolvable enum: token there still yields a silent plain byte. The reported #115 symptom is fixed; that failure mode is not fully closed. - Offline project.* parity is UNGATED. verify_offline_parity.py covers cppreflect, network, decision, risk and source, and has no project cases -- confirmed in its own case list. So the three project search implementations are kept in step by hand and make_release.ps1's parity gate cannot catch a drift. Until that is closed, an edit to one is an edit to all three. - A query naming columns from both FTS tables is refused rather than served, because per-table projection was the one thing PR #113's rejected parser provided. - PR #102 part 2 is deferred pending redesign, not rejected: the need is real and ReconstructNode() after a node-property write is the genuinely new insight in that submission.
|
Partly shipped in v0.22.0. I land contributor fixes as my own commits rather than merging the branch — I keep the shipped history single-author for release integrity, and credit you in the release notes instead. No reflection on the patch. Taken: the error classification ( The classifier is the load-bearing piece and it went in verbatim. Its five I have not taken the FTS5 projection parser. I had it extracted, compiled standalone and fuzzed before deciding, and the reason is concrete: its AST destructor recurses without a depth guard and nothing caps query length, so a query of roughly 12,000–16,000 chained terms — about 32 KB, trivially reachable over MCP — kills the editor process. It dies inside A 15,935-query differential against real SQLite also showed 62 false accepts and 315 false rejects — so the parser does not eliminate FTS5 syntax errors either; your ~8-line classifier does. That is 1449 lines of permanent maintenance surface for a property already delivered in eight. The query-length cap went in anyway. It is free defence in depth and it is the mitigation if any AST-building layer ever lands. One thing you found that I want to credit separately, because it is a genuine release-integrity hole and it is now fixed: I fixed it with conditional execution rather than the 63-line restructure, because Deferred: per-table query projection (the one thing the parser genuinely provided — a query naming columns from both FTS tables is still refused) and closing the |
2 participants
Goal
Harden project-search match provenance, text boundaries, FTS5 query projection, and error classification while preserving the existing
fts_assetsandfts_nodestables, and add a dry-run-first repair path limited to the affected FTS scope.Plain-language explanation
Search results now state exactly where and why they matched, and each query condition is projected only to the asset or node table where it can apply. Invalid search syntax is reported as caller error
-32602, while real database or schema failures are reported as internal error-32603. Live C++, the native offline executable, and the Python offline path follow the same syntax and failure contract.Improvements
NEAR(...), column filters, grouping, andAND/OR/NOTare projected without changing precedence.NEARphrases,*prefixes,+concatenation, and optional unsigned-decimal distances are fully validated before projection.project.repair_ftssupports preview-first repair forassets,nodes, orall.Before → After
-32602; storage and schema failures are-32603NEARcontents through as opaque textNEARinside a branch for another table could disappear during projectionOperationalErrorcould leak a tracebacksuccess:falseenvelope%ERRORLEVEL%after the first C++ translation unit failedSide-effect analysis
NEARexpressions are rejected even inside an inapplicable branch.NEAR(..., N)and the existing distance-freeNEARform remain supported; invalid phrases, operators, anchors, commas, and distances are rejected explicitly.Resolved review feedback
The latest commit,
7f4252d9ad6ea9122433658c18f166749580745e, addresses all remaining review scope in code:NEARgrammar validation to the Python offline path with behavior equivalent to the shared C++ implementation.NEARsyntax could be hidden.sqlite3.DatabaseErrorfailures as structured internal errors and added a real corrupted-FTS5 fixture.%ERRORLEVEL%masking inTools/MonolithQuery/build.bat, discovered during verification, and proved it with compiler-failure injection.Review threads were intentionally left for the reviewer to verify against the new diff rather than being resolved or replied to unilaterally.
Verification
python -m py_compile: PASS.NEAR, a hidden malformed branch exposed before projection, phrase and distance errors, and a real FTS5 corruption fixture with no traceback.2cc5d9c574f46623, with matching executable and manifest.Binaries\monolith_query.exeSHA remained unchanged.NEARsyntax, classification, and JSON differential: 45/45 PASS.ProjectSearchAction.cpp, the hardening test, and database source were recompiled, andUnrealEditor-MonolithIndex.dllwas relinked successfully.Monolith.Index.ProjectSearch.HardeningAndRepair: 1/1 PASS, with 0 test warnings, 0 errors, and process exit 0. Report:D:\P4\MonolithPR113ReviewUE58Host\Saved\Automation\PR113NearGrammarFinalUE58\index.json.D:\P4\MonolithPR113FinalUE57Host\Saved\Automation\PR113NearGrammarFinalUE57\index.json.offline_exe_freshnessdisabled because binary identities differ on the base.Saved\EngineSource.db.parity_spec_revequality was confirmed; this limitation is separate from the 45/45, 25/25, and 250/250 project-search verification above.git diff --check: PASS. The branch was 0 commits behind and 3 commits ahead of the latesttumourlove/master.Docs/testing/2026-07-26-project-search-hardening.md.WorkFingerprint
agent: Codexcategory: correctness / search / repairmodule: MonolithIndex, MonolithQuery offline paritycomponent/action/helper:project.search,project.repair_fts, recursive FTS5 projector, project-search database, native offline buildintended files:ProjectSearchAction.{h,cpp},ProjectRepairFtsAction.{h,cpp},ProjectSearchQueryProjection*.h,MonolithIndexDatabase.{h,cpp},MonolithProjectSearchHardeningTests.cpp, offline query/parity/build scripts, matching Index/API specsrisk type: query misprojection, wrong error classification, misleading provenance, over-broad repair, stale offline binary acceptancepublic API impact: yes, richer result provenance and explicit invalid-query behaviordocs/spec impact: yesDuplicate check
On 2026-07-28, open PRs #104, #112, #113, and #114, their related remote branches, and their actual changed files were checked again. #104 targets the updater, #112 targets discovery, and #114 targets activation and the index-writer lifecycle. #114 overlaps in the Index subsystem and some shared documentation, but #113 owns the read/search/FTS-repair contract while #114 owns writer activation and acceptance. No other open PR provides the same FTS projection, provenance, and repair implementation. Shared files may require a rebase after an earlier PR merges; that remains an explicit conflict risk rather than duplicate semantic ownership.