Skip to content

Commit 69ac82c

Browse files
claude[bot]claude
andauthored
fix(metadata-protocol,rest,spec): one predicate for capabilities.search and the /search route (#7541) (#7577)
* fix(metadata-protocol,rest,spec): one predicate for `capabilities.search` and the /search route (#7541) `/discovery` reported `capabilities.search = {enabled:false}` while `GET /api/v1/search?q=audit` answered 200 with real hits. Prime Directive #10 inverted: a live endpoint no conforming client will ever call, because the document whose only job is to say what is available said it was not. Two producers, two unrelated predicates. The bit came from a registered `search` service slot; the route 501s on `typeof protocol.searchAll !== 'function'`. Nothing registers that slot in either repository and the protocol implements `searchAll` unconditionally, so the two did not merely risk disagreeing — they disagreed on every host that exists. `search` was the last well-known capability still on bare slot presence. Its neighbours already carry the rule stated in the builder ("the predicate is deliberately the SAME one that decides whether the route is advertised"), most recently `chunkedUpload` in #5672. This puts `search` on that footing: - metadata-protocol: the bit is now `typeof this.searchAll === 'function'` — the route's own refusal predicate. - rest: `/discovery` ANDs it with `api.enableSearch`, the flag that decides whether this server mounts the route at all. Same two-layer conjunction `transactionalBatch` uses with `api.enableBatch`. No fallback was added to the route. `services.search` keeps its own answer: the slot is a search ENGINE (Elastic/Meili, `ISearchService`), so it reports which implementation occupies it while the capability reports whether the surface is served. Those now legitimately differ, so spec gives the slot a REMEDY_DETAIL sentence — the treatment `ui` already carries for the same shape (#4146) — keeping the unchanged "no implementation ships" fact and adding which question it answers. The pin drives the real capability builder and the real route in one test and asserts they agree, over three hosts that genuinely differ (served 200, unmounted 404, unimplemented 501) — never `enabled === true`, which would pass again the day someone hardcodes the bit. Fixes #7541 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHTC2SvMXprxV9eEHqfbCP * fix(rest): build a real IHttpRequest in the #7541 pin instead of a partial literal The new test drove both handlers with `{ params, query }` object literals, which do not satisfy `IHttpRequest` — 2 x TS2345, taking @objectstack/rest's TEST_DEBT from its recorded 155 to 157. That ledger is a ratchet: it may only shrink. Fixed at the source rather than by raising the entry, and the fix is not bookkeeping. `enforceAuth` runs BEFORE either predicate under test and reads `req.method` and `req.path` — both `undefined` in the old literals — so the pin was measuring statuses a real caller would not necessarily get. The helper now returns a complete `IHttpRequest` typed against the contract, so the gate reads zero errors from this file AND the measured 200/404/501 are the statuses the real request shape produces. Not cast to `any`: the neighbouring conformance test casts its handler that way, but a cast here would have hidden exactly the missing members that make the measurement faithful. Measured with the gate's own command (packages/rest/tsconfig.json with the test globs dropped from `exclude`): 157 before, 155 after — equal to the recorded ledger entry, which is left untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHTC2SvMXprxV9eEHqfbCP --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7f1d4d0 commit 69ac82c

6 files changed

Lines changed: 346 additions & 10 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
"@objectstack/rest": patch
4+
"@objectstack/spec": patch
5+
---
6+
7+
fix(metadata-protocol,rest,spec): derive `capabilities.search` from what serves `/search`, not from an empty service slot (#7541)
8+
9+
Every REST host advertised `capabilities.search = { enabled: false }` in
10+
`/discovery` while `GET /api/v1/search?q=…` answered `200` with real hits. This
11+
is Prime Directive #10 inverted: not an advertised endpoint that 404s, but a
12+
live endpoint **no conforming client will ever call**, because the document
13+
whose only job is to say what is available said it was not.
14+
15+
**Two producers, two unrelated predicates.** The capability bit came from a
16+
registered `search` service slot (`registeredServices.has('search')`), while the
17+
route refused on something else entirely — `registerSearchEndpoints` returns
18+
`501 NOT_IMPLEMENTED` exactly when `typeof protocol.searchAll !== 'function'`.
19+
Nothing in either repository registers that slot (`CORE_SERVICE_PROVIDER`
20+
records this, verified), and the protocol implements `searchAll`
21+
unconditionally, so the two answers were not merely capable of disagreeing —
22+
they disagreed on every host that exists.
23+
24+
`search` was the last well-known capability still on bare slot presence. Its
25+
neighbours were moved onto serveability with the rule stated in the builder —
26+
*"the predicate is deliberately the SAME one that decides whether the route is
27+
advertised — what we advertise and what we claim cannot disagree"* — most
28+
recently `chunkedUpload` in #5672. This brings `search` onto that footing: **one
29+
predicate, both ends.**
30+
31+
- `@objectstack/metadata-protocol``capabilities.search` is now
32+
`typeof this.searchAll === 'function'`, the route's own refusal predicate.
33+
- `@objectstack/rest` — the `/discovery` producer ANDs that with
34+
`api.enableSearch`, the flag that decides whether this server mounts the route
35+
at all. Exactly the two-layer conjunction `transactionalBatch` already uses
36+
with `api.enableBatch`: the protocol states what it can serve, the server
37+
states what it mounted, and a deployment that opts out reports `false` rather
38+
than promising a 404. Nothing was added to the route itself.
39+
40+
**`services.search` is unchanged, and deliberately so.** The slot answers a
41+
different question — `CoreServiceName` declares it "Search Engine
42+
(Elastic/Meili)" and `ISearchService` is an index/query contract — so it still
43+
reports *which engine occupies the slot*, while the capability reports *whether
44+
the surface is served*. On an ordinary host those now differ
45+
(`capabilities.search.enabled: true` beside `services.search.status:
46+
'unavailable'`), and both statements are true. So that the two halves of one
47+
document do not read as contradicting each other, `@objectstack/spec` gives the
48+
slot a `REMEDY_DETAIL` sentence — the same treatment `ui` carries for the same
49+
shape (#4146) — which keeps the unchanged "no implementation ships" fact and
50+
adds which question the entry answers. The `status` itself stays
51+
`unavailable`: no engine is registered, and saying otherwise would be the
52+
original defect pointed the other way.
53+
54+
**Client impact.** A client that gated its search UI on
55+
`capabilities.search.enabled` was hiding a working feature on every deployment;
56+
it now sees `true` wherever the endpoint really serves, and `false` when the
57+
protocol cannot search (route `501`) or the server did not mount it (`404`).

packages/metadata-protocol/src/protocol.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3540,7 +3540,34 @@ export class ObjectStackProtocolImplementation implements
35403540
comments: !!this.engine.registry?.getObject?.('sys_comment'),
35413541
automation: registeredServices.has('automation'),
35423542
cron: registeredServices.has('job'),
3543-
search: registeredServices.has('search'),
3543+
// [#7541] Serveability-gated on the protocol's OWN search
3544+
// implementation, was slot presence. This is the same predicate the
3545+
// route refuses on: `registerSearchEndpoints`
3546+
// (packages/rest/src/rest-server.ts) 501s exactly when
3547+
// `typeof protocol.searchAll !== 'function'`, so the two ends can no
3548+
// longer answer the same question differently — the rule stated at
3549+
// the top of this block, applied to the key that was still exempt.
3550+
//
3551+
// The old predicate was wrong in the direction discovery exists to
3552+
// prevent: `searchAll` is implemented by this class unconditionally,
3553+
// nothing in either repository registers the `search` slot
3554+
// (CORE_SERVICE_PROVIDER records that, verified), so every REST host
3555+
// served `GET /api/v1/search` 200 while advertising
3556+
// `capabilities.search = false`. A conforming client — one that
3557+
// trusts the document instead of probing — skipped a working
3558+
// surface. Prime Directive #10 inverted.
3559+
//
3560+
// `services.search` is deliberately NOT collapsed into this. The
3561+
// slot is a distinct question with its own answer: `CoreServiceName`
3562+
// declares it "Search Engine (Elastic/Meili)" and `ISearchService`
3563+
// is an index/query contract, so `services.search` reports WHICH
3564+
// ENGINE occupies the slot while this bit reports WHETHER THE
3565+
// SURFACE IS SERVED (`WellKnownCapabilitiesSchema.search`: "whether
3566+
// the backend supports full-text search"). They may legitimately
3567+
// differ — an empty slot with a served endpoint is today's normal
3568+
// host — and `serviceUnavailableMessage('search')` now says so in
3569+
// the same document, the way `ui` does for the same shape (#4146).
3570+
search: typeof this.searchAll === 'function',
35443571
export: registeredServices.has('automation') || registeredServices.has('queue'),
35453572
// [#5672] Serveability-gated, was presence-only. Two reasons, and
35463573
// the second is the binding one:

packages/objectql/src/protocol-discovery.test.ts

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -448,27 +448,43 @@ describe('ObjectStackProtocolImplementation - Dynamic Service Discovery', () =>
448448
expect(discovery.capabilities).toBeDefined();
449449
// ui is registered but doesn't map to a well-known capability directly
450450
expect(discovery.services.ui.enabled).toBe(true);
451-
// All well-known capabilities should be disabled since ui doesn't map to any
452-
// (comments derives from the sys_comment object, which is not registered here).
451+
// The SLOT-DERIVED well-known capabilities should be disabled since ui maps
452+
// to none of them (comments derives from the sys_comment object, which is
453+
// not registered here).
453454
expect(discovery.capabilities!.comments).toEqual({ enabled: false });
454455
expect(discovery.capabilities!.automation).toEqual({ enabled: false });
455456
expect(discovery.capabilities!.cron).toEqual({ enabled: false });
456-
expect(discovery.capabilities!.search).toEqual({ enabled: false });
457457
expect(discovery.capabilities!.export).toEqual({ enabled: false });
458458
expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: false });
459+
// [#7541] `search` is NOT in that list any more. It is no longer derived
460+
// from a service slot at all — it reports whether this protocol can serve
461+
// `/search` (`typeof searchAll === 'function'`, the predicate the route's
462+
// own 501 uses), and this class always can. Asserting `false` here was
463+
// asserting the defect: the endpoint served 200s while the document said
464+
// the capability was off.
465+
expect(discovery.capabilities!.search).toEqual({ enabled: true });
459466
});
460467

461-
it('should set all capabilities to false when no services are registered', async () => {
468+
it('should set all slot-derived capabilities to false when no services are registered', async () => {
462469
protocol = new ObjectStackProtocolImplementation(engine);
463470
const discovery = await protocol.getDiscovery();
464471

465472
expect(discovery.capabilities).toBeDefined();
466473
expect(discovery.capabilities!.comments).toEqual({ enabled: false });
467474
expect(discovery.capabilities!.automation).toEqual({ enabled: false });
468475
expect(discovery.capabilities!.cron).toEqual({ enabled: false });
469-
expect(discovery.capabilities!.search).toEqual({ enabled: false });
470476
expect(discovery.capabilities!.export).toEqual({ enabled: false });
471477
expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: false });
478+
// [#7541] Same reason as above — and this is the exact host the issue was
479+
// reported against: an empty registry, a live `/search`. The two halves of
480+
// the document stay independent and both stay honest: the SLOT is still
481+
// empty here...
482+
expect(discovery.capabilities!.search).toEqual({ enabled: true });
483+
expect(discovery.services.search.enabled).toBe(false);
484+
expect(discovery.services.search.status).toBe('unavailable');
485+
// ...and its message now says which question that answers, instead of
486+
// reading as "search is dead on this host".
487+
expect(discovery.services.search.message).toMatch(/capabilities\.search/);
472488
});
473489

474490
it('should dynamically set capabilities based on registered services', async () => {
@@ -482,11 +498,18 @@ describe('ObjectStackProtocolImplementation - Dynamic Service Discovery', () =>
482498

483499
expect(discovery.capabilities!.automation).toEqual({ enabled: true });
484500
expect(discovery.capabilities!.cron).toEqual({ enabled: false });
485-
expect(discovery.capabilities!.search).toEqual({ enabled: true });
486501
expect(discovery.capabilities!.export).toEqual({ enabled: true });
487502
expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: true });
488503
// comments is independent of services — it tracks the sys_comment object (#3180).
489504
expect(discovery.capabilities!.comments).toEqual({ enabled: false });
505+
// [#7541] `search` is true here too, but NOT because the slot above is
506+
// filled — this line proves nothing about the slot and is kept only so the
507+
// reader is not left thinking it does. The discriminating cases live in
508+
// `packages/rest/src/discovery-search-capability-agreement.test.ts`, which
509+
// drives the capability builder and the route together.
510+
expect(discovery.capabilities!.search).toEqual({ enabled: true });
511+
// What the slot DOES still decide, unchanged: the `services` half.
512+
expect(discovery.services.search.enabled).toBe(true);
490513
});
491514

