feat(mcp): add metadata predicates and field projection to POSIX find - #1423
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7c5e8a3642
ℹ️ 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".
6 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d2b099cf1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df784b1ea7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f4aca5f62
ℹ️ 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".
phernandez
force-pushed
the
posix-meta-predicates
branch
from
September 1, 2026 18:55
7f4aca5 to
ea34934
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea349342a5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb40aa751f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47a07770c7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d24133c6e
ℹ️ 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".
phernandez
force-pushed
the
posix-meta-predicates
branch
from
September 1, 2026 23:47
bba38ae to
c71dfa5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71dfa5e47
ℹ️ 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".
find(1) gains predicates and a SELECT clause: --meta status=active, confidence>0.6, review.approved=true parse onto the existing search metadata_filters grammar (strict subset, verified test-side against the server-side parser — unsupported operators fail fast naming the set), and --fields returns the requested frontmatter per hit so callers stop reading N notes to check one field. Without --meta, find stays the byte-identical directory listing. Closes the measured 2x metadata gap from A/B run 6, where POSIX agents brute-force-read notes that rich answered in one metadata_filters call. A parity test pins that find --meta and search_notes with equivalent filters return the same note set through the real ASGI stack. Field projection hydrates entities concurrently (semaphore-bounded gather) because the search index carries only note_type, not full frontmatter; a batch entity read or a wider search projection would remove the round trips entirely — follow-up, not a blocker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
…ators
Two review findings on find --meta.
The subtree scope was built from permalink_match, but a permalink stops
reflecting file_path once a note carries an explicit frontmatter
permalink or is moved with update_permalinks_on_move=False (the default).
So 'find /specs --meta ...' silently dropped matching files under specs/
and admitted files elsewhere whose permalink began with specs/ — while
reporting the wrong total as exact. The search stack has no file-path
filter to fix this with: search_index stores the column but nothing
queries it, and threading one through would be a public schema change
across both backends and the vector path. Rather than ship a
half-correct filter, a non-root scope now refuses and names the
limitation; the predicates are the whole WHERE, so the reported total
describes the query that actually ran. A bare <project> still routes.
The predicate regex also folded a second operator character into the
value, so 'status==active' became {'status': '=active'} and returned
empty instead of erroring. An unquoted value starting with an operator
character now refuses and teaches both the grammar and the quoting
escape, which is verified end to end.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
…cope
find --meta scoped by permalink_match, which stops reflecting file_path
once a note carries an explicit frontmatter permalink or is moved with
update_permalinks_on_move=False (the default). The previous commit
refused non-root scopes rather than answer wrongly; this adds the filter
that lets them be answered correctly.
SearchQuery.file_path_prefix is threaded through the service, the
repository contract, both dialect implementations, and the vector and
hybrid retrieval paths — including the post-filter and stable-pool
refetch, which is where a filter wired only into FTS would leak. One
shared helper builds the predicate, so parity is structural rather than
two hand-written predicates that happen to agree:
SUBSTR(file_path, 1, :len) = :prefix
Chosen over LIKE deliberately. There is no pattern language to escape,
and LIKE would be wrong twice: _ and % are ordinary characters in
directory names, so my_notes would silently admit my-notes, and LIKE
case-folds differently per backend, so one filter would answer two
different questions. The compared prefix carries its trailing slash, so
specs never admits specs-archive.
The divergence test now proves the correct behavior instead of the
refusal: a note living under specs/ is returned regardless of its
permalink, and one whose permalink merely starts with specs/ is not.
Reintroducing permalink scoping fails it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
Three predicate findings, all the same shape: a malformed or unsupported predicate produced a silent empty result or a generic transport error instead of naming the problem. null now matches for real. 'owner=null' finds notes carrying no owner, on both dialects, because SQLite's json_extract and Postgres's jsonb_extract_path_text already collapse a missing key and an explicit JSON null to SQL NULL — so one IS NULL clause answers the same set on either. The parser emits a distinct is_null op rather than eq with a None value, so neither consumer can fall through to the equality branch, which is the bug this is fixing. null outside equality is refused: '>', 'in', 'between' and 'has' all compile to comparisons that are never true against NULL, so they were the same silent zero in four more operators. A non-finite number (score=NaN, 1e999) reached HTTPX's JSON encoder and surfaced as a transport failure; it is now a predicate error naming the value and the quoting escape. An unterminated scalar quote (status="active) was treated as literal text, so the search ran and reported no matches with the malformed predicate hidden. Rather than mirror the list parser's check, that check was removed: a split only happens outside quotes, so a dangling quote always lands inside one element and a single check covers scalars and list elements alike. One refusal site, one message, five fewer lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Four review findings on the unshipped metadata-predicate surface, each the same shape: a wrong answer wearing the same confident total as a right one. A metadata filter now matches Markdown notes only. Every indexed file gets an ENTITY row, and a PDF or an image carries no frontmatter keys at all — which is exactly what IS NULL asks for, so 'owner=null' returned the whole non-note half of a project and counted it into an exact total. Positive predicates hid the hole, because nothing a regular file carries could satisfy one; the content-type clause therefore applies to any metadata filter, so the frontmatter-only contract belongs to the clause rather than to whichever operator happened to be used. It is one shared predicate for both dialects, sitting next to the subtree scope for the same reason: a filter admitting different rows per backend would report an exact total for a match set the other never produces. The path scope now means what it says. Exactly two things are notation — a leading './' and the surrounding separators — and DirectoryService already strips those for the plain listing, so one PATH names one subtree with or without --meta. './specs' used to reach the SQL as the prefix './specs/' and match nothing at all, and './' scoped to a directory named '.' rather than to the root. Everything else survives byte for byte, whitespace included: a directory really can be named ' specs ', and stripping answered for 'specs/' instead — a different subtree, not an empty result. find stopped pre-stripping as well, so SearchQuery's field validator is the single boundary parser instead of two half-normalizations in sequence. Field projection no longer leaves reads running past the client that issued them. gather raises the first failure and leaves its siblings alive, and find then unwound out of get_project_client, closing the client underneath as many as ~199 reads still parked on the semaphore — each firing against a closed client and raising into a task nobody awaits. Cancel-and-drain in a finally: the cancels are no-ops on success, and the original failure is still what reaches the caller. Deliberately not TaskGroup, whose ExceptionGroup would replace a precise ToolError with "unhandled errors in a TaskGroup". Signed-off-by: phernandez <paul@basicmachines.co>
An array-contains metadata filter — `tags has 100%` from find, or
`{"tags": ["100%"]}` on the search API — is answered by two clauses ORed
together: an exact JSON-membership test (json_each on SQLite, `@>` on
Postgres), plus a substring LIKE fallback that reaches frontmatter holding
the array's text rather than a real array.
The fallback interpolated the searched-for value straight into its LIKE
pattern, so "%" and "_" in a value were read as wildcards. `tags has 100%`
matched "100-percent", and because the same query builder serves search()
and count(), the wrong rows counted into the exact total too. Reproduced
identically on both backends before this change.
Escape "%", "_" and the escape character itself, and name the escape
character in an explicit ESCAPE clause — required rather than decorative,
since SQLite's LIKE has no default escape character while Postgres's is
already the backslash, so spelling it out is what makes one pattern mean
one thing on both dialects.
The escaping lives in a shared metadata_contains_like_condition() beside
the existing file_path_prefix_condition and
metadata_filter_content_type_condition, for the reason those are shared: a
filter that admitted different rows per dialect would report an exact total
for a match set the other never produces. Bind-parameter names are
unchanged, and the exact JSON-membership half — the primary path — is
untouched.
Regression tests cover "%", "_" and the escape character on both backends,
on the tags column and on a nested json_extract path, with a positive
control proving the fallback still finds an element inside an array stored
as text.
Signed-off-by: phernandez <paul@basicmachines.co>
find's predicate key capture class admits '.' anywhere, so a malformed dot path — 'review..approved', '.owner', 'owner.' — parsed cleanly and travelled to the search API, which refuses it. Every other predicate mistake in this grammar is refused locally, naming the offending value and teaching the grammar; this one spent a request to come back with "Unsupported metadata filter key", wording that names neither find nor the shape a key must have. Validate the key against METADATA_KEY_RE — the search API parser's own key grammar, lifted from a private name so there is one definition rather than a second copy free to drift from what the repository actually accepts. The capture class stays deliberately loose: tightening it would make '.owner=null' match no regex at all and be reported as a missing operator rather than as the bad key it is. No router change. The reviewer's report that this reaches the caller as a 500 does not reproduce: the v2 search router has mapped ValueError to a 400 since d1d6f27, and all three keys already returned 400 with a readable detail. Added router tests pinning that 400 so it stays a client error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
find's --fields took the same dotted frontmatter paths as --meta predicates
but validated none of them. `_project_metadata_fields` split the string
itself, so an empty segment walked to null: `.owner` reported null for every
hit on notes that all carry `owner`, byte-identical to the null a genuinely
absent field produces. Unlike a bad predicate key — which at least reached a
server that refused it — this had nothing to fail against, so a typo came
back as a uniform, plausible, wrong answer, after paying the search and one
entity GET per hit.
Fixed as coverage of one rule rather than a second check. metadata_filters
now owns a MetadataPath value and parse_metadata_path, and the key regex goes
back to private: the parse is the only way to apply the grammar, and `parts`
exists only on a parsed path. All three surfaces that accept a caller-written
path — the search API's metadata_filters, --meta predicates, --fields
projection — go through it, and there is no longer a second `.split(".")` in
the codebase to forget.
That is what makes it closed rather than patched. A fourth consumer cannot
repeat this: `_project_metadata_fields` takes list[MetadataPath], so handing
it raw strings is a type error, and parse_metadata_path returns
MetadataPath | None, so reaching `.parts` without handling the invalid case
is a type error. Both are rejected by `ty` today, not by review vigilance.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
…vas (#1424) Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
phernandez
force-pushed
the
posix-meta-predicates
branch
from
September 2, 2026 04:40
c71dfa5 to
4f73c16
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f73c16868
ℹ️ 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".
`--fields` is the SELECT to `--meta`'s WHERE, and the whole reason to call find instead of reading every note. It was decorating the full SearchResult instead of replacing it, so every projected row still carried the note's `content` — up to SearchIndexRow.CONTENT_DISPLAY_LIMIT (4000) characters. The 200-row inventory calls the literary-analysis skill documents therefore answered a request for two frontmatter values with most of a megabyte of prose, spending exactly the tokens the projection exists to save. Projection mode now builds the row rather than adding to it: the note's identity — title, permalink, file_path, external_id, updated_at — plus the `fields` object. A whitelist, not a content blocklist, so a SearchResult that later grows another bulky column cannot leak into a projected response. Left out: bodies (content, matched_chunk), a ranking no text query produced (score is -0.0 on every metadata hit), second spellings of an identity already in the row (entity, entity_id), and index-row metadata a caller can ask for by name instead (`--fields type`). Kept because callers read them: file_path and fields drive the CLI's projected renderers, title drives the skill's jq coverage loop, external_id is the hosted deep-link id (#1423). Only projection narrows. Without `--fields` the metadata arm still answers with the full search response grep's renderers read, because there the hit is the answer; a test pins that so the narrowing cannot spread. The CLI test's mock payload was a grep row with a `fields` key bolted on — a shape the tool no longer produces — so it is spelled out as a real projected row. Signed-off-by: phernandez <paul@basicmachines.co>
A comparison such as `score>` followed by 400 digits reached the server as an
ordinary finite Python int — json.loads keeps an oversized integer literal as
an int, so find's non-finite check, which only sees floats, passed it through.
`_normalize_numeric` then called float() on it, which raises OverflowError.
OverflowError is not a ValueError, and ValueError is the only thing the search
router translates, so a predicate typo surfaced as a 500 instead of a
predicate error.
Fixed where the rule already lives rather than in find's grammar.
`_normalize_numeric` owns "this value is numeric, normalize it to a float
bound"; the defect is that it accepted as numeric a value it could not
normalize. Refusing there covers direct
`search_notes(metadata_filters={"score": {"$gt": 10**400}})` callers too, and
the router's existing ValueError translation answers 400 with no second
implementation of which numbers this system can compare. The pre-transport
non-finite check in `_predicate_scalar` stays: `1e999` dies in the request
encoder before any server sees it, which is a different failure.
Stating the rule once as "the bound must be a finite float" also closes a
silent sibling the finding did not name: the same magnitude spelled as a
numeric string does not raise at all — float() answers it with inf — so the
bound went infinite and the comparison matched every note, or none.
Signed-off-by: phernandez <paul@basicmachines.co>
The literary-analysis skill twice taught that a positional `--meta` scope matches slugified permalinks. It does not: `_find_by_metadata` sends `file_path_prefix`, and both repositories compare `search_index.file_path` on a directory boundary. find(1) was corrected when the behavior changed during this PR; the skill was not, so an agent following it would form the wrong expectation for notes that pin an explicit `permalink:` in frontmatter, or that were moved with update_permalinks_on_move off (the default) and kept their old permalink. Both places now describe the directory-boundary match on the indexed file path, and say what that buys: `/characters` reaches everything filed under `characters/`, nested subdirectories included, and never `characters-cut/`. No example command depended on the permalink reading — the scopes in the skill (`/characters`, the coverage-check block) are directories, and the convention it teaches files notes under `characters/major/` and `characters/minor/`, which a file-path prefix reaches. Signed-off-by: phernandez <paul@basicmachines.co>
phernandez
added a commit
that referenced
this pull request
Sep 2, 2026
`--fields` is the SELECT to `--meta`'s WHERE, and the whole reason to call find instead of reading every note. It was decorating the full SearchResult instead of replacing it, so every projected row still carried the note's `content` — up to SearchIndexRow.CONTENT_DISPLAY_LIMIT (4000) characters. The 200-row inventory calls the literary-analysis skill documents therefore answered a request for two frontmatter values with most of a megabyte of prose, spending exactly the tokens the projection exists to save. Projection mode now builds the row rather than adding to it: the note's identity — title, permalink, file_path, external_id, updated_at — plus the `fields` object. A whitelist, not a content blocklist, so a SearchResult that later grows another bulky column cannot leak into a projected response. Left out: bodies (content, matched_chunk), a ranking no text query produced (score is -0.0 on every metadata hit), second spellings of an identity already in the row (entity, entity_id), and index-row metadata a caller can ask for by name instead (`--fields type`). Kept because callers read them: file_path and fields drive the CLI's projected renderers, title drives the skill's jq coverage loop, external_id is the hosted deep-link id (#1423). Only projection narrows. Without `--fields` the metadata arm still answers with the full search response grep's renderers read, because there the hit is the answer; a test pins that so the narrowing cannot spread. The CLI test's mock payload was a grep row with a `fields` key bolted on — a shape the tool no longer produces — so it is spelled out as a real projected row. Signed-off-by: phernandez <paul@basicmachines.co>
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
2 participants
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
A/B run 6 closed the wrong-scope failure class (#1421) and left exactly one measured gap: POSIX metadata tasks cost ~2× rich's tokens (90.0k vs 44.3k per completed task). The cause was visible in the transcripts — with no structured predicates, agents
catnote after note to check frontmatter fields thatsearch_notes(metadata_filters=…)answers in one call.This gives
findthe two clauses it was missing: a WHERE (predicates) and a SELECT (projection).Stacked on #1421 → #1416.
What changed
--metapredicates:status=active,confidence>0.6,review.approved=true,label in "a,b",c— parsed onto the existingmetadata_filtersgrammar as a strict subset. Unsupported operators fail fast naming the supported set; nothing silently degrades to equality. A test runs every accepted predicate's output through the server-sideparse_metadata_filters, so the POSIX grammar can't drift into a parallel dialect that only fails at request time.--fieldsprojection: returns the requested frontmatter per hit, killing the read-per-note loop. Missing fields render null, never dropped rows.--meta= byte-identical directory listing (pinned by test).--name/--depth/--fields-without---metacombinations refuse before any I/O rather than silently ignoring an input.find --metaandsearch_noteswith equivalent filters return the same permalink set through the real ASGI stack — cross-surface capability parity under the fairness contract.--meta/--fieldsthrough the shared layer;man1/find(1).mdrewritten with a PREDICATE GRAMMAR section (the manual ships with the feature).Review findings, all applied
meta=[]silently became an unfiltered project-wide search — now refused before I/O, mirroringfields=[].label in "a,b",c) produced a silently wrong filter and an empty result set — the splitter is now quote-aware with backslash handling, and an unterminated quote fails fast.max_tokens=1suggestion was deliberately not taken: the router 404s on content-less entities when any slice param is set, trading a correctness cliff for a body-size trim. Reason recorded on the constant.Known follow-up: the search index carries only
note_type, not full frontmatter (verified insearch_service.py), so projection must hydrate. A batch entity read or a wider search projection would remove the round trips entirely.Verification
ty— cleantests/mcp1124 passed,tests/cli944 passed (real exit codes captured, not piped); +100 tests across the three touched files;posix_tools.pyat 100% line coverage🤖 Generated with Claude Code
https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp