fix(datasource-customizer): let a narrowed search be permission-checked instead of refused

[PMerlet]

Why

#1840 refuses an extended search whenever getSearchedFields cannot state what the search reads, and the search decorator answers null for any replaceSearch:

override getSearchedFields(search: string, extended: boolean): SearchedField[] | null {
  if (this.replacer) return null;
  ...

That refusal is correct for a handler that picks its own fields. It is too broad for the common case — a handler that only narrows the default search:

collection.replaceSearch((value, extended, context) =>
  context.generateSearchFilter(value, { extended, includeFields: ['project:name'] }),
);

Here the fields are a declarative list and the footprint is exactly computable, yet the collection loses extended search entirely. Three collections in the Forest SaaS back-end hit this on the 1.97.3 bump (trial, subscription, invoice) — one caught by a test, two silently. subscription and invoice pass two-level paths (billing:project:name) that the plain default search cannot express, so dropping the handler is not a workaround.

The refusal exists because the footprint could not be stated, not because it could not be known. generateSearchFilter already computes it: searchableFields is the answer.

What

replaceSearch now takes either form:

Definition	Footprint	Extended search
function (unchanged)	unknown	refused
field selection (new)	exact	checked per path, denied ones refused by name

// permission-checked, extended search preserved
collection.replaceSearch({ includeFields: ['project:name'], excludeFields: ['description'] });

extended is deliberately absent from the object type (Omit<SearchOptions, 'extended'>): the caller owns that flag, so it is forwarded from the request rather than pinned by the customization. A test pins that forwarding, and tsc rejects both { extended: true } and a misspelt key.

Migrating a handler is not a no-op — it is stricter

Worth stating plainly, because it is the one thing that can surprise: a field selection is more restrictive than the handler it replaces, not less.

An included relation path is reported on a plain search too, not only an extended one. So it gets permission-checked where the handler was exempted — and that exemption only ever existed because the footprint was unknown. Concretely, replaceSearch({ includeFields: ['project:name'] }) starts refusing a plain search for a role that cannot read projects, where the handler form served it.

That is the correct posture, and it is the same permission sweep #1840 already asks for. But it means converting a handler is a behavioural change to plan, not a mechanical rewrite.

The part worth reviewing

Both derivations are unified behind getSearchableFields, and generateSearchFilter / getSearchedFields now come from it. They were computed separately — getSearchedFields reimplemented a subset via getSearchedFieldPaths + getFields — which is exactly why the footprint could only be stated for the plain default search, and would have drifted from what the query reads.

That invariant is what makes the permission check trustworthy, so it is pinned rather than assumed, in two tests:

• it covers every path the search actually reads — every path in the generated condition tree is in the reported footprint, asserting the traversed path by name so it cannot pass vacuously on a search that never leaves the root collection.
• it covers what a dot-syntax term reads once onlyFields replaced the set — the sharp case: onlyFields drops the field:term syntax from the searchable set, and the walker then searches the replaced set with the term reassembled. A separately-derived footprint would have missed which columns that actually reads.

The invariant is also structural, not just tested: ConditionTreeQueryWalker only ever builds leaves from the field list it is handed, which is that same map.

Two incidental effects of the unification, both improvements: the footprint is deduplicated (it is a Map, and the specified-field and default-field lists could overlap), and specified fields resolve through the same lenientGetSchema(this, …) call the query builder uses instead of a parallel one. One consequence to be aware of: the footprint's order changes (defaults first), so when several paths are denied, the error names a different one than before. No test depended on it.

Not in scope

• getSearchedFields still claims a footprint it cannot guarantee when the child collection searches natively. refineFilter hands that search straight down (!this.childCollection.schema.searchable), so the child reads whatever it likes, while getSearchedFields reports an enumeration of the child's columns. agent-ruby guards this case explicitly (enumerable_search? is @replacer.nil? && !@child_collection.schema[:searchable]) and node does not. Narrow today — no shipped datasource calls enableSearch(), only packages/_example — but it is an unverified claim, and closing it would refuse extended search on those collections, which is a separate decision from this PR.
• getSearchedFieldPaths is now unused in src. It is exported from the package index as of 1.71.x, so it is left in place rather than removed in a patch.
• A handler's footprint is still unknowable. Inferring it would mean either running customer code twice or authorizing after refineFilter, both out of proportion here.

Tests

packages/datasource-customizer/test/decorators/search/collections.test.ts: 8 added — the footprint answered rather than refused, excludeFields / includeFields / onlyFields reflected in it, the leaf collection of a relation path named on a plain search as well as an extended one, the extended flag forwarded rather than pinned, and the two footprint-covers-what-is-read invariants.

Verified locally: datasource-customizer 875 passed / 63 suites; packages/agent security + authorization + utils 508 passed / 25 suites (the #1840 suite included, unchanged); tsc clean; eslint clean.

🤖 Generated with Claude Code

Note

Let field-selection replaceSearch be permission-checked instead of refused

• Extends CollectionCustomizer.replaceSearch to accept a SearchReplaceDefinition union: either a handler function or a field-selection object (includeFields, excludeFields, onlyFields).
• When a field selection is provided, SearchCollectionDecorator.refineFilter generates a narrowed search filter via generateSearchFilter and forwards the request's extended flag, so the search goes through normal permission checks rather than being refused.
• getSearchedFields now returns a concrete footprint for field-selection and default-search modes (instead of null); it returns null only when a handler function is installed.
• getSearchableFields unifies resolution of searchable fields from default fields, query-specified fields, and the selection's sets; excludeFields/includeFields are resolved case-insensitively and leniently against actual schema names, ignoring unresolved names.
• Risk: getSearchedFields now returns a concrete footprint in field-selection mode where it previously returned null — any consumer relying on null to detect a customized search will see different values; excludeFields with names not matching any schema field are silently ignored instead of erroring.

^{Macroscope summarized cf0af5a.}

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (1)

Rating	File	% Diff	Uncovered Line #s
	...ages/datasource-customizer/src/decorators/search/collection.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[matthv]

Ran a multi-angle review (correctness, tests, comments, type design) on this PR. The core invariant — getSearchedFields()'s footprint must be a superset of what generateSearchFilter() actually reads — holds: verified empirically across 11 combinations of includeFields/excludeFields/onlyFields/extended, no divergence found, and it fails closed (over-reports rather than under-reports).

A few things worth addressing before merge:

1. excludeFields matches strictly while includeFields/onlyFields match fuzzily (collection.ts:111). The include/onlyFields paths go through lenientGetSchema (case/-/_-tolerant), but excludeFields does a raw .includes() against the resolved field name. Concretely, on a column pan_last4, replaceSearch({ excludeFields: ['panLast4'] }) silently fails to exclude it — while includeFields: ['panLast4'] resolves fine. Not a permission bypass (footprint and actual search stay consistent), but a silent no-op that misleads whoever configured it.

2. Unresolvable paths are silently dropped (collection.ts:107, the .filter(Boolean)). A typo in onlyFields/includeFields (e.g. onlyFields: ['secretNoteTypo']) empties the searchable set with no error/log — the search then permanently matches nothing. Since replaceSearch is a queued customization, this could validate/throw/log at registration time instead.

3. A field selection silently discards a datasource's native search (collection.ts:59 — this.replacer || !childCollection.schema.searchable routes any field selection, including {}, to the generic per-column search). On a collection with enableSearch() (native search), installing a field selection replaces that native search rather than narrowing it — e.g. replaceSearch({}) on such a collection makes the search return nothing. The JSDoc's "narrows the same default search" doesn't hold in that case.

4. getSearchedFieldPaths is now dead production code but still publicly exported (field-paths.ts, re-exported from index.ts). Its docstring claims it resolves paths "the same way the search decorator resolves them" — no longer true/enforced now that the decorator's own logic moved to getSearchableFields. Worth removing or at least fixing the doc so it doesn't drift further from reality.

5. No end-to-end test of the actual permission-check change. Tests in packages/agent/test/security/related-read-permissions.test.ts all stub getSearchedFields directly; nothing exercises a real replaceSearch({ includeFields: [...] }) through assertCanReadQueryFields for both the newly-allowed case (extended search now permitted) and the refusal case (an included relation path the caller can't read).

6. SearchDefinition changes meaning under an already-published public name. It used to be exactly the handler function type; it's now SearchHandlerDefinition | SearchFieldsDefinition. Verified with tsc --strict: any external consumer who typed something as SearchDefinition and then called it directly (a natural thing to do with what used to be a pure function type) fails to compile after upgrading (TS2349: not all constituents are callable). No usage inside this monorepo breaks, but it's a real semver-breaking change for downstream consumers hiding inside what reads as a feat:. Suggest keeping SearchDefinition = the handler type (or an alias) and naming the new union something else (e.g. SearchReplaceDefinition), or explicitly calling out the breaking change.

7. Missing test symmetry for excludeFields. The two tests proving "footprint covers what the search actually reads" (it covers every path the search actually reads, it covers what a dot-syntax term reads once onlyFields replaced the set) only exercise includeFields/onlyFields. An equivalent test for excludeFields would have caught point 1.

Nothing here contradicts the PR's stated goal or the security invariant it's meant to restore — these are refinements on top of a sound refactor.

[matthv]

Thanks for the follow-up commit — it addresses the excludeFields matching bug, the SearchDefinition breaking-rename concern, the missing end-to-end permission test, and documents the native-search / getSearchedFieldPaths points well.

One item from the original review is still open:

Unresolvable paths are silently dropped (collection.ts, the .filter(Boolean) after lenientGetSchema). A typo in onlyFields/includeFields (e.g. onlyFields: ['secretNoteTypo']) empties the searchable set with no error or log — the search then permanently matches nothing, with nothing telling the integrator why. Since replaceSearch is a queued customization, this seems like it could validate/throw at registration time (similar in spirit to how other declarative customizations fail fast on an unknown field) rather than degrading silently at query time.

Not blocking if you'd rather track it separately — flagging so it doesn't fall through the cracks.

[PMerlet]

Thanks — five of the seven are addressed in 2bb8ef8. Two I left alone on purpose, reasons below.

Fixed

1. excludeFields matched strictly. Real, and worth noting it predates this PR: the raw .includes() is on main inside generateSearchFilter, already reachable through context.generateSearchFilter(value, { excludeFields }). This PR only makes it reachable declaratively. Both sides now resolve through lenientGetSchema; an unresolvable name is kept as written, since it excludes nothing either way. Two tests pin it — one on a column, one on a relation path — and both fail without the fix.

4. getSearchedFieldPaths docstring. Fixed: it no longer claims to resolve paths the way the decorator does, and says outright that the decorator doesn't use it and nothing keeps the two in step. Left exported, since it has been in the package index since 1.71.x.

5. No end-to-end test of the permission change. Added — four tests driving a real replaceSearch({ includeFields }) through DataSourceCustomizer and the list route rather than a stubbed getSearchedFields: the extended search refused by name instead of for want of a footprint, the plain search a handler used to be served, the extended search a field selection buys back, and the handler form still refused.

6. SearchDefinition semver break. The strongest point, and you're right. SearchDefinition now points at the handler alone again; the union replaceSearch accepts is SearchReplaceDefinition, with SearchHandlerDefinition naming the handler form descriptively. Checked with tsc --strict against a consumer file that types a value as SearchDefinition and calls it: compiles again, no TS2349.

7. Missing excludeFields test symmetry. The footprint-covers-what-is-read invariant now has its excludeFields case, and the two existing ones share the collection they were duplicating.

Not changed

2. Unresolvable paths silently dropped. Also pre-existing on main, same call path as point 1. Validating at registration time would fail the boot of agents that start fine today — that's a behavioural decision of its own, not a review follow-up, so I'd rather it not ride along in this PR.

3. A field selection discards a datasource's native search. The mechanism is real — it replaces the native search with the generic per-column one rather than narrowing it — but the stated consequence isn't. I probed it: replaceSearch({}) on a natively searchable collection yields {"field":"label","operator":"IContains","value":"martin"}, not an empty result, because onlyFields is absent so defaultFields is still every column. So the search doesn't return nothing; it stops being the datasource's own. I've corrected replaceSearch's JSDoc to state that limit, and left the behaviour alone — it's the "Not in scope" item on enableSearch() collections in the description.

Verified

datasource-customizer 878 passed / 63 suites, agent 1489 passed / 82 suites, tsc and eslint clean on both.

[matthv]

LGTM