492515
// ── Atomic cross-object batch capability (#3298 / #1604 / ADR-0034) ─────────
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// [#7541] The two producers of "can this host search" must answer the same
4+
// question the same way.
5+
//
6+
// The defect: `capabilities.search` came from a registered `search` SERVICE
7+
// SLOT, while `GET {basePath}/search` refused on something else entirely —
8+
// `typeof protocol.searchAll !== 'function'`. Nothing in either repository
9+
// registers that slot and the protocol implements `searchAll` unconditionally,
10+
// so every REST host advertised `capabilities.search = {enabled:false}` while
11+
// serving 200s with real hits. A client that trusts the discovery document —
12+
// which is the document's only purpose — skipped a working surface. Prime
13+
// Directive #10 inverted: not an advertised endpoint that 404s, but a live
14+
// endpoint no conforming client will ever call.
15+
//
16+
// WHAT THIS FILE ASSERTS, and why it is shaped this way: it does NOT assert
17+
// `enabled === true`. That assertion passes again the day someone hardcodes the
18+
// bit, which is the same class of defect one layer over. It asserts AGREEMENT —
19+
// `declared === served` — with both sides MEASURED from the real producers in
20+
// the same test: `capabilities.search` off the real `getDiscovery()` through
21+
// the real `/discovery` handler, and the served status off the real
22+
// `registerSearchEndpoints` handler. Three hosts that genuinely differ (below)
23+
// keep the agreement from holding vacuously.
24+
25+
import { describe, it, expect, vi } from 'vitest';
26+
import type { IHttpRequest } from '@objectstack/spec/contracts';
27+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
28+
import { RestServer } from './rest-server.js';
29+
30+
/**
31+
* A complete `IHttpRequest`, typed against the contract rather than cast to
32+
* `any`. Not ceremony: `enforceAuth` — which runs before either predicate under
33+
* test — reads `method` and `path`, so a partial literal would exercise the
34+
* gate with `undefined` on both. Building the real shape is what makes the
35+
* measured statuses below the statuses a real caller gets.
36+
*/
37+
function request(path: string, query: Record<string, string> = {}): IHttpRequest {
38+
return { params: {}, query, headers: {}, method: 'GET', path };
39+
}
40+
41+
function createMockServer() {
42+
return {
43+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
44+
use: vi.fn(),
45+
listen: vi.fn().mockResolvedValue(undefined),
46+
close: vi.fn().mockResolvedValue(undefined),
47+
};
48+
}
49+
50+
/**
51+
* An engine carrying ONE searchable object with ONE matching row, so the
52+
* served path is a real 200-with-hits rather than a 200-with-nothing — the
53+
* exact reproduction in the issue (`?q=audit` → 200, real hits).
54+
*/
55+
function createEngine() {
56+
const widget = {
57+
name: 'widget',
58+
fields: [{ name: 'title', type: 'text', searchable: true }],
59+
};
60+
return {
61+
registry: {
62+
getObject: (n: string) => (n === 'widget' ? widget : undefined),
63+
getAllObjects: () => [widget],
64+
getRegisteredTypes: () => [],
65+
},
66+
find: async () => [{ id: 'w1', title: 'audit trail' }],
67+
};
68+
}
69+
70+
type Host = {
71+
/** `capabilities.search.enabled` as the composed `/discovery` body reports it. */
72+
declared: boolean;
73+
/** HTTP status `GET {basePath}/search?q=audit` actually answers. */
74+
status: number;
75+
/** Hit count when it answered 200. */
76+
hits: number;
77+
};
78+
79+
/**
80+
* Boot a REST server over the REAL protocol and read BOTH producers off it.
81+
*
82+
* `enableSearch` selects whether this server mounts the route at all;
83+
* `withSearchAll: false` removes the protocol's own implementation, which is
84+
* the input the route's 501 branch exists for.
85+
*/
86+
async function measure(opts: {
87+
enableSearch?: boolean;
88+
withSearchAll?: boolean;
89+
} = {}): Promise<Host> {
90+
const protocol: any = new ObjectStackProtocolImplementation(
91+
createEngine() as any,
92+
() => new Map(),
93+
);
94+
if (opts.withSearchAll === false) {
95+
// Shadow the prototype method on the instance. Both predicates read the
96+
// same property off the same object, so this single override is what makes
97+
// the "protocol cannot search" host measurable at all — and it is why the
98+
// test cannot pass by two independent predicates coincidentally agreeing.
99+
Object.defineProperty(protocol, 'searchAll', { value: undefined, configurable: true });
100+
}
101+
102+
const config: any = {
103+
api: {
104+
requireAuth: false,
105+
...(opts.enableSearch === false ? { enableSearch: false } : {}),
106+
},
107+
};
108+
const rest = new RestServer(createMockServer() as any, protocol as any, config);
109+
// Authenticated caller — step 1 of the issue's reproduction. `enforceAuth`
110+
// runs BEFORE the `searchAll` probe, so an anonymous request 401s and never
111+
// reaches either predicate; this is the house stub the other rest tests use
112+
// for authed handlers, and it is upstream of everything under test here.
113+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
114+
rest.registerRoutes();
115+
const routes = rest.getRouteManager();
116+
117+
const discoveryEntry = routes.get('GET', '/api/v1/discovery');
118+
if (!discoveryEntry) throw new Error('discovery route not registered');
119+
let discoveryBody: any;
120+
const discoveryRes: any = {
121+
json: (b: any) => { discoveryBody = b; },
122+
status: () => discoveryRes,
123+
};
124+
await discoveryEntry.handler(request('/api/v1/discovery'), discoveryRes);
125+
const declared = discoveryBody?.capabilities?.search?.enabled;
126+
127+
const searchEntry = routes.get('GET', '/api/v1/search');
128+
if (!searchEntry) {
129+
// Not mounted — a client calling it gets the router's 404. That IS the
130+
// served answer for this host.
131+
return { declared, status: 404, hits: 0 };
132+
}
133+
let status = 200;
134+
let searchBody: any;
135+
const searchRes: any = {
136+
status: (s: number) => { status = s; return searchRes; },
137+
json: (b: any) => { searchBody = b; },
138+
};
139+
await searchEntry.handler(request('/api/v1/search', { q: 'audit' }), searchRes);
140+
return { declared, status, hits: searchBody?.hits?.length ?? 0 };
141+
}
142+
143+
/** Served ⇔ a caller can get search results out of this host. */
144+
const isServed = (h: Host) => h.status !== 404 && h.status !== 501;
145+
146+
describe('[#7541] `capabilities.search` and the /search route answer one question', () => {
147+
it('agrees on the ordinary host — where the document used to contradict the endpoint', async () => {
148+
const host = await measure();
149+
150+
// The symptom, measured: the endpoint really does serve real hits here.
151+
expect(host.status).toBe(200);
152+
expect(host.hits).toBeGreaterThan(0);
153+
154+
// The pin: whatever the endpoint does, the document says the same thing.
155+
// Before the fix `declared` was false against a 200 — the inversion.
156+
expect(host.declared).toBe(isServed(host));
157+
});
158+
159+
it('agrees on a host that does not mount the route (`api.enableSearch: false`)', async () => {
160+
const host = await measure({ enableSearch: false });
161+
162+
expect(host.status).toBe(404);
163+
expect(host.declared).toBe(isServed(host));
164+
});
165+
166+
it('agrees on a protocol that cannot search — both ends refuse on the same predicate', async () => {
167+
const host = await measure({ withSearchAll: false });
168+
169+
// The route's own 501 branch, reached through the real handler.
170+
expect(host.status).toBe(501);
171+
expect(host.declared).toBe(isServed(host));
172+
});
173+
174+
it('anti-vacuity: the three hosts are genuinely discriminated, in both directions', async () => {
175+
const [served, unmounted, unimplemented] = await Promise.all([
176+
measure(),
177+
measure({ enableSearch: false }),
178+
measure({ withSearchAll: false }),
179+
]);
180+
181+
// Without this, `declared === served` would hold for the empty reason if
182+
// some future edit pinned the bit — or the route — to one constant.
183+
expect([served.declared, unmounted.declared, unimplemented.declared])
184+
.toEqual([true, false, false]);
185+
expect([served.status, unmounted.status, unimplemented.status])
186+
.toEqual([200, 404, 501]);
187+
});
188+
});

packages/rest/src/rest-server.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3790,6 +3790,25 @@ export class RestServer {
37903790
+ '{ $ref: <opIndex> } parent references (#1604 / ADR-0034).',
37913791
};
37923792

3793+
// [#7541] Global search — the same two-layer AND, for the
3794+
// same reason. The protocol answered whether IT can serve a
3795+
// search (`typeof searchAll === 'function'`, the predicate
3796+
// `registerSearchEndpoints` 501s on); this server answers
3797+
// whether it MOUNTED the route at all (`api.enableSearch`,
3798+
// the flag gated in registerRoutes). A deployment that opts
3799+
// out gets a 404, so advertising the protocol's `true`
3800+
// unqualified would re-open the declared ≠ enforced gap one
3801+
// layer up from the one this issue closed. Neither half is a
3802+
// fallback for a wrong bit: each layer states the fact only
3803+
// it knows, and `enabled` is their conjunction.
3804+
//
3805+
// The flag is read with the mount's own `?? true` spelling
3806+
// rather than the equivalent `!== false` — same predicate,
3807+
// same characters, so the two cannot be edited apart.
3808+
caps.search = {
3809+
enabled: !!caps.search?.enabled && (this.config.api.enableSearch ?? true),
3810+
};
3811+
37933812
// Attach scoping metadata so clients can detect dual-mode routing.
37943813
(discovery as any).scoping = {
37953814
enabled: this.config.api.enableProjectScoping,

0 commit comments

Comments
 (0)